Skip to content
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

Bug: logging in after clearing session redirects to clearing session #112

Conversation

create-issue-branch[bot]
Copy link
Contributor

@create-issue-branch create-issue-branch bot commented May 1, 2021

closes #28

@Flix6x Flix6x self-assigned this May 1, 2021
@Flix6x Flix6x added bug UI labels May 1, 2021
@Flix6x Flix6x added this to the 0.5.0 milestone May 1, 2021
@Flix6x Flix6x marked this pull request as ready for review May 1, 2021
@Flix6x
Copy link
Contributor

@Flix6x Flix6x commented May 1, 2021

In this PR I try to address the underlying problem: that clearing the session is not supposed to log out the user.

We had some logic to prevent this already, which keeps around a few session keys that relate to the user being logged in. I added three more Flask session keys that I saw were immediately in use after a user logs in. I think these new session keys may not have been in use when @nhoening first set up this logic in f559b72?

@Flix6x Flix6x requested a review from nhoening May 1, 2021
@nhoening
Copy link
Contributor

@nhoening nhoening commented May 1, 2021

The logout function in Flask_Security does invalidate the fs_paa and fs_cc fields, so seen from that side we seem to do it correctly to leave them alone if we do not want a logout. I also checked how old the lines are in that function which deal with these fields and they are younger than our last commit about this stuff.

I also learned the following:
"fs_" is a slask security namespace.
"paa" stands for "Primary authentication at" and "Cc" for "CSRF cookie".
"_fresh" is from flask_login and marks if the session is fresh. It gets invalidates in flask_login's implementation of logout_user.

So ― I'm fine with them staying if that helps.

@nhoening
Copy link
Contributor

@nhoening nhoening commented May 1, 2021

Should we add a change log entry for this?

@Flix6x Flix6x merged commit 52a8f41 into main May 1, 2021
2 checks passed
@Flix6x Flix6x deleted the issue-28-Bug_logging_in_after_clearing_session_redirects_to_clearing_session branch May 1, 2021
Flix6x added a commit that referenced this issue May 6, 2021
… clearing session (#112)

Prevent user from being logged out when clearing the session.

* Create draft PR for #28

* More session keys to avoid clearing

* Changelog entry

Co-authored-by: Flix6x <Flix6x@users.noreply.github.com>
Co-authored-by: F.N. Claessen <felix@seita.nl>
Co-authored-by: Felix Claessen <30658763+Flix6x@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants