Fix duplicate session token after remembered login #6544
Conversation
|
@ChristophWurst you mention my previous posts but as far as I know I never used "remember login session". The thing that solved #6203 #5083 and other issues regarding app passwords being revoked was merely an Admin setting that I changed from AJAX to Cron. Just to let you know... Cheers, and thanx for keeping up the good work! |
ChristophWurst
added
3. to review
|
Applied this patch on my production instance and it seems to work nicely - no more endless list of sessions though I'm using remembered login 🚀 |
|
Those CI failures look strange. @MorrisJobke could you please re-trigger builds? |
rullzer
left a comment
rullzer
left a comment
There was a problem hiding this comment.
Makes a lot of sense to me.
Lets merge it now so we can properly test it in production as well.
On a remembered login session, we create a new session token in the database with the values of the old one. As we actually don't need the old session token anymore, we can delete it right away. Signed-off-by: Christoph Wurst <[email protected]>
rullzer
force-pushed
the
fix/duplicate-session-token
branch
from
b40a78e to
38bb6e1
Compare
Codecov Report
@@ Coverage Diff @@
## master #6544 +/- ##
============================================
+ Coverage 53.06% 53.06% +<.01%
Complexity 22552 22552
============================================
Files 1414 1414
Lines 87741 87742 +1
Branches 1340 1340
============================================
+ Hits 46561 46562 +1
Misses 41180 41180
|
MorrisJobke
left a comment
MorrisJobke
left a comment
There was a problem hiding this comment.
Code makes sense and works 👍
|
@oparoz just adding the label here doesn't make anybody notice it on time. @ChristophWurst how much work would it be to backport this? |
Code-wise it's not much. Should be enough to backport the commits from here plus the ones from #6360. We'd however have to thoroughly test this as it could also potentially break. As mentioned in the description I tried to apply this fix earlier already but then it broke some functionality. Thus, I'm a bit hesitant. What backport are we talking about? nc12? |
|
I would say nc12 only yes |
|
I've put it onto my (already very full) list of tasks: https://github.com/orgs/nextcloud/projects/4#card-6110783. I'll try to find some time to look into. |
|
Backport in #7568 |
On a remembered login session, we create a new session token
in the database with the values of the old one. As we actually
don't need the old session token anymore, we can delete it right
away.
IIRC this caused problems back when I fixed the remember me login last year. There, deleting the old token always logged the user out, for some unknown reason. Now I'm thinking that the issue fixed by #6360 could also have fixed this problem. Thus I've added the token deletion and it actually seems to work, as far as I could test this.
Steps to test this
On master, you'll see one additional session, which is the new one. On this PR's branch, however, you'll only see your new session.
This might look like a tiny difference, but if you use your Nextcloud many times a day and remember-me login is used all the time (because you close your browser in between) you'll see a lot of old sessions. This is confusing and bad for UX. Kind of related issues reported by @eggithub #6203 #5083
FYI this is to review, but I'm a bit hesitant to label it like that as the session handling code has shown to be fragile in some situations and I'd like to extensively test this before we merge it to be super sure it doesn't break remember-me login again 😀
cc @MorrisJobke this is more or less what we've been discussing recently. However, I'm afraid this does not yet solve #1075.