diff --git a/app/controllers/openid_connect/authorization_controller.rb b/app/controllers/openid_connect/authorization_controller.rb index 29627a123e3..e903364b2c1 100644 --- a/app/controllers/openid_connect/authorization_controller.rb +++ b/app/controllers/openid_connect/authorization_controller.rb @@ -4,27 +4,19 @@ class AuthorizationController < ApplicationController include VerifyProfileConcern before_action :build_authorize_form_from_params, only: [:index] + before_action :validate_authorize_form, only: [:index] before_action :store_request, only: [:index] before_action :add_sp_metadata_to_session, only: [:index] before_action :apply_secure_headers_override, only: [:index] - # rubocop:disable Metrics/AbcSize def index return confirm_two_factor_authenticated(request_id) unless user_fully_authenticated? return redirect_to_account_or_verify_profile_url if profile_or_identity_needs_verification? - result = @authorize_form.submit(current_user, session.id) - - track_authorize_analytics(result) - - if (redirect_uri = result.extra[:redirect_uri]) - redirect_to redirect_uri - delete_branded_experience - else - render :error - end + @authorize_form.link_identity_to_service_provider(current_user, session.id) + redirect_to @authorize_form.success_redirect_uri + delete_branded_experience end - # rubocop:enable Metrics/AbcSize private @@ -70,11 +62,23 @@ def authorization_params params.permit(OpenidConnectAuthorizeForm::ATTRS) end + def validate_authorize_form + result = @authorize_form.submit + track_authorize_analytics(result) + + return if result.success? + + if (redirect_uri = result.extra[:redirect_uri]) + redirect_to redirect_uri + else + render :error + end + end + def store_request return if sp_session[:request_id] client_id = @authorize_form.client_id - return if client_id.blank? @request_id = SecureRandom.uuid ServiceProviderRequest.find_or_create_by(uuid: @request_id) do |sp_request| diff --git a/app/forms/openid_connect_authorize_form.rb b/app/forms/openid_connect_authorize_form.rb index c5723c0eb84..eb880f33f2e 100644 --- a/app/forms/openid_connect_authorize_form.rb +++ b/app/forms/openid_connect_authorize_form.rb @@ -48,11 +48,9 @@ def initialize(params) ) end - def submit(user, rails_session_id) + def submit @success = valid? - link_identity_to_client_id(user, rails_session_id) if success - FormResponse.new(success: success, errors: errors.messages, extra: extra_analytics_attributes) end @@ -68,6 +66,22 @@ def service_provider @_service_provider ||= ServiceProvider.from_issuer(client_id) end + def link_identity_to_service_provider(current_user, rails_session_id) + identity_linker = IdentityLinker.new(current_user, client_id) + @identity = identity_linker.link_identity( + nonce: nonce, + rails_session_id: rails_session_id, + ial: ial, + scope: scope.join(' '), + code_challenge: code_challenge + ) + end + + def success_redirect_uri + code = identity&.session_uuid + openid_connect_redirector.success_redirect_uri(code: code) if code + end + private attr_reader :identity, :success, :openid_connect_redirector, :already_linked @@ -96,17 +110,6 @@ def validate_scope errors.add(:scope, t('openid_connect.authorization.errors.no_valid_scope')) end - def link_identity_to_client_id(current_user, rails_session_id) - identity_linker = IdentityLinker.new(current_user, client_id) - @identity = identity_linker.link_identity( - nonce: nonce, - rails_session_id: rails_session_id, - ial: ial, - scope: scope.join(' '), - code_challenge: code_challenge - ) - end - def ial case acr_values.sort.max when Saml::Idp::Constants::LOA1_AUTHN_CONTEXT_CLASSREF @@ -127,10 +130,6 @@ def result_uri success ? success_redirect_uri : error_redirect_uri end - def success_redirect_uri - openid_connect_redirector.success_redirect_uri(code: identity.session_uuid) - end - def error_redirect_uri openid_connect_redirector.error_redirect_uri end diff --git a/app/models/null_identity.rb b/app/models/null_identity.rb index 340c8e78b56..4cf193824be 100644 --- a/app/models/null_identity.rb +++ b/app/models/null_identity.rb @@ -12,4 +12,8 @@ def deactivate def sp_metadata {} end + + def session_uuid + nil + end end diff --git a/spec/controllers/openid_connect/authorization_controller_spec.rb b/spec/controllers/openid_connect/authorization_controller_spec.rb index 98f20b72581..0315fa4c4d9 100644 --- a/spec/controllers/openid_connect/authorization_controller_spec.rb +++ b/spec/controllers/openid_connect/authorization_controller_spec.rb @@ -130,6 +130,25 @@ end context 'user is not signed in' do + context 'without valid acr_values' do + before { params.delete(:acr_values) } + + it 'handles the error and does not blow up' do + action + + expect(response).to redirect_to(/^#{params[:redirect_uri]}/) + end + end + + context 'with a bad redirect_uri' do + before { params[:redirect_uri] = '!!!' } + + it 'renders the error page' do + action + expect(controller).to render_template('openid_connect/authorization/error') + end + end + it 'redirects to SP landing page with the request_id in the params' do action sp_request_id = ServiceProviderRequest.last.uuid diff --git a/spec/forms/openid_connect_authorize_form_spec.rb b/spec/forms/openid_connect_authorize_form_spec.rb index 187160c5d9a..5d1d0d49aa3 100644 --- a/spec/forms/openid_connect_authorize_form_spec.rb +++ b/spec/forms/openid_connect_authorize_form_spec.rb @@ -34,64 +34,22 @@ let(:code_challenge_method) { nil } describe '#submit' do - let(:user) { create(:user) } - let(:rails_session_id) { SecureRandom.hex } - subject(:result) { form.submit(user, rails_session_id) } + subject(:result) { form.submit } context 'with valid params' do it 'is successful' do allow(FormResponse).to receive(:new) - form.submit(user, rails_session_id) - - identity = user.identities.where(service_provider: client_id).first + form.submit extra_attributes = { client_id: client_id, - redirect_uri: "#{redirect_uri}?code=#{identity.session_uuid}&state=#{state}", + redirect_uri: nil, } expect(FormResponse).to have_received(:new). with(success: true, errors: {}, extra: extra_attributes) end - - it 'links an identity for this client with the code as the session_uuid' do - redirect_uri = URI(result.to_h[:redirect_uri]) - code = URIService.params(redirect_uri)[:code] - - identity = user.identities.where(service_provider: client_id).first - expect(identity.session_uuid).to eq(code) - expect(identity.nonce).to eq(nonce) - end - - it 'sets the max ial/loa from the request on the identity' do - result - - identity = user.identities.where(service_provider: client_id).first - expect(identity.ial).to eq(3) - end - - context 'with PKCE' do - let(:code_challenge) { 'abcdef' } - let(:code_challenge_method) { 'S256' } - - it 'records the code_challenge on the identity' do - allow(FormResponse).to receive(:new) - - form.submit(user, rails_session_id) - - identity = user.identities.where(service_provider: client_id).first - - extra_attributes = { - client_id: client_id, - redirect_uri: "#{redirect_uri}?code=#{identity.session_uuid}&state=#{state}", - } - - expect(identity.code_challenge).to eq(code_challenge) - expect(FormResponse).to have_received(:new). - with(success: true, errors: {}, extra: extra_attributes) - end - end end context 'with invalid params' do @@ -291,4 +249,45 @@ expect(form.client_id).to eq 'foobar' end end + + describe '#link_identity_to_service_provider' do + let(:user) { create(:user) } + let(:rails_session_id) { SecureRandom.hex } + + context 'with PKCE' do + let(:code_challenge) { 'abcdef' } + let(:code_challenge_method) { 'S256' } + + it 'records the code_challenge on the identity' do + form.link_identity_to_service_provider(user, rails_session_id) + + identity = user.identities.where(service_provider: client_id).first + + expect(identity.code_challenge).to eq(code_challenge) + expect(identity.nonce).to eq(nonce) + expect(identity.ial).to eq(3) + end + end + end + + describe '#success_redirect_uri' do + let(:user) { create(:user) } + let(:rails_session_id) { SecureRandom.hex } + + context 'when the identity has been linked' do + it 'returns a redirect URI with the code from the identity session_uuid' do + form.link_identity_to_service_provider(user, rails_session_id) + identity = user.identities.where(service_provider: client_id).first + + expect(form.success_redirect_uri). + to eq "#{redirect_uri}?code=#{identity.session_uuid}&state=#{state}" + end + end + + context 'when the identity has not been linked' do + it 'returns nil' do + expect(form.success_redirect_uri).to be_nil + end + end + end end