Disallow protocol change via Registration Access Token - #51419
Conversation
There was a problem hiding this comment.
🟡 Not ready to approve
Unrelated username-validation and formatting changes should be split from the focused Registration Access Token security fix.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Prevents Registration Access Tokens from changing a client’s protocol, addressing #51340. The PR also contains unrelated username-validation changes.
Changes:
- Rejects protocol changes authenticated by a Registration Access Token.
- Adds RAT rejection and admin-token success tests.
- Adds unrelated 255-character username validation and tests.
File summaries
| File | Description |
|---|---|
tests/base/src/test/java/org/keycloak/tests/organization/authentication/OrganizationAuthenticationTest.java |
Tests organization username length validation. |
tests/base/src/test/java/org/keycloak/tests/oid4vc/preauth/OID4VCredentialOfferPreAuthTest.java |
Removes whitespace. |
tests/base/src/test/java/org/keycloak/tests/oauth/DirectGrantInputValidationTest.java |
Tests direct-grant username validation. |
tests/base/src/test/java/org/keycloak/tests/forms/ResetPasswordTest.java |
Tests reset-password username length. |
tests/base/src/test/java/org/keycloak/tests/forms/LoginTest.java |
Tests login username validation. |
tests/base/src/test/java/org/keycloak/tests/client/ClientRegistrationTest.java |
Tests protocol-change authorization. |
services/src/main/java/org/keycloak/services/validation/Validation.java |
Defines a username length limit. |
services/src/main/java/org/keycloak/services/clientregistration/AbstractClientRegistrationProvider.java |
Rejects RAT-authenticated protocol changes. |
services/src/main/java/org/keycloak/organization/authentication/authenticators/browser/OrganizationAuthenticator.java |
Enforces username length. |
services/src/main/java/org/keycloak/authentication/authenticators/resetcred/ResetCredentialChooseUser.java |
Enforces username length during reset. |
services/src/main/java/org/keycloak/authentication/authenticators/directgrant/ValidateUsername.java |
Validates and trims direct-grant usernames. |
services/src/main/java/org/keycloak/authentication/authenticators/browser/AbstractUsernameFormAuthenticator.java |
Enforces username length during login. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟢 Ready to approve
The authorization check is correctly scoped and covered by focused integration tests.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟢 Ready to approve
The authorization check is narrowly scoped, symmetric across protocols, and covered by appropriate integration tests.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
rmartinc
left a comment
There was a problem hiding this comment.
Thanks @msdaly200! LGTM!
Just a nickpick for the tests. If you prefer to maintain like it is now just let me know and I can merge as it is now. But I prefer to use the registerClient as it is used in all the rest of methods.
|
thanks @rmartinc I've made those changes |
There was a problem hiding this comment.
🟢 Ready to approve
The focused validation correctly closes the reported authorization gap and is covered by integration tests.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
…cess token Signed-off-by: Marie Daly <marie.daly1@ibm.com>
…AT in DCR Signed-off-by: Marie Daly <marie.daly1@ibm.com>
rmartinc
left a comment
There was a problem hiding this comment.
Thanks @msdaly200 for the PR and @mabartos for the review!
Closes #51340
Added check to in AbstractClientRegistrationProvider.update() that rejects any attempt to change a client's protocol field when the caller authenticates with a Registration Access Token, preventing an OIDC client from being silently converted to SAML via DCR.