search

doorkeeper-gem / doorkeeper-openid_connect

OpenID Connect extension for Doorkeeper
MIT License
173 stars 114 forks source link

Remove json-jwt, migrate ruby-jwt #177

Closed kristof-mattei closed 1 year ago

kristof-mattei commented 1 year ago

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

phlegx commented 1 year ago

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

kristof-mattei commented 1 year ago

@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 commented 1 year ago

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

phlegx commented 1 year ago

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

toupeira commented 1 year ago

@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 :sweat_smile: I don't really expect any problems though.

@nbulaj could you give this a look too? :pray:

kristof-mattei commented 1 year ago

LGTM :+1:

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 :smiley:

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 commented 1 year ago

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.