Fix lost client auth state for OIDC redirects (#3273)

elixir/apps/web/lib/web/auth.ex

@@ -41,6 +41,16 @@ defmodule Web.Auth do
     Plug.Conn.put_session(conn, :sessions, sessions)
   end
 
+  defp delete_account_session(conn, context_type, account_id) do
+    sessions =
+      Plug.Conn.get_session(conn, :sessions, [])
+      |> Enum.reject(fn {session_context_type, session_account_id, _encoded_fragment} ->
+        session_context_type == context_type and session_account_id == account_id
+      end)
+
+    Plug.Conn.put_session(conn, :sessions, sessions)
+  end
+
   # Signing In and Out
 
   @doc """
@@ -295,17 +305,24 @@ defmodule Web.Auth do
     context_type = fetch_auth_context_type!(params)
     context = get_auth_context(conn, context_type)
 
-    with account when not is_nil(account) <- Map.get(conn.assigns, :account),
-         sessions = Plug.Conn.get_session(conn, :sessions, []),
-         {:ok, encoded_fragment} <- fetch_token(sessions, account.id, context.type),
-         {:ok, subject} <- Auth.authenticate(encoded_fragment, context),
-         true <- subject.account.id == account.id do
-      conn
-      |> Plug.Conn.put_session(:live_socket_id, "sessions:#{subject.token_id}")
-      |> Plug.Conn.assign(:subject, subject)
+    if account = Map.get(conn.assigns, :account) do
+      sessions = Plug.Conn.get_session(conn, :sessions, [])
+
+      with {:ok, encoded_fragment} <- fetch_token(sessions, account.id, context.type),
+           {:ok, subject} <- Auth.authenticate(encoded_fragment, context),
+           true <- subject.account.id == account.id do
+        conn
+        |> Plug.Conn.put_session(:live_socket_id, "sessions:#{subject.token_id}")
+        |> Plug.Conn.assign(:subject, subject)
+      else
+        {:error, :unauthorized} ->
+          delete_account_session(conn, context.type, account.id)
+
+        _ ->
+          conn
+      end
     else
-      {:error, :unauthorized} -> renew_session(conn)
-      _ -> conn
+      conn
     end
   end
 

elixir/apps/web/lib/web/controllers/auth_controller.ex

@@ -185,10 +185,11 @@ defmodule Web.AuthController do
         } = params
       ) do
     with {:ok, provider} <- Domain.Auth.fetch_active_provider_by_id(provider_id) do
+      redirect_params = Web.Auth.take_sign_in_params(params)
+
       redirect_url =
         url(~p"/#{provider.account_id}/sign_in/providers/#{provider.id}/handle_callback")
 
-      redirect_params = Web.Auth.take_sign_in_params(params)
       redirect_to_idp(conn, redirect_url, provider, %{}, redirect_params)
     else
       {:error, :not_found} ->

elixir/apps/web/lib/web/router.ex

@@ -84,6 +84,10 @@ defmodule Web.Router do
       ## Email
       live "/sign_in/providers/email/:provider_id", SignIn.Email
     end
+  end
+
+  scope "/:account_id_or_slug", Web do
+    pipe_through [:browser, :account]
 
     scope "/sign_in/providers/:provider_id" do
       # UserPass

elixir/apps/web/test/web/acceptance/auth/email_test.exs

@@ -118,7 +118,7 @@ defmodule Web.Acceptance.SignIn.EmailTest do
     |> visit(~p"/#{account}/sites")
     |> assert_el(Query.css("#user-menu-button"))
 
-    # Browser is stored correctly
+    # Browser session is stored correctly
     {:ok, cookie} = Auth.fetch_session_cookie(session)
     assert [{:browser, account_id, _fragment}] = cookie["sessions"]
     assert account_id == account.id

elixir/apps/web/test/web/acceptance/auth/openid_connect_test.exs

@@ -110,4 +110,128 @@ defmodule Web.Acceptance.Auth.OpenIDConnectTest do
     assert Domain.Auth.authenticate(redirect_params["nonce"], context) == {:error, :unauthorized}
     assert {:ok, _subject} = Domain.Auth.authenticate(token, context)
   end
+
+  feature "allows to log in to the browser and then to the client", %{
+    session: session
+  } do
+    nonce = Ecto.UUID.generate()
+    state = Ecto.UUID.generate()
+
+    Auth.mock_client_sign_in_callback()
+
+    redirect_params = %{
+      "as" => "client",
+      "state" => "state_#{state}",
+      "nonce" => "nonce_#{nonce}"
+    }
+
+    account = Fixtures.Accounts.create_account()
+    provider = Vault.setup_oidc_provider(account, @endpoint.url)
+
+    oidc_login = "firezone-1"
+    oidc_password = "firezone1234_oidc"
+    email = Fixtures.Auth.email()
+
+    {:ok, entity_id} = Vault.upsert_user(oidc_login, email, oidc_password)
+
+    actor = Fixtures.Actors.create_actor(type: :account_admin_user, account: account)
+
+    identity =
+      Fixtures.Auth.create_identity(
+        actor: actor,
+        account: account,
+        provider: provider,
+        provider_identifier: entity_id
+      )
+
+    # Sign In as an portal user
+    session
+    |> visit(~p"/#{account}")
+    |> assert_el(Query.text("Sign into #{account.name}"))
+    |> click(Query.link("Sign in with Vault"))
+    |> Vault.userpass_flow(oidc_login, oidc_password)
+    |> assert_el(Query.css("#user-menu-button"))
+    |> Auth.assert_authenticated(identity)
+    |> assert_path(~p"/#{account.slug}/sites")
+
+    # And then to a client
+    session
+    |> visit(~p"/#{account}?#{redirect_params}")
+    |> assert_el(Query.text("Sign into #{account.name}"))
+    |> click(Query.link("Sign in with Vault"))
+    |> assert_el(Query.text("Client redirected"))
+    |> assert_path(~p"/handle_client_sign_in_callback")
+
+    # The browser sessions stays active
+    session
+    |> visit(~p"/#{account}/sites")
+    |> assert_el(Query.css("#user-menu-button"))
+
+    # Browser session is stored correctly
+    {:ok, cookie} = Auth.fetch_session_cookie(session)
+    assert [{:browser, account_id, _fragment}] = cookie["sessions"]
+    assert account_id == account.id
+  end
+
+  feature "allows to log in to the client and then to the browser", %{
+    session: session
+  } do
+    nonce = Ecto.UUID.generate()
+    state = Ecto.UUID.generate()
+
+    Auth.mock_client_sign_in_callback()
+
+    redirect_params = %{
+      "as" => "client",
+      "state" => "state_#{state}",
+      "nonce" => "nonce_#{nonce}"
+    }
+
+    account = Fixtures.Accounts.create_account()
+    provider = Vault.setup_oidc_provider(account, @endpoint.url)
+
+    oidc_login = "firezone-1"
+    oidc_password = "firezone1234_oidc"
+    email = Fixtures.Auth.email()
+
+    {:ok, entity_id} = Vault.upsert_user(oidc_login, email, oidc_password)
+
+    actor = Fixtures.Actors.create_actor(type: :account_admin_user, account: account)
+
+    identity =
+      Fixtures.Auth.create_identity(
+        actor: actor,
+        account: account,
+        provider: provider,
+        provider_identifier: entity_id
+      )
+
+    # And then to a client
+    session
+    |> visit(~p"/#{account}?#{redirect_params}")
+    |> assert_el(Query.text("Sign into #{account.name}"))
+    |> click(Query.link("Sign in with Vault"))
+    |> Vault.userpass_flow(oidc_login, oidc_password)
+    |> assert_el(Query.text("Client redirected"))
+    |> assert_path(~p"/handle_client_sign_in_callback")
+
+    # Sign In as an portal user
+    session
+    |> visit(~p"/#{account}")
+    |> assert_el(Query.text("Sign into #{account.name}"))
+    |> click(Query.link("Sign in with Vault"))
+    |> assert_el(Query.css("#user-menu-button"))
+    |> Auth.assert_authenticated(identity)
+    |> assert_path(~p"/#{account.slug}/sites")
+
+    # The browser sessions stays active
+    session
+    |> visit(~p"/#{account}/sites")
+    |> assert_el(Query.css("#user-menu-button"))
+
+    # Browser session is stored correctly
+    {:ok, cookie} = Auth.fetch_session_cookie(session)
+    assert [{:browser, account_id, _fragment}] = cookie["sessions"]
+    assert account_id == account.id
+  end
 end

elixir/apps/web/test/web/acceptance/auth/userpass_test.exs

@@ -193,6 +193,57 @@ defmodule Web.Acceptance.Auth.UserPassTest do
     assert {:ok, _subject} = Domain.Auth.authenticate(token, context)
   end
 
+  feature "allows to log in using email link to the client even with active browser session", %{
+    session: session
+  } do
+    nonce = Ecto.UUID.generate()
+    state = Ecto.UUID.generate()
+
+    Auth.mock_client_sign_in_callback()
+
+    redirect_params = %{
+      "as" => "client",
+      "state" => "state_#{state}",
+      "nonce" => "nonce_#{nonce}"
+    }
+
+    account = Fixtures.Accounts.create_account()
+    actor = Fixtures.Actors.create_actor(type: :account_admin_user, account: account)
+    provider = Fixtures.Auth.create_userpass_provider(account: account)
+    password = "Firezone1234"
+
+    identity =
+      Fixtures.Auth.create_identity(
+        account: account,
+        provider: provider,
+        actor: actor,
+        provider_virtual_state: %{"password" => password, "password_confirmation" => password}
+      )
+
+    # Sign In as an portal user
+    session
+    |> password_login_flow(account, identity.provider_identifier, password)
+    |> assert_el(Query.css("#user-menu-button"))
+    |> assert_path(~p"/#{account.slug}/sites")
+    |> Auth.assert_authenticated(identity)
+
+    # And then to a client
+    session
+    |> password_login_flow(account, identity.provider_identifier, password, redirect_params)
+    |> assert_el(Query.text("Client redirected"))
+    |> assert_path(~p"/handle_client_sign_in_callback")
+
+    # The browser sessions stays active
+    session
+    |> visit(~p"/#{account}/sites")
+    |> assert_el(Query.css("#user-menu-button"))
+
+    # Browser session is stored correctly
+    {:ok, cookie} = Auth.fetch_session_cookie(session)
+    assert [{:browser, account_id, _fragment}] = cookie["sessions"]
+    assert account_id == account.id
+  end
+
   defp password_login_flow(session, account, username, password, redirect_params \\ %{}) do
     session
     |> visit(~p"/#{account}?#{redirect_params}")

elixir/apps/web/test/web/auth_test.exs

@@ -714,20 +714,22 @@ defmodule Web.AuthTest do
       refute Map.has_key?(conn.assigns, :subject)
     end
 
-    test "renews session when token is invalid", %{
+    test "removes invalid tokens from session", %{
       conn: conn,
-      account: account,
-      context: context
+      account: account
     } do
       conn =
         %{conn | remote_ip: {100, 64, 100, 58}}
-        |> put_session(:sessions, [{context.type, account.id, "invalid"}])
+        |> put_session(:sessions, [
+          {:client, account.id, "valid"},
+          {:browser, account.id, "invalid"}
+        ])
         |> assign(:account, account)
         |> fetch_subject([])
 
       refute Map.has_key?(conn.assigns, :subject)
 
-      assert get_session(conn, :sessions) == []
+      assert get_session(conn, :sessions) == [{:client, account.id, "valid"}]
     end
   end
 

elixir/apps/web/test/web/controllers/auth_controller_test.exs

@@ -759,11 +759,32 @@ defmodule Web.AuthControllerTest do
       conn =
         get(conn, ~p"/#{account.id}/sign_in/providers/#{provider.id}/redirect", redirect_params)
 
-      {params, _state, _verifier} =
+      assert to = redirected_to(conn)
+      uri = URI.parse(to)
+      assert uri.host == "localhost"
+      assert uri.path == "/authorize"
+
+      callback_url =
+        url(~p"/#{account.id}/sign_in/providers/#{provider.id}/handle_callback")
+
+      {params, state, verifier} =
         conn.cookies["fz_auth_state_#{provider.id}"]
         |> :erlang.binary_to_term([:safe])
 
       assert params == redirect_params
+
+      code_challenge = Domain.Auth.Adapters.OpenIDConnect.PKCE.code_challenge(verifier)
+
+      assert URI.decode_query(uri.query) == %{
+               "access_type" => "offline",
+               "client_id" => provider.adapter_config["client_id"],
+               "code_challenge" => code_challenge,
+               "code_challenge_method" => "S256",
+               "redirect_uri" => callback_url,
+               "response_type" => "code",
+               "scope" => "openid email profile",
+               "state" => state
+             }
     end
   end