diff --git a/elixir/apps/api/lib/api/controllers/identity_controller.ex b/elixir/apps/api/lib/api/controllers/identity_controller.ex index e0ef01c2c..bdb49ecff 100644 --- a/elixir/apps/api/lib/api/controllers/identity_controller.ex +++ b/elixir/apps/api/lib/api/controllers/identity_controller.ex @@ -49,13 +49,7 @@ defmodule API.IdentityController do "identity" => params }) do subject = conn.assigns.subject - - params = - Map.put_new( - params, - "provider_identifier_confirmation", - Map.get(params, "provider_identifier") - ) + params = put_identifier_confirmation(params) with {:ok, actor} <- Domain.Actors.fetch_actor_by_id(actor_id, subject), {:ok, provider} <- Auth.fetch_provider_by_id(provider_id, subject), @@ -140,4 +134,12 @@ defmodule API.IdentityController do {:provider_check, valid?} end + + defp put_identifier_confirmation(params) do + Map.put_new( + params, + "provider_identifier_confirmation", + Map.get(params, "provider_identifier") + ) + 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 528b25c85..cc610b7f4 100644 --- a/elixir/apps/api/lib/api/schemas/identity_schema.ex +++ b/elixir/apps/api/lib/api/schemas/identity_schema.ex @@ -42,8 +42,7 @@ defmodule API.Schemas.Identity do required: [:identity], example: %{ "identity" => %{ - "provider_identifier" => "2551705710219359", - "email" => "foo@bar.com" + "provider_identifier" => "2551705710219359 or foo@bar.com" } } }) 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 bf94c8a72..528f3c0ce 100644 --- a/elixir/apps/api/test/api/controllers/identity_controller_test.exs +++ b/elixir/apps/api/test/api/controllers/identity_controller_test.exs @@ -253,37 +253,7 @@ defmodule API.IdentityControllerTest do } end - test "returns error on invalid identity attrs", %{ - 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"} - - 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", %{ + test "creates an identity with provider_identifier attr that is not an email address", %{ conn: conn, account: account, actor: api_actor @@ -308,7 +278,7 @@ defmodule API.IdentityControllerTest do assert resp["data"]["email"] == nil end - test "creates an identity with provider_identifier attr only and is an email address", %{ + test "creates an identity with provider_identifier attr that is an email address", %{ conn: conn, account: account, actor: api_actor @@ -332,89 +302,6 @@ defmodule API.IdentityControllerTest do 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 describe "delete/2" do diff --git a/elixir/apps/domain/lib/domain/auth.ex b/elixir/apps/domain/lib/domain/auth.ex index b607009ac..b10b9075d 100644 --- a/elixir/apps/domain/lib/domain/auth.ex +++ b/elixir/apps/domain/lib/domain/auth.ex @@ -435,11 +435,6 @@ defmodule Domain.Auth do %Provider{account_id: account_id} = provider, attrs ) do - attrs = - attrs - |> maybe_put_email() - |> maybe_put_identifier() - Identity.Changeset.create_identity(actor, provider, attrs) |> Adapters.identity_changeset(provider) |> Repo.insert() @@ -911,59 +906,8 @@ defmodule Domain.Auth do 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 - - defp maybe_put_email(params) do - email = - params["email"] - |> to_string - |> String.trim() - - identifier = - params["provider_identifier"] - |> to_string() - |> String.trim() - - cond do - valid_email?(email) -> - params - - 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 - - valid_email?(email) -> - Map.put(params, "provider_identifier", email) - |> Map.put("provider_identifier_confirmation", email) - - true -> - params - end + ~r/^[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\.[A-Za-z0-9]+$/ end end diff --git a/elixir/apps/domain/lib/domain/auth/identity/changeset.ex b/elixir/apps/domain/lib/domain/auth/identity/changeset.ex index d90a05d4f..154a44618 100644 --- a/elixir/apps/domain/lib/domain/auth/identity/changeset.ex +++ b/elixir/apps/domain/lib/domain/auth/identity/changeset.ex @@ -21,8 +21,9 @@ defmodule Domain.Auth.Identity.Changeset do attrs ) do %Identity{} - |> cast(attrs, ~w[email provider_identifier provider_virtual_state]a) + |> cast(attrs, ~w[provider_identifier provider_virtual_state]a) |> validate_required(~w[provider_identifier]a) + |> maybe_put_email_from_identifier() |> put_change(:actor_id, actor.id) |> put_change(:provider_id, provider.id) |> put_change(:account_id, account_id) @@ -35,8 +36,9 @@ defmodule Domain.Auth.Identity.Changeset do attrs ) do %Identity{} - |> cast(attrs, ~w[email provider_identifier provider_state provider_virtual_state]a) + |> cast(attrs, ~w[provider_identifier provider_state provider_virtual_state]a) |> validate_required(~w[provider_identifier]a) + |> maybe_put_email_from_state() |> cast_assoc(:actor, with: fn _actor, attrs -> Actors.Actor.Changeset.create(account_id, attrs) @@ -51,7 +53,8 @@ defmodule Domain.Auth.Identity.Changeset do def update_identity_and_actor(%Identity{} = identity, attrs) do identity - |> cast(attrs, ~w[email provider_state]a) + |> cast(attrs, ~w[provider_state]a) + |> maybe_put_email_from_state() |> cast_assoc(:actor, with: fn actor, attrs -> Actors.Actor.Changeset.sync(actor, attrs) @@ -85,4 +88,29 @@ defmodule Domain.Auth.Identity.Changeset do |> put_change(:provider_virtual_state, %{}) |> put_default_value(:deleted_at, DateTime.utc_now()) end + + defp maybe_put_email_from_identifier(changeset) do + identifier = get_field(changeset, :provider_identifier) + email = get_field(changeset, :email) + + if is_nil(email) and valid_email?(identifier) do + put_change(changeset, :email, identifier) + else + changeset + end + end + + defp maybe_put_email_from_state(changeset) do + case get_field(changeset, :provider_state) do + %{"userinfo" => %{"email" => email}} -> + put_change(changeset, :email, email) + + _ -> + changeset + end + end + + defp valid_email?(email) do + to_string(email) =~ Domain.Auth.email_regex() + end end diff --git a/elixir/apps/domain/lib/domain/auth/identity/sync.ex b/elixir/apps/domain/lib/domain/auth/identity/sync.ex index 3aa9a7b98..67713d560 100644 --- a/elixir/apps/domain/lib/domain/auth/identity/sync.ex +++ b/elixir/apps/domain/lib/domain/auth/identity/sync.ex @@ -97,7 +97,6 @@ defmodule Domain.Auth.Identity.Sync do |> Enum.reduce_while({:ok, []}, fn provider_identifier, {:ok, acc} -> attrs = Map.get(attrs_by_provider_identifier, provider_identifier) - |> add_email_attr() changeset = Identity.Changeset.create_identity_and_actor(provider, attrs) @@ -142,11 +141,7 @@ 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) - |> add_email_attr() - + attrs = Map.get(attrs_by_provider_identifier, provider_identifier) changeset = Identity.Changeset.update_identity_and_actor(identity, attrs) case Repo.update(changeset) do @@ -158,14 +153,4 @@ 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/priv/repo/migrations/20250512171004_backfill_missing_emails.exs b/elixir/apps/domain/priv/repo/migrations/20250512171004_backfill_missing_emails.exs new file mode 100644 index 000000000..9c6a44b4e --- /dev/null +++ b/elixir/apps/domain/priv/repo/migrations/20250512171004_backfill_missing_emails.exs @@ -0,0 +1,18 @@ +defmodule Domain.Repo.Migrations.BackfillMissingEmails do + use Ecto.Migration + + def up do + execute(""" + UPDATE auth_identities ai + SET email = ai.provider_identifier + FROM auth_providers ap + WHERE ai.provider_id = ap.id + AND ap.adapter = 'email' + AND ai.email IS NULL; + """) + end + + def down do + # Nothing to do as we don't know which records to rollback + end +end diff --git a/elixir/apps/domain/test/domain/auth_test.exs b/elixir/apps/domain/test/domain/auth_test.exs index 79ffffc33..bc3518a2d 100644 --- a/elixir/apps/domain/test/domain/auth_test.exs +++ b/elixir/apps/domain/test/domain/auth_test.exs @@ -2339,7 +2339,7 @@ defmodule Domain.AuthTest do test "creates an identity" do account = Fixtures.Accounts.create_account() provider = Fixtures.Auth.create_userpass_provider(account: account) - provider_identifier = Fixtures.Auth.random_provider_identifier(provider) + provider_identifier = to_string(Domain.Fixture.unique_integer()) actor = Fixtures.Actors.create_actor( diff --git a/elixir/apps/domain/test/support/fixtures/auth.ex b/elixir/apps/domain/test/support/fixtures/auth.ex index a842ba41b..766e787de 100644 --- a/elixir/apps/domain/test/support/fixtures/auth.ex +++ b/elixir/apps/domain/test/support/fixtures/auth.ex @@ -435,18 +435,8 @@ 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 aa55f45c6..a75b58693 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 @@ -104,8 +104,6 @@ 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, @@ -137,14 +135,4 @@ 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