Make a 2.x release series compatible with 6.x releases for JWT library

Can we try this patch.

Reviewed and Tested. php_jwt_v6.patch patch is working as expected.

The last submitted patch, 2: php_jwt_v6.patch, failed testing. View results →

We need to update the minimum version to 5.5 (note that this is still considered insecure) and make some changes to fix the BC breaking changes in 6.0.

The last submitted patch, 5: 3338703-5.patch, failed testing. View results →

I think we might need a new 2.x release for this one...

a.kovrigin → made their first commit to this issue’s fork.

@a.kovrigin how come you made a merge request? What's wrong with the patch in #7? Reviewing that patch would be more valuable.

It seems that MR is a good way to include this in the composer https://getcomposer.org/doc/05-repositories.md#vcs
Otherwise, if you have firebase 6 deps in another package it isn't possible to include drupal/jwt.
Is it possible to have this patch/MR RTBC?

@Andriy Khomych - that's not a good way to do that. If you use the branch for the MR then anyone can get access and inject code into your site by uploading code here. Only real way of doing something like that is fork the module to some other code hosting service where only you have commit access and use that.

@Andriy Khomych with respect to RTBC - maybe that's something you can do after testing the new version with upgraded dependencies on your site.

@Alex Pott , thank you for suggestions.
Well, making own Drupal fork is better, but we can use another fork by relying on the specific commit hash - which I think should be fine and safe (I don't think it is easy to bypass/hack git commit hash with another code) and it seems a common practice to do so.

Totally agree with you, I'll perform testing of the MR. So far it looks good to me.

@Andriy Khomych nice idea relying on the commit hash - that would be more secure.

I'm happy this only requires a smallish patch.

For \Drupal\users_jwt\UsersJwtKeyRepository::offsetGet(). I think I added the attribute #[\ReturnTypeWillChange] with this transition in mind. Given we are making a new major version, that should be removed since this is the change. Also from the interface.

A really complete update might be to make class \Drupal\users_jwt\UsersKey extend \Firebase\JWT\Keybut that seems more complicated and would require updating any stored keys.

Why is there a cast to array in this change in the prior patches?

-    $encoded = $this->transcoder->encode($jwt->getPayload(), $key, $this->algorithm);
+    $encoded = $this->transcoder->encode((array) $jwt->getPayload(), $key, $this->algorithm);

both array and object should work, and stdClass object is actually better since it it's empty it encodes to a JSON object as {} vs an empty array that encodes to [].

I've reverted that change in this patch.

The last submitted patch, 17: 3338703-17.patch, failed testing. View results →

Ah, I see:

TypeError: Argument 1 passed to Firebase\JWT\JWT::encode() must be of the type array, object given, called in /var/www/html/modules/contrib/jwt/src/Transcoder/JwtTranscoder.php on line 201

I didn't realize that additional API change. Pus back the array cast.

Looking at the test coverage, especially for users_jwt

I think it's reasonably complete, though it would be good to add there a few more checks that 200 response codes indicating authentication also have the expected page content (in \Drupal\Tests\users_jwt\Functional\FormsTest::testForms()). Also, would be nice to try switching around the algorithm from RSA to HMAC just to verify it fails.

Still, I think this is gtg to make a 2.x if someone would like to verify that they have tested it in a real-world situation and mark it RTBC.

Also... looking at 📌 Drupal 10 roadmap/hitlist (make 2.0 release) Fixed

There are a couple more deprecation fixes there that should be done before a 2.0 release (everything but ed25519 signatures) so that we minimize any other api/class changes.

Automatically closed - issue fixed for 2 weeks with no activity.