Skip to content

#50798 Apply protocol defaults when creating clients through Admin AP… - #50801

Merged
vmuzikar merged 6 commits into
keycloak:mainfrom
arnabnandy7:fix/client-v2-creation
Jul 21, 2026
Merged

#50798 Apply protocol defaults when creating clients through Admin AP…#50801
vmuzikar merged 6 commits into
keycloak:mainfrom
arnabnandy7:fix/client-v2-creation

Conversation

@arnabnandy7

@arnabnandy7 arnabnandy7 commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Apply protocol defaults when creating clients through Admin API v2

Reuse LoginProtocolFactory client defaults in the Admin API v2 creation path. Preserve omitted SAML fields and add interoperability coverage for OIDC backchannel logout and SAML creation defaults.

Closes #50798

Splits from #50894

Copilot AI review requested due to automatic review settings July 10, 2026 20:04
@arnabnandy7
arnabnandy7 requested review from a team as code owners July 10, 2026 20:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR ensures Admin API v2 client creation applies the same protocol-specific defaults as the legacy (v1-based) creation flow by reusing LoginProtocolFactory.setupClientDefaults, and adds interoperability tests to verify key defaults for minimal OIDC and SAML client creation.

Changes:

  • Invoke LoginProtocolFactory.setupClientDefaults(...) during Admin API v2 client creation to apply protocol defaults before validation.
  • Preserve “omitted vs explicitly set” semantics for specific SAML fields so protocol defaults are applied correctly.
  • Add interop tests asserting OIDC backchannel logout and SAML creation defaults are applied when creating minimal clients via v2.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
rest/admin-v2/services/src/main/java/org/keycloak/services/client/DefaultClientService.java Apply protocol factory defaults during v2 client creation (with SAML omission-preservation for primitive-backed fields).
rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/client/v2/InteropTest.java Add minimal OIDC/SAML creation interop tests to validate protocol defaults are applied.

@shawkins

Copy link
Copy Markdown
Contributor

Thank you for the PR, the other thing still missing is

if (Profile.isFeatureEnabled(Profile.Feature.CLIENT_TYPES) && resourceRep.getType() != null) {

@arnabnandy7

Copy link
Copy Markdown
Contributor Author

Thank you for the PR, the other thing still missing is

if (Profile.isFeatureEnabled(Profile.Feature.CLIENT_TYPES) && resourceRep.getType() != null) {

I'll look into it

Copilot AI review requested due to automatic review settings July 13, 2026 18:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

@keycloak-github-bot keycloak-github-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unreported flaky test detected, please review

@keycloak-github-bot

Copy link
Copy Markdown

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.forms.BruteForceTest#testExceedMaxTemporaryLockouts

Keycloak CI - Base IT (5)

org.opentest4j.AssertionFailedError: Expected error event ==> expected: <true> but was: <false>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63)
	at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36)
...

Report flaky test

Copilot AI review requested due to automatic review settings July 13, 2026 21:22
@arnabnandy7
arnabnandy7 requested a review from a team as a code owner July 13, 2026 21:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Copilot AI review requested due to automatic review settings July 13, 2026 21:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Copilot AI review requested due to automatic review settings July 13, 2026 21:46
@arnabnandy7

Copy link
Copy Markdown
Contributor Author

Addressed the remaining client-type creation path and follow-up review findings in 2f77c90. Admin API v2 now preserves omitted client-type defaults, applies explicit values through type validation, restores ordinary service-account flow handling, and exposes the requested type to client policy contexts.

@arnabnandy7
arnabnandy7 force-pushed the fix/client-v2-creation branch from 2f77c90 to 2c7d27a Compare July 13, 2026 21:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 11 comments.

Copilot AI review requested due to automatic review settings July 13, 2026 21:52
@arnabnandy7

Copy link
Copy Markdown
Contributor Author

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

the flaky test is not caused by this PR

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 11 comments.

@michalvavrik michalvavrik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for your PR.

I suspect Copilot comments regarding NPEs (this one #50801 (comment)) and PATCH/PUT with explicit null on typed clients (this one #50801 (comment)) are right. The fact that PR CI is not failing means there is gap in test coverage.

Please have a look if there is something on it.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

shawkins
shawkins previously approved these changes Jul 16, 2026

@shawkins shawkins left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thank you @arnabnandy7

edit - removed the approval due to the failing test. Looking into that now.

@shawkins
shawkins self-requested a review July 16, 2026 13:45
@michalvavrik

Copy link
Copy Markdown
Member

That PR CI failure is suspicious, I haven't checked yet (I can do it later), but did you investigate @arnabnandy7 if it is related or flaky?


Error:  Failures: 
Error:    ClientApiV2Test.createJwtClientWithoutSecret:1103 
Expected: null
     but: was "H4691osA5ZalXznmS0TQ7Ee4yZdNVnzfZhFxilQr4w3A9QFviUDrsxsTSutEOyAIfZa0E9dud7ZqZODhvIOhl4"

@arnabnandy7

arnabnandy7 commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

That PR CI failure is suspicious, I haven't checked yet (I can do it later), but did you investigate @arnabnandy7 if it is related or flaky?


Error:  Failures: 
Error:    ClientApiV2Test.createJwtClientWithoutSecret:1103 
Expected: null
     but: was "H4691osA5ZalXznmS0TQ7Ee4yZdNVnzfZhFxilQr4w3A9QFviUDrsxsTSutEOyAIfZa0E9dud7ZqZODhvIOhl4"

Not related to my changes.

@shawkins

shawkins commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Not related to my changes.

Unfortunately it is related to the changes here. The OIDCLoginProtocolFactory will generate a secret for all non-public clients. However the v2 logic expects that to happen only if specific auth methods are used.

To keep that convention, either the generated secret will need cleared prior to the v2 logic setting a generated one, or we'll need to only conditionally call setupClientDefaults for non-OIDC clients and make sure that any needed defaulting logic from OIDCLoginProtocolFactory is forked.

@arnabnandy7

Copy link
Copy Markdown
Contributor Author

Not related to my changes.

Unfortunately it is related to the changes here. The OIDCLoginProtocolFactory will generate a secret for all non-public clients. However the v2 logic expects that to happen only if specific auth methods are used.

To keep that convention, either the generated secret will need cleared prior to the v2 logic setting a generated one, or we'll need to only conditionally call setupClientDefaults for non-OIDC clients and make sure that any needed defaulting logic from OIDCLoginProtocolFactory is forked.

Sorry I was travelling so I thought it was referred to previous CI failure. As I can see the issue will add a fix to make createJwtClientWithoutSecret pass while preserving automatic secret generation for client-secret and client-secret-jwt

Signed-off-by: Arnab Nandy <arnab_nandy7@yahoo.com>
Copilot AI review requested due to automatic review settings July 16, 2026 15:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings July 16, 2026 15:53
@arnabnandy7

Copy link
Copy Markdown
Contributor Author

@shawkins @michalvavrik your review is required

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

@arnabnandy7
arnabnandy7 requested a review from michalvavrik July 16, 2026 17:14
Copilot AI review requested due to automatic review settings July 19, 2026 13:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Signed-off-by: Arnab Nandy <arnab_nandy7@yahoo.com>
Copilot AI review requested due to automatic review settings July 19, 2026 14:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

@shawkins shawkins left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks @arnabnandy7

Let's get another @keycloak/cloud-native review, and then this can be merged.

&& oidcClient.getAuth() != null
&& !isClientSecret(oidcClient.getAuth().getMethod())
&& isBlank(oidcClient.getAuth().getSecret())) {
// OIDCLoginProtocolFactory generates a secret for every confidential client, while Admin API v2

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

but you do this even when factory == null, how should I understand this comment then?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm ok with leaving it as is, but it's certainly fine to move this block into the preceding if (factory != null) block.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, not a bug, I I wasn't certain that it can't have side effects which brought me to reading the comment. At least the comment is wrong, but I am fine with it as is.

@vmuzikar
vmuzikar merged commit 1aac3fe into keycloak:main Jul 21, 2026
91 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Missing client creation logic in client v2

6 participants