You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Fix NPE for incomplete deprecated credentials - #55181
Fixes#41640. Creating a user through the admin API with an incomplete deprecated credential (e.g. a password credential with a null value) threw a NullPointerException in RepresentationToModel.convertDeprecatedCredentialsFormat because nullable Integer fields auto unbox into the credential data constructors. This validates the fields first and throws a controlled ModelValidationException instead, for both the password and OTP paths, so the admin endpoint returns a 400 naming the missing fields instead of a 500. Requiring algorithm also moves a latent failure forward: on main a null algorithm imports without error and only fails at first login, when the password hash provider is looked up with a null id. Complete credentials convert exactly as before. Tests: 5 new tests in RepresentationToModelTest (reject incomplete password/OTP credentials, name the missing field, convert complete ones), 6/6 passing including the pre existing test. Built with Muse (Aurora) as co-author.
Follow up: createUser and updateUser now catch ModelValidationException separately, before the generic ModelException catch, so the named missing fields reach the client with the 400 instead of the generic message. Co-authored by Muse (Aurora).
Endpoint tests added: create with incomplete deprecated password and OTP credentials returns 400 naming the missing fields and leaves no user behind; update with an incomplete deprecated password credential returns 400 and leaves the profile unchanged.
The reason will be displayed to describe this comment to others. Learn more.
🟢 Approval recommended
There are no active review issues for the deprecated password and OTP credential validation changes.
No open issues 🎉
What changed in this PR
This PR addresses #41640 by validating deprecated password and OTP credentials before conversion, replacing null-related failures with controlled ModelException errors.
Changes:
Reject incomplete deprecated credentials before constructing credential data.
Add four tests covering rejection of incomplete credentials and conversion of complete credentials.
Turning the auto-unboxing NPE into a clear error is a good fix. Two thoughts, from reading the diff against main:
Name the missing field. "Invalid deprecated password credential format" doesn't say which field is missing, and someone fixing a large realm import would want to know, for example "...: hashIterations is missing". Requiring algorithm too is right, for a reason worth a line in the description. On main, a null algorithm imports without error and then fails at the first login: PasswordCredentialProvider looks up session.getProvider(PasswordHashProvider.class, algorithm), and DefaultKeycloakSession.getProvider builds List.of(clazz.getName(), id), which throws on a null id. So this PR also moves that failure from login time to import time.
The status code. It's worth confirming what the admin endpoint returns now. If ModelException reaches the client as a 400 through the usual mapper, good. If it still surfaces as a 500, the issue's symptom only changes its message.
(Edited: an earlier version of this comment guessed that a null algorithm falls back to a default. It doesn't, as above. Read only; not run. Written with help from an AI assistant, Claude Code, and checked by me against the code before posting.)
Thanks, both addressed. The error now names the missing fields, and it throws ModelValidationException so the endpoint returns a 400. Also noted the algorithm point in the description.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Admin user creation and update discard field-specific validation messages, so the resources need separate ModelValidationException handling and endpoint regression tests to ensure missing password and OTP fields are identified to clients.
No open issues 🎉
1 previously missed issue
In code that hasn't changed since last review
Admin API hides field-specific user validation errors
The admin API will return 400, but it will not name the missing fields as described: UsersResource.createUser catches ModelException (the superclass of ModelValidationException) and replaces this message with "Could not create user"; UserResource.updateUser similarly discards it. Handle ModelValidationException separately in those resources and add an endpoint regression test so the field-specific password and OTP errors reach the client.
Thanks for the review. I also addressed the Copilot follow up: createUser and updateUser were catching ModelException and replacing the message with a generic one, so the field names never reached the client. I added a separate ModelValidationException catch in both, before the ModelException catch, so the message passes through with the 400. Wanted to check with you whether that is the right place to handle it, or if you would rather the handling live elsewhere. On the endpoint regression test, the current test setup has no existing pattern for asserting admin client error responses on user creation, so I left it out for now. Happy to add one here or as a follow up if you want it.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
The new tests cover only the private converter, leaving admin endpoints’ field-specific HTTP 400 responses and create/update rollback behavior without regression coverage.
Added the endpoint regression tests. Create with incomplete deprecated password and OTP credentials asserts the 400 and the missing field message, and that no user is left behind. Update with an incomplete deprecated password credential asserts the 400 and that the profile change is rolled back.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #41640. Creating a user through the admin API with an incomplete deprecated credential (e.g. a password credential with a null value) threw a NullPointerException in
RepresentationToModel.convertDeprecatedCredentialsFormatbecause nullableIntegerfields auto unbox into the credential data constructors. This validates the fields first and throws a controlledModelValidationExceptioninstead, for both the password and OTP paths, so the admin endpoint returns a 400 naming the missing fields instead of a 500. Requiringalgorithmalso moves a latent failure forward: on main a null algorithm imports without error and only fails at first login, when the password hash provider is looked up with a null id. Complete credentials convert exactly as before. Tests: 5 new tests inRepresentationToModelTest(reject incomplete password/OTP credentials, name the missing field, convert complete ones), 6/6 passing including the pre existing test. Built with Muse (Aurora) as co-author.Follow up: createUser and updateUser now catch ModelValidationException separately, before the generic ModelException catch, so the named missing fields reach the client with the 400 instead of the generic message. Co-authored by Muse (Aurora).
Endpoint tests added: create with incomplete deprecated password and OTP credentials returns 400 naming the missing fields and leaves no user behind; update with an incomplete deprecated password credential returns 400 and leaves the profile unchanged.