Skip to content

Remove internal uses of GET-based logout route #8998

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 1 commit into from
Aug 14, 2023
Merged

Remove internal uses of GET-based logout route #8998

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 9 additions & 5 deletions app/controllers/users/sessions_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,15 @@ def create
end

def destroy
analytics.logout_initiated(sp_initiated: false, oidc: false)
irs_attempts_api_tracker.logout_initiated(
success: true,
)
super
if request.method == 'GET' && IdentityConfig.store.disable_logout_get_request
redirect_to root_path
else
analytics.logout_initiated(sp_initiated: false, oidc: false)
irs_attempts_api_tracker.logout_initiated(
success: true,
)
super
end
end

private
Expand Down
4 changes: 3 additions & 1 deletion app/views/accounts/_nav_auth.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@
<span class="display-none tablet:display-inline padding-right-1 margin-right-1 border-right border-base-darker">
<%= t('account.welcome') %> <strong><%= greeting %></strong>
</span>
<%= link_to t('links.sign_out'), destroy_user_session_path %>

<%= button_to t('links.sign_out'), logout_path, method: :delete, class: 'usa-button usa-button--unstyled' %>

<% if enable_mobile_nav %>
<button class="usa-menu-btn margin-left-2"><%= t('account.navigation.menu') %></button>
<% end %>
Expand Down
5 changes: 4 additions & 1 deletion app/views/session_timeout/_warning.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,10 @@
t('continue', scope: modal_presenter.translation_scope),
) %>
<%= render ButtonComponent.new(
action: ->(**tag_options, &block) { link_to(destroy_user_session_path, **tag_options, &block) },
action: ->(**tag_options, &block) do
button_to(logout_path, **tag_options, &block)
end,
method: :delete,
big: true,
full_width: true,
outline: true,
Expand Down
6 changes: 5 additions & 1 deletion app/views/shared/_cancel.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@
<% if user_signing_up? %>
<%= link_to cancel_link_text, sign_up_cancel_path, method: :get, class: 'usa-button usa-button--unstyled' %>
<% else %>
<%= link_to cancel_link_text, link || return_to_sp_cancel_path %>
<% if defined?(link_method) %>
Copy link
Contributor

@aduth aduth Aug 14, 2023

Choose a reason for hiding this comment

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

Wonder if this might have been a good candidate for the button_or_link_to helper.

identity-idp/app/helpers/link_helper.rb

Line 17 in 0579d71

def button_or_link_to(name = nil, options = nil, html_options = nil, &block)

The usa-button--unstyled logic isn't handled there, but (a) maybe it should be or (b) maybe it's fine to apply usa-button--unstyled to a link.

Copy link
Contributor Author

@mitchellhenke mitchellhenke Aug 14, 2023

Choose a reason for hiding this comment

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

The helper did come up while I was working on it, but it initially seemed like it would be more effort to make the helper work for this case.

Somewhat related, but I'd also like to re-work some of this template too

<%= button_to cancel_link_text, link, method: link_method, class: 'usa-button usa-button--unstyled' %>
<% else %>
<%= link_to cancel_link_text, link %>
<% end %>
<% end %>
<% end %>
2 changes: 1 addition & 1 deletion app/views/sign_up/passwords/new.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,6 @@

<%= render 'shared/password_accordion' %>

<%= render 'shared/cancel', link: destroy_user_session_path %>
<%= render 'shared/cancel' %>

<%= javascript_packs_tag_once 'pw-strength' %>
2 changes: 1 addition & 1 deletion app/views/users/piv_cac_setup_from_sign_in/success.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,4 @@
<%= button_to t('forms.buttons.continue'), login_add_piv_cac_success_path,
class: 'usa-button usa-button--big usa-button--wide', method: :post %>

<%= render 'shared/cancel', link: destroy_user_session_path %>
<%= render 'shared/cancel', link: logout_path, link_method: :delete %>
2 changes: 1 addition & 1 deletion app/views/users/two_factor_authentication_setup/index.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,4 @@
<%= f.submit t('forms.buttons.continue'), class: 'margin-bottom-1' %>
<% end %>

<%= render 'shared/cancel', link: destroy_user_session_path %>
<%= render 'shared/cancel', link: logout_path, link_method: :delete %>
2 changes: 2 additions & 0 deletions config/application.yml.default
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ deleted_user_accounts_report_configs: '[]'
development_mailer_deliver_method: letter_opener
disable_csp_unsafe_inline: true
disable_email_sending: true
disable_logout_get_request: true
disallow_all_web_crawlers: true
disposable_email_services: '[]'
doc_auth_attempt_window_in_minutes: 360
Expand Down Expand Up @@ -438,6 +439,7 @@ production:
database_worker_jobs_host: ''
database_worker_jobs_password: ''
disable_email_sending: false
disable_logout_get_request: false
disallow_all_web_crawlers: false
doc_auth_vendor: 'acuant'
doc_auth_vendor_randomize: false
Expand Down
1 change: 1 addition & 0 deletions lib/identity_config.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ def self.build_store(config_map)
config.add(:development_mailer_deliver_method, type: :symbol, enum: [:file, :letter_opener])
config.add(:disable_csp_unsafe_inline, type: :boolean)
config.add(:disable_email_sending, type: :boolean)
config.add(:disable_logout_get_request, type: :boolean)
config.add(:disallow_all_web_crawlers, type: :boolean)
config.add(:disposable_email_services, type: :json)
config.add(:doc_auth_attempt_window_in_minutes, type: :integer)
Expand Down
20 changes: 3 additions & 17 deletions spec/controllers/users/sessions_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,25 +5,11 @@
let(:mock_valid_site) { 'http://example.com' }

describe 'GET /logout' do
it 'tracks a logout event' do
stub_analytics
stub_attempts_tracker
expect(@analytics).to receive(:track_event).with(
'Logout Initiated',
hash_including(
sp_initiated: false,
oidc: false,
),
)

it 'does not log user out and redirects to root' do
sign_in_as_user

expect(@irs_attempts_api_tracker).to receive(:logout_initiated).with(
success: true,
)

get :destroy
expect(controller.current_user).to be nil
expect(controller.current_user).to_not be nil
expect(response).to redirect_to root_url
end
end

Expand Down
2 changes: 1 addition & 1 deletion spec/features/idv/clearing_and_restarting_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
context 'after signing out' do
before do
visit account_path
first(:link, t('links.sign_out')).click
first(:button, t('links.sign_out')).click
start_idv_from_sp
sign_in_live_with_2fa(user)
end
Expand Down
2 changes: 1 addition & 1 deletion spec/features/idv/steps/gpo_step_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ def complete_idv_and_sign_out
click_continue
visit root_path
click_on t('idv.gpo.return_to_profile')
first(:link, t('links.sign_out')).click
first(:button, t('links.sign_out')).click
end

def complete_idv_and_return_to_gpo_step
Expand Down
2 changes: 1 addition & 1 deletion spec/features/multiple_emails/sign_in_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@

expect(page).to have_current_path(account_path)

first(:link, t('links.sign_out')).click
first(:button, t('links.sign_out')).click

signin(email2, user.password)
fill_in_code_with_last_phone_otp
Expand Down
4 changes: 3 additions & 1 deletion spec/features/openid_connect/openid_connect_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -797,7 +797,9 @@
uncheck(t('forms.messages.remember_device'))
fill_in_code_with_last_phone_otp
click_submit_default
visit destroy_user_session_url

visit account_path
click_button t('links.sign_out')

visit_idp_from_ial1_oidc_sp(prompt: 'select_account')
fill_in_credentials_and_submit(user.email, user.password)
Expand Down
4 changes: 2 additions & 2 deletions spec/features/remember_device/phone_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ def remember_device_and_sign_out_user
check t('forms.messages.remember_device')
fill_in_code_with_last_phone_otp
click_submit_default
first(:link, t('links.sign_out')).click
first(:button, t('links.sign_out')).click
user
end

Expand All @@ -36,7 +36,7 @@ def remember_device_and_sign_out_user
click_submit_default
skip_second_mfa_prompt

first(:link, t('links.sign_out')).click
first(:button, t('links.sign_out')).click
user
end

Expand Down
6 changes: 3 additions & 3 deletions spec/features/remember_device/revocation_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
find_sidenav_forget_browsers_link.click
click_on(t('forms.buttons.confirm'))

first(:link, t('links.sign_out')).click
first(:button, t('links.sign_out')).click

expect_mfa_to_be_required_for_user(user)
end
Expand All @@ -34,7 +34,7 @@
find_sidenav_forget_browsers_link.click
click_on(t('forms.buttons.confirm'))

first(:link, t('links.sign_out')).click
first(:button, t('links.sign_out')).click

expect_mfa_to_be_required_for_user(user)
end
Expand All @@ -51,7 +51,7 @@ def sign_in_with_remember_device_and_sign_out
check t('forms.messages.remember_device')
fill_in_code_with_last_phone_otp
click_submit_default
first(:link, t('links.sign_out')).click
first(:button, t('links.sign_out')).click
end

def expect_mfa_to_be_required_for_user(user)
Expand Down
2 changes: 1 addition & 1 deletion spec/features/remember_device/session_expiration_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
check t('forms.messages.remember_device')
fill_in_code_with_last_phone_otp
click_submit_default
first(:link, t('links.sign_out')).click
first(:button, t('links.sign_out')).click

IdentityLinker.new(
user, build(:service_provider, issuer: OidcAuthHelper::OIDC_IAL1_ISSUER)
Expand Down
2 changes: 1 addition & 1 deletion spec/features/remember_device/sp_expiration_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ def visit_sp(protocol, aal)
click_submit_default
skip_second_mfa_prompt

first(:link, t('links.sign_out')).click
first(:button, t('links.sign_out')).click
user_record
end

Expand Down
6 changes: 3 additions & 3 deletions spec/features/remember_device/totp_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ def remember_device_and_sign_out_user
fill_in :code, with: generate_totp_code(user.auth_app_configurations.first.otp_secret_key)
check t('forms.messages.remember_device')
click_submit_default
first(:link, t('links.sign_out')).click
first(:button, t('links.sign_out')).click
user
end

Expand All @@ -32,7 +32,7 @@ def remember_device_and_sign_out_user
click_submit_default
skip_second_mfa_prompt

first(:link, t('links.sign_out')).click
first(:button, t('links.sign_out')).click
user
end

Expand All @@ -52,7 +52,7 @@ def remember_device_and_sign_out_user
check t('forms.messages.remember_device')
click_submit_default
expect(page).to have_current_path(account_two_factor_authentication_path)
first(:link, t('links.sign_out')).click
first(:button, t('links.sign_out')).click
user
end

Expand Down
12 changes: 6 additions & 6 deletions spec/features/remember_device/user_opted_preference_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
click_button 'Submit'
skip_second_mfa_prompt

first(:link, t('links.sign_out')).click
first(:button, t('links.sign_out')).click
sign_in_user(user)
end

Expand All @@ -44,7 +44,7 @@
click_continue
skip_second_mfa_prompt

first(:link, t('links.sign_out')).click
first(:button, t('links.sign_out')).click
sign_in_user(user)
end

Expand Down Expand Up @@ -72,7 +72,7 @@
click_submit_default
skip_second_mfa_prompt

first(:link, t('links.sign_out')).click
first(:button, t('links.sign_out')).click
sign_in_user(user)
end

Expand All @@ -92,7 +92,7 @@
fill_in :code, with: generate_totp_code(user.auth_app_configurations.first.otp_secret_key)
uncheck t('forms.messages.remember_device')
click_submit_default
first(:link, t('links.sign_out')).click
first(:button, t('links.sign_out')).click
sign_in_user(user)
end

Expand All @@ -119,7 +119,7 @@
sign_in_user(user)
uncheck(:remember_device)
mock_successful_webauthn_authentication { click_webauthn_authenticate_button }
first(:link, t('links.sign_out')).click
first(:button, t('links.sign_out')).click

sign_in_user(user)
end
Expand All @@ -138,7 +138,7 @@
uncheck t('forms.messages.remember_device')
fill_in_code_with_last_phone_otp
click_submit_default
first(:link, t('links.sign_out')).click
first(:button, t('links.sign_out')).click

sign_in_user(user)
end
Expand Down
12 changes: 6 additions & 6 deletions spec/features/remember_device/webauthn_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ def remember_device_and_sign_out_user
sign_in_user(user)
check t('forms.messages.remember_device')
mock_successful_webauthn_authentication { click_webauthn_authenticate_button }
first(:link, t('links.sign_out')).click
first(:button, t('links.sign_out')).click
user
end

Expand All @@ -46,7 +46,7 @@ def remember_device_and_sign_out_user
mock_press_button_on_hardware_key_on_setup
skip_second_mfa_prompt

first(:link, t('links.sign_out')).click
first(:button, t('links.sign_out')).click
user
end

Expand All @@ -64,7 +64,7 @@ def remember_device_and_sign_out_user
check t('forms.messages.remember_device')
mock_press_button_on_hardware_key_on_setup
expect(page).to have_current_path(account_two_factor_authentication_path)
first(:link, t('links.sign_out')).click
first(:button, t('links.sign_out')).click
user
end

Expand All @@ -89,7 +89,7 @@ def remember_device_and_sign_out_user
sign_in_user(user)
check t('forms.messages.remember_device')
mock_successful_webauthn_authentication { click_webauthn_authenticate_button }
first(:link, t('links.sign_out')).click
first(:button, t('links.sign_out')).click
user
end

Expand Down Expand Up @@ -140,7 +140,7 @@ def remember_device_and_sign_out_user
fill_in_code_with_last_phone_otp
click_submit_default

first(:link, t('links.sign_out')).click
first(:button, t('links.sign_out')).click
user
end

Expand All @@ -157,7 +157,7 @@ def remember_device_and_sign_out_user
check t('forms.messages.remember_device')
mock_press_button_on_hardware_key_on_setup
expect(page).to have_current_path(account_two_factor_authentication_path)
first(:link, t('links.sign_out')).click
first(:button, t('links.sign_out')).click
user
end

Expand Down
2 changes: 1 addition & 1 deletion spec/features/saml/ial2_sso_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ def update_mailing_address
end

def sign_out_user
first(:link, t('links.sign_out')).click
first(:button, t('links.sign_out')).click
end

context 'First time registration' do
Expand Down
2 changes: 1 addition & 1 deletion spec/features/saml/saml_logout_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

# Sign out of the IDP
visit account_path
first(:link, t('links.sign_out')).click
first(:button, t('links.sign_out')).click
expect(current_path).to eq root_path

# SAML logout request
Expand Down
2 changes: 1 addition & 1 deletion spec/features/two_factor_authentication/backup_code_sign_up_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,6 @@
end

def sign_out_user
first(:link, t('links.sign_out')).click
first(:button, t('links.sign_out')).click
end
end
Loading