Add oauth code flow support #4704

LukasReschke · 2017-05-04T23:07:26Z

This adds the OAuth Code Flow support to our existing code base.

One example is the Moodle plugin from https://pssl16.github.io/.

TODOs:

Known Issues

Create two oauth clients in the admin settings: First the right one, then a fake client. Login will no longer work: wrong redirect exception.

mention-bot · 2017-05-04T23:07:27Z

@LukasReschke, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ChristophWurst, @icewind1991 and @blizzz to be potential reviewers.

LukasReschke · 2017-05-04T23:18:58Z

core/Controller/ClientFlowLoginController.php

 	 * @return TemplateResponse
 	 */
-	public function showAuthPickerPage() {
-		if($this->userSession->isLoggedIn()) {


What we should do instead here is that at least one of the following conditions is met:

Verify that a valid client identifier is given

Verify that the request contains a OCS-APIRequest: true HTTP header

If none of those are fulfilled show an error page.

handled here: 52734b3

LukasReschke · 2017-05-04T23:32:33Z

lib/private/User/Session.php

@@ -720,15 +720,15 @@ private function validateToken($token, $user = null) {
 	 */
 	public function tryTokenLogin(IRequest $request) {
 		$authHeader = $request->getHeader('Authorization');
-		if (strpos($authHeader, 'token ') === false) {


Wasn't used before by any client and didn't even work via WebDAV so that change should be fine :)

cc @ChristophWurst fyi :)

LukasReschke · 2017-05-04T23:33:37Z

apps/dav/lib/Connector/Sabre/Auth.php

+				\OC_Util::setupFS($user);
+				$this->currentUser = $user;
+				$this->session->close();
+				return [true, $this->principalPrefix . $user];


This return is a huge mess and copied code from below. Basically if we just call tryTokenLogin then it would lead into the code path to parent::check and there it checks if a Basic Auth header is set. None is set and thus SabreDAV throws an exception. We should clean this up.

LukasReschke · 2017-05-04T23:35:03Z

core/Controller/ClientFlowLoginController.php

 			$token,
-			$this->userSession->getUser()->getUID(),
+			$uid,
 			$loginName,
 			$password,
 			$this->getClientName(),


This should use the name of the OAuth application just like https://github.com/nextcloud/server/pull/4704/files#diff-652646310472be421d6b80289b907cb8R160 has.

codecov · 2017-05-12T12:56:52Z

Codecov Report

Merging #4704 into master will increase coverage by 0.03%.
The diff coverage is 68.9%.

@@             Coverage Diff              @@
##             master    #4704      +/-   ##
============================================
+ Coverage      54.2%   54.23%   +0.03%     
- Complexity    22195    22228      +33     
============================================
  Files          1367     1378      +11     
  Lines         84876    85142     +266     
  Branches       1322     1322              
============================================
+ Hits          46003    46179     +176     
- Misses        38873    38963      +90

Impacted Files	Coverage Δ	Complexity Δ
core/templates/loginflow/redirect.php	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
core/templates/loginflow/authpicker.php	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
apps/dav/lib/Connector/Sabre/Auth.php	`80.8% <ø> (ø)`	`33 <0> (ø)`	⬇️
settings/personal.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
apps/oauth2/templates/admin.php	`0% <0%> (ø)`	`0 <0> (?)`
apps/oauth2/appinfo/routes.php	`0% <0%> (ø)`	`0 <0> (?)`
...rivate/Authentication/Token/DefaultTokenMapper.php	`100% <100%> (ø)`	`11 <1> (+1)`	⬆️
apps/dav/lib/Server.php	`47.05% <100%> (+2.02%)`	`12 <0> (ø)`	⬇️
apps/oauth2/lib/Db/Client.php	`100% <100%> (ø)`	`1 <1> (?)`
apps/oauth2/lib/Db/AccessToken.php	`100% <100%> (ø)`	`1 <1> (?)`
... and 23 more

MorrisJobke · 2017-05-16T20:48:38Z

@LukasReschke How to move this forward?

LukasReschke · 2017-05-17T16:28:41Z

Rebased upon master. Will continue tonight.

THX a lot already, @schiessle 🚀

LukasReschke · 2017-05-18T14:34:21Z

tests/lib/Authentication/Token/DefaultTokenMapperTest.php

@@ -58,8 +58,8 @@ protected function setUp() {

 	private function resetDatabase() {
 		$qb = $this->dbConnection->getQueryBuilder();
-		$qb->delete('authtoken')->execute();
-		$qb->insert('authtoken')->values([
+		$qb->delete('AuthToken')->execute();


Renaming this breaks as databases tend to be case sensitive 😉

cc @schiessle – will revert the renaming.

rullzer · 2017-05-18T18:13:53Z

apps/dav/lib/Connector/Sabre/BearerAuth.php

+
+		if(!$this->userSession->isLoggedIn()) {
+			$this->userSession->tryTokenLogin($this->request);
+		}


Ah no... tryTokenLogin might log the user in...

Yep. 🙈 – We don't have a method where I can just pass the token and login and so as a short-term solution I thought of just abusing this behaviour 😉

rullzer · 2017-05-18T18:16:12Z

apps/oauth2/appinfo/routes.php

+		],
+		[
+			'name' => 'Settings#deleteClient',
+			'url' => '/clients/{id}/delete',


uhm... why not a DELETE to /clients/{id} ?

rullzer · 2017-05-18T18:19:05Z

apps/oauth2/lib/Controller/SettingsController.php

+		$client->setSecret($this->secureRandom->generate(64, self::validChars));
+		$client->setClientIdentifier($this->secureRandom->generate(64, self::validChars));
+		$this->clientMapper->insert($client);
+		return new RedirectResponse($this->urlGenerator->getAbsoluteURL('/index.php/settings/admin/security'));


Why this redirect magic and not just AJAX calls?

Yeah. JS isn't mine and @schiessle most preferred activity. Let's adjust that in the future :)

rullzer · 2017-05-18T18:22:46Z

core/Controller/ClientFlowLoginController.php

@@ -25,6 +25,9 @@
 use OC\Authentication\Exceptions\PasswordlessTokenException;
 use OC\Authentication\Token\IProvider;
 use OC\Authentication\Token\IToken;
+use OCA\OAuth2\Db\AccessToken;
+use OCA\OAuth2\Db\AccessTokenMapper;
+use OCA\OAuth2\Db\ClientMapper;


I get why. But I'm really not a fan of this hard app dependency.
We should think how we can make this somewhat modular in the future. Just so we have a clear defined interface between the two.

👍 – Yes. We need to think how to solve that better in the future.

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

rullzer

rullzer approved! ✅ 👍

hanzei · 2017-05-31T13:24:51Z

Will there be documentation for the endpoints?

LukasReschke added the 1. to develop Accepted and waiting to be taken care of label May 4, 2017

LukasReschke added this to the Nextcloud 12.0 milestone May 4, 2017

LukasReschke assigned schiessle May 4, 2017

LukasReschke requested review from rullzer and schiessle May 4, 2017 23:07

LukasReschke commented May 4, 2017

View reviewed changes

schiessle force-pushed the add-oauth-code-flow-support branch 3 times, most recently from 5f80b53 to fb72698 Compare May 12, 2017 11:07

schiessle force-pushed the add-oauth-code-flow-support branch 3 times, most recently from a8fdf43 to cd02164 Compare May 12, 2017 14:50

LukasReschke force-pushed the add-oauth-code-flow-support branch from cd02164 to 49a6eb1 Compare May 17, 2017 16:28

schiessle force-pushed the add-oauth-code-flow-support branch 2 times, most recently from 40886b0 to 44f4b2c Compare May 18, 2017 09:42

LukasReschke commented May 18, 2017

View reviewed changes

LukasReschke unassigned schiessle May 18, 2017

rullzer reviewed May 18, 2017

View reviewed changes

LukasReschke added 6 commits May 18, 2017 20:49

Add basic implementation for OAuth 2.0 Authorization Code Flow

5f71805

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

Use query builder

a5ddd65

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

Get a single row

0a2b57c

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

Increase length of encrypted token in database

c5ad3c9

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

Change to POST

4d96cd3

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

Fetch signle row

1470ec9

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

LukasReschke added 12 commits May 18, 2017 20:49

Add OAuth state to session

b07a0f5

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

Add app to autoenabled provisioning API scenario

a411622

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

Add tests for ClientFlowLoginController

26ee889

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

Rename table back to lowercase

77827eb

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

Fix style issues pointed out in review

d90eba3

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

Add test for DefaultTokenMapper

59e9689

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

Add tests for OAuth2 app

691646b

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

Don't ignore OAuth2 app

3055209

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

Use Bearer backend for SabreDAV

df3909a

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

Use a standardized Bearer now

f2a01e1

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

Add additional test for accessing DAV using Bearer Auth

5381121

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

Adjust token name

7927aed

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

LukasReschke force-pushed the add-oauth-code-flow-support branch from f4ddd43 to e34942f Compare May 18, 2017 18:49

LukasReschke added 2 commits May 18, 2017 20:58

Add indexes

fa6ec47

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

Delete token after usage in test

ba7b6bd

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

LukasReschke force-pushed the add-oauth-code-flow-support branch from 0e5dbf8 to ba7b6bd Compare May 18, 2017 18:58

LukasReschke added 4 commits May 18, 2017 21:19

Make legacy DAV backend use the BearerAuth backend as well

f93db72

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

Adjust realm from SabreDAV to Nextcloud

639ba52

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

Bearer comes first on the new endpoint

b8de3f4

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

Function accepts only integers

f418969

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

LukasReschke added 3. to review Waiting for reviews and removed 1. to develop Accepted and waiting to be taken care of labels May 18, 2017

rullzer approved these changes May 18, 2017

View reviewed changes

LukasReschke changed the title ~~[WIP] Add oauth code flow support~~ Add oauth code flow support May 18, 2017

LukasReschke merged commit 0eb4970 into master May 18, 2017

LukasReschke deleted the add-oauth-code-flow-support branch May 18, 2017 21:30

LEDfan mentioned this pull request Aug 25, 2017

Nextcloud as OAuth 2.0 provider #3599

Closed

ChristophWurst mentioned this pull request Oct 24, 2017

Get App PIN by normal login/rest call #6895

Closed

palant mentioned this pull request Mar 20, 2018

Nextcloud sync support palant/pfp#85

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add oauth code flow support #4704

Add oauth code flow support #4704

LukasReschke commented May 4, 2017 •

edited by hanzei

Loading

mention-bot commented May 4, 2017

LukasReschke May 4, 2017

schiessle May 12, 2017 •

edited

Loading

LukasReschke May 4, 2017

LukasReschke May 4, 2017

LukasReschke May 4, 2017

codecov bot commented May 12, 2017 •

edited

Loading

MorrisJobke commented May 16, 2017

LukasReschke commented May 17, 2017

LukasReschke May 18, 2017

rullzer May 18, 2017

rullzer May 18, 2017

LukasReschke May 18, 2017

rullzer May 18, 2017

rullzer May 18, 2017

LukasReschke May 18, 2017

rullzer May 18, 2017

LukasReschke May 18, 2017

rullzer left a comment

hanzei commented May 31, 2017

Add oauth code flow support #4704

Add oauth code flow support #4704

Conversation

LukasReschke commented May 4, 2017 • edited by hanzei Loading

mention-bot commented May 4, 2017

Choose a reason for hiding this comment

schiessle May 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 12, 2017 • edited Loading

Codecov Report

MorrisJobke commented May 16, 2017

LukasReschke commented May 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rullzer left a comment

Choose a reason for hiding this comment

hanzei commented May 31, 2017

LukasReschke commented May 4, 2017 •

edited by hanzei

Loading

schiessle May 12, 2017 •

edited

Loading

codecov bot commented May 12, 2017 •

edited

Loading