Skip to content

[LG-7878] Fully remove SP session during unconfirmed OIDC logout#7232

Merged
orenyk merged 1 commit intomainfrom
oyk-fix-oidc-logout-bug
Oct 28, 2022
Merged

[LG-7878] Fully remove SP session during unconfirmed OIDC logout#7232
orenyk merged 1 commit intomainfrom
oyk-fix-oidc-logout-bug

Conversation

@orenyk
Copy link
Contributor

@orenyk orenyk commented Oct 27, 2022

Resolves LG-7878

🎫 Ticket

https://cm-jira.usa.gov/browse/LG-7878

🛠 Summary of changes

Add an option for delete_branded_experience to remove the SP session and use it during OIDC logout to ensure that users that elect not to terminate their Login.gov session don't end up being prompted to return to the SP that logged them out.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • Visit OIDC sample app
  • Sign in
  • Sign out
  • On confirmation page, choose not to sign out from Login.gov and visit account page

Expected: There will be no alert prompting the user to return to the SP

@orenyk orenyk requested review from a team and mitchellhenke October 27, 2022 04:21
Resolves LG-7878

changelog: Bug Fixes, Authentication, Remove invalid prompt to send user back to SP during OIDC logout
@orenyk orenyk force-pushed the oyk-fix-oidc-logout-bug branch from 5ec24b2 to b137b9f Compare October 27, 2022 04:23
@orenyk
Copy link
Contributor Author

orenyk commented Oct 27, 2022

Build failure is a time zone issue - should pass tomorrow

}
@params[:state] = logout_params[:state] if !logout_params[:state].nil?
@service_provider_name = @logout_form.service_provider&.friendly_name
delete_branded_experience(logout: true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we do this on every branded experience deletion?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering the same. Looking at all the places where this is used and I don't think I see one where that would be inappropriate to remove the sp session.

Copy link
Contributor

Choose a reason for hiding this comment

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

The one thing it would potentially break is the sp bounce check which gets stored in the SP session (but we've discussed moving it into the user session)

Copy link
Contributor Author

@orenyk orenyk Oct 28, 2022

Choose a reason for hiding this comment

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

@mitchellhenke @jmhooper In the interest of time I'd like to propose keeping this the way it is - leaving delete_branded_experience the same for existing use cases that aren't part of logout requests and additionally clearing the session[:sp] in logout / auth terminating flows. We can have Katherine refactor after the bug is fixed, I'd just rather not touch the sp bounce code since that's not something I'm familiar with. Does that make sense?

@orenyk orenyk merged commit 30a6fc8 into main Oct 28, 2022
@orenyk orenyk deleted the oyk-fix-oidc-logout-bug branch October 28, 2022 13:50
@aduth aduth mentioned this pull request Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants