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

fix(model-server): make specifying a key ID optional #1039

Merged
merged 3 commits into from
Nov 6, 2024
Merged

Conversation

odzhychko
Copy link
Contributor

If no static key ID is configured, then the key ID specified in the JWT is used to verify the token. The key is looked up at the JWK URL with the key ID from the token.

This is a fix because it restores functionality that accidentally broke in version 8.14.0.

To be verified by reviewers

  • Relevant public API members have been documented
  • Documentation related to this PR is complete
    • Boundary conditions are documented
    • Exceptions are documented
    • Nullability is documented if used
  • Touched existing code has been extended with documentation if missing
  • Code is readable
  • New features and fixed bugs are covered by tests

Copy link
Contributor

github-actions bot commented Sep 6, 2024

Test Results

  179 files    179 suites   23m 52s ⏱️
1 012 tests 1 005 ✅ 7 💤 0 ❌
1 022 runs  1 015 ✅ 7 💤 0 ❌

Results for commit 069ee0b.

♻️ This comment has been updated with latest results.

@odzhychko
Copy link
Contributor Author

The build is broken because of new bug in Redocly.
See Redocly/redocly-cli#1717

Copy link
Contributor

github-actions bot commented Sep 6, 2024

JVM coverage report

Overall Project 53.92% -0.03%
Files changed 79.15%

File Coverage
AuthorizationConfig.kt 83.33% -7.25%
AuthorizationPlugin.kt 65.09% -1.33% 🍏

@odzhychko odzhychko force-pushed the fix/MODELIX-1016 branch 3 times, most recently from 1fa7789 to 837d64f Compare September 19, 2024 11:26
@odzhychko odzhychko marked this pull request as ready for review September 20, 2024 06:29
@odzhychko odzhychko requested review from slisson and a team as code owners September 20, 2024 06:29
Copy link
Contributor

@languitar languitar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not totally confident on the configuration changes, which are pretty hard to understand given the already existing complexity. Here's just my two cents of things I found while reading.

If no static key ID is configured, then the key ID specified in the JWT is used to verify the token.
The key is looked up at the JWK URL with the key ID from the token.

This is a fix because it restores functionality that accidentally broke in version 8.14.0.
The logic determining the verifier was a duplicate of the logic used in the additional validation.

The logic from the validation (aka `ModelixAuthorizationConfig,nullIfInvalid`) is also used elsewhere.
For example, it is used in the workspaces through the modelix-authorization lib.
So it was reused in a custom verifier.

Calling `ModelixAuthorizationConfig.nullIfInvalid` again in `validate` is redundant.
It is now only used to create a custom principal object.
Copy link
Member

@slisson slisson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested modelix.kubernetes with these changes.

@slisson slisson enabled auto-merge November 6, 2024 13:12
@slisson slisson merged commit 4119de1 into main Nov 6, 2024
19 checks passed
@slisson slisson deleted the fix/MODELIX-1016 branch November 6, 2024 14:20
@slisson
Copy link
Member

slisson commented Nov 6, 2024

🎉 This PR is included in version 9.1.3 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants