[BUG FIX] [MER-4138] Fix maybe create sub (#5333)

* full remove maybe_create_unique_sub

* set sub explicity, rename test module conflict from 29.3 change

* keep sub in memory

lib/oli/accounts/schemas/user.ex

@@ -126,7 +126,6 @@ defmodule Oli.Accounts.User do
     |> cast_embed(:preferences)
     |> validate_required_if([:email], &is_independent_learner_not_guest/1)
     |> unique_constraint(:email, name: :users_email_independent_learner_index)
-    |> maybe_create_unique_sub()
     |> lowercase_email()
     |> maybe_name_from_given_and_family()
   end
@@ -171,7 +170,6 @@ defmodule Oli.Accounts.User do
     ])
     |> cast_embed(:preferences)
     |> validate_required_if([:email], &is_independent_learner_not_guest/1)
-    |> maybe_create_unique_sub()
     |> lowercase_email()
     |> maybe_name_from_given_and_family()
   end
@@ -199,7 +197,6 @@ defmodule Oli.Accounts.User do
     |> cast(attrs, [:given_name, :family_name, :email])
     |> validate_required_if([:email], &is_independent_learner_not_guest/1)
     |> unique_constraint(:email, name: :users_email_independent_learner_index)
-    |> maybe_create_unique_sub()
     |> lowercase_email()
     |> maybe_name_from_given_and_family()
   end
@@ -257,7 +254,6 @@ defmodule Oli.Accounts.User do
       "You must verify you are old enough to access our site in order to continue"
     )
     |> unique_constraint(:email, name: :users_email_independent_learner_index)
-    |> maybe_create_unique_sub()
     |> lowercase_email()
     |> validate_email_confirmation()
     |> maybe_name_from_given_and_family()
@@ -266,7 +262,6 @@ defmodule Oli.Accounts.User do
   def user_identity_changeset(user_or_changeset, user_identity, attrs, user_id_attrs) do
     user_or_changeset
     |> Ecto.Changeset.cast(attrs, [:name, :given_name, :family_name, :picture])
-    |> maybe_create_unique_sub()
     |> pow_assent_user_identity_changeset(user_identity, attrs, user_id_attrs)
   end
 

lib/oli/utils.ex

@@ -101,25 +101,6 @@ defmodule Oli.Utils do
     end
   end
 
-  def maybe_create_unique_sub(changeset) do
-    case changeset do
-      # if changeset is valid and doesn't have a name in changes or data, derive name from given_name and family_name
-      %Ecto.Changeset{valid?: true, changes: changes, data: data} ->
-        case {Map.get(changes, :sub), Map.get(data, :sub)} do
-          {nil, nil} ->
-            sub = UUID.uuid4()
-
-            Ecto.Changeset.put_change(changeset, :sub, sub)
-
-          _ ->
-            changeset
-        end
-
-      _ ->
-        changeset
-    end
-  end
-
   def maybe_name_from_given_and_family(changeset) do
     case changeset do
       # here we try to derive a full display name using changes or data for name

lib/oli_web/live/delivery/student/assignments_live.ex

@@ -23,7 +23,7 @@ defmodule OliWeb.Delivery.Student.AssignmentsLive do
          :contains_explorations,
          :contains_deliberate_practice
        ], %Section{}},
-    current_user: {[:id, :name, :email], %User{}}
+    current_user: {[:id, :name, :email, :sub], %User{}}
   }
 
   def mount(_params, _session, socket) do

lib/oli_web/live/delivery/student/learn_live.ex

@@ -38,7 +38,7 @@ defmodule OliWeb.Delivery.Student.LearnLive do
          :contains_deliberate_practice,
          :open_and_free
        ], %Sections.Section{}},
-    current_user: {[:id, :name, :email], %User{}}
+    current_user: {[:id, :name, :email, :sub], %User{}}
   }
 
   @page_resource_type_id Oli.Resources.ResourceType.get_id_by_type("page")

lib/oli_web/live/delivery/student/lesson_live.ex

@@ -33,7 +33,7 @@ defmodule OliWeb.Delivery.Student.LessonLive do
   @required_keys_per_assign %{
     section:
       {[:id, :slug, :title, :brand, :lti_1p3_deployment, :customizations], %Sections.Section{}},
-    current_user: {[:id, :name, :email], %User{}}
+    current_user: {[:id, :name, :email, :sub], %User{}}
   }
 
   @default_selected_view :gallery

lib/oli_web/live/delivery/student/prologue_live.ex

@@ -25,7 +25,7 @@ defmodule OliWeb.Delivery.Student.PrologueLive do
     section:
       {[:id, :slug, :title, :brand, :lti_1p3_deployment, :resource_gating_index, :customizations],
        %Sections.Section{}},
-    current_user: {[:id, :name, :email], %User{}}
+    current_user: {[:id, :name, :email, :sub], %User{}}
   }
 
   def mount(params, _session, socket) do

lib/oli_web/pow/user_context.ex

@@ -92,6 +92,13 @@ defmodule OliWeb.Pow.UserContext do
             _ -> params
           end
 
+        # Ensure that we have a unique sub for this user creation. We used to
+        # do this in the changeset, but that lead to some serious issues with
+        # accidentally changing the sub during updates. So we're moving it here to
+        # make the sub generation explicit.
+        sub = UUID.uuid4()
+        params = Map.put(params, "sub", sub)
+
         %User{}
         |> User.verification_changeset(params)
         |> Repo.insert()

test/oli/lti_accounts_test.exs

@@ -1,4 +1,4 @@
-defmodule Oli.AccountsTest do
+defmodule Oli.LtiAccountsTest do
   use Oli.DataCase
 
   alias Oli.Accounts