From c9ee2598078f18d7fd8f16593959cfcba4059954 Mon Sep 17 00:00:00 2001 From: Marian Date: Mon, 20 Jul 2026 14:38:28 +0000 Subject: [PATCH] feat(oidc): refuse API writes to the env-managed provider Startup rewrites this row from BAMBUDDY_OIDC_* on every boot, so an edit through the UI would be accepted and then silently reverted at the next restart -- the operator would watch their change vanish with nothing explaining why. A 409 says so instead. Covers all four mutating routes, including the two icon ones: the icon comes from BAMBUDDY_OIDC_ICON_URL and would be restored the same way. Extracted as one helper rather than four copies of the same check, so a fifth route cannot be added with the guard silently missing. Locking it is safe because BAMBUDDY_LOCAL_LOGIN (#1589) remains the documented recovery path if the provider itself becomes unusable. A test pins that UI-created providers stay editable -- the lock must not leak onto them. Refs #2593 --- backend/app/api/routes/mfa.py | 16 ++ .../tests/integration/test_oidc_env_lock.py | 137 ++++++++++++++++++ 2 files changed, 153 insertions(+) create mode 100644 backend/tests/integration/test_oidc_env_lock.py diff --git a/backend/app/api/routes/mfa.py b/backend/app/api/routes/mfa.py index b7238fcb6..e653c6813 100644 --- a/backend/app/api/routes/mfa.py +++ b/backend/app/api/routes/mfa.py @@ -1404,6 +1404,18 @@ async def create_oidc_provider( return _build_provider_response(provider) +def _refuse_if_env_managed(provider: OIDCProvider) -> None: + """Startup rewrites this provider from BAMBUDDY_OIDC_* on every boot, so an + edit here would be accepted and then silently reverted at the next restart. + BAMBUDDY_LOCAL_LOGIN (#1589) remains the recovery path if it becomes + unusable, so refusing outright cannot lock anyone out.""" + if provider.is_env_managed: + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail="This OIDC provider is managed by environment variables and cannot be modified.", + ) + + @router.put("/oidc/providers/{provider_id}", response_model=OIDCProviderResponse) async def update_oidc_provider( provider_id: int, @@ -1426,6 +1438,7 @@ async def update_oidc_provider( provider = result2.scalar_one_or_none() if not provider: raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Provider not found") + _refuse_if_env_managed(provider) if body.default_group_id is not None: grp_chk = await db.execute(select(Group).where(Group.id == body.default_group_id)) @@ -1503,6 +1516,7 @@ async def delete_oidc_provider( provider = result2.scalar_one_or_none() if not provider: raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Provider not found") + _refuse_if_env_managed(provider) await db.delete(provider) await db.commit() @@ -1571,6 +1585,7 @@ async def delete_oidc_provider_icon( provider = result.scalar_one_or_none() if provider is None: raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Provider not found") + _refuse_if_env_managed(provider) # Setting deferred columns is safe — no read happens, just a write. provider.icon_url = None @@ -1603,6 +1618,7 @@ async def refresh_oidc_provider_icon( provider = result.scalar_one_or_none() if provider is None: raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Provider not found") + _refuse_if_env_managed(provider) if not provider.icon_url: raise HTTPException( status_code=status.HTTP_400_BAD_REQUEST, diff --git a/backend/tests/integration/test_oidc_env_lock.py b/backend/tests/integration/test_oidc_env_lock.py new file mode 100644 index 000000000..6c8c99358 --- /dev/null +++ b/backend/tests/integration/test_oidc_env_lock.py @@ -0,0 +1,137 @@ +"""The env-managed provider is read-only through the API (#2593). + +Startup rewrites this row from BAMBUDDY_OIDC_* on every boot, so a UI edit +would silently disappear at the next restart -- the operator would see their +change accepted and then reverted, with nothing explaining why. Refusing the +write is the honest answer. + +Locking it out is safe because BAMBUDDY_LOCAL_LOGIN (#1589) is the documented +recovery path if the provider itself becomes unusable. +""" + +from __future__ import annotations + +import pytest +from httpx import AsyncClient + +from backend.app.models.oidc_provider import OIDCProvider +from backend.tests.integration.test_mfa_api import _auth_header, _setup_and_login + + +async def _env_managed_provider(db_session) -> int: + provider = OIDCProvider( + name="Env Keycloak", + issuer_url="https://sso.example.com/realms/main", + client_id="bambuddy", + icon_url="https://sso.example.com/logo.png", + is_env_managed=True, + ) + provider.client_secret = "s3cr3t" + db_session.add(provider) + await db_session.commit() + await db_session.refresh(provider) + return provider.id + + +@pytest.mark.asyncio +@pytest.mark.integration +async def test_put_is_refused(async_client: AsyncClient, db_session): + provider_id = await _env_managed_provider(db_session) + token = await _setup_and_login(async_client, "envlockput", "envlockput123") + + response = await async_client.put( + f"/api/v1/auth/oidc/providers/{provider_id}", + json={"name": "hijacked"}, + headers=_auth_header(token), + ) + + assert response.status_code == 409 + + +@pytest.mark.asyncio +@pytest.mark.integration +async def test_delete_is_refused(async_client: AsyncClient, db_session): + provider_id = await _env_managed_provider(db_session) + token = await _setup_and_login(async_client, "envlockdel", "envlockdel123") + + response = await async_client.delete( + f"/api/v1/auth/oidc/providers/{provider_id}", + headers=_auth_header(token), + ) + + assert response.status_code == 409 + + +@pytest.mark.asyncio +@pytest.mark.integration +async def test_icon_delete_is_refused(async_client: AsyncClient, db_session): + """The icon is part of the env config too -- BAMBUDDY_OIDC_ICON_URL.""" + provider_id = await _env_managed_provider(db_session) + token = await _setup_and_login(async_client, "envlockicondel", "envlockicondel123") + + response = await async_client.delete( + f"/api/v1/auth/oidc/providers/{provider_id}/icon", + headers=_auth_header(token), + ) + + assert response.status_code == 409 + + +@pytest.mark.asyncio +@pytest.mark.integration +async def test_icon_refresh_is_refused(async_client: AsyncClient, db_session): + provider_id = await _env_managed_provider(db_session) + token = await _setup_and_login(async_client, "envlockiconref", "envlockiconref123") + + response = await async_client.post( + f"/api/v1/auth/oidc/providers/{provider_id}/icon/refresh", + headers=_auth_header(token), + ) + + assert response.status_code == 409 + + +@pytest.mark.asyncio +@pytest.mark.integration +async def test_a_ui_provider_is_still_editable(async_client: AsyncClient): + """The lock must not leak onto providers the operator created themselves -- + they coexist with the env one and stay fully editable.""" + token = await _setup_and_login(async_client, "envlockui", "envlockui123") + created = await async_client.post( + "/api/v1/auth/oidc/providers", + json={ + "name": "UI provider", + "issuer_url": "https://other.example.com", + "client_id": "ui", + "client_secret": "ui-secret", + "scopes": "openid", + "is_enabled": True, + "auto_create_users": False, + }, + headers=_auth_header(token), + ) + provider_id = created.json()["id"] + + response = await async_client.put( + f"/api/v1/auth/oidc/providers/{provider_id}", + json={"name": "Renamed"}, + headers=_auth_header(token), + ) + + assert response.status_code == 200 + assert response.json()["name"] == "Renamed" + + +@pytest.mark.asyncio +@pytest.mark.integration +async def test_the_response_says_which_provider_is_env_managed(async_client: AsyncClient, db_session): + """The frontend needs this to render the lock; without it the UI would show + editable fields whose writes the API then refuses.""" + await _env_managed_provider(db_session) + token = await _setup_and_login(async_client, "envlockflag", "envlockflag123") + + response = await async_client.get("/api/v1/auth/oidc/providers/all", headers=_auth_header(token)) + + assert response.status_code == 200 + providers = response.json() + assert any(p["is_env_managed"] for p in providers)