Avoid database query associated with active session polling by aduth · Pull Request #7966 · 18F/identity-idp

aduth · 2023-03-10T15:32:11Z

🛠 Summary of changes

This ~~introduces a~~ uses the new Api::SessionsController to manage active session polling behavior.

Proposed benefits:

Avoids database query resulting from testing against current_user, instead using internal Warden session values
Move JSON API behaviors out of existing SessionsController to own dedicated controller with conventionally named methods
Reduce number of redundant values tracked in session (remove session_expires_at, pinged_at)
Move flash behavior for "Sign In" form timeout out of ApplicationController and consolidate with session timeout behavior
Simplify JavaScript for session ping behavior
Eliminate redundancy in ping response structure (return single expiration time rather than both expiration time as well as remaining seconds)
Avoid logging "Session Ping Error" NewRelic page actions
- It's expected that those page actions are a result of ping responses being returned as HTML, expected to be resolved as part of these changes

📜 Testing Plan

Check to ensure there are no regressions in the behavior of session timeout, both when signed in and when "partially" signed in (after entering email and password but before MFA).
- "Keep me signed in" dismisses modal and extends session
- "Sign me out" logs out
- Timer counts down to 0 and then redirects

It's easier to test by setting a very low session timeout in local config/application.yml:

session_timeout_in_minutes: 1

mitchellhenke

One question about backwards compatibility, but this is great!

config/routes.rb

app/controllers/api/sessions_controller.rb

app/controllers/users/sessions_controller.rb

aduth · 2023-03-21T19:21:16Z

Maybe for a separate pull request, but: I'm wondering if we're polling more aggressively than we should need to. At most, it seems like could ping only just prior to showing the modal (to confirm that the session is in-fact soon to expire) and just as the session is expiring† (to avoid signing out in case the session was extended in another tab). Polling while the modal is active could be nice to automatically dismiss the modal if the session was extended in another tab, but that seems like a nice-to-have and an edge-case, at the cost of at least 3-4 extra network requests per session timing out.

† In the case of expiring, we also send two separate requests in quick succession, one to check if the session is active, and then another to trigger the sign-out. Maybe we could do this at the same time? On the other hand, I'm not sure I like the idea of destroying the session in a GET request (i.e. part of the active check), but that is what we do today 😅 🙈

app/controllers/api/sessions_controller.rb

The CSRF won't be updated until after the request finishes, even if the modal will be disappeared immediately

…8084) * Remove redundant "remaining" value from timeout API changelog: Internal, Session Timeout, Remove redundant timeout value from session timeout API * Revert session API remaining removal Temporary for first deploy, see https://github.com/18F/identity-idp/pull/8084/files#r1150918192 * Optimistically hide modal on session continue 1. Fixes specs 2. Better user experience, don't have to wait for request to finish before modal is closed, thus making app feel more responsive 3. The previous result handling with `success` would cause a second polling interval to begin * Bring over rest of spec changes from #7966 The CSRF won't be updated until after the request finishes, even if the modal will be disappeared immediately

aduth · 2023-04-25T16:23:18Z

I rebased to resolve conflicts and was working to try to get this over the line. I encountered an issue that will need to be investigated further. There seems to be some base validation that's redirecting requests to DELETE /api/internal/sessions when the session is no longer live, and I was surprised to find that the user's Warden session is reset if they visit the homepage before finishing sign-in.

Steps to reproduce:

Set config values very low like session_timeout_in_minutes: 2, session_check_delay: 5 session_check_frequency: 30
Go to http://localhost:3000
Sign in
When prompted for MFA, go to http://localhost:3000 in a separate tab
Wait for next polling request

Expected: Either (a) the initial session is still live, or (b) if it's not live, the user is redirected
Actual: The session is not live, but the user is not redirected as expected. Instead, the DELETE /api/internal/sessions is being internally redirected to the MFA page

aduth · 2023-05-26T18:07:35Z

I was surprised to find that the user's Warden session is reset if they visit the homepage before finishing sign-in.

This does seem to happen on main as well, for what it's worth. Still seems excessive to terminate the pending sign-in if they visit the homepage in a separate tab 🤷

aduth · 2023-05-26T18:28:57Z

I encountered an issue that will need to be investigated further. There seems to be some base validation that's redirecting requests to DELETE /api/internal/sessions when the session is no longer live

What's happening here is we're trying to destroy the session from the JavaScript as a DELETE request after the session is already ended, or at least after the CSRF token we include in the request is no longer valid, so it's the ApplicationController#invalid_auth_token kicking in to cause the redirect.

Initially I considered overriding the CSRF exception handling in the controller, but there's probably no way to retain the request_id in doing that. So now I'm thinking maybe it's best to move away from having a destroy action at all in this API, and instead have the timed-out redirect URL known up-front, like what we had previously.

The only challenge with this would be to find an equivalent behavior to log analytics.session_timed_out. Maybe SessionsController could handle that on the receiving end? i.e. GET /?timeout=session

aduth · 2023-05-26T18:35:22Z

but there's probably no way to retain the request_id in doing that

~~I guess the request_id is lost at this point anyways if the session has expired, so not really a big deal to lose this.~~

Edit: It may be lost as far as not being available in the session, but it should still be valid, right?

app/javascript/packs/session-timeout-ping.ts

aduth · 2023-05-26T19:51:10Z

Marking this ready for review again. I tried to simplify the diff to limit the impact. Separately, I'd like to iteratively work to slice up session-timeout-ping.ts toward some of our newer code conventions that'll always hopefully make it more testable.

zachmargolis

LGTM

app/controllers/api/internal/sessions_controller.rb

zachmargolis · 2023-05-26T20:01:30Z

app/helpers/session_timeout_warning_helper.rb

All the specs call this with string 'form' instead, should we do that for consistency? (I know it gets converted to a string when we parse anyways)

Suggested change

timeout: :form,

timeout: 'form',

zachmargolis · 2023-05-26T20:02:31Z

app/helpers/session_timeout_warning_helper.rb

Do we still need the html_safe on there? It looks like the only use of this method is for a data- attribute which I expect would be "smarter" about HTML escaping already?

I'll check this on Monday, this might be one of the things we could remove altogether as a post-deploy cleanup, once we move everything off the current session API endpoints.

I should probably get that follow-on pull request prepped anyways.

To this and the prior comment, I think I might leave this alone for now. It's still needed, but ties into some bigger problems with the auto-refresh that I'd like to sort out (related Slack discussion). At the very least, it might make sense to just have the JavaScript be responsible for navigating to ?timeout=form, rather than passing a full URL path from the server.

aduth · 2023-05-30T15:58:01Z

Post-deploy clean-up pull request at #8507

changelog: Internal, Performance, Avoid database query for session heartbeat polling

session_expired behavior applies to all logged-out screens, so this should continue to exist (for now) in ApplicationController. it also simplifies the diff

To avoid issues for 50/50 deploy and for clients running stale version of JavaScript, to be reverted in follow-on pull request

See: #7966 (comment)

See: - https://github.com/18F/identity-idp/pull/7966/files#r1207202821 - #8117

aduth · 2023-06-14T13:01:39Z

I've been monitoring this in production, and it seems like it has a small improvement over the legacy sessions endpoint in response time (-6% average, -15% median), but notably I observed in the breakdown that a database load is still happening for the user, which... was one of the main motivators for this work in the first place. I'll plan to find some time to troubleshoot why that's happening.

It's getting into diminishing returns of value, since the endpoint is already quite fast, and the database load doesn't contribute greatly to the overall response time. It used to be a very highly trafficked endpoint (and still is overall), but we might find more value in other optimizations like preventing some of the intermediate polling in the first place if it's not necessary.

aduth · 2023-06-30T15:39:19Z

I observed in the breakdown that a database load is still happening for the user, which... was one of the main motivators for this work in the first place. I'll plan to find some time to troubleshoot why that's happening.

Follow-up: I traced this back to ApplicationController#append_info_to_payload, where we include the user's UUID in request logging. I don't know that there'll be any way around this assuming we want to have that detail.

Curious if we could skip a full user load, I dug into what Devise/Warden store in the session, and it looks like it's the user ID (session['warden.user.user.key'].first.first). If we really wanted to avoid a user load, we could consider switching from UUID to ID in the logs (or maybe vice-versa, try to find a way to force the Warden session to track users by UUID instead of ID), but I dunno that we'd have the appetite for that.

aduth · 2023-07-03T13:28:55Z

If we really wanted to avoid a user load, we could consider switching from UUID to ID in the logs (or maybe vice-versa, try to find a way to force the Warden session to track users by UUID instead of ID)

For posterity: A bit of extra discussion on these points in Slack at https://gsa-tts.slack.com/archives/C0NGESUN5/p1688146380110749

aduth changed the title ~~Avoid database query associated with heartbeat poll~~ Avoid database query associated with active session polling Mar 10, 2023

mitchellhenke approved these changes Mar 10, 2023

View reviewed changes

config/routes.rb Outdated Show resolved Hide resolved

zachmargolis reviewed Mar 10, 2023

View reviewed changes

app/controllers/api/sessions_controller.rb Outdated Show resolved Hide resolved

zachmargolis reviewed Mar 10, 2023

View reviewed changes

app/controllers/users/sessions_controller.rb Outdated Show resolved Hide resolved

aduth mentioned this pull request Mar 14, 2023

Simplify session timeout modal presenter #7985

Merged

aduth force-pushed the aduth-try-avoid-poll-db-query branch 2 times, most recently from 76677e8 to 5e529a7 Compare March 20, 2023 14:44

aduth marked this pull request as ready for review March 20, 2023 18:34

aduth commented Mar 21, 2023

View reviewed changes

app/controllers/api/sessions_controller.rb Outdated Show resolved Hide resolved

aduth marked this pull request as draft March 22, 2023 14:39

aduth mentioned this pull request Mar 23, 2023

Add automatic CSRF refresh on session keepalive #8067

Merged

aduth force-pushed the aduth-try-avoid-poll-db-query branch 3 times, most recently from b590fe3 to f051b01 Compare March 28, 2023 13:21

aduth mentioned this pull request Mar 28, 2023

Remove redundant "remaining" value from session timeout API (Part 1) #8084

Merged

aduth added a commit that referenced this pull request Mar 28, 2023

Bring over rest of spec changes from #7966

5114192

The CSRF won't be updated until after the request finishes, even if the modal will be disappeared immediately

aduth mentioned this pull request Mar 28, 2023

Add Sessions API controller implementation #8089

Merged

aduth mentioned this pull request Mar 29, 2023

Extract session JavaScript package #8092

Merged

aduth force-pushed the aduth-try-avoid-poll-db-query branch from f051b01 to 567001a Compare March 30, 2023 14:10

aduth force-pushed the aduth-try-avoid-poll-db-query branch from 567001a to 13e95da Compare April 25, 2023 16:19

aduth force-pushed the aduth-try-avoid-poll-db-query branch from 13e95da to 776f1cf Compare May 26, 2023 18:49

aduth commented May 26, 2023

View reviewed changes

app/javascript/packs/session-timeout-ping.ts Outdated Show resolved Hide resolved

aduth marked this pull request as ready for review May 26, 2023 19:49

zachmargolis approved these changes May 26, 2023

View reviewed changes

aduth mentioned this pull request May 30, 2023

Remove legacy session timeout polling behavior #8507

Merged

aduth mentioned this pull request Jun 1, 2023

Update session timeout poll response handling to omit timeout on inactive session #8521

Merged

aduth added 11 commits June 9, 2023 09:34

Avoid database query associated with heartbeat poll

d7d21f0

changelog: Internal, Performance, Avoid database query for session heartbeat polling

Restore flash redirect behavior to ApplicationController

8cc25a3

session_expired behavior applies to all logged-out screens, so this should continue to exist (for now) in ApplicationController. it also simplifies the diff

Fix handling for legacy timeout parameter

d366330

Restore legacy endpoints for initial deploy

fee65ac

To avoid issues for 50/50 deploy and for clients running stale version of JavaScript, to be reverted in follow-on pull request

Initialize session timeout JavaScript with timeout URL

5699ee9

See: #7966 (comment)

Reincorporate timeRemaining safeguards

842bbc6

See: - https://github.com/18F/identity-idp/pull/7966/files#r1207202821 - #8117

Remove route for deleted SessionsController#destroy

060ffd4

Only update last_request_at if still live

f4abd1e

Add specs for SessionsController#update

53beced

Add specs for updated redirect_with_flash_if_timeout

1a4643d

Add specs for SessionsController#update analytics

9b3f3d3

aduth force-pushed the aduth-try-avoid-poll-db-query branch from 6369488 to 9b3f3d3 Compare June 9, 2023 13:42

aduth added 2 commits June 9, 2023 11:25

Update spec session value

6b7b7cc

Simplify diff

508215f

aduth merged commit c1d9a4c into main Jun 9, 2023

aduth deleted the aduth-try-avoid-poll-db-query branch June 9, 2023 16:05

jmhooper mentioned this pull request Jun 13, 2023

Deploy RC 288 to prod #8587

Merged

This was referenced Nov 3, 2023

Update Lograge configuration to ignore active ping controller #9541

Merged

Avoid appending info for ignored Lograge actions #9544

Merged

Conversation

aduth commented Mar 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🛠 Summary of changes

📜 Testing Plan

Uh oh!

mitchellhenke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aduth commented Mar 21, 2023

Uh oh!

Uh oh!

aduth commented Apr 25, 2023

Uh oh!

aduth commented May 26, 2023

Uh oh!

aduth commented May 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduth commented May 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

aduth commented May 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zachmargolis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zachmargolis May 26, 2023

Choose a reason for hiding this comment

Uh oh!

zachmargolis May 26, 2023

Choose a reason for hiding this comment

Uh oh!

aduth May 26, 2023

Choose a reason for hiding this comment

Uh oh!

aduth May 30, 2023

Choose a reason for hiding this comment

Uh oh!

aduth commented May 30, 2023

Uh oh!

aduth commented Jun 14, 2023

Uh oh!

aduth commented Jun 30, 2023

Uh oh!

aduth commented Jul 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aduth commented Mar 10, 2023 •

edited

Loading

aduth commented May 26, 2023 •

edited

Loading

aduth commented May 26, 2023 •

edited

Loading

aduth commented May 26, 2023 •

edited

Loading