Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 32 additions & 20 deletions app/controllers/openid_connect/logout_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,34 +5,35 @@ class LogoutController < ApplicationController
include SecureHeadersConcern
include FullyAuthenticatable

before_action :set_devise_failure_redirect_for_concurrent_session_logout, only: [:index]
before_action :set_devise_failure_redirect_for_concurrent_session_logout,
only: [:logout]
before_action :confirm_two_factor_authenticated, only: [:delete]

def index
# Handle logout (with confirmation if initiated by relying partner)
def logout
@logout_form = build_logout_form

result = @logout_form.submit
analytics.oidc_logout_requested(**result.to_h.except(:redirect_uri))
redirect_uri = result.extra[:redirect_uri]

analytics.oidc_logout_requested(**to_event(result))

if result.success? && result.extra[:redirect_uri]
handle_successful_logout_request(result, result.extra[:redirect_uri])
if result.success? && redirect_uri
handle_successful_logout_request(result, redirect_uri)
else
render :error
end
end

# Sign out without confirmation
Comment thread
lmgeorge marked this conversation as resolved.
def delete
@logout_form = build_logout_form
result = @logout_form.submit
redirect_uri = result.extra[:redirect_uri]

analytics.oidc_logout_submitted(**result.to_h.except(:redirect_uri))

if result.success? && (redirect_uri = result.extra[:redirect_uri])
analytics.logout_initiated(**result.to_h.except(:redirect_uri))
irs_attempts_api_tracker.logout_initiated(success: result.success?)
analytics.oidc_logout_submitted(**to_event(result))

redirect_user(redirect_uri, @logout_form.service_provider&.issuer, current_user&.uuid)
sign_out
if result.success? && redirect_uri
handle_logout(result, redirect_uri)
else
render :error
end
Expand Down Expand Up @@ -104,7 +105,8 @@ def build_logout_form
def handle_successful_logout_request(result, redirect_uri)
apply_logout_secure_headers_override(redirect_uri, @logout_form.service_provider)
if require_logout_confirmation?
analytics.oidc_logout_visited(**result.to_h.except(:redirect_uri))
analytics.oidc_logout_visited(**to_event(result))

@params = {
client_id: logout_params[:client_id],
post_logout_redirect_uri: logout_params[:post_logout_redirect_uri],
Expand All @@ -113,15 +115,25 @@ def handle_successful_logout_request(result, redirect_uri)
@service_provider_name = @logout_form.service_provider&.friendly_name
delete_branded_experience(logout: true)

render :index
render :confirm_logout
else
analytics.logout_initiated(**result.to_h.except(:redirect_uri))
irs_attempts_api_tracker.logout_initiated(success: result.success?)
handle_logout(result, redirect_uri)
end
end

sign_out
def handle_logout(result, redirect_uri)
analytics.logout_initiated(**to_event(result))
irs_attempts_api_tracker.logout_initiated(success: result.success?)

redirect_user(redirect_uri, @logout_form.service_provider&.issuer, current_user&.uuid)
end
sign_out

redirect_user(redirect_uri, @logout_form.service_provider&.issuer, current_user&.uuid)
Comment on lines +128 to +130
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noting that the reordering redirect_user to happen after sign_out caused a change in behavior with current_user always being nil. It wasn't caught in our specs since our stub_sign_in helper wasn't faithfully recreating the behavior of Devise's handling of current_user

I accidentally stumbled into this in #10887 with a change to improve the stubbed sign-out behavior. More info at https://github.com/18F/identity-idp/pull/10887/files#r1661091791.

end

# Convert FormResponse into loggable analytics event
# @param [FormResponse] result
def to_event(result)
result.to_h.except(:redirect_uri)
end

def logout_params
Expand Down
2 changes: 1 addition & 1 deletion config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@
post '/api/logger' => 'frontend_log#create'

get '/openid_connect/authorize' => 'openid_connect/authorization#index'
get '/openid_connect/logout' => 'openid_connect/logout#index'
match '/openid_connect/logout' => 'openid_connect/logout#logout', via: %i[get post]
delete '/openid_connect/logout' => 'openid_connect/logout#delete'

get '/robots.txt' => 'robots#index'
Expand Down
Loading