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