Skip to content

Log completed SAML Remote Logout requests#8261

Merged
mitchellhenke merged 3 commits intomainfrom
mitchellhenke/remote-logout-logging
Apr 24, 2023
Merged

Log completed SAML Remote Logout requests#8261
mitchellhenke merged 3 commits intomainfrom
mitchellhenke/remote-logout-logging

Conversation

@mitchellhenke
Copy link
Contributor

🛠 Summary of changes

To better understand the impact of remote logout on the user experience, this PR adds a log event for when a user's session is deleted from Redis (though it may already be expired).

changelog: Internal, Logging, Log completed SAML Remote Logout requests
@mitchellhenke mitchellhenke requested a review from a team April 21, 2023 22:11
# Service provider completed remote logout
# @param [String] service_provider
# @param [String] user_id
def remote_logout_completed(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this worth adding a test for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thank you! Added in 8258d62

session_id = ServiceProviderIdentity.
find_by(user_id: user_id, service_provider: saml_request.issuer).
rails_session_id
where(user_id: user_id, service_provider: saml_request.issuer).pick(:rails_session_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

[question] is saml_request.issuer different from the issuer passed in?

Copy link
Contributor Author

@mitchellhenke mitchellhenke Apr 24, 2023

Choose a reason for hiding this comment

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

whoops, it should be, but I intended to replace saml_request.issuer with the argument passed to the function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 5f24ce6

@mitchellhenke mitchellhenke merged commit 112ba52 into main Apr 24, 2023
@mitchellhenke mitchellhenke deleted the mitchellhenke/remote-logout-logging branch April 24, 2023 15:00
@jmhooper jmhooper mentioned this pull request Apr 26, 2023
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