LG-7434: Allow cross origin for POST OIDC Logout (#10697)

* LG-7434: Allow cross origin for POST OIDC Logout **Why**: - It is expected that requests will be made by relying parties on external domains - The specification for OpenID Connect RP-Initiated Logout 1.0 requires both HTTP `GET` and `POST` methods to be supported. See: https://openid.net/specs/openid-connect-rpinitiated-1_0.html#RPLogout **How**: - The same OpenID Provider endpoint shall be used, `/openid_connect/logout`, but the request data must be sent as part of the body and use form serialization as required for HTTP `POST` requests (RFC 9110, sec. 9.3.3). - Disables Rail's CSRF token verification for the POST route only - `POST` requests will redirect to the `GET` endpoint on the OpenID Provider's API for consistent behavior and handling. resolves https://gitlab.login.gov/lg-people/lg-people-appdev/protocols/openid-connect/-/issues/3 changelog: Bug Fixes, Security, Fix CORS stopping POST for OIDC RP-Initiated Logout 1.0
18F · May 30, 2024 · d44ec8f · d44ec8f
1 parent 8fb67cc
commit d44ec8f
Show file tree

Hide file tree

Showing 5 changed files with 84 additions and 71 deletions.
diff --git a/app/controllers/openid_connect/logout_controller.rb b/app/controllers/openid_connect/logout_controller.rb
@@ -7,16 +7,21 @@ class LogoutController < ApplicationController
     include OpenidConnectRedirectConcern
 
     before_action :set_devise_failure_redirect_for_concurrent_session_logout,
-                  only: [:logout]
+                  only: [:show, :create]
     before_action :confirm_two_factor_authenticated, only: [:delete]
+    skip_before_action :verify_authenticity_token, only: [:create]
 
-    # Handle logout (with confirmation if initiated by relying partner)
-    def logout
+    # +GET+ Handle logout (with confirmation if initiated by relying partner)
+    # @see {OpenID Connect RP-Initiated Logout 1.0 Specification}[https://openid.net/specs/openid-connect-rpinitiated-1_0.html#RPLogout]  # rubocop:disable Layout/LineLength
+    def show
       @logout_form = build_logout_form
       result = @logout_form.submit
       redirect_uri = result.extra[:redirect_uri]
-
-      analytics.oidc_logout_requested(**to_event(result))
+      analytics.oidc_logout_requested(
+        **to_event(result),
+        method: request.method.to_s,
+        original_method: session[:original_method],
+      )
 
       if result.success? && redirect_uri
         handle_successful_logout_request(result, redirect_uri)
@@ -25,6 +30,13 @@ def logout
       end
     end
 
+    # +POST+ Handle logout request (with confirmation if initiated by relying partner)
+    # @see {OpenID Connect RP-Initiated Logout 1.0 Specification}[https://openid.net/specs/openid-connect-rpinitiated-1_0.html#RPLogout] # rubocop:disable Layout/LineLength
+    def create
+      session[:original_method] = request.method.to_s
+      redirect_to action: :show, **logout_params
+    end
+
     # Sign out without confirmation
     def delete
       @logout_form = build_logout_form
@@ -89,13 +101,16 @@ def require_logout_confirmation?
         current_user
     end
 
+    # @return [OpenidConnectLogoutForm]
     def build_logout_form
       OpenidConnectLogoutForm.new(
         params: logout_params,
         current_user: current_user,
       )
     end
 
+    # @param result [FormResponse] Response from submitting @logout_form
+    # @param redirect_uri [String] The URL to redirect the user to after logout
     def handle_successful_logout_request(result, redirect_uri)
       apply_logout_secure_headers_override(redirect_uri, @logout_form.service_provider)
       if require_logout_confirmation?

diff --git a/app/forms/openid_connect_logout_form.rb b/app/forms/openid_connect_logout_form.rb
@@ -46,6 +46,7 @@ def initialize(params:, current_user:)
     @identity = load_identity
   end
 
+  # @return [FormResponse]
   def submit
     @success = valid?
 

diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb
@@ -3945,6 +3945,7 @@ def no_js_detect_stylesheet_loaded(location:, **extra)
   # @param [Hash] errors
   # @param [Hash] error_details
   # @param [String] method
+  # @param [String] original_method Method of referring request
   # OIDC Logout Requested
   def oidc_logout_requested(
     success: nil,
@@ -3957,6 +3958,7 @@ def oidc_logout_requested(
     errors: nil,
     error_details: nil,
     method: nil,
+    original_method: nil,
     **extra
   )
     track_event(
@@ -3971,6 +3973,7 @@ def oidc_logout_requested(
       oidc: oidc,
       saml_request_valid: saml_request_valid,
       method: method,
+      original_method: original_method,
       **extra,
     )
   end

diff --git a/config/routes.rb b/config/routes.rb
@@ -55,7 +55,8 @@
   post '/api/logger' => 'frontend_log#create'
 
   get '/openid_connect/authorize' => 'openid_connect/authorization#index'
-  match '/openid_connect/logout' => 'openid_connect/logout#logout', via: %i[get post]
+  get '/openid_connect/logout' => 'openid_connect/logout#show'
+  post '/openid_connect/logout' => 'openid_connect/logout#create'
   delete '/openid_connect/logout' => 'openid_connect/logout#delete'
 
   get '/robots.txt' => 'robots#index'

diff --git a/spec/controllers/openid_connect/logout_controller_spec.rb b/spec/controllers/openid_connect/logout_controller_spec.rb
@@ -33,21 +33,20 @@
     ).id_token
   end
 
-  shared_examples 'set redirect URL for concurrent session logout' do |req_method|
+  shared_examples 'set redirect URL for concurrent session logout' do |req_action, req_method|
     it "#{req_method}: assigns devise session limited failure redirect url" do
-      process :logout,
-              method: req_method
+      process(req_action, method: req_method)
 
       expect(request.env['devise_session_limited_failure_redirect_url']).to eq(request.url)
     end
   end
 
-  shared_examples 'logout allows id_token_hint' do |req_method|
+  shared_examples 'when allowing id_token_hint' do |req_action, req_method|
     let(:id_token_hint) { valid_id_token_hint }
 
     context 'when sending id_token_hint' do
       subject(:action) do
-        process :logout,
+        process req_action,
                 method: req_method,
                 params: {
                   id_token_hint: id_token_hint,
@@ -210,16 +209,17 @@
             expect(@analytics).to receive(:track_event).
               with(
                 'OIDC Logout Requested',
-                success: false,
-                client_id: service_provider.issuer,
-                client_id_parameter_present: false,
-                id_token_hint_parameter_present: true,
-                errors: errors,
-                error_details: hash_including(*errors.keys),
-                sp_initiated: true,
-                oidc: true,
-                method: nil,
-                saml_request_valid: nil,
+                hash_including(
+                  success: false,
+                  client_id: service_provider.issuer,
+                  client_id_parameter_present: false,
+                  id_token_hint_parameter_present: true,
+                  errors: errors,
+                  error_details: hash_including(*errors.keys),
+                  sp_initiated: true,
+                  oidc: true,
+                  saml_request_valid: nil,
+                ),
               )
 
             action
@@ -235,18 +235,18 @@
             expect(@analytics).to receive(:track_event).
               with(
                 'OIDC Logout Requested',
-                success: false,
-                client_id: nil,
-                client_id_parameter_present: false,
-                id_token_hint_parameter_present: true,
-                errors: hash_including(*errors_keys),
-                error_details: hash_including(*errors_keys),
-                sp_initiated: true,
-                oidc: true,
-                method: nil,
-                saml_request_valid: nil,
+                hash_including(
+                  success: false,
+                  client_id: nil,
+                  client_id_parameter_present: false,
+                  id_token_hint_parameter_present: true,
+                  errors: hash_including(*errors_keys),
+                  error_details: hash_including(*errors_keys),
+                  sp_initiated: true,
+                  oidc: true,
+                  saml_request_valid: nil,
+                ),
               )
-
             action
           end
         end
@@ -283,7 +283,7 @@
 
     context 'when sending client_id' do
       subject(:action) do
-        process :logout,
+        process req_action,
                 method: req_method,
                 params: {
                   client_id: service_provider.issuer,
@@ -366,7 +366,6 @@
                   error_details: hash_including(*errors.keys),
                   sp_initiated: true,
                   oidc: true,
-                  method: nil,
                   saml_request_valid: nil,
                 ),
               )
@@ -406,10 +405,10 @@
     end
   end
 
-  shared_examples 'logout rejects id_token_hint' do |req_method|
+  shared_examples 'when rejecting id_token_hint' do |req_action, req_method|
     let(:id_token_hint) { nil }
     subject(:action) do
-      process :logout,
+      process req_action,
               method: req_method,
               params: {
                 client_id: service_provider.issuer,
@@ -487,16 +486,17 @@
           expect(@analytics).to receive(:track_event).
             with(
               'OIDC Logout Requested',
-              success: false,
-              client_id: service_provider.issuer,
-              client_id_parameter_present: true,
-              id_token_hint_parameter_present: true,
-              errors: errors,
-              error_details: hash_including(*errors.keys),
-              sp_initiated: true,
-              oidc: true,
-              method: nil,
-              saml_request_valid: nil,
+              hash_including(
+                success: false,
+                client_id: service_provider.issuer,
+                client_id_parameter_present: true,
+                id_token_hint_parameter_present: true,
+                errors: errors,
+                error_details: hash_including(*errors.keys),
+                sp_initiated: true,
+                oidc: true,
+                saml_request_valid: nil,
+              ),
             )
 
           action
@@ -527,16 +527,17 @@
           expect(@analytics).to receive(:track_event).
             with(
               'OIDC Logout Requested',
-              success: false,
-              client_id: service_provider.issuer,
-              client_id_parameter_present: true,
-              id_token_hint_parameter_present: false,
-              errors: errors,
-              error_details: hash_including(*errors.keys),
-              sp_initiated: true,
-              oidc: true,
-              method: nil,
-              saml_request_valid: nil,
+              hash_including(
+                success: false,
+                client_id: service_provider.issuer,
+                client_id_parameter_present: true,
+                id_token_hint_parameter_present: false,
+                errors: errors,
+                error_details: hash_including(*errors.keys),
+                sp_initiated: true,
+                oidc: true,
+                saml_request_valid: nil,
+              ),
             )
 
           action
@@ -576,9 +577,9 @@
     end
   end
 
-  describe '#logout' do
-    it_behaves_like 'set redirect URL for concurrent session logout', 'GET'
-    it_behaves_like 'set redirect URL for concurrent session logout', 'POST'
+  describe 'concurrent session management' do
+    it_behaves_like 'set redirect URL for concurrent session logout', :show, 'GET'
+    it_behaves_like 'set redirect URL for concurrent session logout', :create, 'POST'
   end
 
   context 'when accepting id_token_hint and client_id' do
@@ -587,12 +588,8 @@
         and_return(false)
     end
 
-    describe 'GET /openid_connect/logout' do
-      it_behaves_like 'logout allows id_token_hint', 'GET'
-    end
-
-    describe 'POST /openid_connect/logout' do
-      it_behaves_like 'logout allows id_token_hint', 'POST'
+    describe '#logout[GET]' do
+      it_behaves_like 'when allowing id_token_hint', :show, 'GET'
     end
 
     describe '#delete' do
@@ -746,12 +743,8 @@
         and_return(true)
     end
 
-    describe 'GET /openid_connect/logout' do
-      it_behaves_like 'logout rejects id_token_hint', 'GET'
-    end
-
-    describe 'POST /openid_connect/logout' do
-      it_behaves_like 'logout rejects id_token_hint', 'POST'
+    describe '#logout[GET]' do
+      it_behaves_like 'when rejecting id_token_hint', :show, 'GET'
     end
 
     describe '#delete' do