fix remember me login #1347

ChristophWurst · 2016-09-09T14:22:11Z

I somehow refactored away the remember-me funcionality while working on the auth code 🙈

TODO:

~~update session token in the DB~~ duplicate token, otherwise the validation fails and the user is logged out
only set remember cookies when the 2FA challenge was solved successfully
only set remember cookie when the checkbox was checked on the login page
flag remember-me session tokens and don't delete them after 24h, but rather after the max remember-me lifetime elapsed, relative to the last usage time
tests

mention-bot · 2016-09-09T14:22:14Z

@ChristophWurst, thanks for your PR! By analyzing the annotation information on this pull request, we identified @LukasReschke, @DeepDiver1975 and @icewind1991 to be potential reviewers

coderkun · 2016-09-10T08:59:05Z

if (true) { ?

ChristophWurst · 2016-09-10T09:28:35Z

if (true) { ?

This is where I'll have to check whether the checkbox was checked or not 😉

MeCias · 2016-09-11T09:33:19Z

Hi,

Maybe you can consider this request as well.

Thanks

ChristophWurst · 2016-09-24T15:35:12Z

The basic remember-me-login works again, but the session token validation logic logs the user out because it cannot find a valid session token in the DB for the current session id on the next request.

Solution 1: Re-use old session token
We could update an existing session token with the new session id. However, we'd have to tweak the TTL of 24h, because currently we delete old tokens in a background job. Additionally, concurrency will be tricky, e.g. handling two or more concurrent requests that should be logged in via remember me – which one gets the new session id?

Solution 2: Create a new session token
Maybe easier to implement, but we wouldn't have a password for the new token. That means we can no longer rely on the kill existing connections by changing the password as sessions without a password obviously don't use that logic. Concurrency might also be a problem here, as concurrent requests might result into multiple new session tokens. Additionally, the list of active sessions is expanded if the session is lost within the 24h time span, so this might be confusing to see multiple dead connections in the personal settings.

I've not come to a clean, robust solution yet. @LukasReschke @MorrisJobke @rullzer @icewind1991 does anybody have an idea how we could fix that in the scope of 10.0.2?

ChristophWurst · 2016-10-11T10:42:45Z

Updated solution 1: Construct a new session token with the data from the old one.

That means we'd have to store the session token ID or similar next to the remember me cookie (either additional cookie or combine somehow). When we try to log in by cookie, we first run the old logic (lookup login token in DB). Then we load the old session token from the database, based on the value of the remember me cookie. That old session token will be cloned (create a new one with same attributes, but with the new session ID of course). Future requests should be successfully authenticated again.

We can then safely delete the old token.

Additional problem: we have to tweak the lifetime of session tokens in the DB because they are currently deleted after 24h of inactivity.

@LukasReschke @rullzer @nickvergessen @MorrisJobke objections? I'd try to implement this if you agree that this is the way to go.

codecov-io · 2016-10-11T14:04:57Z

Current coverage is 57.53% (diff: 59.03%)

Merging #1347 into master will increase coverage by 0.19%

@@             master      #1347   diff @@
==========================================
  Files          1078       1078          
  Lines         61498      62197   +699   
  Methods        6875       7009   +134   
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          35266      35787   +521   
- Misses        26232      26410   +178   
  Partials          0          0

Diff Coverage	File Path
0%	lib/private/legacy/user.php
0%	lib/base.php
••• 35%	...rivate/Authentication/Token/DefaultTokenProvider.php
•••••• 61%	lib/private/User/Session.php
•••••••• 87%	lib/private/Authentication/TwoFactorAuth/Manager.php
•••••••••• 100%	lib/private/Server.php
•••••••••• 100%	version.php
•••••••••• 100%	core/Controller/TwoFactorChallengeController.php
•••••••••• 100%	...ate/AppFramework/DependencyInjection/DIContainer.php
•••••••••• 100%	core/Controller/LoginController.php

Review all 12 files changed

Powered by Codecov. Last update a973c1b...ac9f2a8

ChristophWurst · 2016-10-25T12:13:54Z

Fixed and added some tests, squashed my commits and rebased to resolve some conflicts.

@LukasReschke @MorrisJobke @rullzer this is ready for testing.

I faked a lost session by deleting the session cookie and php's session files.

LukasReschke · 2016-11-02T12:06:17Z

lib/private/Authentication/TwoFactorAuth/Manager.php

@@ -51,6 +52,7 @@ class Manager {
 	 * @param AppManager $appManager
 	 * @param ISession $session
 	 * @param IConfig $config
+	 * @param Session $userSession


We're passing a ISession and not a Session

Actually it seems like that is even not ever used :)

* try to reuse the old session token for remember me login * decrypt/encrypt token password and set the session id accordingly * create remember-me cookies only if checkbox is checked and 2fa solved * adjust db token cleanup to store remembered tokens longer * adjust unit tests Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

ChristophWurst · 2016-11-02T16:33:44Z

LukasReschke requested changes

@LukasReschke anything left?

LukasReschke · 2016-11-02T16:34:09Z

.htaccess

+#### DO NOT CHANGE ANYTHING ABOVE THIS LINE ####
+
+ErrorDocument 403 /core/templates/403.php
+ErrorDocument 404 /core/templates/404.php


@LukasReschke anything left?

Yes.

LukasReschke · 2016-11-02T16:34:31Z

LGTM once the last issue is fixed :)

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>

chaos-prevails · 2017-01-11T11:30:14Z

Hello,

I'm using NC11 just downloaded yesterday and I want to disable saving the login session cookie. In fact, I want that users get automatically logged out after 10 minutes of inactivity.

This works fine:
'remember_login_cookie_lifetime' => 0,
'session_lifetime' => 600,
'session_keepalive' => false,

But the "stay logged in" checkbox on the login screen is still visible (although disabled by 'remember_login_cookie_lifetime' => 0 AFAIK).

The addon "disable remember login" (from OC) is not available any more in the app store of NC11, so I assume (wrongly?) I shouldn't use it any more with NC11 (was available with NC10).

Is there a way to hide the checkbox on the login screen? Following this thread I assumed I have to set a config.php parameter but I don't see which one.

thanks alot,
Tim

My1 · 2017-01-11T11:41:10Z

@chaos-prevails just enable the external storage app but in the admin interface do not give the users the permission to add external storages, this should take care of the remember login checkbox.

chaos-prevails · 2017-01-11T11:46:24Z

Thanks, that did the trick. However, even when disabling the ability to add external storage, the "external storage" link at the left side menu shows up for all users.

Probably there is a more direct way to hide the checkbox? I might rather try the old app on NC11.

My1 · 2017-01-11T11:50:58Z

well, if I would know how the external storage kicks out the checkbox we could solve both our problems.
I could get external storage and remember me, and you could for example create a dummy plugin which does nothing except for killing off the checkbox.

otherwise you may play with the login page files and remove anything regarding the checkbox.

chaos-prevails · 2017-01-11T12:03:23Z

I'm not sure whether the checkbox is disappearing without reason when enabling the external storage app. probably it's not possible any more due to the login necessities to the external storage sites? (I have no idea, sorry).

I will try out the old app to check whether it works on NC11

MorrisJobke · 2017-01-11T12:54:17Z

well, if I would know how the external storage kicks out the checkbox we could solve both our problems.

This is how it kicks out the checkbox:

server/apps/files_external/appinfo/info.xml

Line 15 in bc26f78

<rememberlogin>false</rememberlogin>

😉

My1 · 2017-01-11T13:03:58Z

@MorrisJobke oh my god, why did I overlook that? I did check the metafiles iirc. thanks, seriously.

chaos-prevails · 2017-01-12T12:09:27Z

I have to revert my answer from yesterday, actually none of the two things I try to achieve work out:

expire NC session when closing browser (independent from session-timeout)
does work in Chromium, does NOT work in Firefox
expire NC session after session timeout
does work for 10 seconds (testing), but doesn't work any more for e.g. 1 minute. I have switched to another tab to make sure I don't trigger any ajax calls, whatever. Still, I'm not disconnected after more than a 1 minute.

my config settings:
'remember_login_cookie_lifetime' => 0, (I also tried with 1 or 60, as probably 0 could mean "indefinitely")
'session_lifetime' => 60,
'session_keepalive' => false,

beside client/browser specific issues (private browsing, etc), is there something else I have to consider so that it works out of the box browser-independently?

thanks
Tim

btw: I deleted all old cookies before testing and hide the remember login checkbox with the help of @MorrisJobke yesterday.

ChristophWurst added bug 2. developing Work in progress labels Sep 9, 2016

ChristophWurst added this to the Nextcloud 11.0 milestone Sep 9, 2016

ChristophWurst added the backport-request label Sep 9, 2016

MorrisJobke mentioned this pull request Sep 14, 2016

Fix remember me behaviour #1067

Closed

ChristophWurst added the help wanted label Sep 24, 2016

ChristophWurst force-pushed the bring-back-remember-me branch from 8fda5ce to cfca4c8 Compare September 24, 2016 15:35

ghost mentioned this pull request Sep 26, 2016

"Stay logged in" checkbox doesn't work owncloud/core#26027

Closed

ChristophWurst force-pushed the bring-back-remember-me branch 2 times, most recently from 3006121 to 67dfc62 Compare October 11, 2016 12:26

jancborchardt added the high label Oct 18, 2016

ChristophWurst force-pushed the bring-back-remember-me branch 3 times, most recently from d382a67 to 5cf1039 Compare October 24, 2016 15:34

ChristophWurst mentioned this pull request Oct 25, 2016

Add Option "Remember on this computer" nextcloud/twofactor_totp#68

Closed

ChristophWurst removed backport-request help wanted labels Oct 25, 2016

ChristophWurst force-pushed the bring-back-remember-me branch from 9ef22d3 to 21a1aa1 Compare October 25, 2016 12:11

ChristophWurst added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 25, 2016

ChristophWurst added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Oct 26, 2016

LukasReschke reviewed Nov 2, 2016

View reviewed changes

ChristophWurst and others added 5 commits November 2, 2016 13:39

inject ISecureRandom into user session and use injected config too

6f86e46

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>

Fix invalid PHPDocs

b269ed5

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

Fix typ in constant name

271f2a4

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

Add missing tests and fix PHPDoc

9d6e01e

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

LukasReschke force-pushed the bring-back-remember-me branch from cb5db29 to 9d6e01e Compare November 2, 2016 12:39

LukasReschke mentioned this pull request Nov 2, 2016

Disable codecov comments #1949

Merged

LukasReschke requested changes Nov 2, 2016

View reviewed changes

document what the method does

4da6b20

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>

ChristophWurst force-pushed the bring-back-remember-me branch from 8b4e54d to 4da6b20 Compare November 2, 2016 16:43

LukasReschke approved these changes Nov 2, 2016

View reviewed changes

LukasReschke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Nov 2, 2016

MorrisJobke merged commit e6b52ef into master Nov 2, 2016

MorrisJobke deleted the bring-back-remember-me branch November 2, 2016 17:32

ChristophWurst mentioned this pull request Nov 27, 2016

copy remember-me value when renewing a session token #2353

Merged

chaos-prevails mentioned this pull request Jan 16, 2017

config.php - Disable Remember Login #1358

Closed

ChristophWurst mentioned this pull request Feb 16, 2017

oc_token should be nc_token #3362

Merged

ChristophWurst mentioned this pull request Sep 18, 2017

Fix duplicate session token after remembered login #6544

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix remember me login #1347

fix remember me login #1347

ChristophWurst commented Sep 9, 2016 •

edited

Loading

mention-bot commented Sep 9, 2016

coderkun commented Sep 10, 2016

ChristophWurst commented Sep 10, 2016

MeCias commented Sep 11, 2016

ChristophWurst commented Sep 24, 2016 •

edited

Loading

ChristophWurst commented Oct 11, 2016

codecov-io commented Oct 11, 2016 •

edited

Loading

ChristophWurst commented Oct 25, 2016

LukasReschke Nov 2, 2016

LukasReschke Nov 2, 2016

ChristophWurst commented Nov 2, 2016

LukasReschke Nov 2, 2016

ChristophWurst Nov 2, 2016

LukasReschke commented Nov 2, 2016 •

edited by MorrisJobke

Loading

chaos-prevails commented Jan 11, 2017

My1 commented Jan 11, 2017

chaos-prevails commented Jan 11, 2017

My1 commented Jan 11, 2017

chaos-prevails commented Jan 11, 2017

MorrisJobke commented Jan 11, 2017

My1 commented Jan 11, 2017 •

edited

Loading

chaos-prevails commented Jan 12, 2017 •

edited

Loading

fix remember me login #1347

fix remember me login #1347

Conversation

ChristophWurst commented Sep 9, 2016 • edited Loading

mention-bot commented Sep 9, 2016

coderkun commented Sep 10, 2016

ChristophWurst commented Sep 10, 2016

MeCias commented Sep 11, 2016

ChristophWurst commented Sep 24, 2016 • edited Loading

ChristophWurst commented Oct 11, 2016

codecov-io commented Oct 11, 2016 • edited Loading

Current coverage is 57.53% (diff: 59.03%)

ChristophWurst commented Oct 25, 2016

LukasReschke Nov 2, 2016

Choose a reason for hiding this comment

LukasReschke Nov 2, 2016

Choose a reason for hiding this comment

ChristophWurst commented Nov 2, 2016

LukasReschke Nov 2, 2016

Choose a reason for hiding this comment

ChristophWurst Nov 2, 2016

Choose a reason for hiding this comment

LukasReschke commented Nov 2, 2016 • edited by MorrisJobke Loading

chaos-prevails commented Jan 11, 2017

My1 commented Jan 11, 2017

chaos-prevails commented Jan 11, 2017

My1 commented Jan 11, 2017

chaos-prevails commented Jan 11, 2017

MorrisJobke commented Jan 11, 2017

My1 commented Jan 11, 2017 • edited Loading

chaos-prevails commented Jan 12, 2017 • edited Loading

ChristophWurst commented Sep 9, 2016 •

edited

Loading

ChristophWurst commented Sep 24, 2016 •

edited

Loading

codecov-io commented Oct 11, 2016 •

edited

Loading

LukasReschke commented Nov 2, 2016 •

edited by MorrisJobke

Loading

My1 commented Jan 11, 2017 •

edited

Loading

chaos-prevails commented Jan 12, 2017 •

edited

Loading