Skip to content

LG-15833: Maintain partner link when session times out at sign-in#11930

Merged
aduth merged 10 commits intomainfrom
aduth-lg-15833-partner-request-session-expire
Feb 28, 2025
Merged

LG-15833: Maintain partner link when session times out at sign-in#11930
aduth merged 10 commits intomainfrom
aduth-lg-15833-partner-request-session-expire

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Feb 27, 2025

🎫 Ticket

LG-15833

🛠 Summary of changes

Fixes an issue where the partner request details are lost if the user clicks "Continue sign in" at sign-in after the session has already expired.

The approach here is to change "Continue sign in" to be a plain link element with the request_id as an added parameter of the current page, and limit the behavior of the JavaScript to only override this behavior while the session is still assumed to be active. When the session expires, the JavaScript won't do anything, and will allow it to behave as a normal link, refreshing the page with the request_id. The sign-in page controller is already set up to interpret and save the request_id parameter.

Review is simplified by reviewing changes with whitespace hidden: https://github.com/18F/identity-idp/pull/11930/files?w=1

📜 Testing Plan

It's easiest to test by shortening your session timeout via configuration, so you don't have to wait a full 15 minutes:

session_timeout_in_minutes: 3
  1. Have the sample application running in a separate process
  2. Go to http://localhost:9292
  3. Click "Sign in"
  4. Wait 30 seconds for the modal to appear
  5. Wait until the session timeout counts all the way down to 0
  6. Click "Continue sign in"
  7. Observe that the "Example Sinatra App is using Login.gov to allow you to sign in to your account safely and securely." text is still shown

A few other scenarios to consider:

  • There should be no regressions in the behavior of the session timeout while fully signed-in (both fully expired session redirect as well as clicking "Stay signed in")
  • Partially-signed-in session expiration applies up 'til fully authenticated, so the behavior of continuing sign in should extend to other unauthenticated screens ("Create an account", or MFA entry screens). In these cases, it's expected that clicking "Continue sign in" should dismiss the modal and maintain page state if clicked prior to the session being expired. If the session is fully expired (it's counted down to 0), then clicking "Continue sign in" should return to the sign-in page with the partner link maintained.

aduth added 10 commits February 27, 2025 15:01
changelog: Bug Fixes, Sign-In, Maintain partner link when session times out at sign-in
Why:

1. To simplify stubbing tests to simulate very short session timeout (e.g. 1 second) to avoid delays in tests waiting
2. For alignment with related `session_timeout_warning_seconds` config
3. To allow more granular control over session durations
e.g. they might be partially signed-in at the MFA step, but in that case we'd still want to bring them back to the sign-in page, since the SessionsController is where the request_id is maintained
This should be assigned through session_timeout_in_seconds, but it may already be cached by the time we've assigned the stub on IdentityConfig
@aduth
Copy link
Contributor Author

aduth commented Feb 28, 2025

I'm marking this ready for review.

A few notes about the implementation:

  • The changes to switch the unit of session timeout configuration from minutes to seconds is largely to support feature specs with very short timeouts. I normally wouldn't be a big fan of changing the implementation purely in support of tests, but I do think it has a few other benefits, as described in the extended commit description of 2fbfab6 .
  1. To simplify stubbing tests to simulate very short session timeout (e.g. 1 second) to avoid delays in tests waiting
  2. For alignment with related session_timeout_warning_seconds config
  3. To allow more granular control over session durations
  • This code is shared between the "partially signed in" and "fully authenticated" session timeouts, which each has their own JavaScript. The changes to the button markup are common to both of them. I think this is an acceptable no-JavaScript fallback behavior in both scenarios, and I'd go further to say that we should probably refactor to a single JavaScript pack, likely using the session-expire-session.ts pack behavior in both cases (see related Slack thread)

@aduth aduth marked this pull request as ready for review February 28, 2025 16:01
</div>
<%= render ButtonComponent.new(
type: :button,
url: new_user_session_url(timeout: :session, request_id: sp_session[:request_id]),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is intentionally identical to the URL used for the fully-authenticated session expiration redirect URL:

timeout_url: new_user_session_url(timeout: :session, request_id: sp_session[:request_id]),

The thought being that we could get rid of session-timeout-ping.ts in favor of session-expire-session.ts, and it would "just work" the same way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only gotcha here is that including ?timeout=session means that a user will see the session timeout alert if they click "Continue sign in" after their session is fully expired. I think it's a reasonable and arguably-expected notice to be shown.

image

@aduth aduth requested a review from a team February 28, 2025 16:09
Copy link
Contributor

@kevinsmaster5 kevinsmaster5 left a comment

Choose a reason for hiding this comment

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

Looks good and local testing has no issues.
I'm curious about the switch to seconds vs. minutes for the timing.

@kevinsmaster5
Copy link
Contributor

kevinsmaster5 commented Feb 28, 2025

Also the config value changes to
session_timeout_in_seconds:

Is that a net new config key? forgot how our config file works 😮‍💨

@aduth
Copy link
Contributor Author

aduth commented Feb 28, 2025

@kevinsmaster5 There's some notes about the config change in my earlier comment #11930 (comment)

@aduth aduth merged commit 9d78a94 into main Feb 28, 2025
2 checks passed
@aduth aduth deleted the aduth-lg-15833-partner-request-session-expire branch February 28, 2025 17:50
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.

2 participants