Skip to content

bugfix: purge sessions on simplesaml upgrade #910

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 11, 2025

Conversation

matthewhilton
Copy link
Contributor

@matthewhilton matthewhilton commented Aug 8, 2025

Background

  • Further testing of simplesaml update in 401 bump simplesamlphp 2.0.15 #907 has uncovered an edge case where if a user is logged in and the plugin is upgraded, their existing session gets stuck and the user cannot log in OR out. They need to purge their browser cache entirely.
    • This seems to be because the token format/deserialisation has changed between simplesaml versions, so it cannot deserialise the existing token and gets stuck

Changes

Testing

  • This requires saml setup locally, which I have.
  • Using previous commit f7f1e99
  • Log in as a user using saml
  • Switch to f4a282b
  • Try and log out as the user, it will throw an exception.
  • Switch to this PR's branch, and run the site upgrade (with newly added upgrade step that deletes existing sessions)
  • Confirm the user can now log in/out without error

I've done the above ^^ on my local environment and it worked

@matthewhilton
Copy link
Contributor Author

^^ ci failing due to phpcs - unit tests passing

Copy link
Contributor

@brendanheywood brendanheywood left a comment

Choose a reason for hiding this comment

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

my comment is not a blocker

@matthewhilton matthewhilton merged commit cf9d974 into MOODLE_401_STABLE Aug 11, 2025
7 of 11 checks passed
@matthewhilton matthewhilton deleted the 41-upgrade-purge-sessions branch August 11, 2025 00:42
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