Skip to content

Commit af295e0

Browse files
committed
OAuth Server enhancements -- more complete access token response and groundwork
for scope Summary: this patch makes the access token response "complete" relative to spec by returning when it expires AND that the token_type is in fact 'Bearer'. This patch also lays the groundwork for scope by fixing the underlying data model and adding the first scope checks for "offline_access" relative to expires and the "whoami" method. Further, conduit is augmented to open up individual methods for access via OAuth generally to enable "whoami" access. There's also a tidy little scope class to keep track of all the various scopes we plan to have as well as strings for display (T849 - work undone) Somewhat of a hack but Conduit methods by default have SCOPE_NOT_ACCESSIBLE. We then don't even bother with the OAuth stuff within conduit if we're not supposed to be accessing the method via Conduit. Felt relatively clean to me in terms of additional code complexity, etc. Next up ends up being T848 (scope in OAuth) and T849 (let user's authorize clients for specific scopes which kinds of needs T850). There's also a bunch of work that needs to be done to return the appropriate, well-formatted error codes. All in due time...! Test Plan: verified that an access_token with no scope doesn't let me see anything anymore. :( verified that access_tokens made awhile ago expire. :( Reviewers: epriestley Reviewed By: epriestley CC: aran, epriestley Maniphest Tasks: T888, T848 Differential Revision: https://secure.phabricator.com/D1657
1 parent 228c378 commit af295e0

17 files changed

+212
-46
lines changed
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
ALTER TABLE `phabricator_oauth_server`.`oauth_server_oauthclientauthorization`
2+
ADD `scope` text NOT NULL;
3+
4+
ALTER TABLE `phabricator_oauth_server`.`oauth_server_oauthserveraccesstoken`
5+
DROP `dateExpires`;
6+

src/__phutil_library_map__.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -619,6 +619,7 @@
619619
'PhabricatorOAuthServerAuthorizationCode' => 'applications/oauthserver/storage/authorizationcode',
620620
'PhabricatorOAuthServerClient' => 'applications/oauthserver/storage/client',
621621
'PhabricatorOAuthServerDAO' => 'applications/oauthserver/storage/base',
622+
'PhabricatorOAuthServerScope' => 'applications/oauthserver/scope',
622623
'PhabricatorOAuthServerTestController' => 'applications/oauthserver/controller/test',
623624
'PhabricatorOAuthServerTokenController' => 'applications/oauthserver/controller/token',
624625
'PhabricatorOAuthUnlinkController' => 'applications/auth/controller/unlink',

src/applications/conduit/controller/api/PhabricatorConduitAPIController.php

Lines changed: 35 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,7 @@ public function processRequest() {
114114
$allow_unguarded_writes = false;
115115
$auth_error = null;
116116
if ($method_handler->shouldRequireAuthentication()) {
117+
$metadata['scope'] = $method_handler->getRequiredScope();
117118
$auth_error = $this->authenticateUser($api_request, $metadata);
118119
// If we've explicitly authenticated the user here and either done
119120
// CSRF validation or are using a non-web authentication mechanism.
@@ -248,24 +249,45 @@ private function authenticateUser(
248249
}
249250

250251
// handle oauth
252+
// TODO - T897 (make error codes for OAuth more correct to spec)
253+
// and T891 (strip shield from Conduit response)
251254
$access_token = $request->getStr('access_token');
252-
if ($access_token) {
255+
$method_scope = $metadata['scope'];
256+
if ($access_token &&
257+
$method_scope != PhabricatorOAuthServerScope::SCOPE_NOT_ACCESSIBLE) {
253258
$token = id(new PhabricatorOAuthServerAccessToken())
254259
->loadOneWhere('token = %s',
255260
$access_token);
256-
if ($token) {
257-
// TODO - T888 -- add expiration date and refresh tokens to oauth
258-
$user_phid = $token->getUserPHID();
259-
if ($user_phid) {
260-
$user = id(new PhabricatorUser())
261-
->loadOneWhere('phid = %s',
262-
$user_phid);
263-
if ($user) {
264-
$api_request->setUser($user);
265-
return null;
266-
}
267-
}
261+
if (!$token) {
262+
return array(
263+
'ERR-INVALID-AUTH',
264+
'Access token does not exist.',
265+
);
266+
}
267+
268+
$oauth_server = new PhabricatorOAuthServer();
269+
$valid = $oauth_server->validateAccessToken($token,
270+
$method_scope);
271+
if (!$valid) {
272+
return array(
273+
'ERR-INVALID-AUTH',
274+
'Access token is invalid.',
275+
);
268276
}
277+
278+
// valid token, so let's log in the user!
279+
$user_phid = $token->getUserPHID();
280+
$user = id(new PhabricatorUser())
281+
->loadOneWhere('phid = %s',
282+
$user_phid);
283+
if (!$user) {
284+
return array(
285+
'ERR-INVALID-AUTH',
286+
'Access token is for invalid user.',
287+
);
288+
}
289+
$api_request->setUser($user);
290+
return null;
269291
}
270292

271293
// Handle sessionless auth. TOOD: This is super messy.

src/applications/conduit/controller/api/__init__.php

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@
1313
phutil_require_module('phabricator', 'applications/conduit/protocol/request');
1414
phutil_require_module('phabricator', 'applications/conduit/protocol/response');
1515
phutil_require_module('phabricator', 'applications/conduit/storage/methodcalllog');
16+
phutil_require_module('phabricator', 'applications/oauthserver/scope');
17+
phutil_require_module('phabricator', 'applications/oauthserver/server');
1618
phutil_require_module('phabricator', 'applications/oauthserver/storage/accesstoken');
1719
phutil_require_module('phabricator', 'applications/people/storage/user');
1820
phutil_require_module('phabricator', 'storage/queryfx');

src/applications/conduit/method/base/ConduitAPIMethod.php

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,11 @@ public function getErrorDescription($error_code) {
3535
return idx($this->defineErrorTypes(), $error_code, 'Unknown Error');
3636
}
3737

38+
public function getRequiredScope() {
39+
// by default, conduit methods are not accessible via OAuth
40+
return PhabricatorOAuthServerScope::SCOPE_NOT_ACCESSIBLE;
41+
}
42+
3843
public function executeMethod(ConduitAPIRequest $request) {
3944
return $this->execute($request);
4045
}

src/applications/conduit/method/base/__init__.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77

88

9+
phutil_require_module('phabricator', 'applications/oauthserver/scope');
910
phutil_require_module('phabricator', 'infrastructure/env');
1011

1112
phutil_require_module('phutil', 'parser/uri');

src/applications/conduit/method/user/whoami/ConduitAPI_user_whoami_Method.php

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
<?php
22

33
/*
4-
* Copyright 2011 Facebook, Inc.
4+
* Copyright 2012 Facebook, Inc.
55
*
66
* Licensed under the Apache License, Version 2.0 (the "License");
77
* you may not use this file except in compliance with the License.
@@ -40,6 +40,10 @@ public function defineErrorTypes() {
4040
);
4141
}
4242

43+
public function getRequiredScope() {
44+
return PhabricatorOAuthServerScope::SCOPE_WHOAMI;
45+
}
46+
4347
protected function execute(ConduitAPIRequest $request) {
4448
return $this->buildUserInformationDictionary($request->getUser());
4549
}

src/applications/conduit/method/user/whoami/__init__.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88

99
phutil_require_module('phabricator', 'applications/conduit/method/user/base');
10+
phutil_require_module('phabricator', 'applications/oauthserver/scope');
1011

1112

1213
phutil_require_source('ConduitAPI_user_whoami_Method.php');

src/applications/oauthserver/controller/auth/PhabricatorOAuthServerAuthController.php

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -49,11 +49,15 @@ public function processRequest() {
4949
);
5050
}
5151

52-
if ($server->userHasAuthorizedClient($client)) {
52+
$server->setClient($client);
53+
if ($server->userHasAuthorizedClient()) {
5354
$return_auth_code = true;
5455
$unguarded_write = AphrontWriteGuard::beginScopedUnguardedWrites();
5556
} else if ($request->isFormPost()) {
56-
$server->authorizeClient($client);
57+
// TODO -- T848 (add scope to Phabricator OAuth)
58+
// should have some $scope based off of user submission here...!
59+
$scope = array(PhabricatorOAuthServerScope::SCOPE_WHOAMI => 1);
60+
$server->authorizeClient($scope);
5761
$return_auth_code = true;
5862
$unguarded_write = null;
5963
} else {
@@ -64,7 +68,7 @@ public function processRequest() {
6468
if ($return_auth_code) {
6569
// step 1 -- generate authorization code
6670
$auth_code =
67-
$server->generateAuthorizationCode($client);
71+
$server->generateAuthorizationCode();
6872

6973
// step 2 -- error or return it
7074
if (!$auth_code) {

src/applications/oauthserver/controller/auth/__init__.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
phutil_require_module('phabricator', 'aphront/writeguard');
1010
phutil_require_module('phabricator', 'applications/auth/controller/base');
1111
phutil_require_module('phabricator', 'applications/oauthserver/response');
12+
phutil_require_module('phabricator', 'applications/oauthserver/scope');
1213
phutil_require_module('phabricator', 'applications/oauthserver/server');
1314
phutil_require_module('phabricator', 'applications/oauthserver/storage/client');
1415
phutil_require_module('phabricator', 'view/form/base');

src/applications/oauthserver/controller/token/PhabricatorOAuthServerTokenController.php

Lines changed: 26 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ public function processRequest() {
3232
$client_phid = $request->getStr('client_id');
3333
$client_secret = $request->getStr('client_secret');
3434
$response = new PhabricatorOAuthResponse();
35+
$server = new PhabricatorOAuthServer();
3536
if (!$code) {
3637
return $response->setMalformed(
3738
'Required parameter code missing.'
@@ -48,16 +49,33 @@ public function processRequest() {
4849
);
4950
}
5051

52+
$client = id(new PhabricatorOAuthServerClient())
53+
->loadOneWhere('phid = %s', $client_phid);
54+
if (!$client) {
55+
return $response->setNotFound(
56+
'Client with client_id '.$client_phid.' not found.'
57+
);
58+
}
59+
$server->setClient($client);
60+
5161
$auth_code = id(new PhabricatorOAuthServerAuthorizationCode())
5262
->loadOneWhere('code = %s', $code);
5363
if (!$auth_code) {
5464
return $response->setNotFound(
5565
'Authorization code '.$code.' not found.'
5666
);
5767
}
68+
69+
$user_phid = $auth_code->getUserPHID();
5870
$user = id(new PhabricatorUser())
59-
->loadOneWhere('phid = %s', $auth_code->getUserPHID());
60-
$server = new PhabricatorOAuthServer($user);
71+
->loadOneWhere('phid = %s', $user_phid);
72+
if (!$user) {
73+
return $response->setNotFound(
74+
'User with phid '.$user_phid.' not found.'
75+
);
76+
}
77+
$server->setUser($user);
78+
6179
$test_code = new PhabricatorOAuthServerAuthorizationCode();
6280
$test_code->setClientSecret($client_secret);
6381
$test_code->setClientPHID($client_phid);
@@ -69,19 +87,15 @@ public function processRequest() {
6987
);
7088
}
7189

72-
$client = id(new PhabricatorOAuthServerClient())
73-
->loadOneWhere('phid = %s', $client_phid);
74-
if (!$client) {
75-
return $response->setNotFound(
76-
'Client with client_id '.$client_phid.' not found.'
77-
);
78-
}
79-
8090
$scope = AphrontWriteGuard::beginScopedUnguardedWrites();
81-
$access_token = $server->generateAccessToken($client);
91+
$access_token = $server->generateAccessToken();
8292
if ($access_token) {
8393
$auth_code->delete();
84-
$result = array('access_token' => $access_token->getToken());
94+
$result = array(
95+
'access_token' => $access_token->getToken(),
96+
'token_type' => 'Bearer',
97+
'expires_in' => PhabricatorOAuthServer::ACCESS_TOKEN_TIMEOUT,
98+
);
8599
return $response->setContent($result);
86100
}
87101

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
<?php
2+
3+
/*
4+
* Copyright 2012 Facebook, Inc.
5+
*
6+
* Licensed under the Apache License, Version 2.0 (the "License");
7+
* you may not use this file except in compliance with the License.
8+
* You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing, software
13+
* distributed under the License is distributed on an "AS IS" BASIS,
14+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
* See the License for the specific language governing permissions and
16+
* limitations under the License.
17+
*/
18+
19+
final class PhabricatorOAuthServerScope {
20+
21+
const SCOPE_OFFLINE_ACCESS = 'offline_access';
22+
const SCOPE_WHOAMI = 'whoami';
23+
const SCOPE_NOT_ACCESSIBLE = 'not_accessible';
24+
25+
/*
26+
* Note this does not contain SCOPE_NOT_ACCESSIBLE which is magic
27+
* used to simplify code for data that is not currently accessible
28+
* via OAuth.
29+
*/
30+
static public function getScopesDict() {
31+
return array(
32+
self::SCOPE_OFFLINE_ACCESS => 1,
33+
self::SCOPE_WHOAMI => 1,
34+
);
35+
}
36+
37+
}
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
<?php
2+
/**
3+
* This file is automatically generated. Lint this module to rebuild it.
4+
* @generated
5+
*/
6+
7+
8+
9+
10+
phutil_require_source('PhabricatorOAuthServerScope.php');

0 commit comments

Comments
 (0)