diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index ba2f2fbc..1100ae30 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -25,11 +25,42 @@ def flash protected def after_sign_in_path_for(resource) - if resource.judge? - judge_dashboard_path - else - root_path + # SAML POSTs from the IdP often arrive without the Rails session cookie + # (SameSite). Prefer RelayState / omniauth.origin, which survive that round-trip. + saml_preserved_return_path || + safe_internal_redirect_path(stored_location_for(resource)) || + default_sign_in_path_for(resource) + end + + def default_sign_in_path_for(resource) + resource.judge? ? judge_dashboard_path : root_path + end + + def saml_preserved_return_path + safe_internal_redirect_path(request.params['RelayState']) || + safe_internal_redirect_path(request.env['omniauth.origin']) + end + + def safe_internal_redirect_path(path) + return if path.blank? + + uri = URI.parse(path) + if uri.host.present? + return unless uri.host == request.host + + path = [ uri.path, uri.query ].compact.join('?') end + + return unless path.start_with?('/') && !path.start_with?('//') + + # Homepage / login origins are not meaningful return destinations; fall + # through to role-based defaults (e.g. judges → judge dashboard). + path_only = path.split('?', 2).first + return if path_only.blank? || path_only == '/' || path_only == root_path + + path + rescue URI::InvalidURIError + nil end def after_sign_out_path_for(resource_or_scope) diff --git a/app/controllers/concerns/contest_invite_session.rb b/app/controllers/concerns/contest_invite_session.rb index 0c42153b..ab549469 100644 --- a/app/controllers/concerns/contest_invite_session.rb +++ b/app/controllers/concerns/contest_invite_session.rb @@ -4,6 +4,7 @@ module ContestInviteSession extend ActiveSupport::Concern REDEEMED_SESSION_KEY = :redeemed_contest_instances + PENDING_SESSION_KEY = :pending_contest_invite_token included do before_action :load_redeemed_contest_instances_into_current @@ -21,6 +22,14 @@ def redeem_contest_instance!(contest_instance) Current.redeemed_contest_instances = session[REDEEMED_SESSION_KEY] end + def remember_pending_contest_invite!(contest_instance) + session[PENDING_SESSION_KEY] = contest_instance.access_token + end + + def consume_pending_contest_invite_token + session.delete(PENDING_SESSION_KEY) + end + def redeemed_token_for(contest_instance) Current.redeemed_contest_instances&.dig(contest_instance.id.to_s) end diff --git a/app/controllers/contest_invites_controller.rb b/app/controllers/contest_invites_controller.rb index e4f637c3..9c229429 100644 --- a/app/controllers/contest_invites_controller.rb +++ b/app/controllers/contest_invites_controller.rb @@ -1,11 +1,14 @@ # frozen_string_literal: true class ContestInvitesController < ApplicationController + skip_before_action :authenticate_user!, only: :show + def show @contest_instance = ContestInstance.find_by!(access_token: params[:token]) - unless current_user.profile - redirect_to new_profile_path, alert: 'Please create your profile before accessing this contest.' + unless user_signed_in? + store_location_for(:user, contest_invite_path(token: params[:token])) + redirect_to root_path, alert: 'Please log in to access this contest.' return end @@ -14,6 +17,12 @@ def show return end + unless current_user.profile + remember_pending_contest_invite!(@contest_instance) + redirect_to new_profile_path, alert: 'Please create your profile before accessing this contest.' + return + end + redeem_contest_instance!(@contest_instance) unless @contest_instance.open? @@ -30,6 +39,7 @@ def show redirect_to new_entry_path(contest_instance_id: @contest_instance.id), notice: "Welcome to #{@contest_instance.contest_description.name}." rescue ActiveRecord::RecordNotFound - redirect_to applicant_dashboard_path, alert: 'Invalid or expired contest invite link.' + redirect_to user_signed_in? ? applicant_dashboard_path : root_path, + alert: 'Invalid or expired contest invite link.' end end diff --git a/app/controllers/profiles_controller.rb b/app/controllers/profiles_controller.rb index 6ca25811..1d135f67 100644 --- a/app/controllers/profiles_controller.rb +++ b/app/controllers/profiles_controller.rb @@ -41,7 +41,7 @@ def create respond_to do |format| if @profile.save - format.html { redirect_to applicant_dashboard_path, notice: 'Profile was successfully created.' } + format.html { redirect_to profile_created_redirect_path, notice: 'Profile was successfully created.' } else format.html { render :new, status: :unprocessable_entity } end @@ -78,6 +78,12 @@ def set_profile def authorize_profile authorize @profile end + + def profile_created_redirect_path + token = consume_pending_contest_invite_token + token.present? ? contest_invite_path(token: token) : applicant_dashboard_path + end + # Only allow a list of trusted parameters through. def profile_params params.require(:profile).permit(:user_id, :umid, :preferred_first_name, :preferred_last_name, :class_level_id, :school_id, :campus_id, diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index ffdce7e3..33b1bb85 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -30,6 +30,12 @@ def redirect_back_or_default(default: root_url) redirect_to(session.delete(:return_to) || default, anchor: 'top') end + # Origin passed to SAML so RelayState can restore private-contest (and other) deep links. + def saml_authorize_params + origin = session['user_return_to'].presence + origin ? { origin: origin } : {} + end + def boolean_to_yes_no(value) if value content_tag(:span, 'Yes', class: 'text-success fw-bold') diff --git a/app/views/devise/shared/_links.html.erb b/app/views/devise/shared/_links.html.erb index 0e5e08d1..7cba2bd5 100644 --- a/app/views/devise/shared/_links.html.erb +++ b/app/views/devise/shared/_links.html.erb @@ -1,6 +1,9 @@ <%- if devise_mapping.omniauthable? %> <%- resource_class.omniauth_providers.each do |provider| %> - <%= button_to "Sign in with U-M account", omniauth_authorize_path(resource_name, provider), class: "btn btn-warning btn-sm mb-4", data: { turbo: false } %>
+ <%= button_to "Sign in with U-M account", omniauth_authorize_path(resource_name, provider), + params: saml_authorize_params, + class: "btn btn-warning btn-sm mb-4", + data: { turbo: false } %>
<% end %> <% end %> diff --git a/app/views/static_pages/home.html.erb b/app/views/static_pages/home.html.erb index 37a45efe..b91c76f4 100644 --- a/app/views/static_pages/home.html.erb +++ b/app/views/static_pages/home.html.erb @@ -29,7 +29,10 @@ <% end %>
- <%= button_to "Login to LSA Evaluate", user_saml_omniauth_authorize_path, class: "btn btn-warning btn-sm mb-4", data: { turbo: "false" } %> + <%= button_to "Login to LSA Evaluate", user_saml_omniauth_authorize_path, + params: saml_authorize_params, + class: "btn btn-warning btn-sm mb-4", + data: { turbo: "false" } %>
diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 2bc4d1d9..cd07d6fd 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -308,6 +308,9 @@ person_affiliation: [ 'urn:oid:1.3.6.1.4.1.5923.1.1.1.1' ], principal_name: [ 'urn:oid:1.3.6.1.4.1.5923.1.1.1.6' ] }, request_attributes: {}, + # Carry post-login destination through the IdP via SAML RelayState. + # Session-based Devise return paths are often lost on the ACS POST. + idp_sso_service_url_runtime_params: { origin: :RelayState }, idp_cert_fingerprint: idp_fingerprint, idp_cert_fingerprint_algorithm: 'http://www.w3.org/2000/09/xmldsig#sha256', allowed_clock_drift: 10, diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb new file mode 100644 index 00000000..856dc601 --- /dev/null +++ b/spec/controllers/application_controller_spec.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe ApplicationController, type: :controller do + controller do + def index + head :ok + end + end + + describe '#after_sign_in_path_for' do + let(:judge) { create(:user, :with_judge_role) } + + it 'uses SAML RelayState before the role-based default' do + allow(controller.request).to receive(:params).and_return( + ActionController::Parameters.new('RelayState' => '/c/private-contest-token') + ) + + expect(controller.send(:after_sign_in_path_for, judge)).to eq('/c/private-contest-token') + end + + it 'uses omniauth.origin when RelayState is absent' do + request.env['omniauth.origin'] = '/c/from-omniauth-origin' + + expect(controller.send(:after_sign_in_path_for, judge)).to eq('/c/from-omniauth-origin') + end + + it 'uses the stored destination before the role-based default' do + session['user_return_to'] = '/c/private-contest-token' + + expect(controller.send(:after_sign_in_path_for, judge)).to eq('/c/private-contest-token') + end + + it 'rejects external RelayState values' do + allow(controller.request).to receive(:params).and_return( + ActionController::Parameters.new('RelayState' => 'https://evil.example/phish') + ) + + expect(controller.send(:after_sign_in_path_for, judge)).to eq(judge_dashboard_path) + end + + it 'ignores homepage RelayState so judges still land on the dashboard' do + allow(controller.request).to receive(:params).and_return( + ActionController::Parameters.new('RelayState' => '/') + ) + request.env['omniauth.origin'] = '/' + + expect(controller.send(:after_sign_in_path_for, judge)).to eq(judge_dashboard_path) + end + + it 'uses the role-based default without a stored destination' do + expect(controller.send(:after_sign_in_path_for, judge)).to eq(judge_dashboard_path) + end + end +end diff --git a/spec/controllers/contest_invites_controller_spec.rb b/spec/controllers/contest_invites_controller_spec.rb index 243db2e1..f9c9098b 100644 --- a/spec/controllers/contest_invites_controller_spec.rb +++ b/spec/controllers/contest_invites_controller_spec.rb @@ -64,6 +64,20 @@ end end + context 'when the user is not signed in' do + let(:contest_instance) { create(:contest_instance) } + + before { sign_out user } + + it 'stores the invite path and redirects to login' do + get :show, params: { token: contest_instance.access_token } + + expect(response).to redirect_to(root_path) + expect(session['user_return_to']).to eq(contest_invite_path(token: contest_instance.access_token)) + expect(flash[:alert]).to match(/log in/i) + end + end + context 'when the user has no profile' do let(:user_without_profile) { create(:user) } let(:contest_instance) { create(:contest_instance) } @@ -74,6 +88,7 @@ get :show, params: { token: contest_instance.access_token } expect(response).to redirect_to(new_profile_path) + expect(session[:pending_contest_invite_token]).to eq(contest_instance.access_token) end end end diff --git a/spec/controllers/profiles_controller_spec.rb b/spec/controllers/profiles_controller_spec.rb new file mode 100644 index 00000000..f47152f0 --- /dev/null +++ b/spec/controllers/profiles_controller_spec.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe ProfilesController, type: :controller do + let(:user) { create(:user) } + let(:contest_instance) { create(:contest_instance) } + let(:profile_attributes) do + attributes_for(:profile).merge( + class_level_id: create(:class_level).id, + school_id: create(:school).id, + campus_id: create(:campus).id + ) + end + + before { sign_in user } + + describe 'POST #create' do + it 'returns to a pending contest invite after creating the profile' do + session[:pending_contest_invite_token] = contest_instance.access_token + + post :create, params: { profile: profile_attributes } + + expect(response).to redirect_to(contest_invite_path(token: contest_instance.access_token)) + expect(session[:pending_contest_invite_token]).to be_nil + end + + it 'uses the applicant dashboard when there is no pending invite' do + post :create, params: { profile: profile_attributes } + + expect(response).to redirect_to(applicant_dashboard_path) + end + end +end