-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Add new public key token provider (tokens survive password change) #9485
Conversation
676d76e
to
9eb24b7
Compare
Codecov Report
@@ 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
|
9eb24b7
to
bee7599
Compare
504a562
to
29c5f86
Compare
@ChristophWurst a first review would be appreciated :) |
cca529a
to
f09b4bb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some early feedback, as requested 😉
@@ -85,6 +89,9 @@ public function __construct() { | |||
$this->addType('lastCheck', 'int'); | |||
$this->addType('scope', 'string'); | |||
$this->addType('expires', 'int'); | |||
$this->addType('version', 'int'); | |||
|
|||
$this->setVersion(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
->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))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd appreciate using a constant for the version number 😉
* @param int $id | ||
*/ | ||
public function invalidateTokenById(IUser $user, int $id); | ||
public function invalidateTokenById(string $uid, int $id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
* @throws InvalidTokenException | ||
*/ | ||
public function updateToken(IToken $token) { | ||
if ($token instanceof DefaultToken) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
$this->addType('privateKey', 'string'); | ||
$this->addType('version', 'int'); | ||
|
||
$this->setVersion(2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above, this could be problematic for the dirty checking of the entity/mapper logic
$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))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again, I would appreciate a constant for the token version
} | ||
|
||
$password = null; | ||
if (!is_null($token->getPassword())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, I though you were in team !== null
🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy paste ;)
throw new InvalidTokenException(); | ||
} | ||
|
||
// When changeing passwords all temp tokens are deleted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo
@@ -98,6 +98,7 @@ protected function getConnection() { | |||
$options['signature_version'] = 'v2'; | |||
} | |||
$this->connection = new S3Client($options); | |||
$this->connection->getCredentials(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure what you're trying to sneak into our code base here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good question...
21cc5f1
to
5dc8dfd
Compare
This is ready for review and testing. |
$provider->updateTokenActivity($token); | ||
} | ||
|
||
public function getTokenByUser(string $uid): array { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the mixture of provided phpdoc and missing phpdoc… here it would be useful to doc IToken[]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(tbc)
* @throws InvalidTokenException | ||
* @return IToken | ||
*/ | ||
public function getTokenById(int $tokenId): IToken { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I don't like it either. But this was basically just copy pasted from the current implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see, the IProvider is not in public namespace, so … but yeah, 🙈 🏁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, it's confusing.
} | ||
|
||
public function setScope($scope) { | ||
if (\is_array($scope)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
\ shouldn't be necessary
5dc8dfd
to
dd83bc5
Compare
@blizzz addressed most. Lets get this in :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just some 💄 remarks, looks good and works otherwise 👍
$secret = $this->config->getSystemValue('secret'); | ||
try { | ||
return $this->crypto->decrypt($cipherText, $token . $secret); | ||
} catch (\Exception $ex) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it not tooo broad? without having checked what else might come upon decrypt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
* @param IUser $user | ||
* @return IToken[] | ||
*/ | ||
public function getTokenByUser(IUser $user): array { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also remove \OCP\IUser use statement
|
||
use OC\Authentication\Exceptions\InvalidTokenException; | ||
use OC\Authentication\Exceptions\PasswordlessTokenException; | ||
use OCP\IUser; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
$this->publicKeyTokenProvider->invalidateOldTokens(); | ||
} | ||
|
||
public function rotate(IToken $token, string $oldTokenId, string $newTokenId): IToken { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@throws annotations would be nice
if ($token instanceof PublicKeyToken) { | ||
return $this->publicKeyTokenProvider; | ||
} | ||
throw new InvalidTokenException(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
annotate? would make IDE more calm ;)
use OCP\AppFramework\Utility\ITimeFactory; | ||
use OCP\IConfig; | ||
use OCP\ILogger; | ||
use OCP\IUser; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
* where a high number of (session) tokens is generated | ||
* | ||
* @param string $uid | ||
* @return DefaultToken[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PublicKeyToken[]
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
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>
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>
dd83bc5
to
82959ca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good!
/** @var IConfig */ | ||
private $config; | ||
|
||
/** @var ILogger $logger */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: $logger
seems to be unnecessary, ie the doc comments are different to the ones above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not worth to wait another iteration, let's just get this out as by catch one next pr
$pkToken->setExpires($defaultToken->getExpires()); | ||
$pkToken->setId($defaultToken->getId()); | ||
|
||
return $this->mapper->update($pkToken); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seemed weird that you're using update
for a new token, but then I realized it's the same table/row 👍
@@ -29,7 +29,7 @@ | |||
// between betas, final and RCs. This is _not_ the public version number. Reset minor/patchlevel | |||
// when updating major/minor version number. | |||
|
|||
$OC_Version = array(14, 0, 0, 4); | |||
$OC_Version = array(14, 0, 0, 5); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note to myself: there will be conflicts with https://github.com/nextcloud/server/pull/9632/files#diff-5bbd3ccd25214f9956eea7e9f714bc08R32
For #9441
todo: