Handle invalid OIDC requests before persisting

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|

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

app/models/null_identity.rb

@@ -12,4 +12,8 @@ def deactivate
   def sp_metadata
     {}
   end
+
+  def session_uuid
+    nil
+  end
 end

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

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