Refreshtoken lifetime + 0.3.0 version bump #182

DeepDiver1975 · 2018-11-30T10:12:39Z

Follow up on #174

…o 0.3.0

codecov · 2018-11-30T12:21:18Z

Codecov Report

Merging #182 into master will increase coverage by 0.05%.
The diff coverage is 68.18%.

@@             Coverage Diff              @@
##             master     #182      +/-   ##
============================================
+ Coverage     64.27%   64.33%   +0.05%     
- Complexity      221      226       +5     
============================================
  Files            34       35       +1     
  Lines           879      900      +21     
============================================
+ Hits            565      579      +14     
- Misses          314      321       +7

Impacted Files	Coverage Δ	Complexity Δ
appinfo/Migrations/Version20181130110158.php	`0% <0%> (ø)`	`2 <2> (?)`
lib/BackgroundJob/CleanUp.php	`100% <100%> (ø)`	`2 <0> (ø)`	⬇️
lib/Db/RefreshTokenMapper.php	`100% <100%> (ø)`	`15 <2> (+1)`	⬆️
lib/Controller/OAuthApiController.php	`97.56% <100%> (+0.03%)`	`19 <0> (ø)`	⬇️
lib/Db/RefreshToken.php	`81.81% <60%> (-18.19%)`	`3 <2> (+2)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e502ac...661f495. Read the comment docs.

PVince81 · 2018-11-30T13:44:19Z

unit tests ?

PVince81 · 2018-11-30T13:44:51Z

lib/Db/RefreshToken.php

+	* @return boolean true if the refresh token has expired, false otherwise.
+	*/
+	public function hasExpired() {
+		return \time() >= $this->getExpires();


use the TimeFactory everywhere so you can mock it in tests

mrow4a · 2018-11-30T16:50:32Z

lib/BackgroundJob/CleanUp.php

 		$this->authorizationCodeMapper = $authorizationCodeMapper;
 		$this->accessTokenMapper = $accessTokenMapper;
+		$this->refreshTokenMapper = $refreshTokenMapper;
+
+		$this->setInterval(86400);


magic number?

mrow4a · 2018-11-30T16:51:33Z

lib/Db/RefreshToken.php

+	/**
+	* Resets the expiry time to EXPIRATION_DAYS days from now.
+	*/
+	public function resetExpires() {


Formating of comment is strange

mrow4a · 2018-11-30T16:55:17Z

lib/Controller/OAuthApiController.php

@@ -206,6 +206,7 @@ public function generateToken($grant_type, $code = null,
 		$refreshToken->setClientId($client->getId());
 		$refreshToken->setUserId($userId);
 		$refreshToken->setAccessTokenId($accessToken->getId());
+		$refreshToken->resetExpires();


What is the rationale behind allowing RefreshToken setting expiration, and not Controller?

T0mWz · 2018-12-03T09:51:49Z

Argg, applied this on our production environment & now run into a problem here..

When your AccessToken is expired and you retrieving a new one, your RefreshToken is regenerated where the AccessToken is linked too.

So, the lifetime of the RefreshToken is automatically extended with the duration of the AccessToken.
Have to update the existing RefreshToken with the ID of the new AccessToken, in stead of generate a new RefreshToken..

See this part of the OAuthApiController;
$userId = $refreshToken->getUserId(); $relatedAccessToken = new AccessToken(); $relatedAccessToken->setId($refreshToken->getAccessTokenId()); $this->accessTokenMapper->delete($relatedAccessToken); $this->refreshTokenMapper->delete($refreshToken); break;

T0mWz · 2018-12-07T13:11:36Z

Ok, have fixed now. There are slightly some more adjustments.
In "oauth2/lib/Db/RefreshTokenMapper.php" we need an extra function;

...
        /**
        * Update valid refresh token with a new Access Token
        */
        public function UpdateRefreshToken($AccessTokenId, $tokenId) {
                $this->logger->info('Update Refresh Token with new AccessToken id.', ['app' => $this->appName]);

                $sql = 'UPDATE `' . $this->tableName . '` SET `access_token_id`= ? WHERE `token` = ?';
                $stmt = $this->execute($sql, [$AccessTokenId, $tokenId], null, null);
                $stmt->closeCursor();
                return true;
        }

In "oauth2/lib/Controller/OAuthApiController.php" we will update the token when it's still valid & return the JSON.

@@ -169,8 +169,10 @@
 				try {
 					/** @var RefreshToken $refreshToken */
 					$refreshToken = $this->refreshTokenMapper->findByToken($refresh_token);
+					$this->logger->info('Refresh token found for client "' . $client->getName() . '".', ['app' => $this->appName]);
 				} catch (DoesNotExistException $exception) {
 					\OC::$server->getLogger()->logException($exception, ['app'=>__CLASS__]);
+					$this->logger->info('Refresh token not found for client "' . $client->getName() . '", it may have expired.', ['app' => $this->appName]);
 					return new JSONResponse(['error' => 'invalid_grant'], $statusCode);
 				}

@@ -179,14 +181,33 @@
 					return new JSONResponse(['error' => 'invalid_grant'], $statusCode);
 				}

-				$this->logger->info('A refresh token has been used by the client "' . $client->getName() . '" to request an access token.', ['app' => $this->appName]);
-
 				$userId = $refreshToken->getUserId();
 				$relatedAccessToken = new AccessToken();
-				$relatedAccessToken->setId($refreshToken->getAccessTokenId());
+				$relatedAccessToken->setId($refreshToken->getAccessTokenId());
 				$this->accessTokenMapper->delete($relatedAccessToken);
-				$this->refreshTokenMapper->delete($refreshToken);
-				break;
+
+        		        $token = Utilities::generateRandom();
+        		        $accessToken = new AccessToken();
+		                $accessToken->setToken($token);
+                		$accessToken->setClientId($client->getId());
+                		$accessToken->setUserId($userId);
+        		        $accessToken->resetExpires();
+		                $this->accessTokenMapper->insert($accessToken);
+				$refreshToken = $this->refreshTokenMapper->UpdateRefreshToken($accessToken->getId(), $refresh_token);
+
+				$this->logger->info('Current refresh token for user "' . $userId . '" with client "' . $client->getName() . '" is still valid and has been used to request an access token.', ['app' => $this->appName]);
+
+		                return new JSONResponse(
+	                        	[
+	                        	        'access_token' => $accessToken->getToken(),
+	                        	        'token_type' => 'Bearer',
+	                        	        'expires_in' => AccessToken::EXPIRATION_TIME,
+        	                	        'refresh_token' => $refresh_token,
+        	                	        'user_id' => $userId,
+        	                	        'message_url' => $this->urlGenerator->linkToRouteAbsolute($this->appName . '.page.authorizationSuccessful')
+        	        	        ]
+        		        );
+				//break;
 			default:
 				\OC::$server->getLogger()->debug("unhandled grant type: {$grant_type}", ['app'=>__CLASS__]);
 				return new JSONResponse(['error' => 'invalid_grant'], Http::STATUS_BAD_REQUEST);
@@ -206,6 +227,7 @@
 		$refreshToken->setClientId($client->getId());
 		$refreshToken->setUserId($userId);
 		$refreshToken->setAccessTokenId($accessToken->getId());
+		$refreshToken->resetExpires();
 		$this->refreshTokenMapper->insert($refreshToken);

 		return new JSONResponse(

Now I got only a nasty Exception when there isn't a RefreshToken found in the follow try statement for "oauth2/lib/Controller/OAuthApiController.php";

                                try {
                                        /** @var RefreshToken $refreshToken */
                                        $refreshToken = $this->refreshTokenMapper->findByToken($refresh_token);
                                        $this->logger->info('Refresh token found for client "' . $client->getName() . '".', ['app' => $this->appName]);
                                }

Maybe you can have a look how this can be fixed in a more cleaner way.

ownCloud[12407]: {OCA\OAuth2\Controller\OAuthApiController} Exception: {"Exception":"OCP\\AppFramework\\Db\\DoesNotExistException","Message":"Did expect one result but found none when executing: query \"SELECT * FROM `*PREFIX*oauth2_refresh_tokens` WHERE `token` = ? AND expires > unix_timestamp(now())\"; parameters Array\n(\n    [0] => WzWrGCJWTJGBXJCrUK99s4txyDWZQO3iBlpMLaHsZu7NYyP87zisXvmlt9MHIwDD\n)\n; limit \"\"; offset \"\"","Code":0,"Trace":"#0 \/var\/www\/surfdrive-tst.1009\/lib\/public\/AppFramework\/Db\/Mapper.php(379): OCP\\AppFramework\\Db\\Mapper->findOneQuery('SELECT * FROM `...', Array, NULL, NULL)\n#1 \/var\/www\/surfdrive-tst.1009\/apps\/oauth2\/lib\/Db\/RefreshTokenMapper.php(84): OCP\\AppFramework\\Db\\Mapper->findEntity('SELECT * FROM `...', Array, NULL, NULL)\n#2 \/var\/www\/surfdrive-tst.1009\/apps\/oauth2\/lib\/Controller\/OAuthApiController.php(171): OCA\\OAuth2\\Db\\RefreshTokenMapper->findByToken('WzWrGCJWTJGBXJC...')\n#3 [internal function]: OCA\\OAuth2\\Controller\\OAuthApiController->generateToken(*** sensitive parameters replaced ***)\n#4 \/var\/www\/surfdrive-tst.1009\/lib\/private\/AppFramework\/Http\/Dispatcher.php(159): call_user_func_array(Array, Array)\n#5 \/var\/www\/surfdrive-tst.1009\/lib\/private\/AppFramework\/Http\/Dispatcher.php(89): OC\\AppFramework\\Http\\Dispatcher->executeController(Object(OCA\\OAuth2\\Controller\\OAuthApiController), 'generateToken')\n#6 \/var\/www\/surfdrive-tst.1009\/lib\/private\/AppFramework\/App.php(103): OC\\AppFramework\\Http\\Dispatcher->dispatch(Object(OCA\\OAuth2\\Controller\\OAuthApiController), 'generateToken')\n#7 \/var\/www\/surfdrive-tst.1009\/lib\/private\/AppFramework\/Routing\/RouteActionHandler.php(46): OC\\AppFramework\\App::main('OCA\\\\OAuth2\\\\Cont...', 'generateToken', Object(OC\\AppFramework\\DependencyInjection\\DIContainer), Array)\n#8 [internal function]: OC\\AppFramework\\Routing\\RouteActionHandler->__invoke(Array)\n#9 \/var\/www\/surfdrive-tst.1009\/lib\/private\/Route\/Router.php(342): call_user_func(Object(OC\\AppFramework\\Routing\\RouteActionHandler), Array)\n#10 \/var\/www\/surfdrive-tst.1009\/lib\/base.php(919): OC\\Route\\Router->match('\/apps\/oauth2\/ap...')\n#11 \/var\/www\/surfdrive-tst.1009\/index.php(55): OC::handleRequest()\n#12 {main}","File":"\/var\/www\/surfdrive-tst.1009\/lib\/public\/AppFramework\/Db\/Mapper.php","Line":294} [cd2169b2-6f62-42a8-bdfd-afeb09e986a4]

T0mWz · 2018-12-07T13:27:46Z

Owja, what's also important. Not sure of you have received it from me. Is the follow changes in "oauth2/lib/Db/RefreshTokenMapper.php"

        public function find($id) {
                if (!is_int($id)) {
                        throw new InvalidArgumentException('Argument id must be an int');
                }

                $sql = 'SELECT * FROM `' . $this->tableName . '` WHERE `id` = ? AND expires > unix_timestamp(now())';
                return $this->findEntity($sql, [$id], null, null);
        }




        public function findByToken($token) {
                if (!is_string($token)) {
                        throw new InvalidArgumentException('Argument token must be a string');
                }

                $sql = 'SELECT * FROM `' . $this->tableName . '` WHERE `token` = ? AND expires > unix_timestamp(now())';
                return $this->findEntity($sql, [$token], null, null);
        }

mmattel · 2020-01-21T06:35:27Z

Ping, any update on this?

T0mWz · 2020-01-21T11:45:18Z

Ping, any update on this?

This is already open for a quite long period.
We have still applied these changes in the latest oAuth2 app release at SURF.
Can provide you an up-to-date patch if desired.

mmattel · 2020-01-21T17:57:14Z

Can provide you an up-to-date patch if desired

Yes please 👍

phil-davis · 2020-01-22T01:18:17Z

@micbar pinging you so you are aware of this PR that has been here since 2018.
(and has minor conflicts)

micbar · 2020-01-22T08:54:17Z

As of discussion in RC closing this, obsolete.

micbar · 2020-01-22T10:17:07Z

Needs reasoning

T0mWz · 2020-01-22T10:40:43Z

@micbar ; From a security point of view, you do not want an infinite session duration. Where we (@ SURF) also use an external Identity Provider (Shibboleth / LDAP), instead of local accounts,
we also want to check once in the X period whether the user can still log in via the own IdP. For example when they forget to block the user within ownCloud.

DeepDiver1975 · 2020-01-22T11:23:45Z

Reasoning:
All is moving into the direct of OpenID Connect - globally and here at ownCloud.
The OAuth2 app is loosing more and more reasons to exist and therefore we will not add new features for now.

michaelstingl · 2020-01-22T11:37:59Z

Policies (like expiration) are managed by the IdP with OIDC (OpenID Connect). All ownCloud clients will respect those.

T0mWz · 2020-01-22T13:07:26Z

@DeepDiver1975 , to make the challenge a little bit bigger 😅.. Now we support multiple login methods, both local and Shibboleth accounts (via SSO). Could this still be supported when switching to OIDC? I guess for local account, only the sessions will be handled then via OIDC, like now is done by oAuth2 or is the "Identity Service" part also there to authenticate to a local database or something?

DeepDiver1975 · 2020-01-22T13:15:36Z

@T0mWz I'm pretty much aware of the challenges in our setup! We need to have a closer look and discussion on how to get you migrated one day.

DeepDiver1975 changed the title ~~T0m wz refreshtoken lifetime~~ Refreshtoken lifetime + 0.3.0 version bump Nov 30, 2018

DeepDiver1975 requested a review from tomneedham November 30, 2018 10:13

DeepDiver1975 self-assigned this Nov 30, 2018

DeepDiver1975 added the 3 - To review label Nov 30, 2018

DeepDiver1975 mentioned this pull request Nov 30, 2018

oAuth2 RefreshToken Lifetime added #174

Closed

T0mWz and others added 3 commits November 30, 2018 12:56

oAuth2 RefreshToken Lifetime added

8b3045c

Add migration to add expires column for refesh tokens - set version t…

614f173

…o 0.3.0

Adjust change log

661f495

DeepDiver1975 force-pushed the T0mWz-refreshtoken-lifetime branch from d6c46f8 to 661f495 Compare November 30, 2018 11:56

PVince81 reviewed Nov 30, 2018

View reviewed changes

PVince81 mentioned this pull request Nov 30, 2018

[For 10.1] OAuth2 0.3.0 #181

Closed

31 tasks

mrow4a reviewed Nov 30, 2018

View reviewed changes

micbar closed this Jan 22, 2020

DeepDiver1975 deleted the T0mWz-refreshtoken-lifetime branch January 22, 2020 10:04

T0mWz mentioned this pull request Oct 12, 2023

Introduce expiration on refresh tokens - an implementation proposal #288

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refreshtoken lifetime + 0.3.0 version bump #182

Refreshtoken lifetime + 0.3.0 version bump #182

DeepDiver1975 commented Nov 30, 2018 •

edited

Loading

codecov bot commented Nov 30, 2018 •

edited

Loading

PVince81 commented Nov 30, 2018

PVince81 Nov 30, 2018

mrow4a Nov 30, 2018

mrow4a Nov 30, 2018

mrow4a Nov 30, 2018

T0mWz commented Dec 3, 2018 •

edited

Loading

T0mWz commented Dec 7, 2018

T0mWz commented Dec 7, 2018

mmattel commented Jan 21, 2020

T0mWz commented Jan 21, 2020

mmattel commented Jan 21, 2020

phil-davis commented Jan 22, 2020

micbar commented Jan 22, 2020

micbar commented Jan 22, 2020

T0mWz commented Jan 22, 2020 •

edited

Loading

DeepDiver1975 commented Jan 22, 2020

michaelstingl commented Jan 22, 2020 •

edited

Loading

T0mWz commented Jan 22, 2020 •

edited

Loading

DeepDiver1975 commented Jan 22, 2020

Refreshtoken lifetime + 0.3.0 version bump #182

Refreshtoken lifetime + 0.3.0 version bump #182

Conversation

DeepDiver1975 commented Nov 30, 2018 • edited Loading

codecov bot commented Nov 30, 2018 • edited Loading

Codecov Report

PVince81 commented Nov 30, 2018

PVince81 Nov 30, 2018

Choose a reason for hiding this comment

mrow4a Nov 30, 2018

Choose a reason for hiding this comment

mrow4a Nov 30, 2018

Choose a reason for hiding this comment

mrow4a Nov 30, 2018

Choose a reason for hiding this comment

T0mWz commented Dec 3, 2018 • edited Loading

T0mWz commented Dec 7, 2018

T0mWz commented Dec 7, 2018

mmattel commented Jan 21, 2020

T0mWz commented Jan 21, 2020

mmattel commented Jan 21, 2020

phil-davis commented Jan 22, 2020

micbar commented Jan 22, 2020

micbar commented Jan 22, 2020

T0mWz commented Jan 22, 2020 • edited Loading

DeepDiver1975 commented Jan 22, 2020

michaelstingl commented Jan 22, 2020 • edited Loading

T0mWz commented Jan 22, 2020 • edited Loading

DeepDiver1975 commented Jan 22, 2020

DeepDiver1975 commented Nov 30, 2018 •

edited

Loading

codecov bot commented Nov 30, 2018 •

edited

Loading

T0mWz commented Dec 3, 2018 •

edited

Loading

T0mWz commented Jan 22, 2020 •

edited

Loading

michaelstingl commented Jan 22, 2020 •

edited

Loading

T0mWz commented Jan 22, 2020 •

edited

Loading