Firebase JWT implementation need update after the upgrade from 5.* to 6.*

[touhidurabir]

Describe the bug
After the upgrade of the package firebase/php-jwt from 5.* to 6.* , the usage/implementation of JWT::encode and JWT::decode need to update to match with latest code specification from package itself .

To Reproduce
Try to create/update a user API Key from Top right user menu --> Edit Profile and see it's failing for API Key tab .
Also try to make any api required using the api key (such as http://OJS_BASE_PATH/index.php/JOURNAL_PATH/api/v1/users/USER_ID?apiToken=USER_API_KEY) and see that it's returning the error 500 .

What application are you using?
OJS, OMP or OPS main branch

Additional information
This cause travis build tests to fail for main branch and also rendering api keys are unusable . Also this need to be updated in any plugins such as JatsTemplate that have the previous implementations .

The solution is as follow

for JWT::encode , the first params need to be an array

JWT::encode([ANY_DATA_TO_ENCODE], $secret, 'HS256');

for JWT::decode, something similar as below

$headers = new stdClass;
$apiToken = ((Array)JWT::decode($jwt, new \Firebase\JWT\Key($secret, 'HS256'), $headers))[0]; 

PRs
pkp-lib --> #9114
ojs --> pkp/ojs#3953

[asmecher]

Hi @touhidurabir! Thanks, this is my fault, but I haven't started working on it yet. Feel free to self-assign if you'd like to tackle it.

[touhidurabir]

The changes required to fix it already incorporated with the PR for pkp/pkp-lib#9073 , waiting for the tests to pass . But as pkp/pkp-lib#9073 target a broader changes(Laravel upgrade form 9.x to 10.x) and if that need more discussion and debate before it can get merged , I can make a separate PR for this.

@asmecher what do you think ?

[asmecher]

@touhidurabir, this looks good, but it does cause a backwards compatibility break -- anyone using 3.4.0-x or older will have a JWT payload that's a string, which has always been out of spec but was previously accepted by the library and is not any longer.

This means that your commits (132b912 and 513c31d) will work for newly created API keys, but will not successfully decode API keys that were generated before the update to JWT-PHP 6.x.

While we could make a breaking change that invalidates all existing API keys, that feels pretty drastic. I'd like to look for options that generate "compliant" new keys, but don't break existing keys. Here are a few, off the top of my head...

• Carry along an old version of the php-jwt library for the older deprecated keys, and use a new version for the new form. I don't think Composer is ready for this, though, so we'd probably need to maintain a forked copy of the library.
• Send in a pull request to php-jwt relaxing the requirement that payloads be arrays. If I were maintaining php-jwt, I'd probably reject it.
• Patch php-jwt so that it allows string payloads for deprecated tokens. This is probably the least-worst option.

What do you think?

[touhidurabir]

@asmecher this is how tested

1. updated with in implementation in branch i9110_main .
2. checkout to branch stable-3_4_0, composer install to have proper dependency and created a new api key for a user . Tested that API Key is working fine .
3. Now again checkout to i9110_main, again composer install to have proper dependency. Used the api key created in branch stable-3_4_0 for that user to make api calls and seems working fine .

am I missing something ? I will double check it . Also API keys have been saved as string in both branch stable-3_4_0 and main .

Update

@asmecher I see the issue now . The reason it's working for my test steps that I mentioned above is because api keys are basically sha1(time()) which stored in DB and the return value is the runtime JWT::encode which generated every time . so this will cause for users who have used the JWT encoded api key used else where but within the app itself , should be no issue as we do not stored the encoded value .

Patch php-jwt so that it allows string payloads for deprecated tokens. This is probably the least-worst option.

@asmecher I think this would be least destructive solution even though we will be forced to maintain our own implementation that override the JWT::decode method . it can be a core file such as PKPJWT with overridden decode method .

BTW one question , the invalidation of API key comes in factor when we consider user is using the encoded version of API key elsewhere and after this, the will be forced to copy the new encoded version from profile and update at those places . what are such cases where user has the preset ?

[asmecher]

Patch php-jwt so that it allows string payloads for deprecated tokens. This is probably the least-worst option.

@asmecher I think this would be least destructive solution even though we will be forced to maintain our own implementation that override the JWT::decode method . it can be a core file such as PKPJWT with overridden decode method .

That's an option too, but what I was referring to is simply writing up a .patch file to remove or comment the 3 relevant lines of the library, and adding it to our Composer patch list.

BTW one question , the invalidation of API key comes in factor when we consider user is using the encoded version of API key elsewhere and after this, the will be forced to copy the new encoded version from profile and update at those places . what are such cases where user has the preset ?

@touhidurabir, I'm not sure this is answering your question, but we have a use case with Coalition Publica where we create a user account with a "subscription manager" role. We generate an API key for that user, and then copy/paste that into the Coalition Publica toolset, where it's used to fetch information from the OJS installation that it wouldn't otherwise be able to access. If existing API keys are invalidated, then it would be necessary to re-generate new keys to replace all of these.

[touhidurabir]

I was referring to is simply writing up a .patch file to remove or comment

@asmecher I have added a PKPJwt file to override the decode method at https://github.com/pkp/pkp-lib/pull/9114/files#diff-a7d13d044df743868ba5b839b364de632539b7ff31c4087289b0fcb5dae47f47 . It does not bring in the whole implementation but only if the payload turn out to be a string, then do a encode in proper way and pass it to parent method . If you think it's unnecessary or more hassle to maintain , I can write up a a patch and remove it .

[asmecher]

A patch might be less susceptible to falling out of sync with the parent implementation -- if a patch no longer applies cleanly, we'll get an immediate error. But this approach is OK with me too and does solve the problem nicely. Please add some notes to the code header in the child implementation describing why we're overriding the function; I would also support the addition of a call to error_log when a deprecated API token is used. We'll eventually want to remove this implementation so will want to give admins a heads-up that there's something to fix when it's convenient. Thanks!

[touhidurabir]

@asmecher Added the class doc and a deprecation warning to error log . If all the tests passes, it should be good to merge .

[asmecher]

Thanks, @touhidurabir! I'll take care of the submodule updates for the other apps.