Jelajahi Sumber

feat(oidc): set the default group from the environment, by name

Without it every account auto-created through the env provider fell back to
Viewers (routes/mfa.py), and because the provider is locked the UI could not
correct it either -- a real limitation for a declarative deployment running
BAMBUDDY_OIDC_AUTO_CREATE_USERS=true.

BAMBUDDY_OIDC_DEFAULT_GROUP names a group rather than an id: ids are handed out
per installation, so the same compose file would point at a different group on
the next deployment. The name is matched exactly, resolved against the database
before anything is written, and default_group_id joins _APPLIED_FIELDS so
dropping the variable clears the group again -- the environment is the whole
truth for this row.

A name that matches no group is refused rather than defaulted: silently landing
users in Viewers is the failure this variable exists to remove, and the API
already answers 422 for a default_group_id that does not exist. The refusal is
logged and survivable, and it says which of the two cases happened, because
they differ sharply -- an existing provider keeps running on its last good
config, while on a first boot nothing is created and no SSO button appears.

Raised by maziggy in review of #2625 as a scope decision; documented in
.env.example and in the companion wiki PR.
Marian 1 bulan lalu
induk
melakukan
3c679459e1

+ 10 - 0
.env.example

@@ -98,9 +98,19 @@ LOG_TO_FILE=true
 # BAMBUDDY_OIDC_REQUIRE_EMAIL_VERIFIED=true
 # BAMBUDDY_OIDC_REQUIRE_EMAIL_VERIFIED=true
 # BAMBUDDY_OIDC_ICON_URL=
 # BAMBUDDY_OIDC_ICON_URL=
 # BAMBUDDY_OIDC_AUTOLOGIN=false
 # BAMBUDDY_OIDC_AUTOLOGIN=false
+# BAMBUDDY_OIDC_DEFAULT_GROUP=
 #
 #
 # Booleans accept true/1/yes; anything else keeps the default.
 # Booleans accept true/1/yes; anything else keeps the default.
 #
 #
+# DEFAULT_GROUP is the group new users land in when AUTO_CREATE_USERS is on;
+# without it they get Viewers. It matches a group NAME exactly (case-sensitive)
+# -- group ids are assigned per install, so the same compose file would point at
+# a different group on every deployment. A name that matches no group is
+# refused: the provider is left as it was and the reason is logged, rather than
+# quietly creating under-privileged users the locked UI could not correct. On a
+# FIRST boot that means no provider is created at all and no SSO button appears
+# -- create the group first. Removing the variable clears the group again.
+#
 # AUTO_LINK_EXISTING binds an OIDC identity to an existing local account with
 # AUTO_LINK_EXISTING binds an OIDC identity to an existing local account with
 # the same email address. With EMAIL_CLAIM=email it is refused unless
 # the same email address. With EMAIL_CLAIM=email it is refused unless
 # REQUIRE_EMAIL_VERIFIED=true, because an identity provider that does not
 # REQUIRE_EMAIL_VERIFIED=true, because an identity provider that does not

+ 1 - 0
backend/app/core/config.py

@@ -149,6 +149,7 @@ _INTENTIONAL_UNSETTINGS = {
     "BAMBUDDY_OIDC_REQUIRE_EMAIL_VERIFIED",
     "BAMBUDDY_OIDC_REQUIRE_EMAIL_VERIFIED",
     "BAMBUDDY_OIDC_ICON_URL",
     "BAMBUDDY_OIDC_ICON_URL",
     "BAMBUDDY_OIDC_AUTOLOGIN",
     "BAMBUDDY_OIDC_AUTOLOGIN",
+    "BAMBUDDY_OIDC_DEFAULT_GROUP",
 }
 }
 
 
 _known_settings_fields = {f.upper() for f in settings.model_fields}
 _known_settings_fields = {f.upper() for f in settings.model_fields}

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

@@ -58,6 +58,11 @@ def read_env_oidc_config() -> dict | None:
         "require_email_verified": _env_bool("BAMBUDDY_OIDC_REQUIRE_EMAIL_VERIFIED", True),
         "require_email_verified": _env_bool("BAMBUDDY_OIDC_REQUIRE_EMAIL_VERIFIED", True),
         "icon_url": os.environ.get("BAMBUDDY_OIDC_ICON_URL"),
         "icon_url": os.environ.get("BAMBUDDY_OIDC_ICON_URL"),
         "is_autologin": _env_bool("BAMBUDDY_OIDC_AUTOLOGIN", False),
         "is_autologin": _env_bool("BAMBUDDY_OIDC_AUTOLOGIN", False),
+        # A name, not an id: ids are assigned per install, so the same compose
+        # file would point at a different group on every deployment. Resolved
+        # against the database in apply_env_oidc_provider -- the reader has no
+        # session and stays dumb.
+        "default_group": (os.environ.get("BAMBUDDY_OIDC_DEFAULT_GROUP") or "").strip() or None,
     }
     }
 
 
 
 
@@ -75,6 +80,10 @@ _APPLIED_FIELDS = (
     "require_email_verified",
     "require_email_verified",
     "icon_url",
     "icon_url",
     "is_autologin",
     "is_autologin",
+    # Written on every boot, so a group that is no longer declared is cleared:
+    # the environment is the whole truth for this row, and the API lock means
+    # a lingering value could not be removed in the UI either.
+    "default_group_id",
 )
 )
 
 
 
 
@@ -86,6 +95,7 @@ async def apply_env_oidc_provider(db: AsyncSession) -> None:
     """
     """
     # Imported here rather than at module scope: app.core is imported by the
     # Imported here rather than at module scope: app.core is imported by the
     # models themselves, so a top-level import would be a cycle.
     # models themselves, so a top-level import would be a cycle.
+    from backend.app.models.group import Group
     from backend.app.models.oidc_provider import OIDCProvider
     from backend.app.models.oidc_provider import OIDCProvider
     from backend.app.schemas.auth import OIDCProviderCreate
     from backend.app.schemas.auth import OIDCProviderCreate
 
 
@@ -128,6 +138,27 @@ async def apply_env_oidc_provider(db: AsyncSession) -> None:
     # function runs in the lifespan, so the app would not boot.
     # 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()
     existing = (await db.execute(select(OIDCProvider).where(OIDCProvider.name == config["name"]))).scalar_one_or_none()
 
 
+    # Resolved before anything is written, so a name that matches no group
+    # leaves the running provider untouched. Refused rather than defaulted:
+    # falling back would put every auto-created user in Viewers (routes/mfa.py)
+    # for as long as the typo lives, and the API answers 422 for a
+    # default_group_id that does not exist -- env config gets the same answer.
+    group_name = config.pop("default_group", None)
+    if group_name is not None:
+        group = (await db.execute(select(Group).where(Group.name == group_name))).scalar_one_or_none()
+        if group is None:
+            # Spelled out because the two cases differ sharply: an existing
+            # provider keeps running on its last good config, while on a first
+            # boot nothing is created at all and the login page has no SSO
+            # button until the name matches.
+            logger.error(
+                "BAMBUDDY_OIDC_DEFAULT_GROUP=%r matches no group, provider not applied (%s).",
+                group_name,
+                "previous config left running" if existing is not None else "no provider created",
+            )
+            return
+        config["default_group_id"] = group.id
+
     try:
     try:
         # The same schema the API uses, so env config cannot reach a state the
         # 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).
         # UI would have refused (notably the SEC-1 auto-link check).

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

@@ -8,6 +8,7 @@ recreating the provider would silently unlink every account bound to it.
 from __future__ import annotations
 from __future__ import annotations
 
 
 import logging
 import logging
+import os
 
 
 import pytest
 import pytest
 from sqlalchemy import select
 from sqlalchemy import select
@@ -32,6 +33,7 @@ ALL_VARS = (
     "BAMBUDDY_OIDC_REQUIRE_EMAIL_VERIFIED",
     "BAMBUDDY_OIDC_REQUIRE_EMAIL_VERIFIED",
     "BAMBUDDY_OIDC_ICON_URL",
     "BAMBUDDY_OIDC_ICON_URL",
     "BAMBUDDY_OIDC_AUTOLOGIN",
     "BAMBUDDY_OIDC_AUTOLOGIN",
+    "BAMBUDDY_OIDC_DEFAULT_GROUP",
 )
 )
 
 
 
 
@@ -378,6 +380,96 @@ async def test_releasing_the_provider_clears_autologin(db_session, monkeypatch):
     assert released.is_autologin is False
     assert released.is_autologin is False
 
 
 
 
+# --- default group by name -----------------------------------------------------
+# Group ids are not stable across installs, so a declarative deployment cannot
+# name one by id. Without this, every auto-created user falls back to Viewers
+# (routes/mfa.py) and the env lock means the UI cannot correct the provider.
+
+
+async def _group(db_session, name: str):
+    from backend.app.models.group import Group
+
+    group = Group(name=name, description=f"Test group {name}")
+    db_session.add(group)
+    await db_session.commit()
+    return group
+
+
+@pytest.mark.asyncio
+async def test_the_default_group_is_resolved_by_name(db_session, monkeypatch):
+    group = await _group(db_session, "Operators")
+    _configure(monkeypatch, BAMBUDDY_OIDC_DEFAULT_GROUP="Operators")
+
+    await apply_env_oidc_provider(db_session)
+
+    assert (await _env_provider(db_session)).default_group_id == group.id
+
+
+@pytest.mark.asyncio
+async def test_an_unknown_group_name_is_rejected_rather_than_defaulted(db_session, monkeypatch, caplog):
+    """Silently falling back to Viewers is how a typo mints under-privileged
+    users for weeks. The API answers 400 for a default_group_id that does not
+    exist; env config gets the same answer, logged and survivable."""
+    _configure(monkeypatch, BAMBUDDY_OIDC_DEFAULT_GROUP="Nope")
+
+    with caplog.at_level(logging.ERROR):
+        await apply_env_oidc_provider(db_session)
+
+    assert await _env_provider(db_session) is None
+    assert "BAMBUDDY_OIDC_DEFAULT_GROUP" in caplog.text
+    assert "Nope" in caplog.text
+
+
+@pytest.mark.asyncio
+async def test_an_unknown_group_name_leaves_the_previous_provider_intact(db_session, monkeypatch):
+    """Rejection happens before the upsert, so the running config survives a
+    bad edit -- the provider keeps working until the operator fixes the name."""
+    group = await _group(db_session, "Operators")
+    _configure(monkeypatch, BAMBUDDY_OIDC_DEFAULT_GROUP="Operators")
+    await apply_env_oidc_provider(db_session)
+
+    monkeypatch.setenv("BAMBUDDY_OIDC_DEFAULT_GROUP", "Typo")
+    await apply_env_oidc_provider(db_session)
+
+    provider = await _env_provider(db_session)
+    assert provider is not None
+    assert provider.default_group_id == group.id
+
+
+@pytest.mark.asyncio
+async def test_no_group_variable_leaves_the_default_group_unset(db_session, monkeypatch):
+    _configure(monkeypatch)
+    await apply_env_oidc_provider(db_session)
+
+    assert (await _env_provider(db_session)).default_group_id is None
+
+
+@pytest.mark.asyncio
+async def test_removing_the_group_variable_clears_the_default_group(db_session, monkeypatch):
+    """The environment is the whole truth for this row; a group that is no
+    longer declared must not linger, since the lock blocks removing it in the UI."""
+    await _group(db_session, "Operators")
+    _configure(monkeypatch, BAMBUDDY_OIDC_DEFAULT_GROUP="Operators")
+    await apply_env_oidc_provider(db_session)
+
+    monkeypatch.delenv("BAMBUDDY_OIDC_DEFAULT_GROUP")
+    await apply_env_oidc_provider(db_session)
+
+    assert (await _env_provider(db_session)).default_group_id is None
+
+
+@pytest.mark.asyncio
+async def test_an_empty_group_variable_counts_as_unset(db_session, monkeypatch):
+    """Same rule the required vars follow: an empty value in a compose file is
+    a forgotten value, not a request to reject the config."""
+    _configure(monkeypatch, BAMBUDDY_OIDC_DEFAULT_GROUP="")
+    await apply_env_oidc_provider(db_session)
+
+    provider = await _env_provider(db_session)
+    assert provider is not None
+    assert provider.default_group_id is None
+
+
 # --- account links and collision behavior ------------------------------------
 # --- account links and collision behavior ------------------------------------
 
 
 
 
@@ -488,6 +580,36 @@ async def test_renaming_with_autologin_updates_the_exclusivity_sweep(db_session,
     assert ui_provider.is_autologin is False
     assert ui_provider.is_autologin is False
 
 
 
 
+@pytest.mark.asyncio
+async def test_group_name_matching_is_case_sensitive(db_session, monkeypatch, caplog):
+    """Group name is resolved by exact match; 'operators' != 'Operators'."""
+    await _group(db_session, "Operators")  # capital O
+    _configure(monkeypatch, BAMBUDDY_OIDC_DEFAULT_GROUP="operators")  # lowercase
+
+    with caplog.at_level(logging.ERROR):
+        await apply_env_oidc_provider(db_session)
+
+    # Config is rejected
+    assert await _env_provider(db_session) is None
+    assert "operators" in caplog.text
+    assert "BAMBUDDY_OIDC_DEFAULT_GROUP" in caplog.text
+
+
+@pytest.mark.asyncio
+async def test_group_name_rejection_does_not_log_the_secret(db_session, monkeypatch, caplog):
+    """Group resolution happens before schema validation, so the secret is
+    not yet in scope, but verify it's not leaked by the error path."""
+    _configure(monkeypatch, BAMBUDDY_OIDC_DEFAULT_GROUP="NonExistent")
+    secret = os.environ["BAMBUDDY_OIDC_CLIENT_SECRET"]
+
+    with caplog.at_level(logging.ERROR):
+        await apply_env_oidc_provider(db_session)
+
+    # Config is rejected but secret is safe
+    assert await _env_provider(db_session) is None
+    assert secret not in caplog.text
+
+
 @pytest.mark.asyncio
 @pytest.mark.asyncio
 async def test_restoring_env_config_after_rename_then_unset_finds_the_original_row(db_session, monkeypatch):
 async def test_restoring_env_config_after_rename_then_unset_finds_the_original_row(db_session, monkeypatch):
     """Rename Keycloak → Authentik, unset everything, restore Keycloak.
     """Rename Keycloak → Authentik, unset everything, restore Keycloak.

+ 18 - 0
backend/tests/unit/test_oidc_env_reader.py

@@ -28,6 +28,7 @@ OPTIONAL = (
     "BAMBUDDY_OIDC_REQUIRE_EMAIL_VERIFIED",
     "BAMBUDDY_OIDC_REQUIRE_EMAIL_VERIFIED",
     "BAMBUDDY_OIDC_ICON_URL",
     "BAMBUDDY_OIDC_ICON_URL",
     "BAMBUDDY_OIDC_AUTOLOGIN",
     "BAMBUDDY_OIDC_AUTOLOGIN",
+    "BAMBUDDY_OIDC_DEFAULT_GROUP",
 )
 )
 
 
 
 
@@ -118,6 +119,23 @@ def test_optional_strings_override_their_defaults(monkeypatch):
     assert cfg["icon_url"] == "https://sso.example.com/logo.png"
     assert cfg["icon_url"] == "https://sso.example.com/logo.png"
 
 
 
 
+def test_the_default_group_is_read_as_a_name(monkeypatch):
+    """A name, not an id: group ids differ per install, so an id in a compose
+    file would point at whatever group happened to be created third."""
+    _set_required(monkeypatch)
+    monkeypatch.setenv("BAMBUDDY_OIDC_DEFAULT_GROUP", "Operators")
+    cfg = read_env_oidc_config()
+    assert cfg["default_group"] == "Operators"
+    assert "default_group_id" not in cfg, "resolution needs the database, not the reader"
+
+
+@pytest.mark.parametrize("raw", ["", "   "])
+def test_a_blank_default_group_is_unset(monkeypatch, raw):
+    _set_required(monkeypatch)
+    monkeypatch.setenv("BAMBUDDY_OIDC_DEFAULT_GROUP", raw)
+    assert read_env_oidc_config()["default_group"] is None
+
+
 def test_every_var_the_reader_knows_is_registered_in_the_typo_guard():
 def test_every_var_the_reader_knows_is_registered_in_the_typo_guard():
     """An unregistered BAMBUDDY_* var logs "possible typo" at every boot, which
     """An unregistered BAMBUDDY_* var logs "possible typo" at every boot, which
     would tell operators their correct config is wrong. Asserted against the
     would tell operators their correct config is wrong. Asserted against the