Jelajahi Sumber

fix(oidc): treat a blank optional env var as unset, not a refusal

BAMBUDDY_OIDC_SCOPES, _EMAIL_CLAIM and _ICON_URL fell back to their
default only when the key was absent, so `BAMBUDDY_OIDC_ICON_URL=` in
a compose file (as .env.example ships it, commented) reached the
schema validator as an empty string and got the whole provider
refused. default_group already treated blank as unset; these three
now follow the same rule.
Marian 1 bulan lalu
induk
melakukan
c547505c64

+ 3 - 3
backend/app/core/oidc_env.py

@@ -51,13 +51,13 @@ def read_env_oidc_config() -> dict | None:
         "issuer_url": os.environ["BAMBUDDY_OIDC_ISSUER_URL"],
         "issuer_url": os.environ["BAMBUDDY_OIDC_ISSUER_URL"],
         "client_id": os.environ["BAMBUDDY_OIDC_CLIENT_ID"],
         "client_id": os.environ["BAMBUDDY_OIDC_CLIENT_ID"],
         "client_secret": os.environ["BAMBUDDY_OIDC_CLIENT_SECRET"],
         "client_secret": os.environ["BAMBUDDY_OIDC_CLIENT_SECRET"],
-        "scopes": os.environ.get("BAMBUDDY_OIDC_SCOPES", "openid email profile"),
+        "scopes": (os.environ.get("BAMBUDDY_OIDC_SCOPES") or "").strip() or "openid email profile",
         "is_enabled": _env_bool("BAMBUDDY_OIDC_ENABLED", True),
         "is_enabled": _env_bool("BAMBUDDY_OIDC_ENABLED", True),
         "auto_create_users": _env_bool("BAMBUDDY_OIDC_AUTO_CREATE_USERS", False),
         "auto_create_users": _env_bool("BAMBUDDY_OIDC_AUTO_CREATE_USERS", False),
         "auto_link_existing_accounts": _env_bool("BAMBUDDY_OIDC_AUTO_LINK_EXISTING", False),
         "auto_link_existing_accounts": _env_bool("BAMBUDDY_OIDC_AUTO_LINK_EXISTING", False),
-        "email_claim": os.environ.get("BAMBUDDY_OIDC_EMAIL_CLAIM", "email"),
+        "email_claim": (os.environ.get("BAMBUDDY_OIDC_EMAIL_CLAIM") or "").strip() or "email",
         "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") or "").strip() or None,
         "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
         # 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
         # file would point at a different group on every deployment. Resolved

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

@@ -547,6 +547,41 @@ async def test_an_empty_group_variable_counts_as_unset(db_session, monkeypatch):
     assert provider.default_group_id is None
     assert provider.default_group_id is None
 
 
 
 
+# --- blank optional strings count as unset, not a refusal ---------------------
+# `.env.example` ships `# BAMBUDDY_OIDC_ICON_URL=` commented out, so uncommenting
+# it must not take the provider down -- same rule default_group already follows.
+
+
+@pytest.mark.asyncio
+async def test_a_blank_scopes_still_creates_the_provider(db_session, monkeypatch):
+    _configure(monkeypatch, BAMBUDDY_OIDC_SCOPES="")
+    await apply_env_oidc_provider(db_session)
+
+    provider = await _env_provider(db_session)
+    assert provider is not None, "a blank optional var must not refuse the whole provider"
+    assert provider.scopes == "openid email profile"
+
+
+@pytest.mark.asyncio
+async def test_a_blank_email_claim_still_creates_the_provider(db_session, monkeypatch):
+    _configure(monkeypatch, BAMBUDDY_OIDC_EMAIL_CLAIM="")
+    await apply_env_oidc_provider(db_session)
+
+    provider = await _env_provider(db_session)
+    assert provider is not None, "a blank optional var must not refuse the whole provider"
+    assert provider.email_claim == "email"
+
+
+@pytest.mark.asyncio
+async def test_a_blank_icon_url_still_creates_the_provider(db_session, monkeypatch):
+    _configure(monkeypatch, BAMBUDDY_OIDC_ICON_URL="")
+    await apply_env_oidc_provider(db_session)
+
+    provider = await _env_provider(db_session)
+    assert provider is not None, "a blank optional var must not refuse the whole provider"
+    assert provider.icon_url is None
+
+
 # --- account links and collision behavior ------------------------------------
 # --- account links and collision behavior ------------------------------------
 
 
 
 

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

@@ -119,6 +119,33 @@ 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"
 
 
 
 
+@pytest.mark.parametrize("raw", ["", "   "])
+def test_a_blank_scopes_is_unset(monkeypatch, raw):
+    """`BAMBUDDY_OIDC_SCOPES=` in a compose file is a forgotten value, not a
+    request for a provider with no scopes -- same rule as default_group."""
+    _set_required(monkeypatch)
+    monkeypatch.setenv("BAMBUDDY_OIDC_SCOPES", raw)
+    assert read_env_oidc_config()["scopes"] == "openid email profile"
+
+
+@pytest.mark.parametrize("raw", ["", "   "])
+def test_a_blank_email_claim_is_unset(monkeypatch, raw):
+    _set_required(monkeypatch)
+    monkeypatch.setenv("BAMBUDDY_OIDC_EMAIL_CLAIM", raw)
+    assert read_env_oidc_config()["email_claim"] == "email"
+
+
+@pytest.mark.parametrize("raw", ["", "   "])
+def test_a_blank_icon_url_is_unset(monkeypatch, raw):
+    """Uncommenting `# BAMBUDDY_OIDC_ICON_URL=` in .env.example must not take
+    the provider down -- the reader must still return a config, not refuse it."""
+    _set_required(monkeypatch)
+    monkeypatch.setenv("BAMBUDDY_OIDC_ICON_URL", raw)
+    cfg = read_env_oidc_config()
+    assert cfg is not None, "a blank optional var must not refuse the whole provider"
+    assert cfg["icon_url"] is None
+
+
 def test_the_default_group_is_read_as_a_name(monkeypatch):
 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
     """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."""
     file would point at whatever group happened to be created third."""