Extract user sign-in to dedicated methods

Also, this commit improves the redirect behavior and uses the supplied Sorcery method for final redirect. Having a single approach for user sign-in and redirect allows future changes to the login workflow.

Author: MrSerth

app/controllers/application_controller.rb

@@ -90,13 +90,22 @@ def login_from_authentication_token
       token.update(expire_at: Time.zone.now)
       session[:study_group_id] = token.study_group_id
 
-      # Sorcery Login only works for InternalUsers
-      return auto_login(token.user) if token.user.is_a? InternalUser
+      session[:return_to_url] = request.fullpath
+      authenticate(token.user)
+    end
+  end
 
+  def authenticate(user)
+    if user.is_a? InternalUser
+      # Sorcery Login only works for InternalUsers
+      auto_login(user)
+    else
       # All external users are logged in "manually"
-      session[:external_user_id] = token.user.id
-      token.user
+      session[:external_user_id] = user.id
     end
+
+    flash = {notice: session.delete(:return_to_url_notice), alert: session.delete(:return_to_url_alert)}.compact_blank
+    sorcery_redirect_back_or_to(:root, flash) unless session[:return_to_url] == request.fullpath
   end
 
   def set_sentry_context

app/controllers/concerns/lti.rb

@@ -237,9 +237,6 @@ def store_lti_session_data(parameters)
 
     @lti_parameters.lti_parameters = parameters.slice(*SESSION_PARAMETERS).permit!.to_h
     @lti_parameters.save!
-
-    session[:external_user_id] = current_user.id
-    session[:pair_programming] = parameters[:custom_pair_programming] || false
   rescue ActiveRecord::RecordNotUnique, ActiveRecord::RecordInvalid
     retry
   end

app/controllers/sessions_controller.rb

@@ -27,21 +27,24 @@ def create_through_lti
     store_lti_session_data(params)
     store_nonce(params[:oauth_nonce])
     if params[:custom_redirect_target]
-      redirect_to(URI.parse(params[:custom_redirect_target].to_s).path)
+      session[:return_to_url] = URI.parse(params[:custom_redirect_target].to_s).path
     elsif params[:custom_pair_programming]
-      redirect_to(new_exercise_programming_group_path(@exercise))
+      session[:return_to_url] = new_exercise_programming_group_path(@exercise)
     else
-      redirect_to(implement_exercise_path(@exercise),
-        notice: t("sessions.create_through_lti.session_#{lti_outcome_service?(@exercise, @user) ? 'with' : 'without'}_outcome",
-          consumer: @consumer))
+      session[:return_to_url] = implement_exercise_path(@exercise)
+      session[:return_to_url_notice] =
+        t("sessions.create_through_lti.session_#{lti_outcome_service?(@exercise, @user) ? 'with' : 'without'}_outcome",
+          consumer: @consumer)
     end
+    session[:pair_programming] = params[:custom_pair_programming] || false
+    authenticate(@user)
   end
 
   def create
     if login(params[:email], params[:password], params[:remember_me])
       # We set the user's default study group to the "internal" group (no external id) for the given consumer.
       session[:study_group_id] = current_user.study_groups.find_by(external_id: nil)&.id
-      sorcery_redirect_back_or_to(:root, notice: t('.success'))
+      session[:return_to_url_notice] = t('.success')
     else
       flash.now[:danger] = t('.failure')
       render(:new)

spec/concerns/lti_spec.rb

@@ -271,14 +271,6 @@ class Controller < AnonymousController
   describe '#store_lti_session_data' do
     let(:parameters) { ActionController::Parameters.new({}) }
 
-    it 'stores data in the session' do
-      controller.instance_variable_set(:@user, create(:external_user))
-      controller.instance_variable_set(:@exercise, create(:fibonacci))
-      expect(controller.session).to receive(:[]=).with(:external_user_id, anything)
-      expect(controller.session).to receive(:[]=).with(:pair_programming, anything)
-      controller.send(:store_lti_session_data, parameters)
-    end
-
     it 'creates an LtiParameter Object' do
       expect do
         controller.instance_variable_set(:@user, create(:external_user))

spec/controllers/sessions_controller_spec.rb

@@ -144,11 +144,24 @@
         expect(assigns(:exercise)).to eq(exercise)
       end
 
-      it 'stores LTI parameters in the session' do
+      it 'persists LTI parameters' do
         expect(controller).to receive(:store_lti_session_data)
         perform_request
       end
 
+      it 'updates the session' do
+        expect(controller.session).to receive(:[]=).with(:locale, anything).and_call_original
+        expect(controller.session).to receive(:[]=).with(:study_group_id, anything).and_call_original
+        expect(controller.session).to receive(:[]=).with(:embed_options, anything).and_call_original
+        expect(controller.session).to receive(:[]=).with(:return_to_url, implement_exercise_path(exercise)).and_call_original # Initial redirect by the controller
+        expect(controller.session).to receive(:[]=).with(:return_to_url, nil).and_call_original # Clearing the URL as done by Sorcery
+        expect(controller.session).to receive(:[]=).with(:return_to_url_notice, anything).and_call_original
+        expect(controller.session).to receive(:[]=).with(:external_user_id, anything).and_call_original
+        expect(controller.session).to receive(:[]=).with(:pair_programming, anything).and_call_original
+        expect(controller.session).to receive(:[]=).with('flash', anything).twice.and_call_original
+        perform_request
+      end
+
       it 'stores the OAuth nonce' do
         expect(controller).to receive(:store_nonce).with(nonce)
         perform_request