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

[stable12] Improve OAuth #9546

Merged
merged 9 commits into from May 23, 2018

Conversation

Projects
None yet
3 participants
@rullzer
Member

rullzer commented May 22, 2018

Backport of:

@rullzer

This comment has been minimized.

Member

rullzer commented May 23, 2018

I have no idea why the test fail it seems unrelated.

@ChristophWurst

Looks good, I just found one spot where it could make sense to add some logging 👍

$appToken->setExpires($this->time->getTime() + 3600);
$this->tokenProvider->updateToken($appToken);
} catch (InvalidTokenException $e) {
//Skip this token

This comment has been minimized.

@ChristophWurst

ChristophWurst May 23, 2018

Member

Didn't see this in the original PR, but wouldn't it make sense to at least have a debug log statement for this in case we have to trace bugs in this code?

@MorrisJobke

This comment has been minimized.

Member

MorrisJobke commented May 23, 2018

With this there is an endless login loop on the first authorization of an app.

cc @rullzer as discussed

rullzer added some commits May 15, 2018

Allow the rotation of tokens
This for example will allow rotating the apptoken for oauth

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Set OAuth token expiration
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Fail if the response type is not properly set
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Authenticate the clients on requesting a token
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Certain tokens can expire
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>
Rotate token
On a refresh token request:
* rorate
* reset expire

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Don't use special chars to avoid confusion
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Add tests
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Properly set expires to NULL when creating a token
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@codecov

This comment has been minimized.

codecov bot commented May 23, 2018

Codecov Report

Merging #9546 into stable12 will increase coverage by 0.05%.
The diff coverage is 70.27%.

@@              Coverage Diff               @@
##             stable12    #9546      +/-   ##
==============================================
+ Coverage       53.91%   53.96%   +0.05%     
- Complexity      22786    22815      +29     
==============================================
  Files            1387     1389       +2     
  Lines           87302    87437     +135     
  Branches         1331     1331              
==============================================
+ Hits            47066    47185     +119     
- Misses          40236    40252      +16
Impacted Files Coverage Δ Complexity Δ
apps/oauth2/lib/Migration/SetTokenExpiration.php 0% <0%> (ø) 5 <5> (?)
version.php 0% <0%> (ø) 0 <0> (ø) ⬇️
...rivate/Authentication/Token/DefaultTokenMapper.php 100% <100%> (ø) 11 <0> (ø) ⬇️
core/Controller/ClientFlowLoginController.php 78.87% <100%> (ø) 20 <0> (ø) ⬇️
...uthentication/Exceptions/ExpiredTokenException.php 100% <100%> (ø) 2 <2> (?)
...vate/Authentication/Token/DefaultTokenProvider.php 98.01% <100%> (+3.7%) 32 <2> (+6) ⬆️
lib/private/Authentication/Token/DefaultToken.php 89.58% <100%> (+2.74%) 17 <4> (+4) ⬆️
...auth2/lib/Controller/LoginRedirectorController.php 71.42% <60%> (+0.84%) 3 <0> (+1) ⬆️
apps/oauth2/lib/Controller/OauthApiController.php 81.53% <82%> (+2.37%) 11 <10> (+9) ⬆️
... and 4 more
@MorrisJobke

Tested and works now 👍

@MorrisJobke MorrisJobke merged commit 6b5fea4 into stable12 May 23, 2018

1 check failed

continuous-integration/drone/pr the build failed
Details

@MorrisJobke MorrisJobke deleted the backport/9517/stable12 branch May 23, 2018

@MorrisJobke MorrisJobke referenced this pull request May 31, 2018

Merged

12.0.8 RC 1 #9702

2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment