From a9fbf9406ea50d49d00edf09946053453c6c550e Mon Sep 17 00:00:00 2001 From: Walton Hoops Date: Wed, 13 Nov 2024 14:34:50 -0700 Subject: [PATCH] feat: Keycloak token refresh (#1028) * fix: remove unneeded KEYCLOAK_IDP_HINT We set this inside Keycloak now, so it's not required here. * feat: session and idle timeouts * chore: setup keycloak locally * fix: use cognito by default for tests * chore: create/update stop within live session --------- Co-authored-by: Paul Swartz --- .envrc.example | 7 +++- README.md | 1 + config/config.exs | 20 ++++++++--- config/runtime.exs | 7 ---- config/test.exs | 4 ++- lib/arrow/stops.ex | 18 ++++++++++ lib/arrow/ueberauth/strategy/fake.ex | 6 +++- lib/arrow_web/auth_manager.ex | 23 ++++++++++++ lib/arrow_web/auth_manager/error_handler.ex | 24 +++++++++++-- lib/arrow_web/auth_manager/pipeline.ex | 18 ++++++++++ lib/arrow_web/controllers/auth_controller.ex | 16 +++++++-- lib/arrow_web/live/stop_live/stop_live.ex | 35 +++++++++++-------- test/arrow/stops_test.exs | 9 +++++ .../auth_manager/error_handler_test.exs | 2 +- .../controllers/auth_controller_test.exs | 6 ++-- .../live/stop_live/stop_live_test.exs | 27 +++++++------- 16 files changed, 172 insertions(+), 51 deletions(-) diff --git a/.envrc.example b/.envrc.example index 3aece74a..925515cd 100644 --- a/.envrc.example +++ b/.envrc.example @@ -3,4 +3,9 @@ export DATABASE_PASSWORD=postgres export ARROW_DOMAIN=https://arrow.mbta.com export ARROW_API_KEY= export AWS_ACCESS_KEY_ID= -export AWS_SECRET_ACCESS_KEY= \ No newline at end of file +export AWS_SECRET_ACCESS_KEY= +export KEYCLOAK_ISSUER=https://login-dev.mbtace.com/auth/realms/MBTA +export KEYCLOAK_API_BASE=https://login-dev.mbtace.com/auth/admin/realms/MBTA/ +export KEYCLOAK_CLIENT_ID=arrow-dev +export KEYCLOAK_CLIENT_UUID=bd84a8e2-2fce-4c7a-bfe3-3c7ac71fb5b2 +export KEYCLOAK_CLIENT_SECRET= \ No newline at end of file diff --git a/README.md b/README.md index 9b7b986e..7bc244b5 100644 --- a/README.md +++ b/README.md @@ -24,6 +24,7 @@ - `cp .envrc.example .envrc` - Update `.envrc` with your local Postgres username and password - Update `.envrc` with your AWS credentials or ensure they are available in your shell +- Update `.envrc` with the Arrow Dev Keycloak client secret (found in 1Password) - `mix ecto.setup` - `brew install chromedriver` - Add your Arrow API key from https://arrow.mbta.com/mytoken to `.envrc` diff --git a/config/config.exs b/config/config.exs index f420b399..8ff064bf 100644 --- a/config/config.exs +++ b/config/config.exs @@ -20,8 +20,8 @@ config :arrow, # map cognito groups to roles "arrow-admin" => "admin" }, - ueberauth_provider: :cognito, - api_login_module: ArrowWeb.TryApiTokenAuth.Cognito, + ueberauth_provider: :keycloak, + api_login_module: ArrowWeb.TryApiTokenAuth.Keycloak, required_roles: %{ view_disruption: ["read-only", "admin"], create_disruption: ["admin"], @@ -103,14 +103,26 @@ config :tailwind, cd: Path.expand("../assets", __DIR__) ] -config :arrow, ArrowWeb.AuthManager, issuer: "arrow" +# 12 hours in seconds +max_session_time = 12 * 60 * 60 + +config :arrow, ArrowWeb.AuthManager, + issuer: "arrow", + max_session_time: max_session_time, + # 30 minutes + idle_time: 30 * 60 config :ueberauth, Ueberauth, providers: [ cognito: {Ueberauth.Strategy.Cognito, []}, keycloak: {Ueberauth.Strategy.Oidcc, - issuer: :keycloak_issuer, userinfo: true, uid_field: "email", scopes: ~w"openid email"} + issuer: :keycloak_issuer, + userinfo: true, + uid_field: "email", + scopes: ~w"openid email", + authorization_params: %{max_age: "#{max_session_time}"}, + authorization_params_passthrough: ~w"prompt login_hint"} ] config :ueberauth, Ueberauth.Strategy.Cognito, diff --git a/config/runtime.exs b/config/runtime.exs index eb71a19e..dcfc7a3e 100644 --- a/config/runtime.exs +++ b/config/runtime.exs @@ -26,13 +26,6 @@ if is_binary(keycloak_issuer) and not is_test? do client_secret: System.fetch_env!("KEYCLOAK_CLIENT_SECRET") ] - keycloak_opts = - if keycloak_idp = System.get_env("KEYCLOAK_IDP_HINT") do - Keyword.put(keycloak_opts, :authorization_params, %{kc_idp_hint: keycloak_idp}) - else - keycloak_opts - end - config :ueberauth_oidcc, issuers: [ %{ diff --git a/config/test.exs b/config/test.exs index 47ec615d..56646bde 100644 --- a/config/test.exs +++ b/config/test.exs @@ -4,7 +4,9 @@ config :arrow, shape_storage_enabled?: false, shape_storage_request_fn: {Arrow.Mock.ExAws.Request, :request}, gtfs_archive_storage_enabled?: false, - gtfs_archive_storage_request_fn: {Arrow.Mock.ExAws.Request, :request} + gtfs_archive_storage_request_fn: {Arrow.Mock.ExAws.Request, :request}, + ueberauth_provider: :cognito, + api_login_module: ArrowWeb.TryApiTokenAuth.Cognito # Configure your database config :arrow, Arrow.Repo, diff --git a/lib/arrow/stops.ex b/lib/arrow/stops.ex index 63878266..27595700 100644 --- a/lib/arrow/stops.ex +++ b/lib/arrow/stops.ex @@ -41,6 +41,24 @@ defmodule Arrow.Stops do """ def get_stop!(id), do: Repo.get!(Stop, id) + @doc """ + Gets a single stop by stop_id. + + Returns nil if no stop exists with the given stop_id. + + ## Examples + + iex> get_stop_by_stop_id("123") + %Stop{} + + iex> get_stop_by_stop_id("456") + nil + + """ + def get_stop_by_stop_id(stop_id) do + Repo.get_by(Stop, stop_id: stop_id) + end + @doc """ Creates a stop. diff --git a/lib/arrow/ueberauth/strategy/fake.ex b/lib/arrow/ueberauth/strategy/fake.ex index c69c8aac..f32408b8 100644 --- a/lib/arrow/ueberauth/strategy/fake.ex +++ b/lib/arrow/ueberauth/strategy/fake.ex @@ -38,7 +38,11 @@ defmodule Arrow.Ueberauth.Strategy.Fake do @impl Ueberauth.Strategy def extra(_conn) do - %Ueberauth.Auth.Extra{raw_info: %{}} + %Ueberauth.Auth.Extra{ + raw_info: %{ + "iat" => System.system_time(:second) + } + } end @impl Ueberauth.Strategy diff --git a/lib/arrow_web/auth_manager.ex b/lib/arrow_web/auth_manager.ex index e9a95d97..4a07ca34 100644 --- a/lib/arrow_web/auth_manager.ex +++ b/lib/arrow_web/auth_manager.ex @@ -3,6 +3,14 @@ defmodule ArrowWeb.AuthManager do use Guardian, otp_app: :arrow + def max_session_time do + Application.get_env(:arrow, __MODULE__)[:max_session_time] + end + + def idle_time do + Application.get_env(:arrow, __MODULE__)[:idle_time] + end + @impl true def subject_for_token(resource, _claims) do {:ok, resource} @@ -14,4 +22,19 @@ defmodule ArrowWeb.AuthManager do end def resource_from_claims(_), do: {:error, :invalid_claims} + + @impl true + def verify_claims(%{"iat" => iat, "auth_time" => auth_time} = claims, _opts) do + now = System.system_time(:second) + # auth_time is when the user entered their password at the SSO provider + auth_time_expires = auth_time + max_session_time() + # iat is when the token was issued + iat_expires = iat + idle_time() + # did either timeout expire? + if min(auth_time_expires, iat_expires) < now do + {:error, {:auth_expired, claims["sub"]}} + else + {:ok, claims} + end + end end diff --git a/lib/arrow_web/auth_manager/error_handler.ex b/lib/arrow_web/auth_manager/error_handler.ex index 4ec6b5bc..cdbf9f7b 100644 --- a/lib/arrow_web/auth_manager/error_handler.ex +++ b/lib/arrow_web/auth_manager/error_handler.ex @@ -9,8 +9,28 @@ defmodule ArrowWeb.AuthManager.ErrorHandler do @behaviour Guardian.Plug.ErrorHandler @impl Guardian.Plug.ErrorHandler - def auth_error(conn, {_type, _reason}, _opts) do + def auth_error(conn, error, _opts) do provider = Application.get_env(:arrow, :ueberauth_provider) - Controller.redirect(conn, to: Routes.auth_path(conn, :request, "#{provider}")) + auth_params = auth_params_for_error(error) + Controller.redirect(conn, to: Routes.auth_path(conn, :request, "#{provider}", auth_params)) + end + + def auth_params_for_error({:invalid_token, {:auth_expired, sub}}) do + # if we know the user who was logged in before, provide that upstream to simplify + # logging in + %{ + prompt: "login", + login_hint: sub + } + end + + def auth_params_for_error({:unauthenticated, _}) do + %{} + end + + def auth_params_for_error(_) do + %{ + prompt: "login" + } end end diff --git a/lib/arrow_web/auth_manager/pipeline.ex b/lib/arrow_web/auth_manager/pipeline.ex index bd2f6afa..59aebb2b 100644 --- a/lib/arrow_web/auth_manager/pipeline.ex +++ b/lib/arrow_web/auth_manager/pipeline.ex @@ -9,4 +9,22 @@ defmodule ArrowWeb.AuthManager.Pipeline do plug(Guardian.Plug.VerifySession, claims: %{"typ" => "access"}) plug(Guardian.Plug.VerifyHeader, claims: %{"typ" => "access"}) plug(Guardian.Plug.LoadResource, allow_blank: true) + plug :refresh_idle_token + + @doc """ + Refreshes the token with each request. + + This allows us to use the `iat` time in the token as an idle timeout. + """ + def refresh_idle_token(conn, _opts) do + old_token = Guardian.Plug.current_token(conn) + + case ArrowWeb.AuthManager.refresh(old_token) do + {:ok, _old, {new_token, _new_claims}} -> + Guardian.Plug.put_session_token(conn, new_token) + + _ -> + conn + end + end end diff --git a/lib/arrow_web/controllers/auth_controller.ex b/lib/arrow_web/controllers/auth_controller.ex index 764ffb85..46972851 100644 --- a/lib/arrow_web/controllers/auth_controller.ex +++ b/lib/arrow_web/controllers/auth_controller.ex @@ -26,6 +26,8 @@ defmodule ArrowWeb.AuthController do groups = Map.get(auth.credentials.other, :groups, []) + auth_time = Map.get(auth.extra.raw_info, "auth_time", auth.extra.raw_info["iat"]) + roles = Enum.flat_map(groups, fn group -> case @cognito_groups[group] do @@ -39,6 +41,7 @@ defmodule ArrowWeb.AuthController do ArrowWeb.AuthManager, username, %{ + auth_time: auth_time, # all cognito users have read-only access roles: roles ++ ["read-only"] }, @@ -49,8 +52,13 @@ defmodule ArrowWeb.AuthController do def callback(%{assigns: %{ueberauth_auth: %{provider: :keycloak} = auth}} = conn, _params) do username = auth.uid - expiration = auth.credentials.expires_at - current_time = System.system_time(:second) + + auth_time = + Map.get( + auth.extra.raw_info.claims, + "auth_time", + auth.extra.raw_info.claims["iat"] + ) roles = auth.extra.raw_info.userinfo["roles"] || [] @@ -66,14 +74,16 @@ defmodule ArrowWeb.AuthController do end conn + |> configure_session(drop: true) |> put_session(:logout_url, logout_url) |> Guardian.Plug.sign_in( ArrowWeb.AuthManager, username, %{ + auth_time: auth_time, roles: roles }, - ttl: {expiration - current_time, :seconds} + ttl: {1, :minute} ) |> redirect(to: Routes.disruption_path(conn, :index)) end diff --git a/lib/arrow_web/live/stop_live/stop_live.ex b/lib/arrow_web/live/stop_live/stop_live.ex index 9eec3dbe..af5c0cf5 100644 --- a/lib/arrow_web/live/stop_live/stop_live.ex +++ b/lib/arrow_web/live/stop_live/stop_live.ex @@ -118,16 +118,6 @@ defmodule ArrowWeb.StopViewLive do {:ok, socket} end - def handle_changeset(socket, changeset) do - case Ecto.Changeset.apply_action(changeset, :validate) do - {:ok, _} -> - {:noreply, assign(socket, form: to_form(changeset), trigger_submit: true)} - - {:error, applied_changeset} -> - {:noreply, assign(socket, form: to_form(applied_changeset), trigger_submit: false)} - end - end - def handle_event("validate", %{"stop" => stop_params}, socket) do form = Stops.change_stop(socket.assigns.stop, stop_params) |> to_form(action: :validate) @@ -138,14 +128,29 @@ defmodule ArrowWeb.StopViewLive do def handle_event("edit", %{"stop" => stop_params}, socket) do stop = Stops.get_stop!(socket.assigns.stop.id) - changeset = Stops.change_stop(stop, stop_params) - handle_changeset(socket, changeset) + case Arrow.Stops.update_stop(stop, stop_params) do + {:ok, _stop} -> + {:noreply, + socket + |> put_flash(:info, "Stop edited successfully") + |> redirect(to: ~p"/stops")} + + {:error, changeset} -> + {:noreply, assign(socket, form: to_form(changeset))} + end end def handle_event("create", %{"stop" => stop_params}, socket) do - changeset = Stops.change_stop(%Stop{}, stop_params) - - handle_changeset(socket, changeset) + case Arrow.Stops.create_stop(stop_params) do + {:ok, _stop} -> + {:noreply, + socket + |> put_flash(:info, "Stop created successfully") + |> redirect(to: ~p"/stops")} + + {:error, changeset} -> + {:noreply, assign(socket, form: to_form(changeset))} + end end end diff --git a/test/arrow/stops_test.exs b/test/arrow/stops_test.exs index 00c43c2c..4c7c2300 100644 --- a/test/arrow/stops_test.exs +++ b/test/arrow/stops_test.exs @@ -171,5 +171,14 @@ defmodule Arrow.StopsTest do stop = stop_fixture() assert %Ecto.Changeset{} = Stops.change_stop(stop) end + + test "get_stop_by_stop_id/1 returns stop when found" do + stop = stop_fixture() + assert Stops.get_stop_by_stop_id(stop.stop_id) == stop + end + + test "get_stop_by_stop_id/1 returns nil when stop not found" do + assert Stops.get_stop_by_stop_id("nonexistent") == nil + end end end diff --git a/test/arrow_web/auth_manager/error_handler_test.exs b/test/arrow_web/auth_manager/error_handler_test.exs index bc63fb93..eeefcd3a 100644 --- a/test/arrow_web/auth_manager/error_handler_test.exs +++ b/test/arrow_web/auth_manager/error_handler_test.exs @@ -10,7 +10,7 @@ defmodule ArrowWeb.AuthManager.ErrorHandlerTest do |> init_test_session(%{}) |> ArrowWeb.AuthManager.ErrorHandler.auth_error({:some_type, :reason}, []) - assert html_response(conn, 302) =~ "\"/auth/#{provider}\"" + assert html_response(conn, 302) =~ "\"/auth/#{provider}?prompt=login\"" end end end diff --git a/test/arrow_web/controllers/auth_controller_test.exs b/test/arrow_web/controllers/auth_controller_test.exs index 1187f638..1723bbc6 100644 --- a/test/arrow_web/controllers/auth_controller_test.exs +++ b/test/arrow_web/controllers/auth_controller_test.exs @@ -38,7 +38,7 @@ defmodule ArrowWeb.Controllers.AuthControllerTest do other: %{id_token: "id_token"} }, extra: %{ - raw_info: %{ + raw_info: %UeberauthOidcc.RawInfo{ userinfo: %{ "roles" => ["admin"] } @@ -69,9 +69,7 @@ defmodule ArrowWeb.Controllers.AuthControllerTest do other: %{id_token: "id_token"} }, extra: %{ - raw_info: %{ - userinfo: %{} - } + raw_info: %UeberauthOidcc.RawInfo{} } } diff --git a/test/arrow_web/live/stop_live/stop_live_test.exs b/test/arrow_web/live/stop_live/stop_live_test.exs index ec084acc..97b0f6fc 100644 --- a/test/arrow_web/live/stop_live/stop_live_test.exs +++ b/test/arrow_web/live/stop_live/stop_live_test.exs @@ -4,6 +4,8 @@ defmodule ArrowWeb.StopLiveTest do import Phoenix.LiveViewTest import Arrow.StopsFixtures + alias Arrow.Shuttles.Stop + @create_attrs %{ stop_id: "some stop_id", stop_name: "some stop_name", @@ -64,19 +66,20 @@ defmodule ArrowWeb.StopLiveTest do describe "create stop" do @tag :authenticated_admin - test "redirects to index when data is valid", %{conn: conn} do + test "creates stop and redirects to index when data is valid", %{conn: conn} do {:ok, new_live, _html} = live(conn, ~p"/stops/new") form = new_live |> form("#stop-form", stop: @create_attrs) - assert render_submit(form) =~ ~r/phx-trigger-action/ + render_submit(form) + + assert_redirected(new_live, "/stops") - conn = follow_trigger_action(form, conn) - assert conn.method == "POST" - params = Enum.map(@create_attrs, fn {k, v} -> {"#{k}", v} end) |> Enum.into(%{}) - assert conn.params == %{"stop" => params} + stop_id = @create_attrs[:stop_id] + + assert %Stop{stop_id: ^stop_id} = Arrow.Stops.get_stop_by_stop_id(stop_id) end @tag :authenticated_admin @@ -110,19 +113,19 @@ defmodule ArrowWeb.StopLiveTest do setup [:create_stop] @tag :authenticated_admin - test "redirects when data is valid", %{conn: conn, stop: stop} do + test "updates and redirects when data is valid", %{conn: conn, stop: stop} do {:ok, edit_live, _html} = live(conn, ~p"/stops/#{stop}/edit") form = edit_live |> form("#stop-form", stop: @update_attrs) + |> render_submit() + + assert_redirected(edit_live, ~p"/stops") - assert render_submit(form) =~ ~r/phx-trigger-action/ + stop_id = @update_attrs[:stop_id] - conn = follow_trigger_action(form, conn) - assert conn.method == "POST" - params = Enum.map(@update_attrs, fn {k, v} -> {"#{k}", v} end) |> Enum.into(%{}) - assert conn.params == %{"stop" => params} + assert %Stop{stop_id: ^stop_id} = Arrow.Stops.get_stop_by_stop_id(stop_id) end @tag :authenticated_admin