Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Login in with mbta_uuid #1449

Merged
merged 2 commits into from
Jun 17, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions apps/alert_processor/lib/model/user.ex
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,15 @@ 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() | nil, mbta_uuid: id() | nil}) :: [t()]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (non-blocking): It might be helpful to have a quick description here describing how mbta_uuid is the old-style ID for accounts from the time before SSO.

def get_by_alternate_id(%{id: id, mbta_uuid: mbta_uuid}) do
from(u in __MODULE__,
where: u.id in [^id, ^mbta_uuid]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: I think it will be possible for a little while for id and mbta_uuid to be the same value. If that's the case, will this just do the "right" think and return a single result (if there is one)? I'm guessing that's probably the case? Would it be worth a quick test case maybe just to demonstrate the possibility and show it behaving correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was surprised, but Repo.all() apparently won't return duplicate records, so it does behave correctly in that case. I'll add a test around it.

)
|> Repo.all()
|> Enum.filter(&(!is_nil(&1)))
end

@spec for_email(String.t()) :: t | nil
def for_email(email) do
email =
Expand Down
23 changes: 23 additions & 0 deletions apps/alert_processor/test/alert_processor/model/user_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,29 @@ 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 two users if both present (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)
Expand Down
55 changes: 42 additions & 13 deletions apps/concierge_site/lib/controllers/auth_controller.ex
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ defmodule ConciergeSite.AuthController do
extra: %{
raw_info: %{
claims: %{"sub" => id},
userinfo: userinfo
userinfo: %{"mbta_uuid" => mbta_uuid} = userinfo
}
}
} = auth
Expand All @@ -34,7 +34,7 @@ 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)

Expand All @@ -44,9 +44,15 @@ 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (non-blocking): If you were to move this case statement up so that it's examining the results of get_or_create_user, then you could skip a bunch of the following lines in the case where you get the result :find_user_error, and use_props_from_token wouldn't have to be changed to handle that case.

nil ->
SessionHelper.sign_out(conn, skip_oidc_sign_out: true)

_ ->
conn
|> put_session("logout_uri", logout_uri)
|> SessionHelper.sign_in(user)
end
end

def callback(%{assigns: %{ueberauth_failure: failure}} = conn, _params) do
Expand All @@ -67,11 +73,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() | nil, mbta_uuid: User.id() | nil},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: I don't think id can ever be nil here, right? (Which is good, because otherwise you'd have to do something about it when you go to create a new account.) This also applies to User.get_by_alternate_id.

String.t(),
String.t() | nil,
String.t()
) ::
User.t() | :find_user_error
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,
Expand All @@ -81,8 +96,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}")
:find_user_error
end
end

Expand All @@ -92,7 +113,15 @@ 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() | :find_user_error,
String.t(),
String.t() | nil,
String.t()
) ::
User.t() | nil
defp use_props_from_token(:find_user_error, _, _, _), do: nil

defp use_props_from_token(user, email, phone_number, role) do
%User{
user
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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: "[email protected]",
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 =
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down
Loading