Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incompatibility with the latest version of jjwt #1724

Closed
PierreBtz opened this issue Oct 12, 2023 · 8 comments · Fixed by #1727
Closed

Incompatibility with the latest version of jjwt #1724

PierreBtz opened this issue Oct 12, 2023 · 8 comments · Fixed by #1727

Comments

@PierreBtz
Copy link
Contributor

PierreBtz commented Oct 12, 2023

Describe the bug

When using the latest version of jjwt (0.12.x) to handle authentication with a JWT token, I observe the following issue:

java.lang.NoSuchMethodError: 'io.jsonwebtoken.JwtBuilder io.jsonwebtoken.JwtBuilder.setIssuedAt(java.util.Date)'

To Reproduce

I created the following reproducer: https://github.com/PierreBtz/jwt-github-bug.
A unit test shows the issue, using an older version of jjwt the test is GREEN, using the latest 0.12.(x), the test is RED (see README for setup details).

Expected behavior

Jwt works properly.

Desktop (please complete the following information):

  • OS: OsX Ventura 13.6
  • Browser N/A
  • Version N/A

Additional context

The root cause seems to be jwtk/jjwt#794 which deprecated a bunch of setters.
An evident fix would be to modify the code to use the new setters instead of the old deprecated one, and I'm happy to provide a PR for this however:

  1. I don't understand why a deprecated method wouldn't work anymore :/
  2. If we start using the new setters, it will be a breaking change forcing folks to update jjwt to 0.12.x.
@bitwiseman
Copy link
Member

0.12.0 is barely a week old.
https://github.com/jwtk/jjwt/releases/tag/0.12.0

I would suggest you file a bug and/or PRs in the https://github.com/jwtk/jjwt project to return these methods. I understand why they removed them, but I don't see why these particular methods had to be removed before 1.0 release.

As to how to address this, we could keep the old dependency in this project and add code that tries to call the new method and catches the java.lang.NoSuchMethodError and falls back to the old method.

ihrigb added a commit to ihrigb/github-api that referenced this issue Oct 16, 2023
@ihrigb
Copy link
Contributor

ihrigb commented Oct 16, 2023

I had a bit of time, so I quickly contributed a change: #1727

@PierreBtz
Copy link
Contributor Author

Thanks for providing a fix but I'm not sure it will be accepted since it's not backward compatible, see what Liam suggested:

As to how to address this, we could keep the old dependency in this project and add code that tries to call the new method and catches the java.lang.NoSuchMethodError and falls back to the old method.

@bitwiseman there is still something unclear to me though, granted the method is not in the JwtBuilder class, but it's still on the super class ClaimsMutator, see https://github.com/jwtk/jjwt/blob/37052b6edbeabb628a630efa25f5502f87a9bdbb/api/src/main/java/io/jsonwebtoken/ClaimsMutator.java#L119.

Furthermore, a direct call to Jwts.builder().setIssuedAt(null); properly works, see my reproducer: https://github.com/PierreBtz/jwt-github-bug/blob/b3804a14856d1aaccb9ac818a7c922d0d75f57a8/src/test/java/ReproducerTest.java#L16.

I'm pretty sure there is some subtlety here I'm not grasping, but I just cannot put my finger on it :/

@ihrigb
Copy link
Contributor

ihrigb commented Oct 16, 2023

@PierreBtz, yes I already mentioned the incompatibility in my pull request text. I do not think this is a compatibility issue as a change to github-api's API would be. This would rather require dependents to update not only the github-api but also the version of the jjwt suite.
In the end, I do not believe this would stop this from being merged, but rather do a proper communication here. This issue now already exists anyway (at least for those who take care of their dependencies).

@bitwiseman
Copy link
Member

@ihrigb

This would rather require dependents to update not only the github-api but also the version of the jjwt suite.

There are clients of this library that cannot always be depended on to be using the most recent version (Jenkins).

@ihrigb
Copy link
Contributor

ihrigb commented Nov 6, 2023

@bitwiseman I took a bit of time to update my pull request. It would now use reflective access to stay compatible with older versions. Please consider this as a draft suggestion, not tested yet. If we agree on this to be a way forward, I can still add tests.

@bitwiseman
Copy link
Member

@ihrigb
That is quite a bit of work, but it is great.
It needs more commenting for sure.

When you're done we also need to do a test to make sure it works. This might be something that we test once manually and make very clear that it should not be changed and how to test it if it must be changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants