Remove json-jwt, migrate ruby-jwt

[kristof-mattei]

This ensures we're using the same libs as in doorkeeper-jwt.

[phlegx]

@kristof-mattei this was my try time ago: https://github.com/phlegx/doorkeeper-openid_connect/tree/feature/change-to-jwt-gem

[kristof-mattei]

@phlegx ha, seems like I could've saved myself some time.

Couple of things:

1. I used the same thumbprint as the json-jwt gem, so less changes
2. No need for the normalized method to slice the data, the export filters that all out for you

[phlegx]

@kristof-mattei very nice! So we dont need to slice anymore. Great job.

[phlegx]

@toupeira looks good. What do you think? Time to move to ruby-jwt.

[toupeira]

@kristof-mattei thanks for this contribution, and apologies for the delay!

This LGTM, but I'm not really active in this project anymore and currently even lack a dev setup to test out these changes in a real app 😅 I don't really expect any problems though.

@nbulaj could you give this a look too? 🙏

[nbulaj]

LGTM 👍

Just changelog entry is missed.
Hope it won't break existing app setups

[kristof-mattei]

LGTM 👍

Just changelog entry is missed.

Hope it won't break existing app setups

I'll update the changelog later today. I'll also add the changes of some string to symbol.

In terms of impacting other apps: does this warrant a major bump?

[toupeira]

In terms of impacting other apps: does this warrant a major bump?

@kristof-mattei @nbulaj probably not since nothing should break (or even change), but one method I used in the past is to cut a prerelease gem, and then do a proper release after a few weeks, or once a user confirmed it to be working.

[kristof-mattei]

In terms of impacting other apps: does this warrant a major bump?

@kristof-mattei @nbulaj probably not since nothing should break (or even change), but one method I used in the past is to cut a prerelease gem, and then do a proper release after a few weeks, or once a user confirmed it to be working.

Do we consider the JWK object as public API? Or just the JSON output. If the former:

https://github.com/doorkeeper-gem/doorkeeper-openid_connect/pull/177/files#diff-ef96e398572c3741045bbd9d4de1a7ee7d3e2059638732dcdd17be0a4c33c6e2R23-R26

Notice how the keys when from strings to symbols (e.g. 'kty' to :kty) and some of the values went from symbols to strings (e.g. :RSA to 'RSA').

If we only consider the JSON output then the public API did not change. =

@nbulaj committed your recommendation. I just had to click 'commit suggestion' 😅

[nbulaj]

Do we consider the JWK object as public API? Or just the JSON output. If the former:

The problem could be if somebody relied on json-jwt under the hood (and done some configuring for it, or patched, or whatever). It;s super common case actually. Just like with Doorkeeper internals - I saw a lot of cases when somebody patched them and then we break some private API 😃

Anyway I think it's OK to bump a minor (not patch) version as we actually don't break public API, yes, so I hope upgrading the gem should be smooth. Thanks @kristof-mattei

[mschoenlaub]

Just out of curiosity, @nbulaj, how did you come to the conclusion that a public method on the Doorkeeper::OpenidConnect module (basically the top-level module of the gem) is not part of the public interface?
I'm just retroactively trying to understand the reasoning process here.

I'm asking partially to make more informed decisions for my own private projects in relation to semantic versioning in the ruby community, but partially also because we got bitten by that today ;-)

To me, that's a bit like saying Doorkeeper::OpenidConnect.configure isn't part of the public API, but maybe I'm missing something.

Additionally, from an integrator's perspective the changelog entry

Replace json-jwt with ruby-jwt to align with doorkeeper-jwt

is not exactly helpful. It just rang a bell because that same commit had broken another thing which had a covering test (due to our dependency on json-jwt-specific behaviour there)

The problem could be if somebody relied on json-jwt under the hood

I agree that under the assumption that if stuff breaks only for upstreams directly, like our first case above, a minor (maybe even a patch) bump would be fine. But in this case the return value of Doorkeeper::OpenidConnect.signing_key_normalized changed in a backwards-incompatible way.
Again, that is a public method on the top-level module of the gem.

I am aware that this might sound a bit like a rant and that maintaining a highly popular library in one's free time is not at all an easy task. So definitely thanks for that.