From f114bc95cdccfa8abf9bb53322ad26acf80dac57 Mon Sep 17 00:00:00 2001 From: Brian Manifold Date: Fri, 13 Dec 2024 09:26:47 -0800 Subject: [PATCH] refactor(portal): Add email as separate column on auth_identities table (#7472) Why: * Currently, when using the API, a user has no way of easily identifying what identities they are pulling back as the response only includes the `provider_identifier` which for most of our AuthProviders is an ID for the IdP and not an email address. Along with that, when adding users to an OIDC provider within Firezone, there is no check for whether or not an identity has already been added with a given email address. By creating a separate email column on the `auth_identities` table, it will be very straight forward to know whether an email address exists for a given identity, return it in an API response and allow the admin of a Firezone account to track users (Identities) by email rather than IdP identifier. Fixes #7392 --- .../api/controllers/identity_controller.ex | 49 ++++ .../api/lib/api/controllers/identity_json.ex | 3 +- .../api/lib/api/schemas/identity_schema.ex | 20 +- .../controllers/identity_controller_test.exs | 223 +++++++++++++++++- elixir/apps/domain/lib/domain/auth.ex | 10 + .../apps/domain/lib/domain/auth/adapters.ex | 22 +- .../domain/auth/adapters/google_workspace.ex | 1 + .../lib/domain/auth/adapters/jumpcloud.ex | 1 + .../domain/auth/adapters/microsoft_entra.ex | 1 + .../domain/lib/domain/auth/adapters/okta.ex | 1 + .../domain/auth/adapters/openid_connect.ex | 14 +- .../apps/domain/lib/domain/auth/identity.ex | 1 + .../lib/domain/auth/identity/changeset.ex | 9 +- .../domain/lib/domain/auth/identity/sync.ex | 23 +- .../apps/domain/lib/domain/repo/changeset.ex | 2 +- ...241120171334_add_identity_email_column.exs | 9 + ...185037_add_identity_email_unique_index.exs | 13 + .../20241205165118_migrate_email_data.exs | 22 ++ .../auth/adapters/openid_connect_test.exs | 14 +- .../apps/domain/test/support/fixtures/auth.ex | 10 + .../lib/web/live/actors/users/new_identity.ex | 12 + 21 files changed, 415 insertions(+), 45 deletions(-) create mode 100644 elixir/apps/domain/priv/repo/migrations/20241120171334_add_identity_email_column.exs create mode 100644 elixir/apps/domain/priv/repo/migrations/20241126185037_add_identity_email_unique_index.exs create mode 100644 elixir/apps/domain/priv/repo/migrations/20241205165118_migrate_email_data.exs diff --git a/elixir/apps/api/lib/api/controllers/identity_controller.ex b/elixir/apps/api/lib/api/controllers/identity_controller.ex index e0ef01c2c..da8bb619f 100644 --- a/elixir/apps/api/lib/api/controllers/identity_controller.ex +++ b/elixir/apps/api/lib/api/controllers/identity_controller.ex @@ -56,6 +56,8 @@ defmodule API.IdentityController do "provider_identifier_confirmation", Map.get(params, "provider_identifier") ) + |> maybe_put_email() + |> maybe_put_identifier() with {:ok, actor} <- Domain.Actors.fetch_actor_by_id(actor_id, subject), {:ok, provider} <- Auth.fetch_provider_by_id(provider_id, subject), @@ -140,4 +142,51 @@ defmodule API.IdentityController do {:provider_check, valid?} end + + defp maybe_put_email(params) do + email = + params["email"] + |> to_string + |> String.trim() + + identifier = + params["provider_identifier"] + |> to_string() + |> String.trim() + + cond do + Domain.Auth.valid_email?(email) -> + params + + Domain.Auth.valid_email?(identifier) -> + Map.put(params, "email", identifier) + + true -> + params + end + end + + defp maybe_put_identifier(params) do + email = + params["email"] + |> to_string() + |> String.trim() + + identifier = + params["provider_identifier"] + |> to_string() + |> String.trim() + + cond do + identifier != "" -> + params + + Domain.Auth.valid_email?(email) -> + Map.put(params, "provider_identifier", email) + |> Map.put("provider_identifier_confirmation", email) + + true -> + params + end + end end diff --git a/elixir/apps/api/lib/api/controllers/identity_json.ex b/elixir/apps/api/lib/api/controllers/identity_json.ex index 428500a73..b7bac29b1 100644 --- a/elixir/apps/api/lib/api/controllers/identity_json.ex +++ b/elixir/apps/api/lib/api/controllers/identity_json.ex @@ -24,7 +24,8 @@ defmodule API.IdentityJSON do id: identity.id, actor_id: identity.actor_id, provider_id: identity.provider_id, - provider_identifier: identity.provider_identifier + provider_identifier: identity.provider_identifier, + email: identity.email } end end diff --git a/elixir/apps/api/lib/api/schemas/identity_schema.ex b/elixir/apps/api/lib/api/schemas/identity_schema.ex index 960e67caf..528b25c85 100644 --- a/elixir/apps/api/lib/api/schemas/identity_schema.ex +++ b/elixir/apps/api/lib/api/schemas/identity_schema.ex @@ -13,14 +13,16 @@ defmodule API.Schemas.Identity do id: %Schema{type: :string, description: "Identity ID"}, actor_id: %Schema{type: :string, description: "Actor ID"}, provider_id: %Schema{type: :string, description: "Identity Provider ID"}, - provider_identifier: %Schema{type: :string, description: "Identifier from Provider"} + provider_identifier: %Schema{type: :string, description: "Identifier from Provider"}, + email: %Schema{type: :string, description: "Email"} }, - required: [:id, :actor_id, :provider_id, :provider_identifier], + required: [:id, :actor_id, :provider_id, :provider_identifier, :email], example: %{ "id" => "42a7f82f-831a-4a9d-8f17-c66c2bb6e205", "actor_id" => "cdfa97e6-cca1-41db-8fc7-864daedb46df", "provider_id" => "989f9e96-e348-47ec-ba85-869fcd7adb19", - "provider_identifier" => "foo@bar.com" + "provider_identifier" => "2551705710219359", + "email" => "foo@bar.com" } }) end @@ -40,7 +42,8 @@ defmodule API.Schemas.Identity do required: [:identity], example: %{ "identity" => %{ - "provider_identifier" => "foo@bar.com" + "provider_identifier" => "2551705710219359", + "email" => "foo@bar.com" } } }) @@ -63,7 +66,8 @@ defmodule API.Schemas.Identity do "id" => "42a7f82f-831a-4a9d-8f17-c66c2bb6e205", "actor_id" => "cdfa97e6-cca1-41db-8fc7-864daedb46df", "provider_id" => "989f9e96-e348-47ec-ba85-869fcd7adb19", - "provider_identifier" => "foo@bar.com" + "provider_identifier" => "2551705710219359", + "email" => "foo@bar.com" } } }) @@ -88,13 +92,15 @@ defmodule API.Schemas.Identity do "id" => "42a7f82f-831a-4a9d-8f17-c66c2bb6e205", "actor_id" => "8f44a02b-b8eb-406f-8202-4274bf60ebd0", "provider_id" => "6472d898-5b98-4c3b-b4b9-d3158c1891be", - "provider_identifier" => "foo@bar.com" + "provider_identifier" => "2551705710219359", + "email" => "foo@bar.com" }, %{ "id" => "8a70eb96-e74b-4cdc-91b8-48c05ef74d4c", "actor_id" => "38c92cda-1ddb-45b3-9d1a-7efc375e00c1", "provider_id" => "04f13eed-4845-47c3-833e-fdd869fab96f", - "provider_identifier" => "baz@bar.com" + "provider_identifier" => "2638957392736483", + "email" => "baz@bar.com" } ], "metadata" => %{ diff --git a/elixir/apps/api/test/api/controllers/identity_controller_test.exs b/elixir/apps/api/test/api/controllers/identity_controller_test.exs index 654e7a9cb..bf94c8a72 100644 --- a/elixir/apps/api/test/api/controllers/identity_controller_test.exs +++ b/elixir/apps/api/test/api/controllers/identity_controller_test.exs @@ -49,7 +49,7 @@ defmodule API.IdentityControllerTest do assert equal_ids?(data_ids, identity_ids) end - test "lists resources with limit", %{conn: conn, account: account, actor: actor} do + test "lists identities with limit", %{conn: conn, account: account, actor: actor} do identities = for _ <- 1..3, do: Fixtures.Auth.create_identity(%{account: account, actor: actor}) @@ -88,7 +88,70 @@ defmodule API.IdentityControllerTest do assert json_response(conn, 401) == %{"error" => %{"reason" => "Unauthorized"}} end - test "returns a single resource", %{conn: conn, account: account, actor: actor} do + test "returns a single identity with populated email field", %{ + conn: conn, + account: account, + actor: actor + } do + identity = + Fixtures.Auth.create_identity(%{ + account: account, + actor: actor, + provider_identifier: "172836495673", + email: "foo@bar.com" + }) + + conn = + conn + |> authorize_conn(actor) + |> put_req_header("content-type", "application/json") + |> get("/actors/#{actor.id}/identities/#{identity.id}") + + assert json_response(conn, 200) == %{ + "data" => %{ + "id" => identity.id, + "actor_id" => actor.id, + "provider_id" => identity.provider_id, + "provider_identifier" => identity.provider_identifier, + "email" => identity.email + } + } + end + + test "returns a single identity with populated email field from provider_identifier", %{ + conn: conn, + account: account, + actor: actor + } do + identity = + Fixtures.Auth.create_identity(%{ + account: account, + actor: actor, + provider_identifier: "foo@bar.com" + }) + + conn = + conn + |> authorize_conn(actor) + |> put_req_header("content-type", "application/json") + |> get("/actors/#{actor.id}/identities/#{identity.id}") + + assert json_response(conn, 200) == %{ + "data" => %{ + "id" => identity.id, + "actor_id" => actor.id, + "provider_id" => identity.provider_id, + "provider_identifier" => identity.provider_identifier, + "email" => identity.provider_identifier + } + } + end + + test "returns a single identity with empty email field", %{ + conn: conn, + account: account, + actor: actor + } do identity = Fixtures.Auth.create_identity(%{account: account, actor: actor}) conn = @@ -102,7 +165,8 @@ defmodule API.IdentityControllerTest do "id" => identity.id, "actor_id" => actor.id, "provider_id" => identity.provider_id, - "provider_identifier" => identity.provider_identifier + "provider_identifier" => identity.provider_identifier, + "email" => nil } } end @@ -158,7 +222,7 @@ defmodule API.IdentityControllerTest do assert resp == %{"error" => %{"reason" => "Not Found"}} end - test "returns error on invalid identity attrs", %{ + test "returns error on empty identity attrs", %{ conn: conn, account: account, actor: api_actor @@ -189,7 +253,7 @@ defmodule API.IdentityControllerTest do } end - test "creates a resource with valid attrs", %{ + test "returns error on invalid identity attrs", %{ conn: conn, account: account, actor: api_actor @@ -199,7 +263,37 @@ defmodule API.IdentityControllerTest do actor = Fixtures.Actors.create_actor(account: account) - attrs = %{"provider_identifier" => "foo@local"} + attrs = %{email: "foo"} + + conn = + conn + |> authorize_conn(api_actor) + |> put_req_header("content-type", "application/json") + |> post("/actors/#{actor.id}/providers/#{oidc_provider.id}/identities", + identity: attrs + ) + + assert resp = json_response(conn, 422) + + assert resp == %{ + "error" => %{ + "reason" => "Unprocessable Entity", + "validation_errors" => %{"provider_identifier" => ["can't be blank"]} + } + } + end + + test "creates an identity with provider_identifier attr only and is not an email address", %{ + conn: conn, + account: account, + actor: api_actor + } do + {oidc_provider, _bypass} = + Fixtures.Auth.start_and_create_openid_connect_provider(account: account) + + actor = Fixtures.Actors.create_actor(account: account) + + attrs = %{"provider_identifier" => "128asdf92qrh9joqwefoiu23"} conn = conn @@ -210,10 +304,116 @@ defmodule API.IdentityControllerTest do ) assert resp = json_response(conn, 201) - assert resp["data"]["provider_identifier"] == attrs["provider_identifier"] - assert resp["data"]["provider_id"] == oidc_provider.id - assert resp["data"]["actor_id"] == actor.id + assert resp["data"]["email"] == nil + end + + test "creates an identity with provider_identifier attr only and is an email address", %{ + conn: conn, + account: account, + actor: api_actor + } do + {oidc_provider, _bypass} = + Fixtures.Auth.start_and_create_openid_connect_provider(account: account) + + actor = Fixtures.Actors.create_actor(account: account) + + attrs = %{"provider_identifier" => "foo@localhost.local"} + + conn = + conn + |> authorize_conn(api_actor) + |> put_req_header("content-type", "application/json") + |> post("/actors/#{actor.id}/providers/#{oidc_provider.id}/identities", + identity: attrs + ) + + assert resp = json_response(conn, 201) + assert resp["data"]["provider_identifier"] == attrs["provider_identifier"] + assert resp["data"]["email"] == attrs["provider_identifier"] + end + + test "creates an identity with email attr only and populates provider_identifier", %{ + conn: conn, + account: account, + actor: api_actor + } do + {oidc_provider, _bypass} = + Fixtures.Auth.start_and_create_openid_connect_provider(account: account) + + actor = Fixtures.Actors.create_actor(account: account) + + attrs = %{"email" => "foo@localhost.local"} + + conn = + conn + |> authorize_conn(api_actor) + |> put_req_header("content-type", "application/json") + |> post("/actors/#{actor.id}/providers/#{oidc_provider.id}/identities", + identity: attrs + ) + + assert resp = json_response(conn, 201) + assert resp["data"]["provider_identifier"] == attrs["email"] + assert resp["data"]["email"] == attrs["email"] + end + + test "creates an identity with provider_identifier attr and email attr being the same value", + %{ + conn: conn, + account: account, + actor: api_actor + } do + {oidc_provider, _bypass} = + Fixtures.Auth.start_and_create_openid_connect_provider(account: account) + + actor = Fixtures.Actors.create_actor(account: account) + + attrs = %{ + "provider_identifier" => "foo@localhost.local", + "email" => "foo@localhost.local" + } + + conn = + conn + |> authorize_conn(api_actor) + |> put_req_header("content-type", "application/json") + |> post("/actors/#{actor.id}/providers/#{oidc_provider.id}/identities", + identity: attrs + ) + + assert resp = json_response(conn, 201) + assert resp["data"]["provider_identifier"] == attrs["provider_identifier"] + assert resp["data"]["email"] == attrs["email"] + end + + test "creates an identity with provider_identifier attr and email attr being different values", + %{ + conn: conn, + account: account, + actor: api_actor + } do + {oidc_provider, _bypass} = + Fixtures.Auth.start_and_create_openid_connect_provider(account: account) + + actor = Fixtures.Actors.create_actor(account: account) + + attrs = %{ + "provider_identifier" => "foo@localhost.local", + "email" => "bar@localhost.local" + } + + conn = + conn + |> authorize_conn(api_actor) + |> put_req_header("content-type", "application/json") + |> post("/actors/#{actor.id}/providers/#{oidc_provider.id}/identities", + identity: attrs + ) + + assert resp = json_response(conn, 201) + assert resp["data"]["provider_identifier"] == attrs["provider_identifier"] + assert resp["data"]["email"] == attrs["email"] end end @@ -224,7 +424,7 @@ defmodule API.IdentityControllerTest do assert json_response(conn, 401) == %{"error" => %{"reason" => "Unauthorized"}} end - test "deletes a resource", %{conn: conn, account: account, actor: actor} do + test "deletes an identity", %{conn: conn, account: account, actor: actor} do identity = Fixtures.Auth.create_identity(%{account: account, actor: actor}) conn = @@ -238,7 +438,8 @@ defmodule API.IdentityControllerTest do "id" => identity.id, "actor_id" => actor.id, "provider_id" => identity.provider_id, - "provider_identifier" => identity.provider_identifier + "provider_identifier" => identity.provider_identifier, + "email" => nil } } diff --git a/elixir/apps/domain/lib/domain/auth.ex b/elixir/apps/domain/lib/domain/auth.ex index 02b98dff0..c6721048a 100644 --- a/elixir/apps/domain/lib/domain/auth.ex +++ b/elixir/apps/domain/lib/domain/auth.ex @@ -626,6 +626,7 @@ defmodule Domain.Auth do {:error, :not_found} -> {:error, :unauthorized} {:error, :invalid} -> {:error, :unauthorized} {:error, :expired} -> {:error, :unauthorized} + {:error, :internal_error} -> {:error, :internal_error} {:error, %Ecto.Changeset{}} -> {:error, :malformed_request} end end @@ -904,4 +905,13 @@ defmodule Domain.Auth do granted_permissions = fetch_type_permissions!(granted_role) MapSet.subset?(granted_permissions, subject.permissions) end + + def valid_email?(email) do + to_string(email) =~ email_regex() + end + + def email_regex do + # Regex to check if string is in the shape of an email + ~r/^[^\s]+@[^\s]+\.[^\s]+$/ + end end diff --git a/elixir/apps/domain/lib/domain/auth/adapters.ex b/elixir/apps/domain/lib/domain/auth/adapters.ex index 2d3e0620e..8d6967409 100644 --- a/elixir/apps/domain/lib/domain/auth/adapters.ex +++ b/elixir/apps/domain/lib/domain/auth/adapters.ex @@ -101,11 +101,23 @@ defmodule Domain.Auth.Adapters do adapter = fetch_provider_adapter!(provider) case adapter.verify_and_update_identity(provider, payload) do - {:ok, %Identity{} = identity, expires_at} -> {:ok, identity, expires_at} - {:error, :not_found} -> {:error, :not_found} - {:error, :invalid} -> {:error, :invalid} - {:error, :expired} -> {:error, :expired} - {:error, :internal_error} -> {:error, :internal_error} + {:ok, %Identity{} = identity, expires_at} -> + {:ok, identity, expires_at} + + {:error, :not_found} -> + {:error, :not_found} + + {:error, :invalid} -> + {:error, :invalid} + + {:error, :expired} -> + {:error, :expired} + + {:error, :internal_error} -> + {:error, :internal_error} + + {:error, %Ecto.Changeset{} = changeset} -> + {:error, changeset} end end diff --git a/elixir/apps/domain/lib/domain/auth/adapters/google_workspace.ex b/elixir/apps/domain/lib/domain/auth/adapters/google_workspace.ex index 9df1b2314..cd7356db2 100644 --- a/elixir/apps/domain/lib/domain/auth/adapters/google_workspace.ex +++ b/elixir/apps/domain/lib/domain/auth/adapters/google_workspace.ex @@ -38,6 +38,7 @@ defmodule Domain.Auth.Adapters.GoogleWorkspace do @impl true def identity_changeset(%Provider{} = _provider, %Ecto.Changeset{} = changeset) do changeset + |> Domain.Repo.Changeset.trim_change(:email) |> Domain.Repo.Changeset.trim_change(:provider_identifier) |> Domain.Repo.Changeset.copy_change(:provider_virtual_state, :provider_state) |> Ecto.Changeset.put_change(:provider_virtual_state, %{}) diff --git a/elixir/apps/domain/lib/domain/auth/adapters/jumpcloud.ex b/elixir/apps/domain/lib/domain/auth/adapters/jumpcloud.ex index 52376ea32..f38fb9f86 100644 --- a/elixir/apps/domain/lib/domain/auth/adapters/jumpcloud.ex +++ b/elixir/apps/domain/lib/domain/auth/adapters/jumpcloud.ex @@ -36,6 +36,7 @@ defmodule Domain.Auth.Adapters.JumpCloud do @impl true def identity_changeset(%Provider{} = _provider, %Ecto.Changeset{} = changeset) do changeset + |> Domain.Repo.Changeset.trim_change(:email) |> Domain.Repo.Changeset.trim_change(:provider_identifier) |> Domain.Repo.Changeset.copy_change(:provider_virtual_state, :provider_state) |> Ecto.Changeset.put_change(:provider_virtual_state, %{}) diff --git a/elixir/apps/domain/lib/domain/auth/adapters/microsoft_entra.ex b/elixir/apps/domain/lib/domain/auth/adapters/microsoft_entra.ex index e51cd5d4a..8ad54d7c0 100644 --- a/elixir/apps/domain/lib/domain/auth/adapters/microsoft_entra.ex +++ b/elixir/apps/domain/lib/domain/auth/adapters/microsoft_entra.ex @@ -37,6 +37,7 @@ defmodule Domain.Auth.Adapters.MicrosoftEntra do @impl true def identity_changeset(%Provider{} = _provider, %Ecto.Changeset{} = changeset) do changeset + |> Domain.Repo.Changeset.trim_change(:email) |> Domain.Repo.Changeset.trim_change(:provider_identifier) |> Domain.Repo.Changeset.copy_change(:provider_virtual_state, :provider_state) |> Ecto.Changeset.put_change(:provider_virtual_state, %{}) diff --git a/elixir/apps/domain/lib/domain/auth/adapters/okta.ex b/elixir/apps/domain/lib/domain/auth/adapters/okta.ex index 2686508b1..78ff1633a 100644 --- a/elixir/apps/domain/lib/domain/auth/adapters/okta.ex +++ b/elixir/apps/domain/lib/domain/auth/adapters/okta.ex @@ -37,6 +37,7 @@ defmodule Domain.Auth.Adapters.Okta do @impl true def identity_changeset(%Provider{} = _provider, %Ecto.Changeset{} = changeset) do changeset + |> Domain.Repo.Changeset.trim_change(:email) |> Domain.Repo.Changeset.trim_change(:provider_identifier) |> Domain.Repo.Changeset.copy_change(:provider_virtual_state, :provider_state) |> Ecto.Changeset.put_change(:provider_virtual_state, %{}) diff --git a/elixir/apps/domain/lib/domain/auth/adapters/openid_connect.ex b/elixir/apps/domain/lib/domain/auth/adapters/openid_connect.ex index 6567defd1..b37eb19cb 100644 --- a/elixir/apps/domain/lib/domain/auth/adapters/openid_connect.ex +++ b/elixir/apps/domain/lib/domain/auth/adapters/openid_connect.ex @@ -32,6 +32,7 @@ defmodule Domain.Auth.Adapters.OpenIDConnect do @impl true def identity_changeset(%Provider{} = _provider, %Ecto.Changeset{} = changeset) do changeset + |> Domain.Repo.Changeset.trim_change(:email) |> Domain.Repo.Changeset.trim_change(:provider_identifier) |> Domain.Repo.Changeset.copy_change(:provider_virtual_state, :provider_state) |> Ecto.Changeset.put_change(:provider_virtual_state, %{}) @@ -119,7 +120,7 @@ defmodule Domain.Auth.Adapters.OpenIDConnect do code_verifier: code_verifier } - with {:ok, provider_identifier, identity_state} <- + with {:ok, provider_identifier, email, identity_state} <- fetch_state(provider, token_params, identifier_claim) do Identity.Query.not_disabled() |> Identity.Query.by_provider_id(provider.id) @@ -134,6 +135,8 @@ defmodule Domain.Auth.Adapters.OpenIDConnect do # if an email was used in provider identifier and it's replaced by sub claim # later, we want to use the ID from sub claim as provider_identifier |> Ecto.Changeset.put_change(:provider_identifier, provider_identifier) + |> Ecto.Changeset.put_change(:email, email) + |> Identity.Changeset.changeset() end ) |> case do @@ -177,9 +180,10 @@ defmodule Domain.Auth.Adapters.OpenIDConnect do code_verifier: code_verifier } - with {:ok, provider_identifier, identity_state} <- + with {:ok, provider_identifier, email, identity_state} <- fetch_state(provider, token_params, identifier_claim) do Domain.Auth.upsert_identity(actor, provider, %{ + email: email, provider_identifier: provider_identifier, provider_virtual_state: identity_state }) @@ -195,7 +199,7 @@ defmodule Domain.Auth.Adapters.OpenIDConnect do refresh_token: provider.adapter_state["refresh_token"] } - with {:ok, _provider_identifier, adapter_state} <- + with {:ok, _provider_identifier, _email, adapter_state} <- fetch_state(provider, token_params, identifier_claim) do Provider.Query.not_deleted() |> Provider.Query.by_id(provider.id) @@ -247,7 +251,7 @@ defmodule Domain.Auth.Adapters.OpenIDConnect do refresh_token: identity.provider_state["refresh_token"] } - with {:ok, _provider_identifier, identity_state} <- + with {:ok, _provider_identifier, _email, identity_state} <- fetch_state(identity.provider, token_params, identifier_claim) do Identity.Query.not_deleted() |> Identity.Query.by_id(identity.id) @@ -281,7 +285,7 @@ defmodule Domain.Auth.Adapters.OpenIDConnect do provider_identifier = claims[identifier_claim] - {:ok, provider_identifier, + {:ok, provider_identifier, claims["email"] || userinfo["email"], %{ "access_token" => tokens["access_token"], "refresh_token" => tokens["refresh_token"], diff --git a/elixir/apps/domain/lib/domain/auth/identity.ex b/elixir/apps/domain/lib/domain/auth/identity.ex index 9f0b01d3f..a7b5283ab 100644 --- a/elixir/apps/domain/lib/domain/auth/identity.ex +++ b/elixir/apps/domain/lib/domain/auth/identity.ex @@ -5,6 +5,7 @@ defmodule Domain.Auth.Identity do belongs_to :actor, Domain.Actors.Actor, on_replace: :update belongs_to :provider, Domain.Auth.Provider + field :email, :string field :provider_identifier, :string field :provider_state, :map, redact: true field :provider_virtual_state, :map, virtual: true, redact: true diff --git a/elixir/apps/domain/lib/domain/auth/identity/changeset.ex b/elixir/apps/domain/lib/domain/auth/identity/changeset.ex index 4c4606999..007487883 100644 --- a/elixir/apps/domain/lib/domain/auth/identity/changeset.ex +++ b/elixir/apps/domain/lib/domain/auth/identity/changeset.ex @@ -21,7 +21,7 @@ defmodule Domain.Auth.Identity.Changeset do attrs ) do %Identity{} - |> cast(attrs, ~w[provider_identifier provider_virtual_state]a) + |> cast(attrs, ~w[email provider_identifier provider_virtual_state]a) |> validate_required(~w[provider_identifier]a) |> put_change(:actor_id, actor.id) |> put_change(:provider_id, provider.id) @@ -35,7 +35,7 @@ defmodule Domain.Auth.Identity.Changeset do attrs ) do %Identity{} - |> cast(attrs, ~w[provider_identifier provider_state provider_virtual_state]a) + |> cast(attrs, ~w[email provider_identifier provider_state provider_virtual_state]a) |> validate_required(~w[provider_identifier]a) |> cast_assoc(:actor, with: fn _actor, attrs -> @@ -51,7 +51,7 @@ defmodule Domain.Auth.Identity.Changeset do def update_identity_and_actor(%Identity{} = identity, attrs) do identity - |> cast(attrs, ~w[provider_state]a) + |> cast(attrs, ~w[email provider_state]a) |> cast_assoc(:actor, with: fn actor, attrs -> Actors.Actor.Changeset.sync(actor, attrs) @@ -66,6 +66,9 @@ defmodule Domain.Auth.Identity.Changeset do |> unique_constraint(:provider_identifier, name: :auth_identities_account_id_provider_id_provider_identifier_idx ) + |> unique_constraint(:email, + name: :auth_identities_account_id_provider_id_email_idx + ) end def update_identity_provider_state(identity_or_changeset, %{} = state, virtual_state \\ %{}) do diff --git a/elixir/apps/domain/lib/domain/auth/identity/sync.ex b/elixir/apps/domain/lib/domain/auth/identity/sync.ex index 721f54ad6..3aa9a7b98 100644 --- a/elixir/apps/domain/lib/domain/auth/identity/sync.ex +++ b/elixir/apps/domain/lib/domain/auth/identity/sync.ex @@ -95,7 +95,10 @@ defmodule Domain.Auth.Identity.Sync do provider_identifiers_to_insert |> Enum.uniq() |> Enum.reduce_while({:ok, []}, fn provider_identifier, {:ok, acc} -> - attrs = Map.get(attrs_by_provider_identifier, provider_identifier) + attrs = + Map.get(attrs_by_provider_identifier, provider_identifier) + |> add_email_attr() + changeset = Identity.Changeset.create_identity_and_actor(provider, attrs) case Repo.insert(changeset) do @@ -122,7 +125,7 @@ defmodule Domain.Auth.Identity.Sync do |> Enum.reduce(%{}, fn identity, acc -> acc_identity = Map.get(acc, identity.provider_identifier) - # make sure that deleted identities are have the least priority in case of conflicts + # make sure that deleted identities have the least priority in case of conflicts cond do is_nil(acc_identity) -> Map.put(acc, identity.provider_identifier, identity) @@ -139,7 +142,11 @@ defmodule Domain.Auth.Identity.Sync do |> Enum.uniq() |> Enum.reduce_while({:ok, []}, fn provider_identifier, {:ok, acc} -> identity = Map.get(identity_by_provider_identifier, provider_identifier) - attrs = Map.get(attrs_by_provider_identifier, provider_identifier) + + attrs = + Map.get(attrs_by_provider_identifier, provider_identifier) + |> add_email_attr() + changeset = Identity.Changeset.update_identity_and_actor(identity, attrs) case Repo.update(changeset) do @@ -151,4 +158,14 @@ defmodule Domain.Auth.Identity.Sync do end end) end + + defp add_email_attr(attrs) do + email = attrs["provider_state"]["userinfo"]["email"] || "" + + if Domain.Auth.valid_email?(email) do + Map.put(attrs, "email", email) + else + attrs + end + end end diff --git a/elixir/apps/domain/lib/domain/repo/changeset.ex b/elixir/apps/domain/lib/domain/repo/changeset.ex index 06d18af44..eb56cd9e9 100644 --- a/elixir/apps/domain/lib/domain/repo/changeset.ex +++ b/elixir/apps/domain/lib/domain/repo/changeset.ex @@ -206,7 +206,7 @@ defmodule Domain.Repo.Changeset do def validate_email(%Ecto.Changeset{} = changeset, field) do changeset - |> validate_format(field, ~r/^[^\s]+@[^\s]+\.[^\s]+$/, message: "is an invalid email address") + |> validate_format(field, Domain.Auth.email_regex(), message: "is an invalid email address") |> validate_length(field, max: 160) end diff --git a/elixir/apps/domain/priv/repo/migrations/20241120171334_add_identity_email_column.exs b/elixir/apps/domain/priv/repo/migrations/20241120171334_add_identity_email_column.exs new file mode 100644 index 000000000..789957633 --- /dev/null +++ b/elixir/apps/domain/priv/repo/migrations/20241120171334_add_identity_email_column.exs @@ -0,0 +1,9 @@ +defmodule Domain.Repo.Migrations.AddIdentityEmailColumn do + use Ecto.Migration + + def change do + alter table(:auth_identities) do + add(:email, :citext) + end + end +end diff --git a/elixir/apps/domain/priv/repo/migrations/20241126185037_add_identity_email_unique_index.exs b/elixir/apps/domain/priv/repo/migrations/20241126185037_add_identity_email_unique_index.exs new file mode 100644 index 000000000..c6a675ab9 --- /dev/null +++ b/elixir/apps/domain/priv/repo/migrations/20241126185037_add_identity_email_unique_index.exs @@ -0,0 +1,13 @@ +defmodule Domain.Repo.Migrations.AddIdentityEmailUniqueIndex do + use Ecto.Migration + + def change do + create( + index(:auth_identities, [:account_id, :provider_id, :email], + name: :auth_identities_account_id_provider_id_email_idx, + where: "deleted_at IS NULL", + unique: true + ) + ) + end +end diff --git a/elixir/apps/domain/priv/repo/migrations/20241205165118_migrate_email_data.exs b/elixir/apps/domain/priv/repo/migrations/20241205165118_migrate_email_data.exs new file mode 100644 index 000000000..f39d247ee --- /dev/null +++ b/elixir/apps/domain/priv/repo/migrations/20241205165118_migrate_email_data.exs @@ -0,0 +1,22 @@ +defmodule Domain.Repo.Migrations.MigrateEmailData do + use Ecto.Migration + + def change do + execute(""" + UPDATE auth_identities AS ai + SET email = COALESCE( + CASE + WHEN p.adapter = 'email' OR p.adapter = 'userpass' THEN ai.provider_identifier + ELSE COALESCE( + provider_state #>> '{claims,email}', + provider_state #>> '{userinfo,email}' + ) + END + ) + FROM auth_providers AS p + WHERE ai.provider_id = p.id + AND ai.email IS NULL + AND ai.deleted_at IS NULL + """) + end +end diff --git a/elixir/apps/domain/test/domain/auth/adapters/openid_connect_test.exs b/elixir/apps/domain/test/domain/auth/adapters/openid_connect_test.exs index 475563377..1951b3460 100644 --- a/elixir/apps/domain/test/domain/auth/adapters/openid_connect_test.exs +++ b/elixir/apps/domain/test/domain/auth/adapters/openid_connect_test.exs @@ -304,7 +304,9 @@ defmodule Domain.Auth.Adapters.OpenIDConnectTest do assert DateTime.diff(identity.provider_state["expires_at"], DateTime.utc_now()) in 3595..3605 end - test "prefers existing identities over manually created identities on a conflict", %{ + # NOTE: The only time this should happen is if an IdP reuses an email address + # that is already in use in Firezone for a given provider + test "return error on identity conflict", %{ account: account, provider: provider, bypass: bypass @@ -312,7 +314,7 @@ defmodule Domain.Auth.Adapters.OpenIDConnectTest do email = Fixtures.Auth.email() sub = Ecto.UUID.generate() - existing_identity = + _existing_identity = Fixtures.Auth.create_identity( account: account, provider: provider, @@ -346,13 +348,7 @@ defmodule Domain.Auth.Adapters.OpenIDConnectTest do redirect_uri = "https://example.com/" payload = {redirect_uri, code_verifier, "MyFakeCode"} - assert {:ok, identity, _expires_at} = verify_and_update_identity(provider, payload) - - assert identity.id == existing_identity.id - assert identity.provider_state["access_token"] == "MY_ACCESS_TOKEN" - assert identity.provider_state["refresh_token"] == "MY_REFRESH_TOKEN" - - assert DateTime.diff(identity.provider_state["expires_at"], DateTime.utc_now()) in 3595..3605 + assert {:error, %Ecto.Changeset{}} = verify_and_update_identity(provider, payload) end test "verifies newly created identities by email profile field", %{ diff --git a/elixir/apps/domain/test/support/fixtures/auth.ex b/elixir/apps/domain/test/support/fixtures/auth.ex index 33f5c964a..dba6636b4 100644 --- a/elixir/apps/domain/test/support/fixtures/auth.ex +++ b/elixir/apps/domain/test/support/fixtures/auth.ex @@ -446,8 +446,18 @@ defmodule Domain.Fixtures.Auth do |> Fixtures.Actors.create_actor() end) + {email, attrs} = + Map.pop_lazy(attrs, :email, fn -> + if Domain.Auth.valid_email?(provider_identifier) do + provider_identifier + else + nil + end + end) + attrs = Map.put(attrs, :provider_identifier, provider_identifier) attrs = Map.put(attrs, :provider_identifier_confirmation, provider_identifier) + attrs = Map.put(attrs, :email, email) {:ok, identity} = Auth.upsert_identity(actor, provider, attrs) diff --git a/elixir/apps/web/lib/web/live/actors/users/new_identity.ex b/elixir/apps/web/lib/web/live/actors/users/new_identity.ex index 185bdd66e..23b236bc0 100644 --- a/elixir/apps/web/lib/web/live/actors/users/new_identity.ex +++ b/elixir/apps/web/lib/web/live/actors/users/new_identity.ex @@ -99,6 +99,8 @@ defmodule Web.Actors.Users.NewIdentity do end def handle_event("submit", %{"identity" => attrs}, socket) do + attrs = add_email(attrs) + with {:ok, identity} <- Auth.create_identity( socket.assigns.actor, @@ -130,4 +132,14 @@ defmodule Web.Actors.Users.NewIdentity do _ -> ~p"/#{socket.assigns.account}/actors/#{socket.assigns.actor}" end end + + defp add_email(attrs) do + identifier = attrs["provider_identifier"] + + if Domain.Auth.valid_email?(identifier) do + Map.put(attrs, "email", identifier) + else + Map.put(attrs, "email", nil) + end + end end