From 1f457d21274b48c0a71ab81e91c110e74cff1d9e Mon Sep 17 00:00:00 2001 From: Brian Manifold Date: Mon, 16 Dec 2024 15:11:25 -0800 Subject: [PATCH] fix(portal): Fixing a few edge cases for identity email (#7532) --- .../api/controllers/identity_controller.ex | 49 ----------------- elixir/apps/domain/lib/domain/auth.ex | 52 +++++++++++++++++++ .../lib/domain/auth/identity/changeset.ex | 2 +- ...0241216183520_migrate_email_data_again.exs | 21 ++++++++ elixir/apps/domain/test/domain/auth_test.exs | 35 +++++++++++++ 5 files changed, 109 insertions(+), 50 deletions(-) create mode 100644 elixir/apps/domain/priv/repo/migrations/20241216183520_migrate_email_data_again.exs diff --git a/elixir/apps/api/lib/api/controllers/identity_controller.ex b/elixir/apps/api/lib/api/controllers/identity_controller.ex index da8bb619f..e0ef01c2c 100644 --- a/elixir/apps/api/lib/api/controllers/identity_controller.ex +++ b/elixir/apps/api/lib/api/controllers/identity_controller.ex @@ -56,8 +56,6 @@ 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), @@ -142,51 +140,4 @@ 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/domain/lib/domain/auth.ex b/elixir/apps/domain/lib/domain/auth.ex index c6721048a..b607009ac 100644 --- a/elixir/apps/domain/lib/domain/auth.ex +++ b/elixir/apps/domain/lib/domain/auth.ex @@ -435,6 +435,11 @@ 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() @@ -914,4 +919,51 @@ defmodule Domain.Auth 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 + 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 007487883..d90a05d4f 100644 --- a/elixir/apps/domain/lib/domain/auth/identity/changeset.ex +++ b/elixir/apps/domain/lib/domain/auth/identity/changeset.ex @@ -67,7 +67,7 @@ defmodule Domain.Auth.Identity.Changeset do name: :auth_identities_account_id_provider_id_provider_identifier_idx ) |> unique_constraint(:email, - name: :auth_identities_account_id_provider_id_email_idx + name: :auth_identities_acct_id_provider_id_email_prov_ident_unique_idx ) end diff --git a/elixir/apps/domain/priv/repo/migrations/20241216183520_migrate_email_data_again.exs b/elixir/apps/domain/priv/repo/migrations/20241216183520_migrate_email_data_again.exs new file mode 100644 index 000000000..a64f86a0e --- /dev/null +++ b/elixir/apps/domain/priv/repo/migrations/20241216183520_migrate_email_data_again.exs @@ -0,0 +1,21 @@ +defmodule Domain.Repo.Migrations.MigrateEmailDataAgain do + use Ecto.Migration + + def change do + execute(""" + UPDATE auth_identities AS ai + SET email = + 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_test.exs b/elixir/apps/domain/test/domain/auth_test.exs index 94bd03af8..b9694855f 100644 --- a/elixir/apps/domain/test/domain/auth_test.exs +++ b/elixir/apps/domain/test/domain/auth_test.exs @@ -2358,6 +2358,41 @@ defmodule Domain.AuthTest do assert identity.provider_id == provider.id assert identity.provider_identifier == provider_identifier assert identity.actor_id == actor.id + assert identity.email == nil + + assert %Ecto.Changeset{} = identity.provider_virtual_state + + assert %{"password_hash" => _} = identity.provider_state + assert %{password_hash: _} = identity.provider_virtual_state.changes + assert identity.account_id == provider.account_id + assert is_nil(identity.deleted_at) + end + + test "creates an identity when provider_identifier is an email address" do + account = Fixtures.Accounts.create_account() + provider = Fixtures.Auth.create_userpass_provider(account: account) + provider_identifier = Fixtures.Auth.email() + + actor = + Fixtures.Actors.create_actor( + type: :account_admin_user, + account: account, + provider: provider + ) + + password = "Firezone1234" + + attrs = %{ + "provider_identifier" => provider_identifier, + "provider_virtual_state" => %{"password" => password, "password_confirmation" => password} + } + + assert {:ok, identity} = create_identity(actor, provider, attrs) + + assert identity.provider_id == provider.id + assert identity.provider_identifier == provider_identifier + assert identity.actor_id == actor.id + assert identity.email == provider_identifier assert %Ecto.Changeset{} = identity.provider_virtual_state