diff --git a/apps/alert_processor/lib/model/user.ex b/apps/alert_processor/lib/model/user.ex index 2b85ed538..406e12a4f 100644 --- a/apps/alert_processor/lib/model/user.ex +++ b/apps/alert_processor/lib/model/user.ex @@ -240,6 +240,18 @@ defmodule AlertProcessor.Model.User do @spec get(id()) :: t() | nil def get(id), do: Repo.get(__MODULE__, id) + @spec get_by_alternate_id(%{id: id(), mbta_uuid: id() | nil}) :: [t()] + def get_by_alternate_id(%{id: id, mbta_uuid: mbta_uuid}) do + # can get accounts using id or mbta_uuid + # which is the old-style ID for accounts from the time before SSO. + + from(u in __MODULE__, + where: u.id in [^id, ^mbta_uuid] + ) + |> Repo.all() + |> Enum.filter(&(!is_nil(&1))) + end + @spec for_email(String.t()) :: t | nil def for_email(email) do email = diff --git a/apps/alert_processor/test/alert_processor/model/user_test.exs b/apps/alert_processor/test/alert_processor/model/user_test.exs index f4f0148c4..4c443a981 100644 --- a/apps/alert_processor/test/alert_processor/model/user_test.exs +++ b/apps/alert_processor/test/alert_processor/model/user_test.exs @@ -204,6 +204,34 @@ defmodule AlertProcessor.Model.UserTest do end end + describe "get_by_alternate_id/1" do + test "returns a user by id if present" do + user = insert(:user) + assert User.get_by_alternate_id(%{id: user.id, mbta_uuid: nil}) == [user] + end + + test "returns a user by mbta_uuid if present" do + user = insert(:user) + assert User.get_by_alternate_id(%{id: nil, mbta_uuid: user.id}) == [user] + end + + test "returns a user if both id's are the same " do + user = insert(:user) + assert User.get_by_alternate_id(%{id: user.id, mbta_uuid: user.id}) == [user] + end + + test "returns two users if both ids present and different (unlikely)" do + user = insert(:user) + user2 = insert(:user) + assert User.get_by_alternate_id(%{id: user.id, mbta_uuid: user2.id}) == [user, user2] + end + + test "returns empty_list if no matching user" do + bad_id = UUID.uuid4() + assert User.get_by_alternate_id(%{id: bad_id, mbta_uuid: nil}) == [] + end + end + describe "for_email/1" do test "returns a user if present" do user = insert(:user) diff --git a/apps/concierge_site/lib/controllers/auth_controller.ex b/apps/concierge_site/lib/controllers/auth_controller.ex index 1c2097748..b746db304 100644 --- a/apps/concierge_site/lib/controllers/auth_controller.ex +++ b/apps/concierge_site/lib/controllers/auth_controller.ex @@ -21,7 +21,7 @@ defmodule ConciergeSite.AuthController do extra: %{ raw_info: %{ claims: %{"sub" => id}, - userinfo: userinfo + userinfo: %{"mbta_uuid" => mbta_uuid} = userinfo } } } = auth @@ -34,9 +34,8 @@ defmodule ConciergeSite.AuthController do role = parse_role({:ok, userinfo}) user = - id + %{id: id, mbta_uuid: mbta_uuid} |> get_or_create_user(email, phone_number, role) - |> use_props_from_token(email, phone_number, role) logout_params = %{ post_logout_redirect_uri: page_url(conn, :landing) @@ -44,9 +43,17 @@ defmodule ConciergeSite.AuthController do {:ok, logout_uri} = UeberauthOidcc.initiate_logout_url(auth, logout_params) - conn - |> put_session("logout_uri", logout_uri) - |> SessionHelper.sign_in(user) + case user do + nil -> + SessionHelper.sign_out(conn, skip_oidc_sign_out: true) + + _ -> + user = use_props_from_token(user, email, phone_number, role) + + conn + |> put_session("logout_uri", logout_uri) + |> SessionHelper.sign_in(user) + end end def callback(%{assigns: %{ueberauth_failure: failure}} = conn, _params) do @@ -67,11 +74,20 @@ defmodule ConciergeSite.AuthController do SessionHelper.sign_out(conn) end - @spec get_or_create_user(User.id(), String.t(), String.t() | nil, String.t()) :: User.t() - defp get_or_create_user(id, email, phone_number, role) do - case User.get(id) do - nil -> - # The user just created their account in Keycloak so we need to add them to our database + @spec get_or_create_user( + %{id: User.id(), mbta_uuid: User.id() | nil}, + String.t(), + String.t() | nil, + String.t() + ) :: + User.t() | nil + defp get_or_create_user(%{id: id, mbta_uuid: mbta_uuid} = id_map, email, phone_number, role) do + # This checks both the normal id from Keycloak, and the legacy mbta_uuid. We should get either 0 or 1 users back. + user_list = User.get_by_alternate_id(id_map) + + case length(user_list) do + 0 -> + # If neither ID is found, the user just created their account in Keycloak so we need to add them to our database Repo.insert!(%User{ id: id, email: email, @@ -81,8 +97,14 @@ defmodule ConciergeSite.AuthController do User.get(id) - user -> - user + 1 -> + # If 1 user is found, we want to return that user + hd(user_list) + + 2 -> + # If 2 users are found, something weird happened. Log and return nil. User will be redirected to landing page. + Logger.warn("User with 2 ids found. sub id: #{id}, mbta_uuid: #{mbta_uuid}") + nil end end @@ -92,7 +114,13 @@ defmodule ConciergeSite.AuthController do # able to use them to send notifications), but in cases where the user just # changed one of these fields in Keycloak, we might not have had time receive # that message yet, so the values in the token are more authoritative. - @spec use_props_from_token(User.t(), String.t(), String.t() | nil, String.t()) :: User.t() + @spec use_props_from_token( + User.t(), + String.t(), + String.t() | nil, + String.t() + ) :: + User.t() | nil defp use_props_from_token(user, email, phone_number, role) do %User{ user diff --git a/apps/concierge_site/test/web/controllers/auth_controller_test.exs b/apps/concierge_site/test/web/controllers/auth_controller_test.exs index c8042373b..6df83ece0 100644 --- a/apps/concierge_site/test/web/controllers/auth_controller_test.exs +++ b/apps/concierge_site/test/web/controllers/auth_controller_test.exs @@ -65,6 +65,40 @@ defmodule ConciergeSite.Web.AuthControllerTest do } = Guardian.Plug.current_resource(conn) end + test "can use mbta_uuid user", + %{conn: conn, rider: %User{id: id, email: email, phone_number: phone_number}} do + auth = auth_for(nil, email, phone_number, ["user"], id) + + conn = + conn + |> assign(:ueberauth_auth, auth) + |> get("/auth/keycloak/callback") + + assert %User{id: ^id, email: ^email, phone_number: ^phone_number} = + Guardian.Plug.current_resource(conn) + end + + @tag capture_log: true + test "redirects if we somehow get 2 users", + %{conn: conn, rider: %User{id: id, email: email, phone_number: phone_number}} do + user2 = + Repo.insert!(%User{ + email: "rider2@example.com", + phone_number: "5551234567", + role: "user" + }) + + auth = auth_for(id, email, phone_number, ["user"], user2.id) + + conn = + conn + |> assign(:ueberauth_auth, auth) + |> get("/auth/keycloak/callback") + + assert is_nil(Guardian.Plug.current_resource(conn)) + assert redirected_to(conn) == "/" + end + test "doesn't allow admin access if the token says they are now just a user", %{conn: conn} do was_an_admin = @@ -110,7 +144,7 @@ defmodule ConciergeSite.Web.AuthControllerTest do end @spec auth_for(User.id(), String.t(), String.t() | nil) :: Auth.t() - defp auth_for(id, email, phone_number, roles \\ ["user"]) do + defp auth_for(id, email, phone_number, roles \\ ["user"], mbta_uuid \\ nil) do %Auth{ uid: email, provider: :keycloak, @@ -147,6 +181,7 @@ defmodule ConciergeSite.Web.AuthControllerTest do "name" => "John Rider", "phone_number" => phone_number, "preferred_username" => email, + "mbta_uuid" => mbta_uuid, "resource_access" => %{ "t-alerts" => %{ "roles" => roles