Skip to content

Include SP name in OIDC logout confirmation #7086

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mitchellhenke merged 7 commits into from
Oct 5, 2022
Merged

Include SP name in OIDC logout confirmation #7086

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
661783f
add failing spec
Oct 4, 2022
36bda9f
Add content indicating users will return to the service provider on l…
Oct 4, 2022
423497d
Rename service_provider name template variable
Oct 4, 2022
32e1209
fix specs
Oct 4, 2022
ae57bb9
translations
Oct 5, 2022
58fe197
Fix indentation
Oct 5, 2022
aa4f9c0
use present?
Oct 5, 2022
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
1 change: 1 addition & 0 deletions app/controllers/openid_connect/logout_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ def handle_successful_logout_request(result, redirect_uri)
post_logout_redirect_uri: logout_params[:post_logout_redirect_uri],
}
@params[:state] = logout_params[:state] if !logout_params[:state].nil?
@service_provider_name = @logout_form.service_provider&.friendly_name
Copy link
Copy Markdown
Contributor

@aduth aduth Oct 5, 2022

Choose a reason for hiding this comment

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

Is it even possible to get to this point without service_provider being present?

Copy link
Copy Markdown
Contributor Author

@mitchellhenke mitchellhenke Oct 5, 2022

Choose a reason for hiding this comment

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

Currently no, but we have some potential changes on the horizon where that may change.

render :index
else
analytics.logout_initiated(**result.to_h.except(:redirect_uri))
Expand Down
26 changes: 15 additions & 11 deletions app/forms/openid_connect_logout_form.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,21 @@ def submit
FormResponse.new(success: success, errors: errors, extra: extra_analytics_attributes)
end

# Used by RedirectUriValidator
def service_provider
return @service_provider if defined?(@service_provider)
sp_from_client_id = ServiceProvider.find_by(issuer: client_id)

@service_provider =
if reject_id_token_hint?
sp_from_client_id
else
identity&.service_provider_record || sp_from_client_id
end

@service_provider
end

private

attr_reader :identity, :success
Expand Down Expand Up @@ -118,17 +133,6 @@ def validate_identity
end
end

# Used by RedirectUriValidator
def service_provider
sp_from_client_id = ServiceProvider.find_by(issuer: client_id)

if reject_id_token_hint?
sp_from_client_id
else
identity&.service_provider_record || sp_from_client_id
end
end

def extra_analytics_attributes
{
client_id_parameter_present: client_id.present?,
Expand Down
6 changes: 5 additions & 1 deletion app/views/openid_connect/logout/index.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,11 @@

<div class='text-center'>
<%= image_tag(asset_url('user-access.svg'), width: '280', height: '91', alt: '') %>
<%= render PageHeadingComponent.new.with_content(t('openid_connect.logout.heading', app_name: APP_NAME)) %>
<% if @service_provider_name.present? %>
<%= render PageHeadingComponent.new.with_content(t('openid_connect.logout.heading_with_sp', app_name: APP_NAME, service_provider_name: @service_provider_name)) %>
<% else %>
<%= render PageHeadingComponent.new.with_content(t('openid_connect.logout.heading', app_name: APP_NAME)) %>
<% end %>
</div>

<%= button_to(
Expand Down
2 changes: 2 additions & 0 deletions config/locales/openid_connect/en.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ en:
no_client_id_or_id_token_hint: This application is misconfigured and must send
either client_id or id_token_hint.
heading: Do you want to sign out of %{app_name}?
heading_with_sp: Do you want to sign out of %{app_name} and return to
%{service_provider_name}?
token:
errors:
expired_code: is expired
Expand Down
2 changes: 2 additions & 0 deletions config/locales/openid_connect/es.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ es:
no_client_id_or_id_token_hint: Esta aplicación está mal configurada y debe
enviar client_id o id_token_hint.
heading: ¿Quieres cerrar sesión en %{app_name}?
heading_with_sp: ¿Quiere cerrar sesión en %{app_name} y regresar a
%{service_provider_name}?
token:
errors:
expired_code: ha expirado
Expand Down
2 changes: 2 additions & 0 deletions config/locales/openid_connect/fr.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ fr:
no_client_id_or_id_token_hint: Cette application est mal configurée et doit
envoyer client_id ou id_token_hint.
heading: Voulez-vous vous déconnecter de %{app_name}?
heading_with_sp: Souhaitez-vous vous déconnecter de %{app_name} et revenir à
%{service_provider_name}?
token:
errors:
expired_code: est expiré
Expand Down
56 changes: 31 additions & 25 deletions spec/controllers/openid_connect/logout_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,22 @@
RSpec.describe OpenidConnect::LogoutController do
let(:state) { SecureRandom.hex }
let(:code) { SecureRandom.uuid }
let(:valid_post_logout_redirect_uri) { 'gov.gsa.openidconnect.test://result/signout' }
let(:post_logout_redirect_uri) { 'gov.gsa.openidconnect.test://result/signout' }

let(:user) { build(:user) }
let(:service_provider) { 'urn:gov:gsa:openidconnect:test' }

let(:service_provider) do
create(
:service_provider, issuer: 'test', redirect_uris: [
valid_post_logout_redirect_uri,
]
)
end
let(:identity) do
create(
:service_provider_identity,
service_provider: service_provider,
service_provider: service_provider.issuer,
user: user,
access_token: SecureRandom.hex,
session_uuid: SecureRandom.uuid,
Expand Down Expand Up @@ -67,7 +75,7 @@
'OIDC Logout Requested',
hash_including(
success: true,
client_id: service_provider,
client_id: service_provider.issuer,
client_id_parameter_present: false,
id_token_hint_parameter_present: true,
errors: {},
Expand All @@ -80,7 +88,7 @@
'Logout Initiated',
hash_including(
success: true,
client_id: service_provider,
client_id: service_provider.issuer,
client_id_parameter_present: false,
id_token_hint_parameter_present: true,
errors: {},
Expand Down Expand Up @@ -123,7 +131,7 @@
with(
'OIDC Logout Requested',
success: false,
client_id: service_provider,
client_id: service_provider.issuer,
client_id_parameter_present: false,
id_token_hint_parameter_present: true,
errors: errors,
Expand Down Expand Up @@ -177,7 +185,7 @@
subject(:action) do
get :index,
params: {
client_id: service_provider,
client_id: service_provider.issuer,
post_logout_redirect_uri: post_logout_redirect_uri,
state: state,
}
Expand All @@ -187,20 +195,16 @@
before { stub_sign_in(user) }

context 'with valid params' do
it 'renders logout confirmation page' do
action
render_views

expect(response).to render_template(:index)
end

it 'tracks events' do
it 'renders logout confirmation page' do
stub_analytics
expect(@analytics).to receive(:track_event).
with(
'OIDC Logout Requested',
hash_including(
success: true,
client_id: service_provider,
client_id: service_provider.issuer,
client_id_parameter_present: true,
id_token_hint_parameter_present: false,
errors: {},
Expand All @@ -213,7 +217,7 @@
'OIDC Logout Page Visited',
hash_including(
success: true,
client_id: service_provider,
client_id: service_provider.issuer,
client_id_parameter_present: true,
id_token_hint_parameter_present: false,
errors: {},
Expand All @@ -223,6 +227,8 @@
)

action
expect(response).to render_template(:index)
expect(response.body).to include service_provider.friendly_name
end
end

Expand Down Expand Up @@ -252,7 +258,7 @@
'OIDC Logout Requested',
hash_including(
success: false,
client_id: service_provider,
client_id: service_provider.issuer,
client_id_parameter_present: true,
id_token_hint_parameter_present: false,
errors: errors,
Expand Down Expand Up @@ -284,7 +290,7 @@
subject(:action) do
delete :delete,
params: {
client_id: service_provider,
client_id: service_provider.issuer,
post_logout_redirect_uri: post_logout_redirect_uri,
state: state,
}
Expand Down Expand Up @@ -350,7 +356,7 @@
subject(:action) do
get :index,
params: {
client_id: service_provider,
client_id: service_provider.issuer,
id_token_hint: id_token_hint,
post_logout_redirect_uri: post_logout_redirect_uri,
state: state,
Expand All @@ -374,7 +380,7 @@
'OIDC Logout Requested',
hash_including(
success: true,
client_id: service_provider,
client_id: service_provider.issuer,
client_id_parameter_present: true,
id_token_hint_parameter_present: false,
errors: {},
Expand All @@ -388,7 +394,7 @@
'OIDC Logout Page Visited',
hash_including(
success: true,
client_id: service_provider,
client_id: service_provider.issuer,
client_id_parameter_present: true,
id_token_hint_parameter_present: false,
errors: {},
Expand Down Expand Up @@ -426,7 +432,7 @@
with(
'OIDC Logout Requested',
success: false,
client_id: service_provider,
client_id: service_provider.issuer,
client_id_parameter_present: true,
id_token_hint_parameter_present: true,
errors: errors,
Expand Down Expand Up @@ -466,7 +472,7 @@
with(
'OIDC Logout Requested',
success: false,
client_id: service_provider,
client_id: service_provider.issuer,
client_id_parameter_present: true,
id_token_hint_parameter_present: false,
errors: errors,
Expand Down Expand Up @@ -496,7 +502,7 @@
subject(:action) do
delete :delete,
params: {
client_id: service_provider,
client_id: service_provider.issuer,
post_logout_redirect_uri: post_logout_redirect_uri,
state: state,
}
Expand All @@ -517,7 +523,7 @@
with(
'OIDC Logout Submitted',
success: true,
client_id: service_provider,
client_id: service_provider.issuer,
client_id_parameter_present: true,
id_token_hint_parameter_present: false,
errors: {},
Expand All @@ -531,7 +537,7 @@
with(
'Logout Initiated',
success: true,
client_id: service_provider,
client_id: service_provider.issuer,
client_id_parameter_present: true,
id_token_hint_parameter_present: false,
errors: {},
Expand Down Expand Up @@ -594,7 +600,7 @@
def add_sp_session_request_url
params = {
acr_values: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF,
client_id: service_provider,
client_id: service_provider.issuer,
nonce: SecureRandom.hex,
redirect_uri: 'gov.gsa.openidconnect.test://result',
response_type: 'code',
Expand Down
Loading