From fc5cad479c99c3eab3f6da5eecffba532c27d5a6 Mon Sep 17 00:00:00 2001 From: Darren Siegel Date: Sat, 4 Jan 2025 10:42:45 -0500 Subject: [PATCH] [BUG FIX] [MER-4138] Fix launch issues (#5332) * db transaction, add lti_institution_id * bump version * bump version * update unit test * Auto format --------- Co-authored-by: darrensiegel --- lib/oli/accounts.ex | 96 ++++++++++++++++--- lib/oli/accounts/schemas/user.ex | 8 +- lib/oli_web/controllers/lti_controller.ex | 31 +++--- ...20250104141507_add_institution_to_user.exs | 11 +++ test/oli/lti_accounts_test.exs | 74 ++++++++++++++ 5 files changed, 192 insertions(+), 28 deletions(-) create mode 100644 priv/repo/migrations/20250104141507_add_institution_to_user.exs create mode 100644 test/oli/lti_accounts_test.exs diff --git a/lib/oli/accounts.ex b/lib/oli/accounts.ex index d1587bd34fb..1537ccb6c4e 100644 --- a/lib/oli/accounts.ex +++ b/lib/oli/accounts.ex @@ -350,24 +350,90 @@ defmodule Oli.Accounts do """ def insert_or_update_lms_user(%{sub: sub} = changes, institution_id) do + # First see if we can find a user that matches the sub and institution id exactly. This + # will end up being the most common case (on all launches for a user beyond the first) + user = + case Repo.get_by(User, sub: sub, lti_institution_id: institution_id) do + # If not found directly, do another read of ALL users with this sub. This step + # isn't strictly necessary (we could just call find_user_through_enrollment), but + # do it to make this more robust by reducing the need to rely on the enrollment. + nil -> + case get_all_users_by_sub(sub) do + # If no users with this sub, we can be absolutely sure that we need to create a new user + [] -> nil + # Otherwise, we now need to check to see if one of these users is enrolled in a section + # that is pinned to this institution + _ -> find_user_through_enrollment(sub, institution_id) + end + + user -> + user + end + + case user do + nil -> create_lms_user(changes) + user -> update_lms_user(user, changes) + end + end + + defp get_all_users_by_sub(sub) do + from(u in User, where: u.sub == ^sub) |> Repo.all() + end + + defp find_user_through_enrollment(sub, institution_id) do # using enrollment records, we can infer the user's institution. This is because # an LTI user can be enrolled in multiple sections, but all sections must be from # the same institution. - from(e in Enrollment, - join: s in Section, - on: e.section_id == s.id, - join: u in User, - on: e.user_id == u.id, - join: institution in Institution, - on: s.institution_id == institution.id, - where: - u.sub == ^sub and institution.id == ^institution_id and s.status == :active and - e.status == :enrolled, - limit: 1, - select: u - ) - |> Repo.one() - |> insert_or_update_external_user(changes) + results = + from(e in Enrollment, + join: s in Section, + on: e.section_id == s.id, + join: u in User, + on: e.user_id == u.id, + join: institution in Institution, + on: s.institution_id == institution.id, + where: u.sub == ^sub and institution.id == ^institution_id, + select: u, + order_by: [desc: u.inserted_at] + ) + |> Repo.all() + + # We must handle the fact that duplicate records can exist in the result set, in + # this case we select the "most recently inserted" user record + case results do + [user | _] -> user + [] -> nil + end + end + + defp create_lms_user(%{sub: sub} = changes) do + %User{sub: sub, independent_learner: false} + |> User.noauth_changeset(changes) + |> Repo.insert() + |> case do + {:ok, %User{id: user_id}} = res -> + AccountLookupCache.delete("user_#{user_id}") + + res + + error -> + error + end + end + + defp update_lms_user(%User{} = user, changes) do + user + |> User.noauth_changeset(changes) + |> Repo.update() + |> case do + {:ok, %User{id: user_id}} = res -> + AccountLookupCache.delete("user_#{user_id}") + + res + + error -> + error + end end @doc """ diff --git a/lib/oli/accounts/schemas/user.ex b/lib/oli/accounts/schemas/user.ex index 9ce587adc90..c1c76205725 100644 --- a/lib/oli/accounts/schemas/user.ex +++ b/lib/oli/accounts/schemas/user.ex @@ -49,8 +49,9 @@ defmodule Oli.Accounts.User do pow_user_fields() - # A user may optionally be linked to an author account + # A user may optionally be linked to an author account and Institution belongs_to :author, Oli.Accounts.Author + field :lti_institution_id, :integer, default: nil has_many :enrollments, Oli.Delivery.Sections.Enrollment, on_delete: :delete_all @@ -112,6 +113,7 @@ defmodule Oli.Accounts.User do :phone_number_verified, :address, :author_id, + :lti_institution_id, :guest, :independent_learner, :research_opt_out, @@ -156,6 +158,7 @@ defmodule Oli.Accounts.User do :phone_number_verified, :address, :author_id, + :lti_institution_id, :guest, :independent_learner, :research_opt_out, @@ -237,6 +240,7 @@ defmodule Oli.Accounts.User do :phone_number_verified, :address, :author_id, + :lti_institution_id, :guest, :independent_learner, :research_opt_out, @@ -321,7 +325,7 @@ defimpl Lti_1p3.Tool.Lti_1p3_User, for: Oli.Accounts.User do preload: [:context_roles], join: s in Section, on: e.section_id == s.id, - where: e.user_id == ^user_id and s.slug == ^section_slug and s.status == :active, + where: e.user_id == ^user_id and s.slug == ^section_slug, select: e case Repo.one(query) do diff --git a/lib/oli_web/controllers/lti_controller.ex b/lib/oli_web/controllers/lti_controller.ex index edaa52b6534..c638c8b6399 100644 --- a/lib/oli_web/controllers/lti_controller.ex +++ b/lib/oli_web/controllers/lti_controller.ex @@ -48,19 +48,27 @@ defmodule OliWeb.LtiController do case Lti_1p3.Tool.LaunchValidation.validate(params, session_state) do {:ok, lti_params} -> - # cache user lti params and store the id in the current session - case LtiParams.create_or_update_lti_params(lti_params) do - {:ok, %{id: lti_params_id}} -> - conn = LtiSession.put_session_lti_params(conn, lti_params_id) + result = + Oli.Repo.transaction(fn -> + # cache user lti params and store the id in the current session + case LtiParams.create_or_update_lti_params(lti_params) do + {:ok, %{id: lti_params_id}} -> + conn = LtiSession.put_session_lti_params(conn, lti_params_id) - # handle the valid lti launch - handle_valid_lti_1p3_launch(conn, lti_params) + # handle the valid lti launch + handle_valid_lti_1p3_launch(conn, lti_params) - _ -> - {_error_id, error_msg} = - log_error("An error occurred while creating/updating LTI params") + _ -> + {_error_id, error_msg} = + log_error("An error occurred while creating/updating LTI params") - throw(error_msg) + throw(error_msg) + end + end) + + case result do + {:ok, conn} -> conn + e -> e end {:error, %{reason: :invalid_registration, msg: _msg, issuer: issuer, client_id: client_id}} -> @@ -383,7 +391,8 @@ defmodule OliWeb.LtiController do locale: lti_params["locale"], phone_number: lti_params["phone_number"], phone_number_verified: lti_params["phone_number_verified"], - address: lti_params["address"] + address: lti_params["address"], + lti_institution_id: institution.id }, institution.id ) do diff --git a/priv/repo/migrations/20250104141507_add_institution_to_user.exs b/priv/repo/migrations/20250104141507_add_institution_to_user.exs new file mode 100644 index 00000000000..6d36cd50699 --- /dev/null +++ b/priv/repo/migrations/20250104141507_add_institution_to_user.exs @@ -0,0 +1,11 @@ +defmodule Oli.Repo.Migrations.AddInstitutionToUser do + use Ecto.Migration + + def change do + alter table(:users) do + add :lti_institution_id, :integer, default: nil + end + + create unique_index(:users, [:sub, :lti_institution_id]) + end +end diff --git a/test/oli/lti_accounts_test.exs b/test/oli/lti_accounts_test.exs new file mode 100644 index 00000000000..c71b38f5ff5 --- /dev/null +++ b/test/oli/lti_accounts_test.exs @@ -0,0 +1,74 @@ +defmodule Oli.AccountsTest do + use Oli.DataCase + + alias Oli.Accounts + alias Oli.Accounts.User + alias Lti_1p3.Tool.ContextRoles + + def make_user(institution_id) do + %{ + sub: "1234-1234", + name: "User Name", + given_name: "User", + family_name: "Name", + middle_name: "", + password: "password", + password_confirmation: "password", + email: "user@example.edu", + email_verified: true, + lti_institution_id: institution_id + } + end + + describe "insert or update" do + setup do + Seeder.base_project_with_resource2() + |> Seeder.create_section() + end + + test "no user at all exists", %{section: section} do + institution_id = section.institution_id + + {:ok, user1} = Accounts.insert_or_update_lms_user(make_user(institution_id), institution_id) + + assert user1.lti_institution_id == institution_id + + # Read it again to make sure it was inserted correctly + user = Oli.Repo.get_by(User, sub: user1.sub, lti_institution_id: institution_id) + assert user.lti_institution_id == institution_id + assert user.id == user1.id + + # Now try to insert it again, but this time it will be an update + {:ok, user2} = Accounts.insert_or_update_lms_user(make_user(institution_id), institution_id) + assert user1.id == user2.id + end + + test "a user exists already with this sub, but with nil institution", %{section: section} do + institution_id = section.institution_id + + # Insert a user with nil institution + {:ok, user} = Oli.Repo.insert(User.changeset(%User{}, make_user(nil))) + + # Now a call to insert_or_update_lms_user should create a new user with the institution_id + # because there is no enrollment record + {:ok, user1} = Accounts.insert_or_update_lms_user(make_user(institution_id), institution_id) + + refute user1.id == user.id + end + + test "a user exists already with this sub, but with nil institution AND enrollment", %{ + section: section + } do + institution_id = section.institution_id + + # Insert a user with nil institution and enroll it in a section + {:ok, user} = Oli.Repo.insert(User.changeset(%User{}, make_user(nil))) + Oli.Delivery.Sections.enroll(user.id, section.id, [ContextRoles.get_role(:context_learner)]) + + # Now a call to insert_or_update_lms_user should UPDATE that user, not create a new one + {:ok, user1} = Accounts.insert_or_update_lms_user(make_user(institution_id), institution_id) + + assert user1.id == user.id + end + end +end