Repository navigation
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The five-argument session-level user creation path still dereferences null usernames before the new validation runs.
Review effort: Balanced
Findings: 1
What changed in this PR
Prevents identity-provider mappings from creating unmanageable users with blank usernames.
Changes:
- Rejects unresolved or blank username templates and adds blank-aware fallbacks.
- Validates usernames during user creation and permits cache invalidation for legacy null usernames.
- Adds mapper tests and upgrade documentation.
| File | Description |
|---|---|
tests/base/.../IdpCreateUserIfUniqueAuthenticatorTest.java |
Tests unresolved broker templates. |
services/src/test/.../saml/UsernameTemplateMapperTest.java |
Adds SAML mapper coverage. |
services/src/test/.../oidc/mappers/UsernameTemplateMapperTest.java |
Adds OIDC mapper coverage. |
services/.../IdentityBrokerService.java |
Treats blank broker usernames as absent. |
services/.../AbstractTokenExchangeProvider.java |
Applies blank-aware token-exchange fallbacks. |
services/.../saml/mappers/UsernameTemplateMapper.java |
Rejects invalid SAML template results. |
services/.../oidc/mappers/UsernameTemplateMapper.java |
Rejects invalid OIDC template results. |
server-spi/.../UserProvider.java |
Updates the username validation contract. |
model/storage-private/.../UserStorageManager.java |
Rejects blank usernames. |
model/jpa/.../JpaUserProvider.java |
Validates usernames before persistence. |
model/infinispan/.../UserCacheManager.java |
Skips null username cache keys. |
model/infinispan/.../UserFullInvalidationEvent.java |
Supports legacy null usernames. |
docs/.../changes-26_8_1.adoc |
Documents the behavior change. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (StringUtil.isBlank(username)) { | ||
| throw new ModelException("Cannot create a user without a username"); | ||
| } |
Signed-off-by: Jyotirmoy Roy <jyotirmoyroy649@gmail.com>
Signed-off-by: Jyotirmoy Roy <jyotirmoyroy649@gmail.com>
Signed-off-by: Jyotirmoy Roy <jyotirmoyroy649@gmail.com>
6ccae19 to
8089c1c
Compare
| * | ||
| * @throws NullPointerException when username or realm is {@code null} | ||
| * @throws NullPointerException when realm is {@code null} | ||
| * @throws ModelException when username is {@code null} or empty |
There was a problem hiding this comment.
Thanks @ItsRoy69 for the PR! I need to look into it more closely, but it seems to me tahat the new IdentityBrokerException thrown by the username template mappers is handled gracefully in the broker login flow (IdentityBrokerService catches it at line 1062 and redirects to an error page), but in the token exchange flow AbstractTokenExchangeProvider.exchangeExternalToken() calls importUserFromExternalIdentity() at line 302 without catching IdentityBrokerException. If a mapper cannot resolve the username template during a token exchange, the exception propagates uncaught and surfaces as a 500 Internal Server Error instead of a 400 Bad Request like the other error
responses in that method.
Should importUserFromExternalIdentity (or exchangeExternalToken) catch IdentityBrokerException and throw a CorsErrorResponseException with 400 status to match the existing error handling pattern?
@sguilhen I'd welcome your eyes here, ptal
There was a problem hiding this comment.
@vramik good catch - confirmed. IdentityBrokerException is a plain RuntimeException, so once it escapes importUserFromExternalIdentity() (called from exchangeExternalToken() around line 302) it isn't caught anywhere in the token exchange path and falls through to KeycloakErrorHandler (the generic Throwable -> 500 mapper), unlike every other failure in exchangeExternalToken() which goes through CorsErrorResponseException and returns 400.
Suggested fix, reusing the same CorsErrorResponseException/OAuthErrorException.INVALID_REQUEST pattern already used elsewhere in this method:
UserModel user;
try {
user = importUserFromExternalIdentity(context);
} catch (IdentityBrokerException e) {
event.detail(Details.REASON, e.getMessage());
event.error(Errors.IDENTITY_PROVIDER_ERROR);
throw new CorsErrorResponseException(cors, OAuthErrorException.INVALID_REQUEST, e.getMessage(), Response.Status.BAD_REQUEST);
}It'd also be good to add a test that exercises token exchange against an IdP whose username template mapper can't resolve a username, to lock in the 400 response.
There was a problem hiding this comment.
🟡 Changes recommended
Unicode-whitespace usernames bypass broker and token-exchange fallbacks despite being rejected by storage, while the regression test does not reliably verify that failed login leaves no new account.
4 open issues
Review effort: Balanced
…vable username templates
There was a problem hiding this comment.
🟡 Changes recommended
When the auth server hosts differ, an issuer mismatch causes token validation to fail before the username mapper runs, so the test passes without exercising mapper rejection.
4 open issues
1 resolved since last review
Review effort: Balanced
| RealmResource consumerRealm = realmsResouce().realm(bc.consumerRealmName()); | ||
| IdentityProviderResource identityProviderResource = consumerRealm.identityProviders().get(bc.getIDPAlias()); |
There was a problem hiding this comment.
🟡 Changes recommended
The regression test can pass without exercising the username guard when hostnames differ, and the upgrade guidance incorrectly suggests unlinking a federated identity assigns a local username.
4 open issues
1 resolved since last review
Review effort: Balanced
|
|
||
| Review the username template of any identity provider mapper that does not resolve for every user, before upgrading. | ||
|
|
||
| No database constraint was added, so existing accounts are not rejected. Use the following query to find accounts that are affected, and assign them a username, for example by resetting their federated identity: |
Closes #53164
Identity provider mappers could resolve a username template to a blank value. The federated account was then created with that blank username and could neither be found by username nor edited or deleted, because invalidating the user in the cache required a username.
Username template mappers now fail with a descriptive error instead of resolving to a blank value,
addUserrejects blank usernames, and the username fallbacks in the first broker login and token exchange flows treat blank as absent. Accounts that were already created with a blank username can be edited and deleted again. An upgrading guide entry documents the behaviorchange.
Verified:
services(757 tests),model/infinispan(47),model/jpa(8) andmodel/storage-private(1) pass. The username template mapper tests were mutation-checked and fail if either guard is removed.