Don't bootstrap a dummy LDAP identity provider when unconfigured - #4061
Open
egorpak528 wants to merge 4 commits into
Open
Don't bootstrap a dummy LDAP identity provider when unconfigured#4061egorpak528 wants to merge 4 commits into
egorpak528 wants to merge 4 commits into
Conversation
IdentityProviderBootstrap.addLdapProvider() unconditionally created an IdentityProvider row with origin_key="ldap" on every startup, even when no `ldap:` config was supplied and the `ldap` Spring profile was merely active (or not active at all). Only the `active` flag depended on real configuration; the row itself was always persisted. Since identity_provider enforces a unique (identity_zone_id, origin_key) index, this blocked creating a real LDAP IdentityProvider via POST /identity-providers (only PUT against the existing dummy row was possible), and risked clobbering a REST-managed LDAP IDP's config on the next restart unless `ldap.override: false` was explicitly set - which itself doesn't prevent the dummy row from being created on a fresh zone, since override only decides whether an *existing* row gets updated, not whether one gets created. Skip creating the bootstrap provider entirely when there's no real LDAP configuration (LdapIdentityProviderDefinition.isConfigured(), i.e. no baseUrl - so a `ldap:` block containing only control flags like `override` still counts as unconfigured) and no provider already exists for this zone. A provider that already exists (from a prior real config, the REST API, or the legacy Flyway migration) is left untouched by this change - it's only new, never-configured zones that no longer get a dummy row. ai-assisted=yes Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ldapProfileAloneDoesNotBootstrapDummyProvider and ldapConfigWithOnlyOverrideFlagDoesNotBootstrapDummyProvider assumed a clean identity_provider table, but @WithDatabaseContext doesn't roll back between tests. Depending on JUnit's (unspecified) method execution order, an earlier test in the class that bootstraps a real LDAP provider (e.g. ldapOverrideFalse) could leave a row behind, making `existing` non-null and causing these tests to fail because no exception was thrown - exactly what happened on CI for PR cloudfoundry#4061 (mysql 8.0/8.4/9, postgresql 15/16/17/18, and the standalone hsqldb run all failed identically). Add the same TestUtils.cleanAndSeedDb(jdbcTemplate) call the neighboring ldapOverrideFalse test already uses to guarantee a clean slate before asserting. ai-assisted=yes Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
cf-foundation-community-automation
Bot
moved this to Inbox
in Foundational Infrastructure Working Group
The IdentityProviderBootstrap guard added in the previous commit only prevents *new* dummy rows from being created - it does nothing for databases that already have them, because V2_0_2__BootstrapIdentityZones (2014) unconditionally inserts a login-server/ldap/keystone/uaa identity_provider row for the uaa zone on *every* database, fresh or upgraded (Flyway replays the whole migration history regardless). That insert predates any application code touching the row, so the app-level guard alone can never make it stop existing - only a migration can. Add V4_114__DeleteUnconfiguredBootstrapIdentityProviders (one shared Java migration class, three thin per-engine subclasses, same pattern as BootstrapIdentityZones itself) which deletes exactly the rows that are still in their never-configured state: - login-server, keystone: config is still the literal NULL the 2014 migration inserted (nothing else has ever written to it under normal operation). - ldap: needs a semantic check, not just NULL - IdentityProviderBootstrap overwrites the literal NULL with a structurally-real-but-empty JSON config on every boot today, so this parses it and checks LdapIdentityProviderDefinition#isConfigured() (baseUrl set). - uaa: never touched. It's the mandatory internal auth source, kept live by IdentityProviderBootstrap#updateDefaultZoneUaaIDP(). Verified no blast radius concerns before writing this: no FK constraints anywhere reference identity_provider(id) (the columns that once did, users/group_membership.identity_provider_id, were dropped back in V2_0_4__Identity_Provider_Adjustments), and the two real callers that look up these origins already handle total absence gracefully (DynamicZoneAwareAuthenticationManager#getProvider() falls back to a synthetic inactive stub on EmptyResultDataAccessException; so does InvitationsController#acceptLdapInvitation()). Because Flyway replays every migration on every database, this closes the loop for both timeframes at once: a brand-new install inserts then immediately deletes these rows within the same initial migrate() run (never persisting), while an already-provisioned database gets them cleaned up retroactively the next time it upgrades - as long as they're still genuinely unconfigured. A real, actively-configured LDAP or Keystone provider is left untouched either way. See TNZ-125736. ai-assisted=yes Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
TestUtils.cleanAndSeedDb() is explicitly documented as seeding a database "similar to how the real Flyway migration does it" - now that a later migration removes the never-configured login-server/ ldap/keystone placeholder rows, a truly fresh, fully-migrated database no longer has them, so the shared test helper needs to stop pretending otherwise. Checked usage first: of the 10 call sites across the test suite, only IdentityProviderBootstrapTest ever looks at those two origins post-seed, and it always creates its own row via the code under test rather than relying on the seeded placeholder - so this is safe (confirmed by running all 10 affected test files). That change surfaced two independent, pre-existing issues in IdentityProviderBootstrapTest once the placeholder rows stopped being free: - upgradeLDAPProvider(): an assertion I added in the previous commit checked origin "ldap", but the row this test actually inserts (from a 2016 commit, 645b8a9, testing that bootstrap tolerates unknown legacy JSON attributes like "ldapdebug") uses origin_key "ldap2" - a different, deliberately unrelated row. That mismatch only ever passed because the old unconditional bootstrap code created *a* "ldap" row regardless of what else existed in the table. Fixed to check the row that's actually under test. - getGenericLdapConfig(): the shared "generic" config used by ldapBootstrap() and ldapOverrideFalse() never set base.url, so LdapIdentityProviderDefinition#isConfigured() was false for it - harmless under the old unconditional-create behavior, but now trips the new "don't bootstrap when unconfigured" guard. Added a base.url so this helper actually represents the realistic, fully-configured LDAP setup its name claims. Verified against the full IdentityProviderBootstrapTest suite plus all other test files that use TestUtils.cleanAndSeedDb/restoreToDefaults (160 tests, all passing). See TNZ-125736. ai-assisted=yes Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
1 participant
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.
Problem
IdentityProviderBootstrap.addLdapProvider()unconditionally creates anIdentityProviderrow withorigin_key="ldap"in each zone on every startup, regardless of whether real LDAP configuration is supplied. Only theactiveflag depends on real config (ldapProfile && json.isConfigured()) - the row itself is always persisted.Since
identity_providerenforces a unique(identity_zone_id, origin_key)index, this causes two problems for anyone trying to manage LDAP identity providers via the REST API:POST /identity-providerswithtype=ldapcollides with the existing dummy row - onlyPUTagainst it is possible, so a real, independently-managed LDAP IDP can never be created fresh.ldap:config supplied with no real connection details (e.g. onlyoverride: false) does not prevent the dummy row from being created on a fresh zone/install -overrideonly controls whether an already-existing row gets updated on a later boot, not whether one gets created in the first place. Soldap.override: falseis not a usable workaround.There's a second, deeper layer to this: even with the app-level fix below,
identity_providerrows foruaa/login-server/ldap/keystoneare also unconditionally inserted by a much older migration,V2_0_2__BootstrapIdentityZones(2014) - on every database, fresh or upgraded, since Flyway replays the full migration history regardless. That insert predates any application code and can't be prevented by fixingIdentityProviderBootstrapalone.Fix
Two parts, addressing both layers:
1.
IdentityProviderBootstrap.addLdapProvider()- skip creating the bootstrap provider when there's no real configuration (LdapIdentityProviderDefinition.isConfigured(), i.e. nobaseUrl- so aldap:block with only control flags likeoverridestill counts as unconfigured) and no provider already exists for this zone. A provider that already exists - from a prior real config, the REST API, or the legacy Flyway migration - is left untouched; it's only new, never-configured zones that no longer get a dummy row on boot.2. A new migration,
V4_114__DeleteUnconfiguredBootstrapIdentityProviders- deletes thelogin-server/ldap/keystonerowsV2_0_2__BootstrapIdentityZonesleft behind, but only if they're still genuinely unconfigured (config IS NULL, or forldapspecifically, a parsed config with nobaseUrl).uaais never touched - it's the mandatory internal auth source. Because Flyway replays every migration on every database, this closes the loop for both timeframes at once: a fresh install inserts-then-immediately-deletes these rows within the same initialmigrate()run (never persisting), while an already-provisioned database gets them cleaned up retroactively on its next upgrade. A real, actively-configured provider is left alone either way.Verified no blast radius concerns before adding the migration: no FK constraints anywhere reference
identity_provider(id)(the columns that once did were dropped back inV2_0_4), and the two real callers that look up these origins already handle total absence gracefully (DynamicZoneAwareAuthenticationManager#getProvider()falls back to a synthetic inactive stub; so doesInvitationsController#acceptLdapInvitation()).Testing
IdentityProviderBootstrapTest- added tests for the profile-alone and override-only-config cases; updatedupgradeLDAPProviderandgetGenericLdapConfig()(pre-existing helpers whose assumptions no longer held once dummy rows stopped being free - see commit messages for details).DeleteUnconfiguredBootstrapIdentityProvidersTest- new, covers null-config deletion, semantic ldap/keystone "still unconfigured" detection, preservation of real configs, and defensive handling of unparseable JSON.TestUtils.cleanAndSeedDb()updated to match the new post-migration baseline; re-verified against all 10 test files that depend on it (160 tests).ai-assisted=yes
🤖 Generated with Claude Code