Skip to content

Conversation

@rullzer
Copy link
Member

@rullzer rullzer commented May 16, 2018

Requires:

Allows for tokens to be expired (for OAuth).

To make sure auth doesn't happen with expired tokens we thrown an ExpiredTokenException (that does hold the token). This seemed easier than adding a check everywhere we use it.

The tokens need to stay in the DB else we can't refresh them later.

@rullzer rullzer added enhancement 3. to review Waiting for reviews labels May 16, 2018
@rullzer rullzer added this to the Nextcloud 14 milestone May 16, 2018
@rullzer rullzer changed the title Feature/noid/tokens can expire Allow token to expire May 16, 2018
@rullzer rullzer force-pushed the feature/noid/tokens_can_expire branch 2 times, most recently from a9e1969 to 04f31ee Compare May 16, 2018 13:10
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good!

@rullzer rullzer force-pushed the feature/noid/tokens_can_expire branch 2 times, most recently from 4ce8863 to 9c78929 Compare May 16, 2018 18:07
@codecov
Copy link

codecov bot commented May 16, 2018

Codecov Report

Merging #9491 into master will decrease coverage by <.01%.
The diff coverage is 61.76%.

@@             Coverage Diff              @@
##             master    #9491      +/-   ##
============================================
- Coverage     51.69%   51.69%   -0.01%     
- Complexity    25725    25735      +10     
============================================
  Files          1641     1643       +2     
  Lines         96454    96482      +28     
  Branches       1393     1393              
============================================
+ Hits          49866    49878      +12     
- Misses        46588    46604      +16
Impacted Files Coverage Δ Complexity Δ
core/Migrations/Version14000Date20180516101403.php 0% <0%> (ø) 2 <2> (?)
version.php 0% <0%> (ø) 0 <0> (ø) ⬇️
...rivate/Authentication/Token/DefaultTokenMapper.php 100% <100%> (ø) 11 <0> (ø) ⬇️
...uthentication/Exceptions/ExpiredTokenException.php 100% <100%> (ø) 2 <2> (?)
...vate/Authentication/Token/DefaultTokenProvider.php 98.03% <100%> (+0.12%) 32 <0> (+4) ⬆️
lib/private/Authentication/Token/DefaultToken.php 85.24% <100%> (+1.31%) 20 <2> (+2) ⬆️
core/js/js.js 65.41% <0%> (-0.56%) 0% <0%> (ø)
lib/private/Files/Cache/Propagator.php 96.2% <0%> (+1.26%) 16% <0%> (ø) ⬇️

}

if ($token->getExpires() !== null && $token->getExpires() < $this->time->getTime()) {
throw new ExpiredTokenException($token);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add this to the PHPDocs?

}

if ($token->getExpires() !== null && $token->getExpires() < $this->time->getTime()) {
throw new ExpiredTokenException($token);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done..

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beside the little documentation nitpicks this looks fine 👍

However due to the nature of what we store in the token (encrypted
passwords etc). We can't just delete the tokens because that would make
the oauth refresh useless.

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer rullzer force-pushed the feature/noid/tokens_can_expire branch from 9c78929 to 6b7cf46 Compare May 17, 2018 14:10
@rullzer rullzer added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels May 17, 2018
@rullzer rullzer merged commit 8696651 into master May 17, 2018
@rullzer rullzer deleted the feature/noid/tokens_can_expire branch May 17, 2018 18:07
This was referenced May 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to release Ready to be released and/or waiting for tests to finish enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants