-
Notifications
You must be signed in to change notification settings - Fork 114
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
Remove json-jwt, migrate ruby-jwt #177
Remove json-jwt, migrate ruby-jwt #177
Conversation
@kristof-mattei this was my try time ago: https://github.com/phlegx/doorkeeper-openid_connect/tree/feature/change-to-jwt-gem |
@phlegx ha, seems like I could've saved myself some time. Couple of things:
|
@kristof-mattei very nice! So we dont need to slice anymore. Great job. |
@toupeira looks good. What do you think? Time to move to |
@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? 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? |
@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. |
Co-authored-by: Markus Koller <markus-koller@gmx.ch>
Do we consider the JWK object as public API? Or just the JSON output. If the former: Notice how the keys when from strings to symbols (e.g. 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' 😅 |
The problem could be if somebody relied on 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 |
doorkeeper-gem#177 was actually released with 1.8.4.
The switch from the `json-jwt` to `jwt` gem in doorkeeper-gem#177 changed the default `kid` generation from RFC 7638 (https://www.rfc-editor.org/rfc/rfc7638) to a format based on the SHA256 digest of the key elements. However, clients may fail if the the `kid` generated by `IdToken` does not match a key listed in JWKS discovery endpoint, which may be implemented by the application using RFC 7638-based `kid` values. To restore the previous behavior, applications have to set a global setting: ``` JWT.configuration.jwk.kid_generator_type = :rfc7638_thumbprint ``` However, relying on this global setting is not ideal since other keys may depend on the legacy `kid` values. In keeping with semantic versioning, restore the `kid` generation to RFC 7638. Whether this should be customizable can be discussed later. Closes doorkeeper-gem#193
The switch from the `json-jwt` to `jwt` gem in doorkeeper-gem#177 changed the default `kid` generation from RFC 7638 (https://www.rfc-editor.org/rfc/rfc7638) to a format based on the SHA256 digest of the key elements. However, clients may fail if the the `kid` generated by `IdToken` does not match a key listed in JWKS discovery endpoint, which may be implemented by the application using RFC 7638-based `kid` values. To restore the previous behavior, applications have to set a global setting: ``` JWT.configuration.jwk.kid_generator_type = :rfc7638_thumbprint ``` However, relying on this global setting is not ideal since other keys may depend on the legacy `kid` values. In keeping with semantic versioning, restore the `kid` generation to RFC 7638. Whether this should be customizable can be discussed later. Closes doorkeeper-gem#193
The switch from the `json-jwt` to `jwt` gem in doorkeeper-gem#177 changed the default `kid` generation from RFC 7638 (https://www.rfc-editor.org/rfc/rfc7638) to a format based on the SHA256 digest of the key elements. However, clients may fail if the the `kid` generated by `IdToken` does not match a key listed in JWKS discovery endpoint, which may be implemented by the application using RFC 7638-based `kid` values. To restore the previous behavior, applications have to set a global setting: ``` JWT.configuration.jwk.kid_generator_type = :rfc7638_thumbprint ``` However, relying on this global setting is not ideal since other keys may depend on the legacy `kid` values. In keeping with semantic versioning, restore the `kid` generation to RFC 7638. Whether this should be customizable can be discussed later. Closes doorkeeper-gem#193
The switch from the `json-jwt` to `jwt` gem in doorkeeper-gem#177 changed the default `kid` generation from RFC 7638 (https://www.rfc-editor.org/rfc/rfc7638) to a format based on the SHA256 digest of the key elements. However, clients may fail if the the `kid` generated by `IdToken` does not match a key listed in JWKS discovery endpoint, which may be implemented by the application using RFC 7638-based `kid` values. To restore the previous behavior, applications have to set a global setting: ``` JWT.configuration.jwk.kid_generator_type = :rfc7638_thumbprint ``` However, relying on this global setting is not ideal since other keys may depend on the legacy `kid` values. In keeping with semantic versioning, restore the `kid` generation to RFC 7638. Whether this should be customizable can be discussed later. Closes doorkeeper-gem#193
Just out of curiosity, @nbulaj, how did you come to the conclusion that a public method on the 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 Additionally, from an integrator's perspective the changelog entry
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
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 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. |
This ensures we're using the same libs as in
doorkeeper-jwt
.