Skip to content

Commit 0327a5f

Browse files
bobtrahanbobtrahan
committed
OAuthServer polish and random sauce
Summary: This diff makes the OAuthServer more compliant with the spec by - making it return well-formatted error codes with error types from the spec. - making it respect the "state" variable, which is a transparent variable the client passes and the server passes back - making it be super, duper compliant with respect to redirect uris -- if specified in authorization step, check if its valid relative to the client registered URI and if so save it -- if specified in authorization step, check if its been specified in the access step and error if it doesn't match or doesn't exist -- note we don't make any use of it in the access step which seems strange but hey, that's what the spec says! This diff makes the OAuthServer suck less by - making the "cancel" button do something in the user authorization flow - making the client list view and client edit view be a bit more usable around client secrets - fixing a few bugs I managed to introduce along the way Test Plan: - create a test phabricator client, updated my conf, and then linked and unlinked phabricator to itself - wrote some tests for PhabricatorOAuthServer -- they pass! -- these validate the various validate URI checks - tried a few important authorization calls -- http://phabricator.dev/oauthserver/auth/?client_id=X&state=test&redirect_uri=http://www.evil.com --- verified error'd from mismatching redirect uri's --- verified state parameter in response --- verified did not redirect to client redirect uri -- http://phabricator.dev/oauthserver/auth/?client_id=X w/ existing authorization --- got redirected to proper client url with error that response_type not specified -- http://phabricator.dev/oauthserver/auth/?client_id=X&response_type=code w/ existing authorization --- got redirected to proper client url with pertinent code! - tried a few important access calls -- verified appropriate errors if missing any required parameters -- verified good access code with appropriate other variables resulted in an access token - verified that if redirect_uri set correctly in authorization required for access and errors if differs at all / only succeeds if exactly the same Reviewers: epriestley Reviewed By: epriestley CC: aran, epriestley, ajtrichards Maniphest Tasks: T889, T906, T897 Differential Revision: https://secure.phabricator.com/D1727
1 parent e9dedb0 commit 0327a5f

19 files changed

+532
-167
lines changed

‎resources/sql/patches/112.oauthaccesscoderedirecturi.sql

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
ALTER TABLE `phabricator_oauth_server`.`oauth_server_oauthserverauthorizationcode`
2+
ADD `redirectURI` varchar(255) NOT NULL
3+

‎src/__phutil_library_map__.php

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -645,6 +645,7 @@
645645
'PhabricatorOAuthServerController' => 'applications/oauthserver/controller/base',
646646
'PhabricatorOAuthServerDAO' => 'applications/oauthserver/storage/base',
647647
'PhabricatorOAuthServerScope' => 'applications/oauthserver/scope',
648+
'PhabricatorOAuthServerTestCase' => 'applications/oauthserver/server/__tests__',
648649
'PhabricatorOAuthServerTestController' => 'applications/oauthserver/controller/test',
649650
'PhabricatorOAuthServerTokenController' => 'applications/oauthserver/controller/token',
650651
'PhabricatorOAuthUnlinkController' => 'applications/auth/controller/unlink',
@@ -1408,6 +1409,7 @@
14081409
'PhabricatorOAuthServerClient' => 'PhabricatorOAuthServerDAO',
14091410
'PhabricatorOAuthServerController' => 'PhabricatorController',
14101411
'PhabricatorOAuthServerDAO' => 'PhabricatorLiskDAO',
1412+
'PhabricatorOAuthServerTestCase' => 'PhabricatorTestCase',
14111413
'PhabricatorOAuthServerTestController' => 'PhabricatorOAuthServerController',
14121414
'PhabricatorOAuthServerTokenController' => 'PhabricatorAuthController',
14131415
'PhabricatorOAuthUnlinkController' => 'PhabricatorAuthController',

‎src/aphront/response/base/AphrontResponse.php

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,9 @@ public function getCacheHeaders() {
114114
$headers[] = array(
115115
'Cache-Control',
116116
'private, no-cache, no-store, must-revalidate');
117+
$headers[] = array(
118+
'Pragma',
119+
'no-cache');
117120
$headers[] = array(
118121
'Expires',
119122
'Sat, 01 Jan 2000 00:00:00 GMT');

‎src/applications/auth/oauth/provider/phabricator/PhabricatorOAuthProviderPhabricator.php

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,20 @@
1818

1919
final class PhabricatorOAuthProviderPhabricator
2020
extends PhabricatorOAuthProvider {
21-
2221
private $userData;
2322

23+
public function getExtraAuthParameters() {
24+
return array(
25+
'response_type' => 'code',
26+
);
27+
}
28+
29+
public function getExtraTokenParameters() {
30+
return array(
31+
'grant_type' => 'authorization_code',
32+
);
33+
34+
}
2435
public function decodeTokenResponse($response) {
2536
$decoded = json_decode($response, true);
2637
if (!is_array($decoded)) {
@@ -85,7 +96,7 @@ public function getUserInfoURI() {
8596
}
8697

8798
public function getMinimumScope() {
88-
return 'email';
99+
return 'whoami';
89100
}
90101

91102
public function setUserData($data) {

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

Lines changed: 142 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -27,78 +27,135 @@ public function shouldRequireLogin() {
2727
}
2828

2929
public function processRequest() {
30-
$request = $this->getRequest();
31-
$current_user = $request->getUser();
32-
$server = new PhabricatorOAuthServer($current_user);
33-
$client_phid = $request->getStr('client_id');
34-
$scope = $request->getStr('scope');
35-
$redirect_uri = $request->getStr('redirect_uri');
36-
$response = new PhabricatorOAuthResponse();
37-
$errors = array();
30+
$request = $this->getRequest();
31+
$current_user = $request->getUser();
32+
$server = new PhabricatorOAuthServer();
33+
$client_phid = $request->getStr('client_id');
34+
$scope = $request->getStr('scope');
35+
$redirect_uri = $request->getStr('redirect_uri');
36+
$state = $request->getStr('state');
37+
$response_type = $request->getStr('response_type');
38+
$response = new PhabricatorOAuthResponse();
3839

40+
// state is an opaque value the client sent us for their own purposes
41+
// we just need to send it right back to them in the response!
42+
if ($state) {
43+
$response->setState($state);
44+
}
3945
if (!$client_phid) {
40-
return $response->setMalformed(
46+
$response->setError('invalid_request');
47+
$response->setErrorDescription(
4148
'Required parameter client_id not specified.'
4249
);
50+
return $response;
4351
}
44-
$client = id(new PhabricatorOAuthServerClient())
45-
->loadOneWhere('phid = %s', $client_phid);
46-
if (!$client) {
47-
return $response->setNotFound(
48-
'Client with id '.$client_phid.' not found. '
49-
);
50-
}
51-
52-
$server->setClient($client);
53-
if ($server->userHasAuthorizedClient()) {
54-
$return_auth_code = true;
55-
$unguarded_write = AphrontWriteGuard::beginScopedUnguardedWrites();
56-
} else if ($request->isFormPost()) {
57-
$scope = PhabricatorOAuthServerScope::getScopesFromRequest($request);
58-
$server->authorizeClient($scope);
59-
$return_auth_code = true;
60-
$unguarded_write = null;
61-
} else {
62-
$return_auth_code = false;
63-
$unguarded_write = null;
64-
}
65-
66-
if ($return_auth_code) {
67-
// step 1 -- generate authorization code
68-
$auth_code =
69-
$server->generateAuthorizationCode();
52+
$server->setUser($current_user);
7053

71-
// step 2 -- error or return it
72-
if (!$auth_code) {
73-
$errors[] = 'Failed to generate an authorization code. '.
74-
'Try again.';
54+
// one giant try / catch around all the exciting database stuff so we
55+
// can return a 'server_error' response if something goes wrong!
56+
try {
57+
$client = id(new PhabricatorOAuthServerClient())
58+
->loadOneWhere('phid = %s', $client_phid);
59+
if (!$client) {
60+
$response->setError('invalid_request');
61+
$response->setErrorDescription(
62+
'Client with id '.$client_phid.' not found.'
63+
);
64+
return $response;
65+
}
66+
$server->setClient($client);
67+
if ($redirect_uri) {
68+
$client_uri = new PhutilURI($client->getRedirectURI());
69+
$redirect_uri = new PhutilURI($redirect_uri);
70+
if (!($server->validateSecondaryRedirectURI($redirect_uri,
71+
$client_uri))) {
72+
$response->setError('invalid_request');
73+
$response->setErrorDescription(
74+
'The specified redirect URI is invalid. The redirect URI '.
75+
'must be a fully-qualified domain with no fragments and '.
76+
'must have the same domain and at least the same query '.
77+
'parameters as the redirect URI the client registered.'
78+
);
79+
return $response;
80+
}
81+
$uri = $redirect_uri;
82+
$access_token_uri = $uri;
7583
} else {
76-
$client_uri = new PhutilURI($client->getRedirectURI());
77-
if (!$redirect_uri) {
78-
$uri = $client_uri;
79-
} else {
80-
$redirect_uri = new PhutilURI($redirect_uri);
81-
if ($redirect_uri->getDomain() !=
82-
$client_uri->getDomain()) {
83-
$uri = $client_uri;
84-
} else {
85-
$uri = $redirect_uri;
86-
}
84+
$uri = new PhutilURI($client->getRedirectURI());
85+
$access_token_uri = null;
86+
}
87+
// we've now validated this request enough overall such that we
88+
// can safely redirect to the client with the response
89+
$response->setClientURI($uri);
90+
91+
if (empty($response_type)) {
92+
$response->setError('invalid_request');
93+
$response->setErrorDescription(
94+
'Required parameter response_type not specified.'
95+
);
96+
return $response;
97+
}
98+
if ($response_type != 'code') {
99+
$response->setError('unsupported_response_type');
100+
$response->setErrorDescription(
101+
'The authorization server does not support obtaining an '.
102+
'authorization code using the specified response_type. '.
103+
'You must specify the response_type as "code".'
104+
);
105+
return $response;
106+
}
107+
if ($scope) {
108+
if (!PhabricatorOAuthServerScope::validateScopesList($scope)) {
109+
$response->setError('invalid_scope');
110+
$response->setErrorDescription(
111+
'The requested scope is invalid, unknown, or malformed.'
112+
);
113+
return $response;
87114
}
115+
$scope = PhabricatorOAuthServerScope::scopesListToDict($scope);
116+
}
88117

89-
$uri->setQueryParam('code', $auth_code->getCode());
90-
return $response->setRedirect($uri);
118+
$authorization = $server->userHasAuthorizedClient($scope);
119+
if ($authorization) {
120+
$return_auth_code = true;
121+
$unguarded_write = AphrontWriteGuard::beginScopedUnguardedWrites();
122+
} else if ($request->isFormPost()) {
123+
$scope = PhabricatorOAuthServerScope::getScopesFromRequest($request);
124+
$authorization = $server->authorizeClient($scope);
125+
$return_auth_code = true;
126+
$unguarded_write = null;
127+
} else {
128+
$return_auth_code = false;
129+
$unguarded_write = null;
91130
}
92-
}
93-
unset($unguarded_write);
94131

95-
$error_view = null;
96-
if ($errors) {
97-
$error_view = new AphrontErrorView();
98-
$error_view->setTitle('Authorization Code Errors');
99-
$error_view->setErrors($errors);
132+
if ($return_auth_code) {
133+
// step 1 -- generate authorization code
134+
$auth_code =
135+
$server->generateAuthorizationCode($access_token_uri);
136+
137+
// step 2 return it
138+
$content = array(
139+
'code' => $auth_code->getCode(),
140+
'scope' => $authorization->getScopeString(),
141+
);
142+
$response->setContent($content);
143+
return $response->setClientURI($uri);
144+
}
145+
unset($unguarded_write);
146+
} catch (Exception $e) {
147+
// Note we could try harder to determine between a server_error
148+
// vs temporarily_unavailable. Good enough though.
149+
$response->setError('server_error');
150+
$response->setErrorDescription(
151+
'The authorization server encountered an unexpected condition '.
152+
'which prevented it from fulfilling the request. '
153+
);
154+
return $response;
100155
}
101156

157+
// display time -- make a nice form for the user to grant the client
158+
// access to the granularity specified by $scope
102159
$name = phutil_escape_html($client->getName());
103160
$title = 'Authorize ' . $name . '?';
104161
$panel = new AphrontPanelView();
@@ -109,10 +166,30 @@ public function processRequest() {
109166
"Do want to authorize {$name} to access your ".
110167
"Phabricator account data?";
111168

112-
$desired_scopes = array(
113-
PhabricatorOAuthServerScope::SCOPE_WHOAMI => 1,
114-
PhabricatorOAuthServerScope::SCOPE_OFFLINE_ACCESS => 1
169+
if ($scope) {
170+
$desired_scopes = $scope;
171+
if (!PhabricatorOAuthServerScope::validateScopesDict($desired_scopes)) {
172+
$response->setError('invalid_scope');
173+
$response->setErrorDescription(
174+
'The requested scope is invalid, unknown, or malformed.'
175+
);
176+
return $response;
177+
}
178+
} else {
179+
$desired_scopes = array(
180+
PhabricatorOAuthServerScope::SCOPE_WHOAMI => 1,
181+
PhabricatorOAuthServerScope::SCOPE_OFFLINE_ACCESS => 1
182+
);
183+
}
184+
185+
$cancel_uri = $this->getClientURI($client, $redirect_uri);
186+
$cancel_params = array(
187+
'error' => 'access_denied',
188+
'error_description' =>
189+
'The resource owner (aka the user) denied the request.'
115190
);
191+
$cancel_uri->setQueryParams($cancel_params);
192+
116193
$form = id(new AphrontFormView())
117194
->setUser($current_user)
118195
->appendChild(
@@ -125,15 +202,14 @@ public function processRequest() {
125202
->appendChild(
126203
id(new AphrontFormSubmitControl())
127204
->setValue('Authorize')
128-
->addCancelButton('/')
205+
->addCancelButton($cancel_uri)
129206
);
130-
// TODO -- T889 (make "cancel" do something more sensible)
131207

132208
$panel->appendChild($form);
133209

134210
return $this->buildStandardPageResponse(
135-
array($error_view,
136-
$panel),
211+
$panel,
137212
array('title' => $title));
138213
}
214+
139215
}

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@
1515
phutil_require_module('phabricator', 'view/form/base');
1616
phutil_require_module('phabricator', 'view/form/control/static');
1717
phutil_require_module('phabricator', 'view/form/control/submit');
18-
phutil_require_module('phabricator', 'view/form/error');
1918
phutil_require_module('phabricator', 'view/layout/panel');
2019

2120
phutil_require_module('phutil', 'markup');

‎src/applications/oauthserver/controller/client/edit/PhabricatorOAuthClientEditController.php

Lines changed: 14 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -78,25 +78,33 @@ public function processRequest() {
7878
->setForbiddenText($message);
7979
}
8080
$submit_button = 'Save OAuth Client';
81+
$secret = null;
8182
// new client - much simpler
8283
} else {
83-
$client = new PhabricatorOAuthServerClient();
84-
$title = 'Create OAuth Client';
84+
$client = new PhabricatorOAuthServerClient();
85+
$title = 'Create OAuth Client';
8586
$submit_button = 'Create OAuth Client';
87+
$secret = Filesystem::readRandomCharacters(32);
8688
}
8789

8890
if ($request->isFormPost()) {
8991
$redirect_uri = $request->getStr('redirect_uri');
9092
$client->setName($request->getStr('name'));
9193
$client->setRedirectURI($redirect_uri);
92-
$client->setSecret(Filesystem::readRandomCharacters(32));
94+
if ($secret) {
95+
$client->setSecret($secret);
96+
}
9397
$client->setCreatorPHID($current_user->getPHID());
9498
$uri = new PhutilURI($redirect_uri);
95-
if (!$this->validateRedirectURI($uri)) {
99+
$server = new PhabricatorOAuthServer();
100+
if (!$server->validateRedirectURI($uri)) {
96101
$error = new AphrontErrorView();
97102
$error->setSeverity(AphrontErrorView::SEVERITY_ERROR);
98103
$error->setTitle(
99-
'Redirect URI must be a fully qualified domain name.'
104+
'Redirect URI must be a fully qualified domain name '.
105+
'with no fragments. See '.
106+
'http://tools.ietf.org/html/draft-ietf-oauth-v2-23#section-3.1.2 '.
107+
'for more information on the correct format.'
100108
);
101109
$bad_redirect = true;
102110
} else {
@@ -140,7 +148,7 @@ public function processRequest() {
140148
->setValue($phid)
141149
)
142150
->appendChild(
143-
id(new AphrontFormTextControl())
151+
id(new AphrontFormStaticControl())
144152
->setLabel('Secret')
145153
->setValue($client->getSecret())
146154
);
@@ -185,13 +193,4 @@ public function processRequest() {
185193
);
186194
}
187195

188-
private function validateRedirectURI(PhutilURI $uri) {
189-
if (PhabricatorEnv::isValidRemoteWebResource($uri)) {
190-
if ($uri->getDomain()) {
191-
return true;
192-
}
193-
}
194-
return false;
195-
}
196-
197196
}

‎src/applications/oauthserver/controller/client/edit/__init__.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,8 @@
1010
phutil_require_module('phabricator', 'aphront/response/404');
1111
phutil_require_module('phabricator', 'aphront/response/redirect');
1212
phutil_require_module('phabricator', 'applications/oauthserver/controller/client/base');
13+
phutil_require_module('phabricator', 'applications/oauthserver/server');
1314
phutil_require_module('phabricator', 'applications/oauthserver/storage/client');
14-
phutil_require_module('phabricator', 'infrastructure/env');
1515
phutil_require_module('phabricator', 'view/form/base');
1616
phutil_require_module('phabricator', 'view/form/control/static');
1717
phutil_require_module('phabricator', 'view/form/control/submit');

0 commit comments

Comments
 (0)