Repository navigation
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The focused implementation satisfies the issue and includes appropriate regression coverage.
0 open findings
What changed in this PR
Sets the OpenID Connect protocol on SSF client scopes so they can be assigned through the Admin Console.
Changes:
- Sets the protocol on newly provisioned SSF scopes.
- Backfills missing protocols on existing scopes.
- Adds regression coverage for creation and backfilling.
| File | Description |
|---|---|
ssf/transmitter/.../SsfScopes.java |
Sets and backfills the OpenID Connect protocol. |
ssf/tests/.../SsfTransmitterEnableCreatesClientScopesTests.java |
Verifies protocol provisioning and backfilling. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Thanks for the fix. Setting the protocol is the right idea, but it exposes a problem that SSF IT catches. The failure isn't a flake: it also failed on the Surefire reruns.
SsfTransmitterStreamManagementTests.testCreateStreamRequiresSsfManageScope:186
client without ssf.manage should not be able to obtain a token with the ssf.manage scope
==> expected: <null> but was: <token>
Cause: addClientScopeIfAbsent still calls realm.addDefaultClientScope(scope, false), so ssf.read and ssf.manage are realm-level default optional scopes. When a new client is created, AbstractLoginProtocolFactory.addDefaultClientScopes gives it every realm default scope whose protocol matches its own:
- Before this PR: the protocol was
null, so nothing matched and the realm-default registration had no effect. - With this PR: every new OIDC client in an SSF-enabled realm automatically gets both
ssf.readandssf.manageas optional scopes, so it can requestssf.manage. That breaks the read-only vs. read-write receiver split the failing test checks. Because of the backfill, existing realms are affected too once the bootstrap runs again.
Suggestion: stop registering the SSF scopes as realm default optional scopes, and let admins assign them to each receiver client explicitly. That's the setup #51818 describes as expected. This would mean:
- dropping or reversing the "registered as optional realm client scopes" assertion in
SsfTransmitterEnableCreatesClientScopesTests(from #49236); - on the backfill path, probably also removing the scopes from the realm's default optional list. Otherwise realms that already have them hit the same problem as soon as the protocol is filled in.
There was a problem hiding this comment.
Requesting changes for the scope-inheritance regression described in my review comment above (SSF IT failure in testCreateStreamRequiresSsfManageScope).
The ssf.read and ssf.manage client scopes were provisioned without a protocol. The admin console's "Add client scope" dialog filters candidate scopes by protocol, so the scopes could not be assigned to a receiver client through the UI. Set the protocol to openid-connect when the scopes are created and backfill it on existing scopes whenever the SSF bootstrap runs again, e.g. when the transmitter is re-enabled on a realm. Fixes keycloak#51818 Signed-off-by: Thomas Darimont <thomas.darimont@googlemail.com>
…oak#51818) Setting the openid-connect protocol on ssf.read and ssf.manage made the realm default optional registration effective: every new OIDC client in an SSF realm inherited both scopes and could request ssf.manage. Receivers get the scopes assigned explicitly, so the bootstrap no longer registers them as realm defaults. Scopes provisioned by earlier versions without a protocol are backfilled and removed from the realm defaults in the same step. Scopes that already carry a protocol are left untouched. Signed-off-by: Thomas Darimont <thomas.darimont@googlemail.com>
9f2cc35 to
52ae287
Compare
|
Thanks @ssilvert I think your suggestion makes sense. I just adapted the code to not register the ssf client scopes as realm default client scopes anymore. |
There was a problem hiding this comment.
Latest changes look good. Approved.
…ycloak#53879) * Set openid-connect protocol on SSF client scopes (keycloak#51818) The ssf.read and ssf.manage client scopes were provisioned without a protocol. The admin console's "Add client scope" dialog filters candidate scopes by protocol, so the scopes could not be assigned to a receiver client through the UI. Set the protocol to openid-connect when the scopes are created and backfill it on existing scopes whenever the SSF bootstrap runs again, e.g. when the transmitter is re-enabled on a realm. Fixes keycloak#51818 Signed-off-by: Thomas Darimont <thomas.darimont@googlemail.com> * SSF: Do not register SSF client scopes as realm default scopes (keycloak#51818) Setting the openid-connect protocol on ssf.read and ssf.manage made the realm default optional registration effective: every new OIDC client in an SSF realm inherited both scopes and could request ssf.manage. Receivers get the scopes assigned explicitly, so the bootstrap no longer registers them as realm defaults. Scopes provisioned by earlier versions without a protocol are backfilled and removed from the realm defaults in the same step. Scopes that already carry a protocol are left untouched. Signed-off-by: Thomas Darimont <thomas.darimont@googlemail.com> --------- Signed-off-by: Thomas Darimont <thomas.darimont@googlemail.com>
The ssf.read and ssf.manage client scopes were provisioned without a protocol. The admin console's "Add client scope" dialog filters candidate scopes by protocol, so the scopes could not be assigned to a receiver client through the UI.
Set the protocol to openid-connect when the scopes are created and backfill it on existing scopes whenever the SSF bootstrap runs again, e.g. when the transmitter is re-enabled on a realm.
Fixes #51818