From c5f1fdce0a855b6505ec057ed4b48364953a2f11 Mon Sep 17 00:00:00 2001 From: Moritz Date: Wed, 4 Feb 2026 19:44:43 +0100 Subject: [PATCH] Code-review follow-ups: policy, docs, seed_admin behaviour - Use OidcRoleSyncContext for set_role_from_oidc_sync; document JWT peek risk. - seed_admin without password sets Admin role on existing user (OIDC-only); update docs and test. - Fix DE translation for 'access this page'; add get? true comment in User. --- docs/admin-bootstrap-and-oidc-role-sync.md | 4 +-- lib/accounts/user.ex | 5 ++-- .../checks/oidc_role_sync_context.ex | 12 +++------ lib/mv/oidc_role_sync.ex | 10 +++++++ lib/mv/release.ex | 27 +++++++++++++++++-- priv/gettext/de/LC_MESSAGES/default.po | 2 +- test/mv/release_test.exs | 10 ++++--- 7 files changed, 51 insertions(+), 19 deletions(-) diff --git a/docs/admin-bootstrap-and-oidc-role-sync.md b/docs/admin-bootstrap-and-oidc-role-sync.md index 87dad27..b0da019 100644 --- a/docs/admin-bootstrap-and-oidc-role-sync.md +++ b/docs/admin-bootstrap-and-oidc-role-sync.md @@ -10,12 +10,12 @@ ### Environment Variables - `ADMIN_EMAIL` – Email of the admin user to create/update. If unset, seed_admin/0 does nothing. -- `ADMIN_PASSWORD` – Password for the admin user. If unset (and no file), no user is created in production. +- `ADMIN_PASSWORD` – Password for the admin user. If unset (and no file), no new user is created; if a user with ADMIN_EMAIL already exists (e.g. OIDC-only), their role is set to Admin (no password change). - `ADMIN_PASSWORD_FILE` – Path to a file containing the password (e.g. Docker secret). ### Release Task -- `Mv.Release.seed_admin/0` – Reads ADMIN_EMAIL and password from ADMIN_PASSWORD or ADMIN_PASSWORD_FILE. If both are set, creates or updates the user with the Admin role. Idempotent. +- `Mv.Release.seed_admin/0` – Reads ADMIN_EMAIL and password from ADMIN_PASSWORD or ADMIN_PASSWORD_FILE. If both email and password are set: creates or updates the user with the Admin role. If only ADMIN_EMAIL is set: sets the Admin role on an existing user with that email (for OIDC-only admins); does not create a user. Idempotent. ### Entrypoint diff --git a/lib/accounts/user.ex b/lib/accounts/user.ex index 8e7e70f..2f35ce4 100644 --- a/lib/accounts/user.ex +++ b/lib/accounts/user.ex @@ -258,6 +258,7 @@ defmodule Mv.Accounts.User do end read :sign_in_with_rauthy do + # Single record expected; required for AshAuthentication OAuth2 strategy (returns list of 0 or 1). get? true argument :user_info, :map, allow_nil?: false argument :oauth_tokens, :map, allow_nil?: false @@ -356,10 +357,10 @@ defmodule Mv.Accounts.User do end # set_role_from_oidc_sync: internal only (called from Mv.OidcRoleSync on registration/sign-in). - # Not exposed in code_interface; must never be callable by clients. + # Not exposed in code_interface; only allowed when context.private.oidc_role_sync is set. bypass action(:set_role_from_oidc_sync) do description "Internal: OIDC role sync (server-side only)" - authorize_if always() + authorize_if Mv.Authorization.Checks.OidcRoleSyncContext end # UPDATE/DESTROY via HasPermission (evaluates PermissionSets scope) diff --git a/lib/mv/authorization/checks/oidc_role_sync_context.ex b/lib/mv/authorization/checks/oidc_role_sync_context.ex index 1f39944..1214d75 100644 --- a/lib/mv/authorization/checks/oidc_role_sync_context.ex +++ b/lib/mv/authorization/checks/oidc_role_sync_context.ex @@ -1,9 +1,9 @@ defmodule Mv.Authorization.Checks.OidcRoleSyncContext do @moduledoc """ - Policy check: true when the action is being run from OIDC role sync (context.private.oidc_role_sync). + Policy check: true when the action is run from OIDC role sync (context.private.oidc_role_sync). - Used to allow the internal set_role_from_oidc_sync action when called by Mv.OidcRoleSync - without an actor. + Used to allow the internal set_role_from_oidc_sync action only when called by Mv.OidcRoleSync, + which sets context.private.oidc_role_sync when performing the update. """ use Ash.Policy.SimpleCheck @@ -12,11 +12,7 @@ defmodule Mv.Authorization.Checks.OidcRoleSyncContext do @impl true def match?(_actor, authorizer, _opts) do - # Context from opts (e.g. Ash.update!(..., context: %{private: %{oidc_role_sync: true}})) context = Map.get(authorizer, :context) || %{} - from_context = get_in(context, [:private, :oidc_role_sync]) == true - # When update runs inside create's after_action, context may not be passed; use process dict. - from_process = Process.get(:oidc_role_sync) == true - from_context or from_process + get_in(context, [:private, :oidc_role_sync]) == true end end diff --git a/lib/mv/oidc_role_sync.ex b/lib/mv/oidc_role_sync.ex index 369b2b4..9073409 100644 --- a/lib/mv/oidc_role_sync.ex +++ b/lib/mv/oidc_role_sync.ex @@ -10,6 +10,16 @@ defmodule Mv.OidcRoleSync do the access_token from oauth_tokens is decoded as JWT and the groups claim is read from there (e.g. Rauthy puts groups in the access token when scope includes "groups"). + + ## JWT access token (security) + + The access_token payload is read without signature verification (peek only). + We rely on the fact that `oauth_tokens` is only ever passed from the + verified OIDC callback (Assent/AshAuthentication after provider token + exchange). If callers passed untrusted or tampered tokens, group claims + could be forged and a user could be assigned the Admin role. Therefore: + do not call this module with user-supplied tokens; it is intended only + for the internal flow from the OIDC callback. """ alias Mv.Accounts.User alias Mv.Authorization.Role diff --git a/lib/mv/release.ex b/lib/mv/release.ex index 45b0c9d..8893dcc 100644 --- a/lib/mv/release.ex +++ b/lib/mv/release.ex @@ -52,14 +52,37 @@ defmodule Mv.Release do :ok is_nil(admin_password) or admin_password == "" -> - # Do not create or update any user without a password (no fallback in production) - :ok + ensure_admin_role_only(admin_email) true -> ensure_admin_user(admin_email, admin_password) end end + defp ensure_admin_role_only(email) do + case Role.get_admin_role() do + {:ok, nil} -> + :ok + + {:ok, %Role{} = admin_role} -> + case get_user_by_email(email) do + {:ok, %User{} = user} -> + user + |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.manage_relationship(:role, admin_role, type: :append_and_remove) + |> Ash.update!(authorize?: false) + + :ok + + _ -> + :ok + end + + {:error, _} -> + :ok + end + end + defp ensure_admin_user(email, password) do if is_nil(password) or password == "" do :ok diff --git a/priv/gettext/de/LC_MESSAGES/default.po b/priv/gettext/de/LC_MESSAGES/default.po index 90dddc8..6ba8022 100644 --- a/priv/gettext/de/LC_MESSAGES/default.po +++ b/priv/gettext/de/LC_MESSAGES/default.po @@ -2306,7 +2306,7 @@ msgstr "Import/Export" #: lib/mv_web/live/import_export_live.ex #, elixir-autogen, elixir-format, fuzzy msgid "You do not have permission to access this page." -msgstr "Du hast keine Berechtigung, auf diese*n Benutzer*in zuzugreifen" +msgstr "Du hast keine Berechtigung, auf diese Seite zuzugreifen." #: lib/mv_web/live/import_export_live.ex #, elixir-autogen, elixir-format, fuzzy diff --git a/test/mv/release_test.exs b/test/mv/release_test.exs index 1879c1d..84a2f34 100644 --- a/test/mv/release_test.exs +++ b/test/mv/release_test.exs @@ -44,18 +44,20 @@ defmodule Mv.ReleaseTest do "seed_admin must not create any user when ADMIN_PASSWORD is unset (expected #{user_count_before}, got #{count_users()})" end - test "with ADMIN_EMAIL but without ADMIN_PASSWORD and user exists: leaves user and role unchanged" do + test "with ADMIN_EMAIL but without ADMIN_PASSWORD and user exists: sets Admin role (OIDC-only bootstrap)" do + System.delete_env("ADMIN_PASSWORD") + System.delete_env("ADMIN_PASSWORD_FILE") + email = "existing-admin-#{System.unique_integer([:positive])}@test.example.com" System.put_env("ADMIN_EMAIL", email) on_exit(fn -> System.delete_env("ADMIN_EMAIL") end) - {:ok, user} = create_user_with_mitglied_role(email) - role_id_before = user.role_id + {:ok, _user} = create_user_with_mitglied_role(email) Mv.Release.seed_admin() {:ok, updated} = get_user_by_email(email) - assert updated.role_id == role_id_before + assert updated.role_id == admin_role_id() end test "with ADMIN_EMAIL and ADMIN_PASSWORD: creates user with Admin role and sets password" do