From 6907b2ed3c8ce5d455337b236203b8611d0f72ff Mon Sep 17 00:00:00 2001 From: Moritz Date: Thu, 24 Jul 2025 15:30:34 +0200 Subject: [PATCH 1/3] feat: fail if oidc provide does not provide a sub or id --- lib/accounts/user.ex | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/lib/accounts/user.ex b/lib/accounts/user.ex index 2da15a1..d8c7a66 100644 --- a/lib/accounts/user.ex +++ b/lib/accounts/user.ex @@ -105,6 +105,8 @@ defmodule Mv.Accounts.User do upsert? true upsert_identity :unique_oidc_id + validate &__MODULE__.validate_oidc_id_present/2 + change AshAuthentication.GenerateTokenChange change fn changeset, _ctx -> @@ -125,6 +127,16 @@ defmodule Mv.Accounts.User do end end + def validate_oidc_id_present(changeset, _context) do + user_info = Ash.Changeset.get_argument(changeset, :user_info) || %{} + + if is_binary(user_info["sub"]) or is_binary(user_info["id"]) do + :ok + else + {:error, [user_info: "OIDC user_info must contain a non-empty 'sub' or 'id' field"]} + end + end + attributes do uuid_primary_key :id From 85071096319051e4c617a40acd94abf233d0eb15 Mon Sep 17 00:00:00 2001 From: Moritz Date: Wed, 23 Jul 2025 00:26:06 +0200 Subject: [PATCH 2/3] feat: test authentication --- lib/accounts/accounts.ex | 2 + .../controllers/auth_controller_test.exs | 190 +++++++++++++++--- .../controllers/oidc_integration_test.exs | 166 +++++++++++++++ 3 files changed, 335 insertions(+), 23 deletions(-) create mode 100644 test/mv_web/controllers/oidc_integration_test.exs diff --git a/lib/accounts/accounts.ex b/lib/accounts/accounts.ex index 333e12e..7bc5073 100644 --- a/lib/accounts/accounts.ex +++ b/lib/accounts/accounts.ex @@ -15,6 +15,8 @@ defmodule Mv.Accounts do define :list_users, action: :read define :update_user, action: :update_user define :destroy_user, action: :destroy + define :create_register_with_rauthy, action: :register_with_rauthy + define :read_sign_in_with_rauthy, action: :sign_in_with_rauthy end resource Mv.Accounts.Token diff --git a/test/mv_web/controllers/auth_controller_test.exs b/test/mv_web/controllers/auth_controller_test.exs index e18ade2..3c3956d 100644 --- a/test/mv_web/controllers/auth_controller_test.exs +++ b/test/mv_web/controllers/auth_controller_test.exs @@ -1,40 +1,121 @@ defmodule MvWeb.AuthControllerTest do use MvWeb.ConnCase, async: true + import Phoenix.LiveViewTest + # Basic UI tests test "GET /sign-in shows sign in form", %{conn: conn} do conn = get(conn, ~p"/sign-in") assert html_response(conn, 200) =~ "Sign in" end - test "POST /sign-in with valid credentials redirects to home", %{conn: conn} do - # Create a test user first - conn = conn_with_oidc_user(conn) - conn = get(conn, ~p"/sign-in") - - assert redirected_to(conn) == ~p"/" - end - - test "POST /sign-in with invalid credentials shows error", %{conn: conn} do - conn = - post(conn, ~p"/auth/sign_in", %{ - "user" => %{ - "email" => "wrong@example.com", - "password" => "wrongpassword" - } - }) - - assert conn.status == 404 - end - test "GET /sign-out redirects to home", %{conn: conn} do - # First sign in a user conn = conn_with_oidc_user(conn) - - # Then sign out conn = get(conn, ~p"/sign-out") assert redirected_to(conn) == ~p"/" end + # Password authentication (LiveView) + test "password user can sign in with valid credentials via LiveView", %{conn: conn} do + _user = + create_test_user(%{ + email: "password@example.com", + password: "secret123", + oidc_id: nil + }) + + {:ok, view, _html} = live(conn, "/sign-in") + + {:error, {:redirect, %{to: to}}} = + view + |> form("#user-password-sign-in-with-password", + user: %{email: "password@example.com", password: "secret123"} + ) + |> render_submit() + + assert to =~ "/auth/user/password/sign_in_with_token" + end + + test "password user with invalid credentials shows error via LiveView", %{conn: conn} do + _user = + create_test_user(%{ + email: "test@example.com", + password: "correct_password", + oidc_id: nil + }) + + {:ok, view, _html} = live(conn, "/sign-in") + + html = + view + |> form("#user-password-sign-in-with-password", + user: %{email: "test@example.com", password: "wrong_password"} + ) + |> render_submit() + + assert html =~ "Email or password was incorrect" + end + + test "password user with non-existent email shows error via LiveView", %{conn: conn} do + {:ok, view, _html} = live(conn, "/sign-in") + + html = + view + |> form("#user-password-sign-in-with-password", + user: %{email: "nonexistent@example.com", password: "anypassword"} + ) + |> render_submit() + + assert html =~ "Email or password was incorrect" + end + + # Registration (LiveView) + test "user can register with valid credentials via LiveView", %{conn: conn} do + {:ok, view, _html} = live(conn, "/register") + + {:error, {:redirect, %{to: to}}} = + view + |> form("#user-password-register-with-password-wrapper form", + user: %{email: "newuser@example.com", password: "newpassword123"} + ) + |> render_submit() + + assert to =~ "/auth/user/password/sign_in_with_token" + end + + test "registration with existing email shows error via LiveView", %{conn: conn} do + _user = + create_test_user(%{ + email: "existing@example.com", + password: "secret123", + oidc_id: nil + }) + + {:ok, view, _html} = live(conn, "/register") + + html = + view + |> form("#user-password-register-with-password-wrapper form", + user: %{email: "existing@example.com", password: "anotherpassword"} + ) + |> render_submit() + + assert html =~ "has already been taken" + end + + test "registration with weak password shows error via LiveView", %{conn: conn} do + {:ok, view, _html} = live(conn, "/register") + + html = + view + |> form("#user-password-register-with-password-wrapper form", + user: %{email: "weakpass@example.com", password: "123"} + ) + |> render_submit() + + assert html =~ "length must be greater than or equal to 8" + end + + # Access control test "unauthenticated user accessing protected route gets redirected to sign-in", %{conn: conn} do conn = get(conn, ~p"/members") assert redirected_to(conn) == ~p"/sign-in" @@ -45,4 +126,67 @@ defmodule MvWeb.AuthControllerTest do conn = get(conn, ~p"/members") assert conn.status == 200 end + + test "password authenticated user can access protected route via LiveView", %{conn: conn} do + _user = + create_test_user(%{ + email: "auth@example.com", + password: "secret123", + oidc_id: nil + }) + + {:ok, view, _html} = live(conn, "/sign-in") + + {:error, {:redirect, %{to: to}}} = + view + |> form("#user-password-sign-in-with-password", + user: %{email: "auth@example.com", password: "secret123"} + ) + |> render_submit() + + assert to =~ "/auth/user/password/sign_in_with_token" + + # After login, user is redirected to /auth/user/password/sign_in_with_token. Session handling for protected routes should be tested in integration or E2E tests. + end + + # Edge cases + test "user with nil oidc_id can still sign in with password via LiveView", %{conn: conn} do + _user = + create_test_user(%{ + email: "nil_oidc@example.com", + password: "secret123", + oidc_id: nil + }) + + {:ok, view, _html} = live(conn, "/sign-in") + + {:error, {:redirect, %{to: to}}} = + view + |> form("#user-password-sign-in-with-password", + user: %{email: "nil_oidc@example.com", password: "secret123"} + ) + |> render_submit() + + assert to =~ "/auth/user/password/sign_in_with_token" + end + + test "user with empty string oidc_id is handled correctly via LiveView", %{conn: conn} do + _user = + create_test_user(%{ + email: "empty_oidc@example.com", + password: "secret123", + oidc_id: "" + }) + + {:ok, view, _html} = live(conn, "/sign-in") + + {:error, {:redirect, %{to: to}}} = + view + |> form("#user-password-sign-in-with-password", + user: %{email: "empty_oidc@example.com", password: "secret123"} + ) + |> render_submit() + + assert to =~ "/auth/user/password/sign_in_with_token" + end end diff --git a/test/mv_web/controllers/oidc_integration_test.exs b/test/mv_web/controllers/oidc_integration_test.exs new file mode 100644 index 0000000..a96e7b1 --- /dev/null +++ b/test/mv_web/controllers/oidc_integration_test.exs @@ -0,0 +1,166 @@ +defmodule MvWeb.OidcIntegrationTest do + use MvWeb.ConnCase, async: true + + # Test OIDC callback scenarios by directly calling the actions + # This simulates what happens during real OIDC authentication + + describe "OIDC sign-in scenarios" do + test "existing OIDC user with unchanged email can sign in" do + # Create user with OIDC ID + user = + create_test_user(%{ + email: "existing@example.com", + oidc_id: "existing_oidc_123" + }) + + # Simulate OIDC callback data + user_info = %{ + "sub" => "existing_oidc_123", + "preferred_username" => "existing@example.com" + } + + # Test sign_in_with_rauthy action directly + {:ok, [found_user]} = + Mv.Accounts.read_sign_in_with_rauthy(%{ + user_info: user_info, + oauth_tokens: %{} + }) + + assert found_user.id == user.id + assert to_string(found_user.email) == "existing@example.com" + assert found_user.oidc_id == "existing_oidc_123" + end + + test "new OIDC user gets created via register_with_rauthy" do + # Simulate OIDC callback for completely new user + user_info = %{ + "sub" => "brand_new_oidc_456", + "preferred_username" => "newuser@example.com" + } + + # Test register_with_rauthy action + case Mv.Accounts.create_register_with_rauthy(%{ + user_info: user_info, + oauth_tokens: %{} + }) do + {:ok, new_user} -> + assert to_string(new_user.email) == "newuser@example.com" + assert new_user.oidc_id == "brand_new_oidc_456" + assert is_nil(new_user.hashed_password) + + {:error, error} -> + flunk("Should have created new user: #{inspect(error)}") + end + end + end + + describe "OIDC error and edge case scenarios" do + test "OIDC registration with conflicting email and OIDC ID shows error" do + # Create user with email and OIDC ID + _existing_user = + create_test_user(%{ + email: "conflict@example.com", + oidc_id: "oidc_conflict_1" + }) + + # Try to register with same email but different OIDC ID + user_info = %{ + "sub" => "oidc_conflict_2", + "preferred_username" => "conflict@example.com" + } + + result = + Mv.Accounts.create_register_with_rauthy(%{ + user_info: user_info, + oauth_tokens: %{} + }) + + # Should fail due to unique constraint + assert {:error, %Ash.Error.Invalid{errors: errors}} = result + + assert Enum.any?(errors, fn + %Ash.Error.Changes.InvalidAttribute{field: :email, message: message} -> + String.contains?(message, "has already been taken") + + _ -> + false + end) + end + + test "OIDC registration with missing sub and id should fail" do + user_info = %{ + "preferred_username" => "nosub@example.com" + } + + result = + Mv.Accounts.create_register_with_rauthy(%{ + user_info: user_info, + oauth_tokens: %{} + }) + + assert {:error, + %Ash.Error.Invalid{ + errors: [%Ash.Error.Changes.InvalidChanges{vars: [user_info: msg]}] + }} = result + + assert String.contains?(msg, "OIDC user_info must contain a non-empty 'sub' or 'id' field") + end + + test "OIDC registration with missing preferred_username should fail" do + user_info = %{ + "sub" => "noemail_oidc_123" + } + + result = + Mv.Accounts.create_register_with_rauthy(%{ + user_info: user_info, + oauth_tokens: %{} + }) + + assert {:error, %Ash.Error.Invalid{errors: errors}} = result + + assert Enum.any?(errors, fn err -> + match?(%Ash.Error.Changes.Required{field: :email}, err) + end) + end + + test "OIDC registration with existing OIDC ID and different email updates email" do + existing_user = + create_test_user(%{ + email: "old@example.com", + oidc_id: "oidc_update_email" + }) + + user_info = %{ + "sub" => "oidc_update_email", + "preferred_username" => "new@example.com" + } + + {:ok, user} = + Mv.Accounts.create_register_with_rauthy(%{ + user_info: user_info, + oauth_tokens: %{} + }) + + assert user.id == existing_user.id + assert to_string(user.email) == "new@example.com" + assert user.oidc_id == "oidc_update_email" + end + + test "OIDC registration with alternative OIDC ID field (id instead of sub)" do + user_info = %{ + "id" => "alt_oidc_id_123", + "preferred_username" => "altid@example.com" + } + + {:ok, user} = + Mv.Accounts.create_register_with_rauthy(%{ + user_info: user_info, + oauth_tokens: %{} + }) + + assert user.oidc_id == "alt_oidc_id_123" + assert to_string(user.email) == "altid@example.com" + end + end +end From fcb71f9360a62ab033b1a8de2ae3bb16bcf9c0fa Mon Sep 17 00:00:00 2001 From: Renovate Bot Date: Thu, 31 Jul 2025 12:30:03 +0000 Subject: [PATCH 3/3] chore(deps): update dependency postgrex to v0.21.0 --- mix.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mix.lock b/mix.lock index 2060db6..3f0af49 100644 --- a/mix.lock +++ b/mix.lock @@ -60,7 +60,7 @@ "phoenix_view": {:hex, :phoenix_view, "2.0.4", "b45c9d9cf15b3a1af5fb555c674b525391b6a1fe975f040fb4d913397b31abf4", [:mix], [{:phoenix_html, "~> 2.14.2 or ~> 3.0 or ~> 4.0", [hex: :phoenix_html, repo: "hexpm", optional: true]}, {:phoenix_template, "~> 1.0", [hex: :phoenix_template, repo: "hexpm", optional: false]}], "hexpm", "4e992022ce14f31fe57335db27a28154afcc94e9983266835bb3040243eb620b"}, "plug": {:hex, :plug, "1.18.1", "5067f26f7745b7e31bc3368bc1a2b818b9779faa959b49c934c17730efc911cf", [:mix], [{:mime, "~> 1.0 or ~> 2.0", [hex: :mime, repo: "hexpm", optional: false]}, {:plug_crypto, "~> 1.1.1 or ~> 1.2 or ~> 2.0", [hex: :plug_crypto, repo: "hexpm", optional: false]}, {:telemetry, "~> 0.4.3 or ~> 1.0", [hex: :telemetry, repo: "hexpm", optional: false]}], "hexpm", "57a57db70df2b422b564437d2d33cf8d33cd16339c1edb190cd11b1a3a546cc2"}, "plug_crypto": {:hex, :plug_crypto, "2.1.1", "19bda8184399cb24afa10be734f84a16ea0a2bc65054e23a62bb10f06bc89491", [:mix], [], "hexpm", "6470bce6ffe41c8bd497612ffde1a7e4af67f36a15eea5f921af71cf3e11247c"}, - "postgrex": {:hex, :postgrex, "0.20.0", "363ed03ab4757f6bc47942eff7720640795eb557e1935951c1626f0d303a3aed", [:mix], [{:db_connection, "~> 2.1", [hex: :db_connection, repo: "hexpm", optional: false]}, {:decimal, "~> 1.5 or ~> 2.0", [hex: :decimal, repo: "hexpm", optional: false]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: true]}, {:table, "~> 0.1.0", [hex: :table, repo: "hexpm", optional: true]}], "hexpm", "d36ef8b36f323d29505314f704e21a1a038e2dc387c6409ee0cd24144e187c0f"}, + "postgrex": {:hex, :postgrex, "0.21.0", "f44797ac23604af2640e84b98aa535b454b61fbd7da0f4034adba1787701913e", [:mix], [{:db_connection, "~> 2.1", [hex: :db_connection, repo: "hexpm", optional: false]}, {:decimal, "~> 1.5 or ~> 2.0", [hex: :decimal, repo: "hexpm", optional: false]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: true]}, {:table, "~> 0.1.0", [hex: :table, repo: "hexpm", optional: true]}], "hexpm", "c35cd8f18b5c59da08eff27cb40c8533c561320af830821c740efd7e97c8ac9f"}, "reactor": {:hex, :reactor, "0.15.6", "d717f9add549b25a089a94c90197718d2d838e35d81dd776b1d81587d4cf2aaa", [:mix], [{:igniter, "~> 0.4", [hex: :igniter, repo: "hexpm", optional: true]}, {:iterex, "~> 0.1", [hex: :iterex, repo: "hexpm", optional: false]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: false]}, {:libgraph, "~> 0.16", [hex: :libgraph, repo: "hexpm", optional: false]}, {:spark, "~> 2.0", [hex: :spark, repo: "hexpm", optional: false]}, {:splode, "~> 0.2", [hex: :splode, repo: "hexpm", optional: false]}, {:telemetry, "~> 1.2", [hex: :telemetry, repo: "hexpm", optional: false]}, {:yaml_elixir, "~> 2.11", [hex: :yaml_elixir, repo: "hexpm", optional: false]}, {:ymlr, "~> 5.0", [hex: :ymlr, repo: "hexpm", optional: false]}], "hexpm", "74db98165e3644d86e0f723672d91ceca4339eaa935bcad7e78bf146a46d77b9"}, "req": {:hex, :req, "0.5.15", "662020efb6ea60b9f0e0fac9be88cd7558b53fe51155a2d9899de594f9906ba9", [:mix], [{:brotli, "~> 0.3.1", [hex: :brotli, repo: "hexpm", optional: true]}, {:ezstd, "~> 1.0", [hex: :ezstd, repo: "hexpm", optional: true]}, {:finch, "~> 0.17", [hex: :finch, repo: "hexpm", optional: false]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: false]}, {:mime, "~> 2.0.6 or ~> 2.1", [hex: :mime, repo: "hexpm", optional: false]}, {:nimble_csv, "~> 1.0", [hex: :nimble_csv, repo: "hexpm", optional: true]}, {:plug, "~> 1.0", [hex: :plug, repo: "hexpm", optional: true]}], "hexpm", "a6513a35fad65467893ced9785457e91693352c70b58bbc045b47e5eb2ef0c53"}, "rewrite": {:hex, :rewrite, "1.1.2", "f5a5d10f5fed1491a6ff48e078d4585882695962ccc9e6c779bae025d1f92eda", [:mix], [{:glob_ex, "~> 0.1", [hex: :glob_ex, repo: "hexpm", optional: false]}, {:sourceror, "~> 1.0", [hex: :sourceror, repo: "hexpm", optional: false]}, {:text_diff, "~> 0.1", [hex: :text_diff, repo: "hexpm", optional: false]}], "hexpm", "7f8b94b1e3528d0a47b3e8b7bfeca559d2948a65fa7418a9ad7d7712703d39d4"},