Просмотр исходного кода

feat(oidc): upsert the env-managed provider

The row is updated in place, never delete-recreated: user_oidc_links
references it with ON DELETE CASCADE, so recreating the provider would
silently unlink every account bound to it. For the same reason, removing the
variables disables the provider rather than deleting it -- the links would not
come back when the config does.

Config goes through OIDCProviderCreate, the schema the API already uses, so
the environment cannot reach a state the UI would have refused. That covers
the SEC-1 auto-link check: auto-link plus unverified email is an account
takeover, and it is rejected here exactly as it is in the UI.

Nothing raises. This runs during startup, so a typo in one variable must not
stop the app from booting -- a rejected config is logged and skipped, leaving
the previous provider untouched.

Refs #2593
Marian 1 месяц назад
Родитель
Сommit
58602a3f1b
2 измененных файлов с 236 добавлено и 0 удалено
  1. 76 0
      backend/app/core/oidc_env.py
  2. 160 0
      backend/tests/integration/test_oidc_env_apply.py

+ 76 - 0
backend/app/core/oidc_env.py

@@ -9,8 +9,14 @@ check the UI enforces.
 
 from __future__ import annotations
 
+import logging
 import os
 
+from sqlalchemy import select, update
+from sqlalchemy.ext.asyncio import AsyncSession
+
+logger = logging.getLogger(__name__)
+
 # All four or nothing: a provider missing its secret would be written to the
 # database and then fail at authorize time, long after the operator could
 # connect the failure to a typo in their compose file.
@@ -52,3 +58,73 @@ def read_env_oidc_config() -> dict | None:
         "icon_url": os.environ.get("BAMBUDDY_OIDC_ICON_URL"),
         "is_autologin": _env_bool("BAMBUDDY_OIDC_AUTOLOGIN", False),
     }
+
+
+# Everything the schema validates and the model stores, except client_secret --
+# that one goes through the property so it is encrypted at rest.
+_APPLIED_FIELDS = (
+    "name",
+    "issuer_url",
+    "client_id",
+    "scopes",
+    "is_enabled",
+    "auto_create_users",
+    "auto_link_existing_accounts",
+    "email_claim",
+    "require_email_verified",
+    "icon_url",
+    "is_autologin",
+)
+
+
+async def apply_env_oidc_provider(db: AsyncSession) -> None:
+    """Upsert the env-managed provider, or disable 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.
+    """
+    # Imported here rather than at module scope: app.core is imported by the
+    # models themselves, so a top-level import would be a cycle.
+    from backend.app.models.oidc_provider import OIDCProvider
+    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
+            await db.commit()
+            logger.info("BAMBUDDY_OIDC_* is unset -- env-managed provider disabled.")
+        return
+
+    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).
+        validated = OIDCProviderCreate(**config)
+    except Exception as exc:  # noqa: BLE001 -- any rejection must be survivable
+        logger.error("BAMBUDDY_OIDC_* config rejected, provider not applied: %s", exc)
+        return
+
+    if existing is None:
+        existing = OIDCProvider(is_env_managed=True)
+        db.add(existing)
+    for field in _APPLIED_FIELDS:
+        setattr(existing, field, getattr(validated, field))
+    existing.client_secret = validated.client_secret
+    existing.is_env_managed = True
+    await db.flush()  # the id is needed by the autologin sweep below
+
+    if existing.is_autologin:
+        await db.execute(
+            update(OIDCProvider)
+            .where(OIDCProvider.id != existing.id, OIDCProvider.is_autologin.is_(True))
+            .values(is_autologin=False)
+        )
+    await db.commit()
+    logger.info("Env-managed OIDC provider %r applied.", existing.name)

+ 160 - 0
backend/tests/integration/test_oidc_env_apply.py

@@ -0,0 +1,160 @@
+"""Upserting the env-managed OIDC provider (#2593).
+
+Startup applies BAMBUDDY_OIDC_* to the database. The row is updated in place,
+never delete-recreated: user_oidc_links.provider_id is FK ON DELETE CASCADE, so
+recreating the provider would silently unlink every account bound to it.
+"""
+
+from __future__ import annotations
+
+import pytest
+from sqlalchemy import select
+
+from backend.app.core.oidc_env import apply_env_oidc_provider
+from backend.app.models.oidc_provider import OIDCProvider
+
+REQUIRED = {
+    "BAMBUDDY_OIDC_NAME": "Keycloak",
+    "BAMBUDDY_OIDC_ISSUER_URL": "https://sso.example.com/realms/main",
+    "BAMBUDDY_OIDC_CLIENT_ID": "bambuddy",
+    "BAMBUDDY_OIDC_CLIENT_SECRET": "s3cr3t",
+}
+
+ALL_VARS = (
+    *REQUIRED,
+    "BAMBUDDY_OIDC_SCOPES",
+    "BAMBUDDY_OIDC_ENABLED",
+    "BAMBUDDY_OIDC_AUTO_CREATE_USERS",
+    "BAMBUDDY_OIDC_AUTO_LINK_EXISTING",
+    "BAMBUDDY_OIDC_EMAIL_CLAIM",
+    "BAMBUDDY_OIDC_REQUIRE_EMAIL_VERIFIED",
+    "BAMBUDDY_OIDC_ICON_URL",
+    "BAMBUDDY_OIDC_AUTOLOGIN",
+)
+
+
+@pytest.fixture(autouse=True)
+def clean_env(monkeypatch):
+    for key in ALL_VARS:
+        monkeypatch.delenv(key, raising=False)
+
+
+def _configure(monkeypatch, **overrides):
+    for key, value in REQUIRED.items():
+        monkeypatch.setenv(key, value)
+    for key, value in overrides.items():
+        monkeypatch.setenv(key, value)
+
+
+async def _env_provider(db_session) -> OIDCProvider | None:
+    result = await db_session.execute(select(OIDCProvider).where(OIDCProvider.is_env_managed.is_(True)))
+    return result.scalar_one_or_none()
+
+
+@pytest.mark.asyncio
+async def test_creates_the_provider_from_env(db_session, monkeypatch):
+    _configure(monkeypatch)
+    await apply_env_oidc_provider(db_session)
+
+    provider = await _env_provider(db_session)
+    assert provider is not None
+    assert provider.name == "Keycloak"
+    assert provider.client_id == "bambuddy"
+    assert provider.is_env_managed is True
+    assert provider.client_secret == "s3cr3t"  # property decrypts
+
+
+@pytest.mark.asyncio
+async def test_a_changed_var_updates_the_same_row(db_session, monkeypatch):
+    """The id must survive: user_oidc_links references it with ON DELETE
+    CASCADE, so a delete-recreate would unlink every bound account."""
+    _configure(monkeypatch)
+    await apply_env_oidc_provider(db_session)
+    original_id = (await _env_provider(db_session)).id
+
+    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.client_id == "rotated"
+
+
+@pytest.mark.asyncio
+async def test_removing_the_env_config_disables_but_keeps_the_row(db_session, monkeypatch):
+    _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)
+
+    provider = await _env_provider(db_session)
+    assert provider is not None, "deleting would cascade away every account link"
+    assert provider.id == original_id
+    assert provider.is_enabled is False
+
+
+@pytest.mark.asyncio
+async def test_env_autologin_clears_it_on_other_providers(db_session, monkeypatch):
+    """Only one provider may be the autologin target; the env one wins."""
+    ui_provider = OIDCProvider(
+        name="UI provider",
+        issuer_url="https://other.example.com",
+        client_id="ui",
+        is_autologin=True,
+    )
+    ui_provider.client_secret = "ui-secret"
+    db_session.add(ui_provider)
+    await db_session.commit()
+
+    _configure(monkeypatch, BAMBUDDY_OIDC_AUTOLOGIN="true")
+    await apply_env_oidc_provider(db_session)
+
+    await db_session.refresh(ui_provider)
+    assert (await _env_provider(db_session)).is_autologin is True
+    assert ui_provider.is_autologin is False
+
+
+@pytest.mark.asyncio
+async def test_a_ui_provider_is_otherwise_left_alone(db_session, monkeypatch):
+    ui_provider = OIDCProvider(name="UI provider", issuer_url="https://other.example.com", client_id="ui")
+    ui_provider.client_secret = "ui-secret"
+    db_session.add(ui_provider)
+    await db_session.commit()
+
+    _configure(monkeypatch)
+    await apply_env_oidc_provider(db_session)
+
+    await db_session.refresh(ui_provider)
+    assert ui_provider.is_env_managed is False
+    assert ui_provider.is_enabled is True
+    assert ui_provider.client_id == "ui"
+
+
+@pytest.mark.asyncio
+async def test_an_unsafe_auto_link_config_is_skipped_not_raised(db_session, monkeypatch):
+    """auto-link + unverified email is the SEC-1 account-takeover shape. The
+    schema rejects it for the UI, and env config must not be a way around that
+    -- but a bad variable must not stop the app from booting either."""
+    _configure(
+        monkeypatch,
+        BAMBUDDY_OIDC_AUTO_LINK_EXISTING="true",
+        BAMBUDDY_OIDC_REQUIRE_EMAIL_VERIFIED="false",
+    )
+
+    await apply_env_oidc_provider(db_session)
+
+    assert await _env_provider(db_session) is None
+
+
+@pytest.mark.asyncio
+async def test_applying_twice_without_changes_is_a_no_op(db_session, monkeypatch):
+    """Every boot re-applies; the second run must not create a second row."""
+    _configure(monkeypatch)
+    await apply_env_oidc_provider(db_session)
+    await apply_env_oidc_provider(db_session)
+
+    result = await db_session.execute(select(OIDCProvider).where(OIDCProvider.is_env_managed.is_(True)))
+    assert len(result.scalars().all()) == 1