Implement PKCE (#789)

* use to_existing_atom

* implement pkce

* fix and add tests

* Update apps/fz_http/lib/fz_http_web/controllers/auth_controller.ex

Co-authored-by: Jamil <jamilbk@users.noreply.github.com>

Co-authored-by: Jamil <jamilbk@users.noreply.github.com>

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
 

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