diff --git a/backend/app/core/oidc_env.py b/backend/app/core/oidc_env.py index 5d02bdd7b..39f9d6078 100644 --- a/backend/app/core/oidc_env.py +++ b/backend/app/core/oidc_env.py @@ -78,7 +78,7 @@ _APPLIED_FIELDS = ( async def apply_env_oidc_provider(db: AsyncSession) -> None: - """Upsert the env-managed provider, or disable it when the config is gone. + """Upsert the env-managed provider, or release it when the config is gone. Never raises: this runs during startup, and a typo in one variable must not stop the app from booting. A rejected config is logged and skipped. @@ -89,20 +89,35 @@ async def apply_env_oidc_provider(db: AsyncSession) -> None: from backend.app.schemas.auth import OIDCProviderCreate config = read_env_oidc_config() - existing = ( - await db.execute(select(OIDCProvider).where(OIDCProvider.is_env_managed.is_(True))) - ).scalar_one_or_none() if config is None: - # Disabled, never deleted: user_oidc_links.provider_id is FK ON DELETE - # CASCADE, so removing the row would unlink every bound account and the - # links would not come back when the variables do. - if existing is not None and existing.is_enabled: - existing.is_enabled = False + # Nothing to look up by name any more, so the previously managed row is + # found by the flag -- and then released. + released = ( + await db.execute(select(OIDCProvider).where(OIDCProvider.is_env_managed.is_(True))) + ).scalar_one_or_none() + if released is not None: + # Disabled, never deleted: user_oidc_links.provider_id is FK ON + # DELETE CASCADE, so removing the row would unlink every bound + # account and the links would not come back when the variables do. + # The flag is cleared as well: with no config behind it, a provider + # the API still refuses to edit or delete would be a dead end + # reachable only through the database. + released.is_enabled = False + released.is_env_managed = False await db.commit() - logger.info("BAMBUDDY_OIDC_* is unset -- env-managed provider disabled.") + logger.info( + "BAMBUDDY_OIDC_* is unset -- provider %r disabled and released to the UI.", + released.name, + ) return + # Identity is the name, which is unique on the table. Matching on the flag + # instead meant an operator who named the env provider after one that + # already existed hit that unique constraint during startup -- and this + # function runs in the lifespan, so the app would not boot. + existing = (await db.execute(select(OIDCProvider).where(OIDCProvider.name == config["name"]))).scalar_one_or_none() + try: # The same schema the API uses, so env config cannot reach a state the # UI would have refused (notably the SEC-1 auto-link check). diff --git a/backend/tests/integration/test_oidc_env_apply.py b/backend/tests/integration/test_oidc_env_apply.py index e83aeeba9..ac222431d 100644 --- a/backend/tests/integration/test_oidc_env_apply.py +++ b/backend/tests/integration/test_oidc_env_apply.py @@ -90,7 +90,10 @@ async def test_removing_the_env_config_disables_but_keeps_the_row(db_session, mo monkeypatch.delenv(key, raising=False) await apply_env_oidc_provider(db_session) - provider = await _env_provider(db_session) + # Looked up by name, not by the flag: releasing the provider clears the flag, + # and the point of this test is that the ROW survives either way. + result = await db_session.execute(select(OIDCProvider).where(OIDCProvider.name == "Keycloak")) + provider = result.scalar_one_or_none() assert provider is not None, "deleting would cascade away every account link" assert provider.id == original_id assert provider.is_enabled is False @@ -158,3 +161,85 @@ async def test_applying_twice_without_changes_is_a_no_op(db_session, monkeypatch result = await db_session.execute(select(OIDCProvider).where(OIDCProvider.is_env_managed.is_(True))) assert len(result.scalars().all()) == 1 + + +# --- identity is the name, not the flag --------------------------------------- +# The provider is looked up by BAMBUDDY_OIDC_NAME, which is unique on the table. +# Matching on is_env_managed instead made three things impossible: adopting a +# provider that already carries the name (the insert hit the unique constraint +# and took startup down with it), releasing the provider when the config goes +# away, and finding it again afterwards. + + +@pytest.mark.asyncio +async def test_a_name_collision_adopts_the_existing_provider(db_session, monkeypatch): + """An operator who names the env provider after one they created in the UI + must not end up with an app that refuses to boot.""" + ui_provider = OIDCProvider(name="Keycloak", issuer_url="https://old.example.com", client_id="ui-client") + ui_provider.client_secret = "ui-secret" + db_session.add(ui_provider) + await db_session.commit() + original_id = ui_provider.id + + _configure(monkeypatch) + await apply_env_oidc_provider(db_session) + + provider = await _env_provider(db_session) + assert provider is not None + assert provider.id == original_id, "adopted, not duplicated" + assert provider.client_id == "bambuddy" + + result = await db_session.execute(select(OIDCProvider).where(OIDCProvider.name == "Keycloak")) + assert len(result.scalars().all()) == 1 + + +@pytest.mark.asyncio +async def test_removing_the_config_releases_the_provider_to_the_ui(db_session, monkeypatch): + """Nothing manages it any more, so the API must stop refusing edits and + deletes -- otherwise the row is a dead end only reachable via the database.""" + _configure(monkeypatch) + await apply_env_oidc_provider(db_session) + + for key in ALL_VARS: + monkeypatch.delenv(key, raising=False) + await apply_env_oidc_provider(db_session) + + result = await db_session.execute(select(OIDCProvider).where(OIDCProvider.name == "Keycloak")) + provider = result.scalar_one() + assert provider.is_enabled is False + assert provider.is_env_managed is False + + +@pytest.mark.asyncio +async def test_restoring_the_config_finds_the_same_row_again(db_session, monkeypatch): + """The account links hang off this row; a second provider would orphan them.""" + _configure(monkeypatch) + await apply_env_oidc_provider(db_session) + original_id = (await _env_provider(db_session)).id + + for key in ALL_VARS: + monkeypatch.delenv(key, raising=False) + await apply_env_oidc_provider(db_session) + + _configure(monkeypatch) + await apply_env_oidc_provider(db_session) + + provider = await _env_provider(db_session) + assert provider.id == original_id + assert provider.is_enabled is True + + +@pytest.mark.asyncio +async def test_the_issuer_and_client_can_change_under_the_same_name(db_session, monkeypatch): + _configure(monkeypatch) + await apply_env_oidc_provider(db_session) + original_id = (await _env_provider(db_session)).id + + monkeypatch.setenv("BAMBUDDY_OIDC_ISSUER_URL", "https://sso.example.com/realms/other") + monkeypatch.setenv("BAMBUDDY_OIDC_CLIENT_ID", "rotated") + await apply_env_oidc_provider(db_session) + + provider = await _env_provider(db_session) + assert provider.id == original_id + assert provider.issuer_url == "https://sso.example.com/realms/other" + assert provider.client_id == "rotated"