Skip to content

Conversation

@labkey-adam
Copy link
Contributor

Rationale

Related Pull Requests

Changes

  • Fix session tracking to respect impersonation. Sessions are always tracked to the original user, not an impersonated user.
  • On API key authentication, stash the API key's RowId in session. Use that to skip invalidation of API key sessions on password change and invalidate associated API key sessions on API key delete.
  • Simplify creation of AuthenticationResponse

@labkey-jeckels
Copy link
Contributor

@labkey-adam there are test failures related to invalid sessions. Should I review now or after they're passing?

@labkey-adam
Copy link
Contributor Author

@labkey-adam there are test failures related to invalid sessions. Should I review now or after they're passing?

Short answer: feel free to wait. I haven't been able to reproduce these failures locally and plan to investigate (e.g., bumping up the logging temporarily). Fairly sure these are race conditions where the tests rapidly delete multiple api keys associated with the same user and/or log out / invalidate a session in parallel with deletes, but I want to verify this because previous failures like this pointed out issues with the fundamental bookkeeping. The answer is likely tolerating an invalidate() that throws, checking isValid() (which is not a public method, but seems to be on the Tomcat impl), or adding synchronization.

return sessionUser;
}

// If currently impersonating, returns the impersonated user with impersonation context set
Copy link
Contributor

Choose a reason for hiding this comment

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

If you make these JavaDoc style comments they'll show up in more places in the IDE

@labkey-adam labkey-adam merged commit 5ab00fa into release24.11-SNAPSHOT Nov 13, 2024
1 check passed
@labkey-adam labkey-adam deleted the 24.11_fb_sessions branch November 13, 2024 01:29
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