rommapp/romm
1
0
Fork 0
mirror of https://github.com/rommapp/romm.git synced 2026-10-11 22:40:04 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(oidc): match any of the comma-separated groups in OIDC_ROLE_* (#5203)

Browse Source 
* fix(oidc): match any of the comma-separated groups in OIDC_ROLE_*

OIDC_ROLE_ADMIN/EDITOR/VIEWER were read as one raw string and checked
for membership in the roles claim, so a value like
"romm-admin,platform-admins" never matched and the login was rejected
with 403. Parse each variable into a set of stripped, non-empty values
and grant the role when it intersects the claim. A single-valued string
claim is matched whole rather than by substring.

Fixes #4302

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>

* test(oidc): reject a string role claim that only contains a mapped group

Addresses Greptile's review on #5203: the string-claim case only covered a
match, which the old substring check would also have passed.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>

---------

Co-authored-by: Claude Opus 5.5 (1M context) <[email protected]>
This commit is contained in:
Georges-Antoine Assi
2026-10-09 13:56:00 -04:00
committed by GitHub
co-authored by Claude Opus 5.5
parent 92b6421e65
commit de62fa3815
6 changed files with 84 additions and 21 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/config/__init__.py
+8 -3
View File
@@ -23,6 +23,11 @@ def _get_env(var: str, fallback: str | None = None) -> str | None:
return val.strip() if val else val
def _get_env_set(var: str) -> frozenset[str]:
"""Parse a comma-separated env var into its non-empty, stripped values."""
return frozenset(v.strip() for v in (_get_env(var) or "").split(",") if v.strip())
ROMM_BASE_URL: Final[str] = _get_env("ROMM_BASE_URL", "http://0.0.0.0")
ROMM_PORT: Final[int] = safe_int(_get_env("ROMM_PORT"), 8080)
@@ -254,9 +259,9 @@ OIDC_REDIRECT_URI: Final[str] = _get_env("OIDC_REDIRECT_URI", "")
OIDC_SERVER_APPLICATION_URL: Final[str] = _get_env("OIDC_SERVER_APPLICATION_URL", "")
OIDC_SERVER_METADATA_URL: Final[str | None] = _get_env("OIDC_SERVER_METADATA_URL")
OIDC_CLAIM_ROLES: Final[str] = _get_env("OIDC_CLAIM_ROLES", "")
OIDC_ROLE_VIEWER: Final[str | None] = _get_env("OIDC_ROLE_VIEWER")
OIDC_ROLE_EDITOR: Final[str | None] = _get_env("OIDC_ROLE_EDITOR")
OIDC_ROLE_ADMIN: Final[str | None] = _get_env("OIDC_ROLE_ADMIN")
OIDC_ROLE_VIEWER: Final[frozenset[str]] = _get_env_set("OIDC_ROLE_VIEWER")
OIDC_ROLE_EDITOR: Final[frozenset[str]] = _get_env_set("OIDC_ROLE_EDITOR")
OIDC_ROLE_ADMIN: Final[frozenset[str]] = _get_env_set("OIDC_ROLE_ADMIN")
OIDC_TLS_CACERTFILE: Final[str | None] = _get_env("OIDC_TLS_CACERTFILE")
OIDC_USERNAME_ATTRIBUTE: Final[str] = _get_env(
"OIDC_USERNAME_ATTRIBUTE", "preferred_username"
backend/handler/auth/base_handler.py
+8 -5
View File
@@ -662,12 +662,15 @@ class OpenIDHandler:
role = Role.USER
claims_provided = OIDC_CLAIM_ROLES and OIDC_CLAIM_ROLES in userinfo
if claims_provided:
roles = userinfo[OIDC_CLAIM_ROLES] or []
if OIDC_ROLE_ADMIN and OIDC_ROLE_ADMIN in roles:
claim = userinfo[OIDC_CLAIM_ROLES] or []
# A single-valued claim must match whole, not by its characters.
roles = {claim} if isinstance(claim, str) else set(claim)
if OIDC_ROLE_ADMIN & roles:
role = Role.ADMIN
elif (OIDC_ROLE_EDITOR and OIDC_ROLE_EDITOR in roles) or (
OIDC_ROLE_VIEWER
and (OIDC_ROLE_VIEWER in roles or OIDC_ROLE_VIEWER == "*")
elif (
OIDC_ROLE_EDITOR & roles
or OIDC_ROLE_VIEWER & roles
or "*" in OIDC_ROLE_VIEWER
):
role = Role.USER
else:
backend/tests/config/test_env.py
+24
View File
@@ -0,0 +1,24 @@
import pytest
from config import _get_env_set
@pytest.mark.parametrize(
"value,expected",
[
(None, frozenset()),
("", frozenset()),
("admins", frozenset({"admins"})),
("romm-admin,platform-admins", frozenset({"romm-admin", "platform-admins"})),
(
" romm-admin , ,platform-admins, ",
frozenset({"romm-admin", "platform-admins"}),
),
],
)
def test_get_env_set(monkeypatch, value, expected):
if value is None:
monkeypatch.delenv("ROMM_TEST_SET", raising=False)
else:
monkeypatch.setenv("ROMM_TEST_SET", value)
assert _get_env_set("ROMM_TEST_SET") == expected
backend/tests/handler/auth/test_oidc.py
+40 -9
View File
@@ -176,9 +176,31 @@ async def test_oidc_valid_token_decoding(
[{}, ["editor", "viewer"], Role.USER],
[{}, ["viewer"], Role.USER],
# OIDC viewer role fallback.
[{"OIDC_ROLE_VIEWER": "*"}, [], Role.USER],
[{"OIDC_ROLE_VIEWER": frozenset({"*"})}, [], Role.USER],
# Any one of several mapped groups grants the role.
[
{"OIDC_ROLE_ADMIN": frozenset({"romm-admin", "platform-admins"})},
["users", "platform-admins"],
Role.ADMIN,
],
[
{"OIDC_ROLE_VIEWER": frozenset({"romm-viewer", "family"})},
["family"],
Role.USER,
],
# A single-valued claim matches whole.
[{}, "admin", Role.ADMIN],
],
ids=[
"no-mapping",
"admin-role",
"editor-role",
"viewer-role",
"viewer-fallback",
"admin-any-of-groups",
"viewer-any-of-groups",
"string-claim",
],
ids=["no-mapping", "admin-role", "editor-role", "viewer-role", "viewer-fallback"],
)
async def test_oidc_valid_add_user(
mocker,
@@ -197,15 +219,15 @@ async def test_oidc_valid_add_user(
)
mocker.patch(
"handler.auth.base_handler.OIDC_ROLE_ADMIN",
config_override.get("OIDC_ROLE_ADMIN", "admin"),
config_override.get("OIDC_ROLE_ADMIN", frozenset({"admin"})),
)
mocker.patch(
"handler.auth.base_handler.OIDC_ROLE_EDITOR",
config_override.get("OIDC_ROLE_EDITOR", "editor"),
config_override.get("OIDC_ROLE_EDITOR", frozenset({"editor"})),
)
mocker.patch(
"handler.auth.base_handler.OIDC_ROLE_VIEWER",
config_override.get("OIDC_ROLE_VIEWER", "viewer"),
config_override.get("OIDC_ROLE_VIEWER", frozenset({"viewer"})),
)
mock_token["userinfo"]["roles"] = token_roles
mock_user = MagicMock(enabled=True, role=Role.USER)
@@ -449,7 +471,7 @@ async def test_oidc_valid_edit_user_role(
):
"""Test role change for existing user on login based on OIDC role mapping."""
mocker.patch("handler.auth.base_handler.OIDC_CLAIM_ROLES", "roles")
mocker.patch("handler.auth.base_handler.OIDC_ROLE_ADMIN", "admin")
mocker.patch("handler.auth.base_handler.OIDC_ROLE_ADMIN", frozenset({"admin"}))
mock_token["userinfo"]["roles"] = ["admin"]
mock_user = MagicMock(enabled=True, role=Role.USER)
mocker.patch(
@@ -499,15 +521,24 @@ async def test_oidc_valid_no_edit_user_role_if_mapping_disabled(
mock_edit_user.assert_not_called()
@pytest.mark.parametrize(
"token_roles",
[["not-mapped"], "superadmin"],
ids=["unmapped-group", "string-claim-substring"],
)
async def test_oidc_invalid_user_no_roles(
mocker,
mock_oidc_enabled,
mock_token,
mock_openid_configuration,
token_roles,
):
"""Test valid token response for user with no roles/access to this application."""
mocker.patch("handler.auth.base_handler.OIDC_CLAIM_ROLES", "roles")
mock_token["userinfo"]["roles"] = ["not-mapped"]
mocker.patch("handler.auth.base_handler.OIDC_ROLE_ADMIN", frozenset({"admin"}))
mocker.patch("handler.auth.base_handler.OIDC_ROLE_EDITOR", frozenset({"editor"}))
mocker.patch("handler.auth.base_handler.OIDC_ROLE_VIEWER", frozenset({"viewer"}))
mock_token["userinfo"]["roles"] = token_roles
mocker.patch.object(
StarletteOAuth2App,
"load_server_metadata",
@@ -581,7 +612,7 @@ async def test_oidc_valid_no_edit_user_role_if_claim_not_in_userinfo(
):
"""Test that role is not changed for existing user on login if OIDC role claim is configured but not provided by the OIDC provider."""
mocker.patch("handler.auth.base_handler.OIDC_CLAIM_ROLES", "roles")
mocker.patch("handler.auth.base_handler.OIDC_ROLE_ADMIN", "admin")
mocker.patch("handler.auth.base_handler.OIDC_ROLE_ADMIN", frozenset({"admin"}))
# The OIDC provider does NOT include the roles claim in userinfo
mock_token["userinfo"].pop("roles", None)
mock_user = MagicMock(enabled=True, role=Role.ADMIN)
@@ -637,7 +668,7 @@ async def test_oidc_fetches_missing_claims_from_userinfo_endpoint(
mock_openid_configuration,
):
mocker.patch("handler.auth.base_handler.OIDC_CLAIM_ROLES", "groups")
mocker.patch("handler.auth.base_handler.OIDC_ROLE_ADMIN", "admins")
mocker.patch("handler.auth.base_handler.OIDC_ROLE_ADMIN", frozenset({"admins"}))
mocker.patch.object(
StarletteOAuth2App,
"load_server_metadata",
docs/BACKEND_ARCHITECTURE.md
+1 -1
View File
@@ -1932,7 +1932,7 @@ Falls back to `FakeRedis` in test mode.
| `OIDC_REDIRECT_URI` | | Redirect URI |
| `OIDC_USERNAME_ATTRIBUTE` | `preferred_username` | Username claim |
| `OIDC_CLAIM_ROLES` | | Roles claim name |
| `OIDC_ROLE_VIEWER/EDITOR/ADMIN` | | Role mappings |
| `OIDC_ROLE_VIEWER/EDITOR/ADMIN` | | Comma-separated role values |
| `OIDC_TLS_CACERTFILE` | | Custom CA bundle for OIDC calls |
| `OIDC_RP_INITIATED_LOGOUT` | `false` | Send logout to OIDC provider |
| `OIDC_END_SESSION_ENDPOINT` | | End-session URL override |
env.template
+3 -3
View File
@@ -59,9 +59,9 @@ OIDC_REDIRECT_URI= # Absolute redirect URI for OIDC authentication
OIDC_SERVER_APPLICATION_URL= # Absolute URL of the OIDC server application
OIDC_SERVER_METADATA_URL= # URL to the OIDC provider metadata endpoint
OIDC_CLAIM_ROLES= # OIDC claim containing user roles
OIDC_ROLE_VIEWER= # Role value mapping to viewer permissions
OIDC_ROLE_EDITOR= # Role value mapping to editor permissions
OIDC_ROLE_ADMIN= # Role value mapping to admin permissions
OIDC_ROLE_VIEWER= # Comma-separated role values mapping to viewer permissions (* matches any role)
OIDC_ROLE_EDITOR= # Comma-separated role values mapping to editor permissions
OIDC_ROLE_ADMIN= # Comma-separated role values mapping to admin permissions
OIDC_TLS_CACERTFILE= # CA bundle file (PEM, DER or .p7b) or directory, trusted alongside the system CAs
OIDC_USERNAME_ATTRIBUTE=preferred_username # Attribute on OIDC user info used as the username
OIDC_RP_INITIATED_LOGOUT=false # Enable RP-initiated logout flow