Add new public key token provider (tokens survive password change) by rullzer · Pull Request #9485 · nextcloud/server

rullzer · 2018-05-15T19:40:39Z

todo:

Finish unit tests

codecov · 2018-05-18T19:09:32Z

Codecov Report

Merging #9485 into master will increase coverage by 0.14%.
The diff coverage is 89.54%.

@@             Coverage Diff              @@
##             master    #9485      +/-   ##
============================================
+ Coverage     51.92%   52.06%   +0.14%     
- Complexity    25808    25901      +93     
============================================
  Files          1637     1642       +5     
  Lines         95513    95872     +359     
  Branches       1318     1318              
============================================
+ Hits          49593    49917     +324     
- Misses        45920    45955      +35

Impacted Files	Coverage Δ	Complexity Δ
core/Migrations/Version14000Date20180518120534.php	`0% <0%> (ø)`	`1 <1> (?)`
version.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...vate/Authentication/Token/DefaultTokenProvider.php	`98.05% <100%> (+0.01%)`	`32 <3> (ø)`	⬇️
lib/private/Server.php	`82.42% <100%> (-0.14%)`	`279 <0> (-1)`
settings/Controller/AuthSettingsController.php	`78.37% <100%> (+0.87%)`	`18 <0> (-2)`	⬇️
...rivate/Authentication/Token/DefaultTokenMapper.php	`100% <100%> (ø)`	`11 <2> (ø)`	⬇️
lib/private/Authentication/Token/DefaultToken.php	`91.93% <100%> (+6.68%)`	`20 <0> (ø)`	⬇️
lib/private/Authentication/Token/Manager.php	`100% <100%> (ø)`	`24 <24> (?)`
...ib/private/Authentication/Token/PublicKeyToken.php	`82.81% <82.81%> (ø)`	`20 <20> (?)`
...vate/Authentication/Token/PublicKeyTokenMapper.php	`90.54% <90.54%> (ø)`	`12 <12> (?)`
... and 8 more

rullzer · 2018-06-01T14:24:33Z

@ChristophWurst a first review would be appreciated :)

ChristophWurst

Some early feedback, as requested 😉

ChristophWurst · 2018-06-04T07:14:28Z

lib/private/Authentication/Token/DefaultToken.php

 		$this->addType('expires', 'int');
+		$this->addType('version', 'int');
+
+		$this->setVersion(1);


this will mark the version field as updated, although it won't actually be updated in many cases. This could cause unnecessary updates

server/lib/public/AppFramework/Db/Entity.php

Line 106 in a00cb2c

$this->markFieldUpdated($name);

You might want to solve this differently.

ChristophWurst · 2018-06-04T07:15:04Z

lib/private/Authentication/Token/DefaultTokenMapper.php

-			->where($qb->expr()->eq('token', $qb->createParameter('token')))
-			->setParameter('token', $token)
+			->where($qb->expr()->eq('token', $qb->createNamedParameter($token, IQueryBuilder::PARAM_STR)))
+			->andWhere($qb->expr()->eq('version', $qb->createNamedParameter(1, IQueryBuilder::PARAM_INT)))


I'd appreciate using a constant for the version number 😉

ChristophWurst · 2018-06-04T07:17:02Z

lib/private/Authentication/Token/IProvider.php

 	 * @param int $id
 	 */
-	public function invalidateTokenById(IUser $user, int $id);
+	public function invalidateTokenById(string $uid, int $id);


I usually prefer to work with IUser objects on higher-level services over stringly-typed UIDs. What's the reasoning for choosing strings over rich objects?

Yes. Because we want to update all other tokens when we cdhange a password. I would need to inject the user manager to get the user object. Because the Tokens only have the uid.

ChristophWurst · 2018-06-04T07:20:55Z

lib/private/Authentication/Token/Manager.php

+	 * @throws InvalidTokenException
+	 */
+	public function updateToken(IToken $token) {
+		if ($token instanceof DefaultToken) {


IMO it would make the code a lot easier if there was a private method getProvider($token).

Like

$this->getProvider($token)->updateToken($token);

where getProvider automatically throws the InvalidTokenException for unkown token types.

esp. since it's used many times in this class

ChristophWurst · 2018-06-04T07:22:28Z

lib/private/Authentication/Token/PublicKeyToken.php

+		$this->addType('privateKey', 'string');
+		$this->addType('version', 'int');
+
+		$this->setVersion(2);


same as above, this could be problematic for the dirty checking of the entity/mapper logic

ChristophWurst · 2018-06-04T07:23:41Z

lib/private/Authentication/Token/PublicKeyTokenMapper.php

+		$qb = $this->db->getQueryBuilder();
+		$qb->delete('authtoken')
+			->where($qb->expr()->eq('token', $qb->createNamedParameter($token)))
+			->andWhere($qb->expr()->eq('version', $qb->createNamedParameter(2, IQueryBuilder::PARAM_INT)))


again, I would appreciate a constant for the token version

ChristophWurst · 2018-06-04T07:25:20Z

lib/private/Authentication/Token/PublicKeyTokenProvider.php

+		}
+
+		$password = null;
+		if (!is_null($token->getPassword())) {


oh, I though you were in team !== null 🙈

Copy paste ;)

ChristophWurst · 2018-06-04T07:26:05Z

lib/private/Authentication/Token/PublicKeyTokenProvider.php

+			throw new InvalidTokenException();
+		}
+
+		// When changeing passwords all temp tokens are deleted


ChristophWurst · 2018-06-04T07:27:44Z

lib/private/Files/ObjectStore/S3ConnectionTrait.php

 			$options['signature_version'] = 'v2';
 		}
 		$this->connection = new S3Client($options);
+		$this->connection->getCredentials();


not sure what you're trying to sneak into our code base here

good question...

Tackled

rullzer · 2018-06-07T07:29:44Z

This is ready for review and testing.

blizzz · 2018-06-14T15:21:51Z

lib/private/Authentication/Token/Manager.php

+		$provider->updateTokenActivity($token);
+	}
+
+	public function getTokenByUser(string $uid): array {


the mixture of provided phpdoc and missing phpdoc… here it would be useful to doc IToken[]

blizzz

(tbc)

blizzz · 2018-06-14T15:28:55Z

lib/private/Authentication/Token/Manager.php

+	 * @throws InvalidTokenException
+	 * @return IToken
+	 */
+	public function getTokenById(int $tokenId): IToken {


besides by the type, the signature looks the same as in getToken. I guess it is database ID vs. token, um, value? Perhaps we can get the naming more clear in either this method or the upper one?

Yeah I don't like it either. But this was basically just copy pasted from the current implementation.

see, the IProvider is not in public namespace, so … but yeah, 🙈 🏁

Yup, it's confusing.

blizzz · 2018-06-14T15:33:59Z

lib/private/Authentication/Token/PublicKeyToken.php

+	}
+
+	public function setScope($scope) {
+		if (\is_array($scope)) {


\ shouldn't be necessary

rullzer · 2018-06-14T18:12:40Z

@blizzz addressed most. Lets get this in :)

blizzz

just some 💄 remarks, looks good and works otherwise 👍

blizzz · 2018-06-14T20:41:15Z

lib/private/Authentication/Token/PublicKeyTokenProvider.php

+		$secret = $this->config->getSystemValue('secret');
+		try {
+			return $this->crypto->decrypt($cipherText, $token . $secret);
+		} catch (\Exception $ex) {


is it not tooo broad? without having checked what else might come upon decrypt

I just want to catch it all. Better to throw an invalid token exception in the login and have it fail then do unexpected stuff.

blizzz · 2018-06-14T20:52:46Z

lib/private/Authentication/Token/DefaultTokenProvider.php

-	 * @param IUser $user
-	 * @return IToken[]
-	 */
-	public function getTokenByUser(IUser $user): array {


also remove \OCP\IUser use statement

blizzz · 2018-06-14T20:57:04Z

lib/private/Authentication/Token/Manager.php

+
+use OC\Authentication\Exceptions\InvalidTokenException;
+use OC\Authentication\Exceptions\PasswordlessTokenException;
+use OCP\IUser;


blizzz · 2018-06-14T20:59:54Z

lib/private/Authentication/Token/Manager.php

+		$this->publicKeyTokenProvider->invalidateOldTokens();
+	}
+
+	public function rotate(IToken $token, string $oldTokenId, string $newTokenId): IToken {


@throws annotations would be nice

blizzz · 2018-06-14T21:00:24Z

lib/private/Authentication/Token/Manager.php

+		if ($token instanceof PublicKeyToken) {
+			return $this->publicKeyTokenProvider;
+		}
+		throw new InvalidTokenException();


annotate? would make IDE more calm ;)

blizzz · 2018-06-14T21:01:34Z

lib/private/Authentication/Token/PublicKeyTokenProvider.php

+use OCP\AppFramework\Utility\ITimeFactory;
+use OCP\IConfig;
+use OCP\ILogger;
+use OCP\IUser;


blizzz · 2018-06-14T21:03:43Z

lib/private/Authentication/Token/PublicKeyTokenMapper.php

+	 * where a high number of (session) tokens is generated
+	 *
+	 * @param string $uid
+	 * @return DefaultToken[]


PublicKeyToken[]

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

We don't have user objects in the code everywhere Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

* When getting the token * When rotating the token * Also store the encrypted password as base64 to avoid weird binary stuff Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

* Add a lot of tests * Fixes related to those tests * Fix tests Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

And don't set the version in the constructor. That would possible cause to many updates. Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

ChristophWurst

Code looks good!

ChristophWurst · 2018-06-19T06:13:18Z

lib/private/Authentication/Token/PublicKeyTokenProvider.php

+	/** @var IConfig */
+	private $config;
+
+	/** @var ILogger $logger */


nit: $logger seems to be unnecessary, ie the doc comments are different to the ones above

not worth to wait another iteration, let's just get this out as by catch one next pr

ChristophWurst · 2018-06-19T06:19:32Z

lib/private/Authentication/Token/PublicKeyTokenProvider.php

+		$pkToken->setExpires($defaultToken->getExpires());
+		$pkToken->setId($defaultToken->getId());
+
+		return $this->mapper->update($pkToken);


It seemed weird that you're using update for a new token, but then I realized it's the same table/row 👍

ChristophWurst · 2018-06-19T06:20:48Z

version.php

 // when updating major/minor version number.

-$OC_Version = array(14, 0, 0, 4);
+$OC_Version = array(14, 0, 0, 5);


note to myself: there will be conflicts with https://github.com/nextcloud/server/pull/9632/files#diff-5bbd3ccd25214f9956eea7e9f714bc08R32

rullzer added enhancement 2. developing Work in progress feature: authentication labels May 15, 2018

rullzer added this to the Nextcloud 14 milestone May 15, 2018

rullzer force-pushed the feature/9441/multiple_token_providers branch from 676d76e to 9eb24b7 Compare May 18, 2018 19:09

rullzer force-pushed the feature/9441/multiple_token_providers branch from 9eb24b7 to bee7599 Compare May 18, 2018 19:11

rullzer force-pushed the feature/9441/multiple_token_providers branch 4 times, most recently from 504a562 to 29c5f86 Compare June 1, 2018 13:34

rullzer force-pushed the feature/9441/multiple_token_providers branch from cca529a to f09b4bb Compare June 1, 2018 19:16

ChristophWurst previously requested changes Jun 4, 2018

View reviewed changes

rullzer force-pushed the feature/9441/multiple_token_providers branch 4 times, most recently from 21cc5f1 to 5dc8dfd Compare June 4, 2018 09:06

ChristophWurst mentioned this pull request Jun 5, 2018

App password/tokens must not be invalidated on password change #2581

Closed

rullzer requested review from ChristophWurst, MorrisJobke and blizzz June 7, 2018 07:29

rullzer added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jun 7, 2018

blizzz reviewed Jun 14, 2018

View reviewed changes

rullzer force-pushed the feature/9441/multiple_token_providers branch from 5dc8dfd to dd83bc5 Compare June 14, 2018 17:39

blizzz approved these changes Jun 14, 2018

View reviewed changes

rullzer added 13 commits June 18, 2018 22:11

Abstract the Provider via a manager

3dd5f3d

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Add new authtoken v2 columns to the authtoken table

8eec3a9

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Initial PKT implementation

02e0af1

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Add first tests

1f17010

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Just pass uid to the Token stuff

4c0d710

We don't have user objects in the code everywhere Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

SetPassword on PublicKeyTokens

4bbc21c

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Add publickey provider to manager

d03d16a

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Actually convert the token

f168ecf

* When getting the token * When rotating the token * Also store the encrypted password as base64 to avoid weird binary stuff Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Generate the new publicKey tokens by default!

1999f7c

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Add more tests

9e7a95f

* Add a lot of tests * Fixes related to those tests * Fix tests Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Use constant for token version

df34571

And don't set the version in the constructor. That would possible cause to many updates. Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Add getProvider helper function

970dea9

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Comments

82959ca

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

rullzer force-pushed the feature/9441/multiple_token_providers branch from dd83bc5 to 82959ca Compare June 19, 2018 05:46

ChristophWurst approved these changes Jun 19, 2018

View reviewed changes

blizzz merged commit 7ef722e into master Jun 19, 2018

blizzz deleted the feature/9441/multiple_token_providers branch June 19, 2018 08:11

ChristophWurst mentioned this pull request Jun 19, 2018

Stateful 2fa providers #9632

Merged

8 tasks

rullzer mentioned this pull request Jun 19, 2018

Improve token handling #9441

Closed

ChristophWurst mentioned this pull request Nov 29, 2018

App password invalidated if IMAP auth server not accessible #12703

Closed

Uh oh!

Conversation

rullzer commented May 15, 2018 • edited by MorrisJobke Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rullzer commented Jun 1, 2018

Uh oh!

ChristophWurst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rullzer commented Jun 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blizzz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rullzer commented Jun 14, 2018

Uh oh!

blizzz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChristophWurst left a comment

Choose a reason for hiding this comment

rullzer commented May 15, 2018 •

edited by MorrisJobke

Loading

codecov bot commented May 18, 2018 •

edited

Loading