Login credential store #2044

ChristophWurst · 2016-11-08T08:56:42Z

Since NC 10 we use session tokens for every connected browser and allow devices to use an app-specific password. Those tokens store the encrypted login password, which we can now use to authenticate external storages. In addition, this functionality is useful for other apps like Mail, where we want to create an account automatically, see nextcloud/mail#28.
This way we can allow remember-me logins for all users regardless of enabled apps. Until now, remember-me was disabled as soon as external storage support was enabled because the login password was stored in the session.

TODO:

~~Provide credentials for legacy clients too (those that log in with username:password)~~ remember-me is not used there
Tests
Use login hook password as fallback if there's neither a device password nor a session token.
Manual testing

Next steps/PRs:

Migrate external storages to use this store too

Tested:

External storage (webdav) using login credentials, stored in session setting and the web ui ☑️
External storage (webdav) using login credentials, stored in session setting via command line using username:password ☑️
External storage (webdav) using login credentials, stored in session setting via command line using username:app-password ☑️

There is a working POC consumer in nextcloud/mail#28.

@LukasReschke as discussed

Fix #2732

mention-bot · 2016-11-08T08:56:43Z

@ChristophWurst, thanks for your PR! By analyzing the history of the files in this pull request, we identified @rullzer, @BernhardPosselt and @DeepDiver1975 to be potential reviewers.

ChristophWurst · 2016-11-08T09:29:55Z

Provide credentials for legacy clients too (those that log in with username:password)

@LukasReschke can we expect them to send the credentials on every request? There is nothing like remember-me for clients anyway …

nickvergessen · 2016-11-08T10:07:12Z

lib/private/Authentication/LoginCredentials/Credentials.php

+	 * @param string $user
+	 * @param string $password
+	 */
+	public function __construct($uid, $user, $password) {


uid + user is confusing, isuser displayname? then call it so.

it's whatever users use to log in. depends on the setup. maybe loginName fits better

nickvergessen · 2016-11-08T11:10:43Z

lib/private/Server.php

@@ -241,6 +243,13 @@ public function __construct($webRoot, \OC\Config $config) {
 			});
 			return $groupManager;
 		});
+		$this->registerService(Store::class, function(Server $c) {
+			$session = $c->getSession();


this needs to be added to "setSession" then:
https://github.com/nextcloud/server/pull/1943/files#diff-0a830b0354b26e84f08f705b5356a05bR949

Otherwise when it was called too early once, it always has the already closed session....

ChristophWurst · 2017-01-09T13:09:10Z

apps/encryption/appinfo/info.xml

@@ -18,7 +18,6 @@
 		<user>user-encryption</user>
 		<admin>admin-encryption</admin>
 	</documentation>
-	<rememberlogin>false</rememberlogin>


cc @nextcloud/encryption

@schiessle ^^

rullzer

Awesome stuff. I did not yet manage to break it but then again my external/encryption setups are not that exciting.

Lets get this in!

@icewind1991 @schiessle have a look please!

ChristophWurst · 2017-01-11T14:10:39Z

@rullzer did you somehow remove some of my commits? E.g. I don't see the changes for external storages any more? 😕

rullzer · 2017-01-11T16:37:31Z

I hope not. Let me try again later...

rullzer · 2017-01-11T17:53:58Z

Shit seems I messed up. @ChristophWurst talk to me on IRC see if we can recover it on your git.

Signed-off-by: Christoph Wurst <[email protected]>

…ial store Signed-off-by: Christoph Wurst <[email protected]>

If no session token is available, we can use the credentials provided by the login hook. Signed-off-by: Christoph Wurst <[email protected]>

The provider might need DB access and therefore depenedency resolution fails on the setup page where we cannot inject the db implementation. Signed-off-by: Christoph Wurst <[email protected]>

Signed-off-by: Christoph Wurst <[email protected]>

codecov-io · 2017-01-17T09:34:58Z

Current coverage is 54.05% (diff: 87.27%)

Merging #2044 into master will increase coverage by 0.13%

@@             master      #2044   diff @@
==========================================
  Files          1302       1304     +2   
  Lines         80061      80448   +387   
  Methods        7902       7983    +81   
  Messages          0          0          
  Branches       1245       1245          
==========================================
+ Hits          43171      43487   +316   
- Misses        36890      36961    +71   
  Partials          0          0

Diff Coverage	File Path
••••• 50%	...xternal/lib/Lib/Auth/Password/SessionCredentials.php
••••• 50%	lib/private/Server.php
••••••••• 96%	new ...ib/private/Authentication/LoginCredentials/Store.php
•••••••••• 100%	new ...vate/Authentication/LoginCredentials/Credentials.php
•••••••••• 100%	lib/private/legacy/util.php
•••••••••• 100%	core/templates/login.php
•••••••••• 100%	core/Controller/LoginController.php
•••••••••• 100%	...ate/AppFramework/DependencyInjection/DIContainer.php

Powered by Codecov. Last update fab82e9...3ee5c7e

all fixed

ChristophWurst · 2017-01-25T11:06:30Z

@icewind1991 @LukasReschke @nickvergessen @rullzer please review :-)

MorrisJobke

Tested and works 👍

ChristophWurst added enhancement 2. developing Work in progress labels Nov 8, 2016

ChristophWurst added this to the Nextcloud 11.0 milestone Nov 8, 2016

nickvergessen previously requested changes Nov 8, 2016

View reviewed changes

ChristophWurst force-pushed the login-credential-store branch from dd5dceb to b109404 Compare November 8, 2016 10:48

nickvergessen reviewed Nov 8, 2016

View reviewed changes

ChristophWurst force-pushed the login-credential-store branch from ec13862 to 2efbb0a Compare November 19, 2016 15:33

LukasReschke assigned LukasReschke and unassigned LukasReschke Nov 21, 2016

rullzer modified the milestones: Nextcloud 12.0, Nextcloud 11.0 Nov 24, 2016

ChristophWurst mentioned this pull request Dec 8, 2016

create a dynamic default account nextcloud/mail#28

Merged

ChristophWurst force-pushed the login-credential-store branch from 2efbb0a to 88599e3 Compare December 12, 2016 08:35

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

ChristophWurst requested review from rullzer, MorrisJobke and LukasReschke December 12, 2016 08:37

ChristophWurst mentioned this pull request Dec 18, 2016

Stay logged in Feature not working in NC 11 #2732

Closed

ChristophWurst force-pushed the login-credential-store branch 4 times, most recently from cc309c9 to cca6897 Compare December 19, 2016 10:31

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

ChristophWurst force-pushed the login-credential-store branch from e62d22b to c10bfe2 Compare January 2, 2017 09:06

ChristophWurst commented Jan 9, 2017

View reviewed changes

ChristophWurst added the 3. to review Waiting for reviews label Jan 9, 2017

rullzer approved these changes Jan 11, 2017

View reviewed changes

rullzer force-pushed the login-credential-store branch from 7a1198a to 17399b3 Compare January 11, 2017 18:19

ChristophWurst added 9 commits January 11, 2017 19:20

add login credential store

a6dca9e

Signed-off-by: Christoph Wurst <[email protected]>

add @SInCE annotations and document methods

730442c

Signed-off-by: Christoph Wurst <[email protected]>

replace session implementation if it changes at runtime

46adb3e

Signed-off-by: Christoph Wurst <[email protected]>

fix @SInCE annotations (9.1->12)

3316a9d

Signed-off-by: Christoph Wurst <[email protected]>

migrate 'password::sessioncredentials' extern storage auth to credent…

e4abb9c

…ial store Signed-off-by: Christoph Wurst <[email protected]>

use login hook credentials as fallback

6f74ecd

If no session token is available, we can use the credentials provided by the login hook. Signed-off-by: Christoph Wurst <[email protected]>

do not hard-require the token provider

21d3fe5

The provider might need DB access and therefore depenedency resolution fails on the setup page where we cannot inject the db implementation. Signed-off-by: Christoph Wurst <[email protected]>

fix constructor params in failing test

6f84eb8

Signed-off-by: Christoph Wurst <[email protected]>

always allow remembered login

140555b

Signed-off-by: Christoph Wurst <[email protected]>

rullzer force-pushed the login-credential-store branch from 17399b3 to 140555b Compare January 11, 2017 18:20

minor version bump

3ee5c7e

Signed-off-by: Christoph Wurst <[email protected]>

ChristophWurst mentioned this pull request Jan 25, 2017

suggestion : a way to set default settings for the first email account [$130 awarded] nextcloud/mail#295

Closed

rullzer approved these changes Jan 26, 2017

View reviewed changes

MorrisJobke approved these changes Jan 31, 2017

View reviewed changes

MorrisJobke merged commit 5bad417 into master Jan 31, 2017

MorrisJobke deleted the login-credential-store branch January 31, 2017 01:30

nickvergessen mentioned this pull request Mar 16, 2017

Remember login option disappears with certain apps enabled #3858

Closed

ArnY mentioned this pull request Mar 16, 2017

[nextcloud] only ask password on relogin jsxc/jsxc#489

Open

ChristophWurst mentioned this pull request Apr 5, 2017

Be more specific about apps which disable the "Stay logged in" checkbox nextcloud/documentation#401

Closed

ChristophWurst mentioned this pull request May 10, 2017

'Encryption App is enabled, but your keys are not initialized. Please log-out and log-in again.' #4797

Closed

ChristophWurst mentioned this pull request Aug 7, 2017

CIFS Shares from NC12 are not working with App nextcloud/android#1267

Closed

joshtrichards mentioned this pull request Jul 9, 2024

[Bug]: Clarify how “Log-in credentials, save in session” still permits External Storage access with tokens #33943

Open

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Login credential store #2044

Login credential store #2044

ChristophWurst commented Nov 8, 2016 •

edited

Loading

mention-bot commented Nov 8, 2016

ChristophWurst commented Nov 8, 2016

nickvergessen Nov 8, 2016

ChristophWurst Nov 8, 2016

nickvergessen Nov 8, 2016

ChristophWurst Nov 19, 2016

ChristophWurst Jan 9, 2017

rullzer Jan 11, 2017

rullzer left a comment

ChristophWurst commented Jan 11, 2017

rullzer commented Jan 11, 2017

rullzer commented Jan 11, 2017

codecov-io commented Jan 17, 2017

ChristophWurst commented Jan 25, 2017

MorrisJobke left a comment

Login credential store #2044

Login credential store #2044

Conversation

ChristophWurst commented Nov 8, 2016 • edited Loading

mention-bot commented Nov 8, 2016

ChristophWurst commented Nov 8, 2016

nickvergessen Nov 8, 2016

Choose a reason for hiding this comment

ChristophWurst Nov 8, 2016

Choose a reason for hiding this comment

nickvergessen Nov 8, 2016

Choose a reason for hiding this comment

ChristophWurst Nov 19, 2016

Choose a reason for hiding this comment

ChristophWurst Jan 9, 2017

Choose a reason for hiding this comment

rullzer Jan 11, 2017

Choose a reason for hiding this comment

rullzer left a comment

Choose a reason for hiding this comment

ChristophWurst commented Jan 11, 2017

rullzer commented Jan 11, 2017

rullzer commented Jan 11, 2017

codecov-io commented Jan 17, 2017

Current coverage is 54.05% (diff: 87.27%)

ChristophWurst commented Jan 25, 2017

MorrisJobke left a comment

Choose a reason for hiding this comment

ChristophWurst commented Nov 8, 2016 •

edited

Loading