Repository navigation
Conversation
Unreported flaky test detectedIf 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.adapter.servlet.SAMLClockSkewAdapterTest#testTokenTimeIsValidKeycloak CI - Adapter IT Strict Cookies org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#employeeSigPostNoIdpKeyTestNoKeyNameInKeyInfoKeycloak CI - Adapter IT Strict Cookies |
vramik
force-pushed
the
48907-multi-org-data-model
branch
2 times, most recently
from
August 28, 2026 07:52
41d09a9 to
8c65cee
Compare
Unreported flaky test detectedIf 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.broker.KcOidcBrokerTest#loginWithExistingUserWithBruteForceEnabledKeycloak CI - Java Distribution IT (windows-latest - temurin - 21) |
vramik
force-pushed
the
48907-multi-org-data-model
branch
3 times, most recently
from
August 31, 2026 12:24
1e50626 to
5ed5f31
Compare
vramik
marked this pull request as ready for review
August 31, 2026 13:51
Contributor
There was a problem hiding this comment.
Pull request overview
Introduces the V3 organization domain and identity-provider data model, including persistence, migration, routing, caching, and updated tests.
Changes:
- Adds normalized domain and organization–IdP relationships.
- Moves IdP routing and auto-redirect settings onto domains.
- Migrates legacy schemas, imports, and test fixtures.
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
testsuite/.../OrganizationPostBrokerLoginTest.java |
Updates domain routing setup. |
testsuite/.../OrganizationIdentityProviderTest.java |
Adapts IdP tests to domain routing. |
testsuite/.../AbstractBrokerSelfRegistrationTest.java |
Migrates broker-flow test configuration. |
testsuite/.../OrganizationThemeTest.java |
Disables redirects through domain settings. |
testsuite/.../admin/AbstractOrganizationTest.java |
Updates organization test creation. |
tests/.../OrganizationMemberTest.java |
Updates member routing setup. |
tests/.../OrganizationMemberIdpLinkTest.java |
Configures domain-to-IdP links. |
tests/.../OrganizationCacheTest.java |
Removes obsolete IdP configuration. |
tests/.../OrganizationAdminRolesPermissionsTest.java |
Removes legacy domain configuration. |
tests/.../OrganizationTest.java |
Updates domain resolution tests. |
tests/.../admin/AbstractOrganizationTest.java |
Updates organization fixtures. |
services/.../OrganizationMemberValidator.java |
Validates domains via domain routing. |
services/.../OrganizationAuthenticator.java |
Redirects using domain metadata. |
server-spi/.../OrganizationModel.java |
Removes legacy routing constants. |
server-spi/.../OrganizationDomainModel.java |
Adds IdP alias and redirect fields. |
server-spi-private/.../RepresentationToModel.java |
Maps and cleans domain configuration. |
server-spi-private/.../ModelToRepresentation.java |
Exports domain routing fields. |
server-spi-private/.../MigrationUtils.java |
Defines legacy migration keys. |
model/storage-private/.../MigrateTo26_8_0.java |
Migrates imported realm routing. |
model/jpa/.../jpa-changelog-26.8.0.xml |
Defines normalized schema migration. |
model/jpa/.../default-persistence.xml |
Registers the join entity. |
model/jpa/.../OrganizationAdapter.java |
Persists global domain relationships. |
model/jpa/.../JpaOrganizationProvider.java |
Persists organization–IdP links. |
model/jpa/.../JpaRealmProvider.java |
Cleans links during realm deletion. |
model/jpa/.../JpaIdentityProviderStorageProvider.java |
Queries and synchronizes organization links. |
model/jpa/.../OrganizationIdentityProviderEntity.java |
Adds the organization–IdP entity. |
model/jpa/.../OrganizationEntity.java |
Adds M:N domain and IdP mappings. |
model/jpa/.../OrganizationDomainEntity.java |
Converts domains into global entities. |
model/jpa/.../IdentityProviderEntity.java |
Removes the direct organization column. |
model/jpa/.../JpaUpdate26_8_0_PopulateOrgDomainJoinMigration.java |
Migrates domain join rows. |
model/jpa/.../JpaUpdate26_8_0_PopulateDomainTableMigration.java |
Populates global domains. |
model/jpa/.../JpaUpdate26_8_0_DomainIdpRoutingMigration.java |
Migrates IdP links and routing. |
model/infinispan/.../InfinispanOrganizationProvider.java |
Adds IdP cache invalidation. |
model/infinispan/.../InfinispanIdentityProviderStorageProvider.java |
Exposes alias cache keys. |
core/.../OrganizationDomainRepresentation.java |
Exposes domain routing through REST. |
Suppressed comments (1)
model/jpa/src/main/java/org/keycloak/connections/jpa/updater/liquibase/custom/JpaUpdate26_8_0_DomainIdpRoutingMigration.java:170
- Exclusions are compared only to existing organization-domain row names. For the former supported setup
ANY+ org domain*.example.com+ exclusion*.sub.example.com, no exclusion row exists, so the broad domain is routed and excluded users are redirected; migration must create/link unrouted DOMAIN rows for exclusion patterns.
String domainId = domRs.getString(1);
String domainName = domRs.getString(2);
if (!excludedDomains.contains(domainName)) {
statements.add(new UpdateStatement(null, null, domainTable)
.addNewColumnValue("IDP_ID", idpInternalId)
.addNewColumnValue("AUTO_REDIRECT", autoRedirect)
.setWhereClause("ID=?")
.addWhereParameter(domainId));
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
vramik
force-pushed
the
48907-multi-org-data-model
branch
from
September 1, 2026 10:11
5ed5f31 to
8d1b99e
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 35 changed files in this pull request and generated 2 comments.
Suppressed comments (4)
model/jpa/src/main/java/org/keycloak/organization/jpa/OrganizationAdapter.java:217
- The V3 model is meant to allow one realm domain to be claimed by multiple organizations, but this reuse path is unreachable for that case because
validateDomainstill rejects a domain owned by another org (lines 304–309). The validation and single-organization lookup contract need to be updated so a second org can link to the existingDOMAINrow.
try {
domainEntity = em.createNamedQuery("getDomainByRealmAndName", OrganizationDomainEntity.class)
.setParameter("realmId", realm.getId())
.setParameter("name", model.getName())
.getSingleResult();
model/jpa/src/main/java/org/keycloak/organization/jpa/OrganizationAdapter.java:207
- Removing a shared domain from one organization deletes the global
DOMAINentity, and the FK cascade consequently removes that domain from every organization. Remove only this organization’s join row and delete the domain entity only after confirming it is orphaned.
this.entity.removeDomain(domainEntity);
domainEntity.setIdentityProvider(null);
// TODO: when separate domain management allows M:N sharing, only remove the join row
// and rely on orphan cleanup instead of deleting the DOMAIN row outright
em.remove(domainEntity);
model/storage-private/src/main/java/org/keycloak/migration/migrators/MigrateTo26_8_0.java:90
- Import migration only preserves exclusions that exactly equal an existing organization domain. A more-specific exclusion under a wildcard domain is dropped when the old config is removed, causing imported realms to auto-redirect previously excluded addresses; add each exclusion as an explicit unrouted organization domain.
Set<OrganizationDomainModel> result = new HashSet<>();
for (OrganizationDomainModel domain : currentDomains) {
if ("ANY".equals(orgDomain)) {
if (!excludedDomains.contains(domain.getName())) {
result.add(new OrganizationDomainModel(domain.getName(), domain.isVerified(), idp.getAlias(), autoRedirect));
} else {
result.add(domain);
model/jpa/src/main/java/org/keycloak/organization/jpa/JpaOrganizationProvider.java:721
- This explicit 1:1 guard rejects linking an IdP already used by another organization, so the new
ORG_IDENTITY_PROVIDERrelationship is not actually M:N as required. The association APIs and singularorganizationIdcompatibility paths need to support all links rather than reject the second one.
// TODO: remove this 1:1 check when M:N is fully supported
String existingOrgId = identityProvider.getOrganizationId();
if (existingOrgId != null) {
throw new ModelValidationException("Identity provider already associated with a different organization");
Unreported flaky test detectedIf 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.MultipleTabsLoginTest#multipleTabsParallelLoginTestWithAuthSessionExpiredAndResetPasswordClickKeycloak CI - Forms IT (chrome) org.keycloak.testsuite.forms.MultipleTabsLoginTest#testInjectValidRedirectUriButIgnoreResponseTypeAndModeKeycloak CI - Forms IT (chrome) org.keycloak.testsuite.forms.MultipleTabsLoginTest#testRedirectToCorrectUrlAfterAuthSessionExpirationKeycloak CI - Forms IT (chrome) org.keycloak.testsuite.forms.MultipleTabsLoginTest#testLoginAfterLogoutFromDifferentTabKeycloak CI - Forms IT (chrome) org.keycloak.testsuite.forms.MultipleTabsLoginTest#loginActionWithoutExecutionKeycloak CI - Forms IT (chrome) org.keycloak.testsuite.forms.MultipleTabsLoginTest#multipleTabsParallelLoginTestWithAuthSessionExpiredAndRequiredActionKeycloak CI - Forms IT (chrome) org.keycloak.testsuite.forms.MultipleTabsLoginTest#expiredAuthenticationAction_expiredCodeExpiredExecutionKeycloak CI - Forms IT (chrome) org.keycloak.testsuite.forms.MultipleTabsLoginTest#testWithAuthSessionExpiredInTheMiddle_badRedirectUriKeycloak CI - Forms IT (chrome) org.keycloak.testsuite.forms.MultipleTabsLoginTest#testEmptyBaseUrlKeycloak CI - Forms IT (chrome) org.keycloak.testsuite.forms.MultipleTabsLoginTest#testRestartFailureWithDifferentClientAfterAuthSessionExpirationKeycloak CI - Forms IT (chrome) org.keycloak.testsuite.forms.MultipleTabsLoginTest#expiredAuthenticationAction_currentCodeExpiredExecutionKeycloak CI - Forms IT (chrome) org.keycloak.testsuite.forms.MultipleTabsLoginTest#testLogoutDifferentBrowserWithAuthenticationSessionStillPresentKeycloak CI - Forms IT (chrome) org.keycloak.testsuite.forms.MultipleTabsLoginTest#multipleTabsLoginAndPassiveCheckKeycloak CI - Forms IT (chrome) org.keycloak.testsuite.forms.MultipleTabsLoginTest#multipleTabsParallelLoginTestWithAuthSessionExpiredInTheMiddleKeycloak CI - Forms IT (chrome) org.keycloak.testsuite.forms.MultipleTabsLoginTest#loginActionWithoutExecutionInRequiredActionsKeycloak CI - Forms IT (chrome) org.keycloak.testsuite.forms.MultipleTabsLoginTest#multipleTabsParallelLoginTestWithAuthSessionExpiredAndRefreshInTab1Keycloak CI - Forms IT (chrome) org.keycloak.testsuite.forms.MultipleTabsLoginTest#testLoginPageRefreshKeycloak CI - Forms IT (chrome) org.keycloak.testsuite.forms.MultipleTabsLoginTest#loginWithDifferentClientsKeycloak CI - Forms IT (chrome) org.keycloak.testsuite.forms.MultipleTabsLoginTest#multipleTabsParallelLoginTestWithAuthSessionExpiredAndRegisterClickKeycloak CI - Forms IT (chrome) org.keycloak.testsuite.forms.MultipleTabsLoginTest#multipleTabsParallelLoginTestKeycloak CI - Forms IT (chrome) org.keycloak.testsuite.forms.MultipleTabsLoginTest#expiredAuthenticationAction_expiredCodeCurrentExecutionKeycloak CI - Forms IT (chrome) org.keycloak.testsuite.forms.MultipleTabsLoginTest#testInjectRedirectUriInClientDataAfterAuthSessionExpirationKeycloak CI - Forms IT (chrome) org.keycloak.testsuite.broker.KcOidcBrokerTest#loginWithExistingUserWithBruteForceEnabledKeycloak CI - Java Distribution IT (windows-latest - temurin - 17) |
vramik
force-pushed
the
48907-multi-org-data-model
branch
from
September 1, 2026 11:39
8d1b99e to
f60864a
Compare
martin-kanis
reviewed
Sep 1, 2026
martin-kanis
left a comment
Member
There was a problem hiding this comment.
@vramik Thanks for the PR.
I did the first round of review that focuses mostly on the migration part. I have some comments/concerns. Please see below.
In the meantime, I'll continue with the review and I might have more comments later.
| Set<OrganizationDomainModel> result = new HashSet<>(); | ||
| for (OrganizationDomainModel domain : currentDomains) { | ||
| if ("ANY".equals(orgDomain)) { | ||
| if (!excludedDomains.contains(domain.getName())) { |
Member
There was a problem hiding this comment.
No wildcard pattern matching - this will not correctly handle wildcard exclusions like *.example.com
Contributor
Author
There was a problem hiding this comment.
As discussed via DM, the exclusions (even the wildcard ones) should be represented by an entry in DOMAIN table with NULL value in IDP_ID column. MigrationTest should verify it.
| String domainId = domRs.getString(1); | ||
| String domainName = domRs.getString(2); | ||
| existingDomainNames.add(domainName); | ||
| if (!excludedDomains.contains(domainName)) { |
Member
There was a problem hiding this comment.
No wildcard pattern matching - this will not correctly handle wildcard exclusions like *.example.com
Contributor
Author
There was a problem hiding this comment.
As discussed via DM, the exclusions (even the wildcard ones) should be represented by an entry in DOMAIN table with NULL value in IDP_ID column. MigrationTest should verify it.
| import liquibase.statement.core.UpdateStatement; | ||
| import liquibase.structure.core.Table; | ||
|
|
||
| public class JpaUpdate26_8_0_DomainIdpRoutingMigration extends CustomKeycloakTask { |
Member
There was a problem hiding this comment.
This migration is quite a complex one. Should we introduce some tests or are we confident enough that it will work fine across many possible edge-cases and large datasets?
Contributor
Author
There was a problem hiding this comment.
Added tests to MigrationTest.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Migration edge cases, Admin UI compatibility, and an IdP-listing N+1 query remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
model/jpa/src/main/java/org/keycloak/connections/jpa/updater/liquibase/custom/JpaUpdate26_8_0_DomainIdpRoutingMigration.java:224
CustomKeycloakTaskexecutes these statements only after generation, so the database lookup cannot see an exclusion-domain insert queued for an earlier organization. BecauseinsertedDomainsis scoped per organization, two organizations in one realm with the same new exclusion queue duplicateDOMAINrows and violateUK_DOMAIN_REALM_NAME; cache generated IDs by(realmId, name)for the whole task and reuse them for each join row.
if (domainId == null && !insertedDomains.contains(excludedPattern)) {
domainId = UUID.randomUUID().toString();
statements.add(new InsertStatement(null, null, domainTable)
.addColumnValue("ID", domainId)
.addColumnValue("NAME", excludedPattern)
.addColumnValue("VERIFIED", false)
.addColumnValue("REALM_ID", realmId)
.addColumnValue("AUTO_REDIRECT", false));
insertedDomains.add(excludedPattern);
model/jpa/src/main/java/org/keycloak/models/jpa/JpaIdentityProviderStorageProvider.java:604
getAllStream()maps every result throughtoModel, so this new per-model lookup issues one additional query for every IdP in a list. Fetch the organization link in the main query or bulk-load links by IdP ID to avoid an N+1 regression on IdP listing and login paths.
identityProviderModel.setOrganizationId(getOrganizationIdForIdp(entity.getInternalId()));
model/jpa/src/main/java/org/keycloak/connections/jpa/updater/liquibase/custom/JpaUpdate26_8_0_DomainIdpRoutingMigration.java:83
- Exclusions are read only for
ANY, but the old redirect logic appliedkc.org.excluded.domainsbefore bothANYand specific-domain matching. A specific wildcard route such as*.example.comwith excluded*.internal.example.comis migrated as an unrestricted redirect; materialize exclusions for the specific-domain branch too.
if (!"ANY".equals(domainValue)) {
// Case A: specific domain name — look up DOMAIN.ID scoped by IdP's realm
- Files reviewed: 39/40 changed files
- Comments generated: 3
- Review effort level: Balanced
Comment on lines
+92
to
+93
| } else if (orgDomain.equals(domain.getName())) { | ||
| result.add(new OrganizationDomainModel(domain.getName(), domain.isVerified(), idp.getAlias(), autoRedirect)); |
Comment on lines
+68
to
+71
| String orgDomain = config.get(MigrationUtils.ORGANIZATION_DOMAIN_ATTRIBUTE); | ||
| if (orgDomain == null) { | ||
| return; | ||
| } |
vramik
force-pushed
the
48907-multi-org-data-model
branch
from
September 2, 2026 11:05
d15b2de to
7616b9c
Compare
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The migration can violate domain uniqueness, and IdP listing introduces an N+1 query regression.
Review details
Suppressed comments (2)
model/jpa/src/main/java/org/keycloak/models/jpa/JpaIdentityProviderStorageProvider.java:604
getAllStream()maps every result throughtoModel, so this lookup introduces one additionalORG_IDENTITY_PROVIDERquery per IdP. Listing IdPs and populating login caches now has an N+1 query pattern; fetch/project the organization link with the main query or batch the link lookup.
identityProviderModel.setOrganizationId(getOrganizationIdForIdp(entity.getInternalId()));
model/jpa/src/main/java/org/keycloak/connections/jpa/updater/liquibase/custom/JpaUpdate26_8_0_DomainIdpRoutingMigration.java:220
InsertStatements are deferred until generation completes, whileinsertedDomainsis reset for each organization. If two IdPs in the same realm exclude the same pattern that is not yet a domain, both queue a newDOMAINrow and violateUK_DOMAIN_REALM_NAME; track pending(realmId, name) -> domainIdvalues across the whole task and reuse them.
if (domainId == null && !insertedDomains.contains(excludedPattern)) {
domainId = UUID.randomUUID().toString();
statements.add(new InsertStatement(null, null, domainTable)
.addColumnValue("ID", domainId)
.addColumnValue("NAME", excludedPattern)
- Files reviewed: 39/40 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Unreported flaky test detectedIf 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.adapter.servlet.SAMLServletAdapterTest#employeeSigPostNoIdpKeyTestKeyIdAsKeyNameInKeyInfoKeycloak CI - Adapter IT Strict Cookies org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#salesPostEncTestKeycloak CI - Adapter IT Strict Cookies |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The migration can create invalid routing, duplicate domain rows, or fail on blank exclusions, and IdP loading introduces an N+1 query regression.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
model/jpa/src/main/java/org/keycloak/connections/jpa/updater/liquibase/custom/JpaUpdate26_8_0_DomainIdpRoutingMigration.java:220
- The database checks cannot see queued
InsertStatements, andinsertedDomainsis reset for each IdP/organization. If two IdPs in one realm have the same exclusion pattern that is not already a domain, this queues duplicate(REALM_ID, NAME)rows and the unique constraint aborts the migration; reuse IDs from a migration-wide realm/name map.
if (domainId == null && !insertedDomains.contains(excludedPattern)) {
domainId = UUID.randomUUID().toString();
statements.add(new InsertStatement(null, null, domainTable)
.addColumnValue("ID", domainId)
.addColumnValue("NAME", excludedPattern)
model/jpa/src/main/java/org/keycloak/models/jpa/JpaIdentityProviderStorageProvider.java:604
- Every identity-provider conversion now runs a separate organization-link query. Bulk IdP paths such as login filtering and realm export call
toModelonce per row, creating an N+1 query regression; fetch the link in the original query or map it on the entity so conversion does not query per IdP.
identityProviderModel.setOrganizationId(getOrganizationIdForIdp(entity.getInternalId()));
- Files reviewed: 43/44 changed files
- Comments generated: 2
- Review effort level: Balanced
Comment on lines
+84
to
+87
| try (PreparedStatement domLookup = connection.prepareStatement( | ||
| "SELECT dom.ID FROM " + getTableName(domainTable) + " dom" + | ||
| " JOIN " + getTableName(idpTable) + " ip ON dom.REALM_ID = ip.REALM_ID" + | ||
| " WHERE dom.NAME = ? AND ip.INTERNAL_ID = ?")) { |
Comment on lines
+77
to
+79
| for (String d : excludedDomainsValue.split(",")) { | ||
| excludedDomains.add(d.trim()); | ||
| } |
vramik
force-pushed
the
48907-multi-org-data-model
branch
from
September 2, 2026 14:00
6c42e2f to
7cdef51
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Imported legacy settings are not always cleaned, and the TypeScript Admin Client contract omits the new domain fields.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 43/44 changed files
- Comments generated: 2
- Review effort level: Balanced
Comment on lines
+68
to
+70
| String orgDomain = config.get(MigrationUtils.ORGANIZATION_DOMAIN_ATTRIBUTE); | ||
| if (orgDomain == null) { | ||
| return; |
Comment on lines
+35
to
+36
| private String identityProviderAlias; | ||
| private boolean autoRedirect; |
vramik
force-pushed
the
48907-multi-org-data-model
branch
from
September 2, 2026 18:43
7cdef51 to
a166663
Compare
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 43 out of 44 changed files in this pull request and generated 5 comments.
Suppressed comments (1)
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/admin/OrganizationThemeTest.java:1
- In this test,
organization.update(orgRep)returns a JAX-RSResponseand should be closed (and ideally asserted) to avoid leaking connections/resources. Please wrap the update call in try-with-resources (or call.close()) like other tests in this suite.
Comment on lines
+104
to
+115
| public static class Key implements Serializable { | ||
|
|
||
| private OrganizationEntity organization; | ||
| private String identityProviderId; | ||
|
|
||
| public Key() { | ||
| } | ||
|
|
||
| public Key(OrganizationEntity organization, String identityProviderId) { | ||
| this.organization = organization; | ||
| this.identityProviderId = identityProviderId; | ||
| } |
Comment on lines
+30
to
+31
| migrateDomainIdpRouting(session, realm); | ||
| super.migrateImport(session, realm, rep, skipUserDependent); |
Comment on lines
+73
to
+97
| Set<OrganizationDomainModel> currentDomains = org.getDomains().collect(Collectors.toSet()); | ||
| String excludedDomainsValue = config.get(MigrationUtils.ORGANIZATION_EXCLUDED_DOMAIN_ATTRIBUTE); | ||
| Set<String> excludedDomains = new HashSet<>(); | ||
| if (excludedDomainsValue != null) { | ||
| for (String d : excludedDomainsValue.split(",")) { | ||
| excludedDomains.add(d.trim()); | ||
| } | ||
| } | ||
|
|
||
| boolean autoRedirect = Boolean.parseBoolean(config.get(MigrationUtils.ORGANIZATION_REDIRECT_MODE_ATTRIBUTE)); | ||
|
|
||
| Set<OrganizationDomainModel> result = new HashSet<>(); | ||
| for (OrganizationDomainModel domain : currentDomains) { | ||
| if ("ANY".equals(orgDomain)) { | ||
| if (!excludedDomains.contains(domain.getName())) { | ||
| result.add(new OrganizationDomainModel(domain.getName(), domain.isVerified(), idp.getAlias(), autoRedirect)); | ||
| } else { | ||
| result.add(domain); | ||
| } | ||
| } else if (orgDomain.equals(domain.getName())) { | ||
| result.add(new OrganizationDomainModel(domain.getName(), domain.isVerified(), idp.getAlias(), autoRedirect)); | ||
| } else { | ||
| result.add(domain); | ||
| } | ||
| } |
Comment on lines
601
to
605
| identityProviderModel.setAuthenticateByDefault(entity.isAuthenticateByDefault()); | ||
| identityProviderModel.setFirstBrokerLoginFlowId(entity.getFirstBrokerLoginFlowId()); | ||
| identityProviderModel.setPostBrokerLoginFlowId(entity.getPostBrokerLoginFlowId()); | ||
| identityProviderModel.setOrganizationId(entity.getOrganizationId()); | ||
| identityProviderModel.setOrganizationId(getOrganizationIdForIdp(entity.getInternalId())); | ||
| identityProviderModel.setStoreToken(entity.isStoreToken()); |
| organizationEntity.getDomains().stream() | ||
| .filter(d -> d.getIdentityProvider() != null | ||
| && d.getIdentityProvider().getInternalId().equals(identityProvider.getInternalId())) | ||
| .forEach(d -> d.setIdentityProvider(null)); |
Closes keycloak#48907 Signed-off-by: vramik <vramik@redhat.com>
vramik
force-pushed
the
48907-multi-org-data-model
branch
from
September 3, 2026 07:50
a166663 to
3c7f5cb
Compare
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Migration deduplication and stale routing state can cause upgrade failures or incorrect domain configuration.
Review details
Suppressed comments (3)
model/jpa/src/main/java/org/keycloak/organization/jpa/JpaOrganizationProvider.java:766
- Unlinking the IdP clears only
IDP_ID, leaving the domain serialized withautoRedirect=trueeven though it has no route. ClearAUTO_REDIRECTat the same time so a later IdP assignment does not silently reactivate automatic redirection from stale state.
organizationEntity.getDomains().stream()
.filter(d -> d.getIdentityProvider() != null
&& d.getIdentityProvider().getInternalId().equals(identityProvider.getInternalId()))
.forEach(d -> d.setIdentityProvider(null));
model/storage-private/src/main/java/org/keycloak/migration/migrators/MigrateTo26_8_0.java:79
- Empty entries are retained here, so an imported value such as
kc.org.excluded.domains=""or a trailing comma creates an emptyOrganizationDomainModel;org.setDomainsthen rejects it and the import migration does not complete. Ignore blank entries, as the database migration already does.
if (excludedDomainsValue != null) {
for (String d : excludedDomainsValue.split(",")) {
excludedDomains.add(d.trim());
}
model/jpa/src/main/java/org/keycloak/connections/jpa/updater/liquibase/custom/JpaUpdate26_8_0_DomainIdpRoutingMigration.java:224
- Liquibase has not executed the queued
InsertStatementwhen the next IdP/organization is processed, andinsertedDomainsis reset per call. If two organization IdPs in one realm use the same previously nonexistent exclusion pattern, both queue aDOMAINrow and migration fails onUK_DOMAIN_REALM_NAME; keep a migration-wide realm/name-to-ID map (and queued-link set) instead of relying only on database lookups.
if (domainId == null && !insertedDomains.contains(excludedPattern)) {
domainId = UUID.randomUUID().toString();
statements.add(new InsertStatement(null, null, domainTable)
.addColumnValue("ID", domainId)
.addColumnValue("NAME", excludedPattern)
.addColumnValue("VERIFIED", false)
.addColumnValue("REALM_ID", realmId)
.addColumnValue("AUTO_REDIRECT", false));
insertedDomains.add(excludedPattern);
- Files reviewed: 43/44 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Contributor
Author
|
Closing in fovor of #52401 |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes #48907