Repository navigation
Admin v2: Mask client secrets for view-only users - #52731
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The shared retrieval path can cause PATCH requests by configure-only users to persist the mask as the actual secret.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Masks client secrets in Admin API v2 GET responses for callers without manage permission.
Changes:
- Applies secret stripping to single-client and list responses.
- Adds authorization tests for view-only and manage users.
File summaries
| File | Description |
|---|---|
rest/admin-v2/services/src/main/java/org/keycloak/services/client/DefaultClientService.java |
Masks secrets in retrieved client representations. |
rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/client/v2/ClientApiV2AuthorizationTest.java |
Tests masking and privileged access. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
5725ee2 to
26b4ffa
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new list tests leave two response-backed streams unclosed.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/client/v2/ClientApiV2AuthorizationTest.java:237
- This second response-backed stream also needs to be closed with try-with-resources, matching the established usage in this test class and
ClientPaginationTest.java:93-96; collecting it withtoList()does not close it.
var manageClients = getClientsApi(manageClientsAdminClient).getClients().toList();
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
The generic masker also hides public SAML signing certificates, causing an unrelated v1 compatibility regression.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
rest/admin-v2/services/src/main/java/org/keycloak/services/client/DefaultClientService.java:132
StripSecretsUtilsV2also masksSAMLClientRepresentation.signingCertificate(StripSecretsUtilsV2.java:36-39), although this is a public certificate and v1's response masking leaves certificates intact (StripSecretsUtils.java:230-247). Thus a view-only GET now returns**********for unrelated SAML configuration; use a response-specific masker that redacts only actual client secrets.
This issue also appears on line 183 of the same file.
rest/admin-v2/services/src/main/java/org/keycloak/services/client/DefaultClientService.java:184
- This also applies the audit-oriented SAML certificate masking to every list item: view-only users lose the public
signingCertificate, unlike v1, even though the PR is intended to mask client secrets. Use a response-specific masker that preserves public certificate fields.
if (!permissions.clients().canManage(client)) {
rep = StripSecretsUtilsV2.stripSecrets(session, rep);
rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/client/v2/ClientApiV2AuthorizationTest.java:239
- The
Streamreturned by this REST call is not closed;toList()is terminal but does not invokeclose(). Wrap this branch in try-with-resources, as the othergetClients()calls in this file do, to release the underlying client response.
var manageClients = getClientsApi(manageClientsAdminClient).getClients().toList();
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Removing SAML masking breaks an existing admin-event test and changes its established behavior.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
| // SAMLClientRepresentation intentionally omitted — it has no secret fields. | ||
| // signingCertificate is a public X.509 cert, not a secret. |
0a3d4d4 to
b5247be
Compare
b5247be to
5b11a87
Compare
closes: keycloak#52731 Signed-off-by: Steve Hawkins <shawkins@redhat.com>
When a user lacks manage permission on a client, strip client secrets
from GET /clients/{client} and GET /clients responses to match v1 behavior.
Closes keycloak#52197
Signed-off-by: Shatrughan Rai <polyglot.dev@outlook.com>
# Conflicts:
# rest/admin-v2/services/src/main/java/org/keycloak/services/client/DefaultClientService.java
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Shatrughan Rai <polyglot.dev@outlook.com>
fixes an unclosed Stream in the authorization test by wrapping it in try-with-resources, matching the pattern used in tests. Fixes: keycloak#52197 Signed-off-by: Shatrughan Rai <polyglot.dev@outlook.com>
Signed-off-by: Steve Hawkins <shawkins@redhat.com>
5b11a87 to
8bafa6e
Compare
|
@poliglots I updated this pr to move the fix to a new central location where the type provider builds clients. @vmuzikar this should be ready for review |
When a user lacks manage permission on a client, strip client secrets from GET /clients/{client} and GET /clients responses to match v1 behavior.
Closes #52197