diff --git a/apps/fz_http/lib/fz_http_web/controllers/auth_controller.ex b/apps/fz_http/lib/fz_http_web/controllers/auth_controller.ex index 5978c475a..a8db0303e 100644 --- a/apps/fz_http/lib/fz_http_web/controllers/auth_controller.ex +++ b/apps/fz_http/lib/fz_http_web/controllers/auth_controller.ex @@ -5,6 +5,9 @@ defmodule FzHttpWeb.AuthController do use FzHttpWeb, :controller require Logger + @oidc_state_key "fz_oidc_state" + @oidc_state_valid_duration 300 + alias FzCommon.FzCrypto alias FzHttp.Users alias FzHttpWeb.Authentication @@ -47,19 +50,11 @@ defmodule FzHttpWeb.AuthController do end end - def callback(conn, params) do - %{"provider" => provider_key} = params + def callback(conn, %{"provider" => provider_key, "state" => state} = params) do openid_connect = Application.fetch_env!(:fz_http, :openid_connect) - atomize = fn key -> - try do - {:ok, String.to_existing_atom(key)} - catch - ArgumentError -> {:error, "OIDC Provider not found"} - end - end - - with {:ok, provider} <- atomize.(provider_key), + with {:ok, provider} <- atomize_provider(provider_key), + {:ok, _state} <- verify_state(conn, state), {:ok, tokens} <- openid_connect.fetch_tokens(provider, params), {:ok, claims} <- openid_connect.verify(provider, tokens["id_token"]) do case UserFromAuth.find_or_create(provider, claims) do @@ -74,7 +69,7 @@ defmodule FzHttpWeb.AuthController do {:error, reason} -> conn |> put_flash(:error, "Error signing in: #{reason}") - |> request(%{}) + |> redirect(to: Routes.root_path(conn, :index)) end else {:error, reason} -> @@ -83,7 +78,7 @@ defmodule FzHttpWeb.AuthController do conn |> put_flash(:error, msg) - |> request(%{}) + |> redirect(to: Routes.root_path(conn, :index)) # Error verifying claims or fetching tokens {:error, action, reason} -> @@ -92,6 +87,24 @@ defmodule FzHttpWeb.AuthController do end end + defp atomize_provider(key) do + {:ok, String.to_existing_atom(key)} + rescue + ArgumentError -> {:error, "OIDC Provider not found"} + end + + defp verify_state(conn, state) do + conn + |> fetch_cookies(signed: [@oidc_state_key]) + |> then(fn + %{cookies: %{@oidc_state_key => ^state}} -> + {:ok, state} + + _ -> + {:error, "Cannot verify state"} + end) + end + def delete(conn, _params) do conn |> Authentication.sign_out() @@ -131,16 +144,23 @@ defmodule FzHttpWeb.AuthController do def redirect_oidc_auth_uri(conn, %{"provider" => provider}) do openid_connect = Application.fetch_env!(:fz_http, :openid_connect) + state = FzCrypto.rand_string() params = %{ - state: FzCrypto.rand_string(), + state: state, # needed for google access_type: "offline" } - uri = openid_connect.authorization_uri(String.to_atom(provider), params) + uri = openid_connect.authorization_uri(String.to_existing_atom(provider), params) conn + |> put_resp_cookie(@oidc_state_key, state, + max_age: @oidc_state_valid_duration, + sign: true, + same_site: "Lax", + secure: Application.fetch_env!(:fz_http, :cookie_secure) + ) |> redirect(external: uri) end diff --git a/apps/fz_http/test/fz_http_web/controllers/auth_controller_test.exs b/apps/fz_http/test/fz_http_web/controllers/auth_controller_test.exs index fd5391c8e..2c1134655 100644 --- a/apps/fz_http/test/fz_http_web/controllers/auth_controller_test.exs +++ b/apps/fz_http/test/fz_http_web/controllers/auth_controller_test.exs @@ -87,7 +87,29 @@ defmodule FzHttpWeb.AuthControllerTest do end describe "creating session from OpenID Connect" do - setup [:create_user] + setup :create_user + + @key "fz_oidc_state" + @state "test" + + @params %{ + "code" => "MyFaketoken", + "provider" => "google", + "state" => @state + } + + setup %{unauthed_conn: conn} = context do + signed_state = + Plug.Crypto.sign( + Application.fetch_env!(:fz_http, FzHttpWeb.Endpoint)[:secret_key_base], + @key <> "_cookie", + @state, + key: Plug.Keys, + max_age: context[:max_age] || 300 + ) + + {:ok, unauthed_conn: put_req_cookie(conn, "fz_oidc_state", signed_state)} + end test "when a user returns with a valid claim", %{unauthed_conn: conn, user: user} do expect(OpenIDConnect.Mock, :fetch_tokens, fn _, _ -> {:ok, %{"id_token" => "abc"}} end) @@ -96,17 +118,12 @@ defmodule FzHttpWeb.AuthControllerTest do {:ok, %{"email" => user.email, "sub" => "12345"}} end) - params = %{ - "code" => "MyFaketoken", - "provider" => "google" - } - - test_conn = get(conn, Routes.auth_oidc_path(conn, :callback, "google"), params) - + test_conn = get(conn, Routes.auth_oidc_path(conn, :callback, "google"), @params) assert redirected_to(test_conn) == Routes.user_index_path(test_conn, :index) end @moduletag :capture_log + test "when a user returns with an invalid claim", %{unauthed_conn: conn} do expect(OpenIDConnect.Mock, :fetch_tokens, fn _, _ -> {:ok, %{}} end) @@ -114,14 +131,26 @@ defmodule FzHttpWeb.AuthControllerTest do {:error, "Invalid token for user!"} end) - params = %{ - "code" => "MyFaketoken", - "provider" => "google" - } - - test_conn = get(conn, Routes.auth_oidc_path(conn, :callback, "google"), params) + test_conn = get(conn, Routes.auth_oidc_path(conn, :callback, "google"), @params) assert get_flash(test_conn, :error) == "OpenIDConnect Error: Invalid token for user!" end + + test "when a user returns with an invalid state", %{unauthed_conn: conn} do + test_conn = + get(conn, Routes.auth_oidc_path(conn, :callback, "google"), %{ + @params + | "state" => "not_valid" + }) + + assert get_flash(test_conn, :error) == "OpenIDConnect Error: Cannot verify state" + end + + @tag max_age: 0 + test "when a user returns with an expired state", %{unauthed_conn: conn} do + test_conn = get(conn, Routes.auth_oidc_path(conn, :callback, "google"), @params) + + assert get_flash(test_conn, :error) == "OpenIDConnect Error: Cannot verify state" + end end describe "when deleting a session" do