8000 Disabled organization set on session context during broker login call… by vramik · Pull Request #52832 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

Disabled organization set on session context during broker login call… - #52832

Merged
pedroigor merged 1 commit into
keycloak:mainfrom
vramik:52831-disabled-orgs-in-session-context
Sep 24, 2026
Merged

Disabled organization set on session context during broker login call…#52832
pedroigor merged 1 commit into
keycloak:mainfrom
vramik:52831-disabled-orgs-in-session-context

Conversation

@vramik
@vramik vramik commented Sep 16, 2026
Copy link
Copy Markdown
Contributor

…back

Closes #52831

@vramik
vramik force-pushed the 52831-disabled-orgs-in-session-context branch 2 times, most recently from e63ea8c to fd762cb Compare September 21, 2026 20:10
@vramik
vramik marked this pull request as ready for review September 21, 2026 20:10
@vramik
vramik requested review from a team as code owners September 21, 2026 20:10
Copilot AI balanced review requested due to automatic review settings September 21, 2026 20:10
Copilot AI reviewed Sep 21, 2026
View reviewed changes
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.

Copilot review overview

🟡 Changes recommended

The regression test does not exercise either changed production branch and belongs in the new test framework.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 Medium severity · 2 Low severity

Open (3)
What changed in this PR

Prevents disabled organizations from propagating through broker login context or OIDC token claims.

Changes:

  • Guards broker organization context assignment.
  • Filters disabled organizations from single-valued claims.
  • Adds regression coverage.
File Description
LoginActionsService.java Validates organization status before setting context.
OrganizationMembershipMapper.java Omits disabled organization claims.
AbstractBrokerSelfRegistrationTest.java Adds disabled-organization login coverage.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread services/src/main/java/org/keycloak/services/resources/LoginActionsService.java Resolved
Comment thread .../main/java/org/keycloak/organization/protocol/mappers/oidc/OrganizationMembershipMapper.java Resolved
Comment thread ...test/java/org/keycloak/testsuite/organization/broker/AbstractBrokerSelfRegistrationTest.java Resolved
@vramik
vramik requested a review from pedroigor September 23, 2026 08:11
martin-kanis
martin-kanis suggested changes Sep 23, 2026
View reviewed changes
@martin-kanis martin-kanis 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.

@vramik Thanks for the PR.

Could you update the test to reproduce the broker-callback issue? I commented out both production changes, and the test still passes. The password login after disabling the organization bypasses the changed broker flow.

@vramik
Disabled organization set on session context during broker login call…
77701cc 
…back

Closes keycloak#52831

Signed-off-by: vramik <vramik@redhat.com>
Copilot AI review requested due to automatic review settings September 23, 2026 11:59
@vramik
vramik force-pushed the 52831-disabled-orgs-in-session-context branch from fd762cb to 77701cc Compare September 23, 2026 11:59
Copilot AI reviewed Sep 23, 2026
View reviewed changes
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.

Copilot review overview

🟡 Changes recommended

Disabled organizations can still cause token failure or persist through authentication-session resolution, and regression coverage is missing.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity · 1 Medium severity · 2 Low severity

Open (4)

Comment thread .../main/java/org/keycloak/organization/protocol/mappers/oidc/OrganizationMembershipMapper.java
Comment on lines +167 to +169
OrganizationModel org = organizations.get(0);
if (!org.isEnabled()) {
return null;
@vramik
vramik commented Sep 23, 2026 •
edited
Loading
Copy link
Copy Markdown
Contributor Author

@vramik Thanks for the PR.

Could you update the test to reproduce the broker-callback issue? I commented out both production changes, and the test still passes. The password login after disabling the organization bypasses the changed broker flow.

@martin-kanis thank you, I ended up removing the test altogether. Here's why:

  • Fix 1 (configureOrganization): Sets the org on session.getContext() during the broker auth flow. But this context is per-request — it doesn't survive to the token endpoint request where the mapper runs. By the time the client exchanges the auth code for a token, session.getContext().getOrganization() is null. So the mapper falls through to resolveFromRequestedScopes(), which calls OrganizationScope.resolveOrganizations() — and that method already filters disabled orgs.
  • Fix 2 (mapper resolveValue): The isEnabled() checks in resolveValue() only matter when a disabled org is already in the organizations list passed to the method. But for the initial code-to-token exchange, the list comes from resolveFromRequestedScopes(), which already filtered disabled orgs. So the mapper fix never triggers either.

Both fixes are defense-in-depth. They protect against future code changes that might bypass the outer filters (e.g., a new authenticator that reads the context org without checking isEnabled(), or a new token generation path that sets a disabled org on context). The test verified the end-to-end behavior (disabled org not in token after broker login) but I can't isolate either fix individually because OrganizationScope.resolveOrganizations() is the layer that actually prevents the leak in the current code.

@vramik
vramik requested a review from martin-kanis September 23, 2026 12:04
@keycloak-github-bot keycloak-github-bot Bot mentioned this pull request Sep 23, 2026
Closed
@pedroigor
pedroigor merged commit f4d8e33 into keycloak:main Sep 24, 2026
94 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@pedroigor pedroigor pedroigor approved these changes
+1 more reviewer
@martin-kanis martin-kanis martin-kanis approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

flaky-test team/core-iam

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Disabled organization set on session context during broker login callback

4 participants

@vramik @pedroigor @martin-kanis
0