fix(portal): Fix sign_up to properly populate email (#9105)

Why:

* During the account sign up flow, the email of the first admin was not
being populated in the `email` column on the auth_identities table. This
was due to atoms being passed in the attrs instead of strings to the
`create_identity` function. A migration was also created to backfill the
missing emails in the `auth_identities` table.

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

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"
         }
       }
     })

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

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

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

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

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

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(

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)
 

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