diff --git a/app/controllers/openid_connect/logout_controller.rb b/app/controllers/openid_connect/logout_controller.rb index bceb0fc2217..0468265e192 100644 --- 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,6 +101,7 @@ def require_logout_confirmation? current_user end + # @return [OpenidConnectLogoutForm] def build_logout_form OpenidConnectLogoutForm.new( params: logout_params, @@ -96,6 +109,8 @@ def build_logout_form ) 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 index 6dbf8734518..582906342a2 100644 --- 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 index a2e67ab2aef..249b2485217 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -4040,6 +4040,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, @@ -4052,6 +4053,7 @@ def oidc_logout_requested( errors: nil, error_details: nil, method: nil, + original_method: nil, **extra ) track_event( @@ -4066,6 +4068,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 index 6680e102702..83509881b6a 100644 --- 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 index 6f70f9fc845..66381873df6 100644 --- 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