Skip to content

Commit 34c890b

Browse files
author
epriestley
committed
Use modern UI and policies in OAuth client editing
Summary: Updates this stuff a bit: - Add a global create permission for OAuth applications. The primary goal is to reduce attack surface area by making it more difficult for an adversary to do anything which requires that they create and configure an OAuth application/client. Normal users shouldn't generally need to create applications, OAuth is complex, and doing things with user accounts is inherently somewhat administrative. - Use normal policies to check create and edit permissions, now that we have infrastructure for it. - Use modern UI kit. Test Plan: - Created a client. - Edited a client. - Tried to create a client as a non-admin. - Tried to edit a client I don't own. {F131511} {F131512} {F131513} {F131514} Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Differential Revision: https://secure.phabricator.com/D8562
1 parent 995a890 commit 34c890b

File tree

7 files changed

+133
-143
lines changed

7 files changed

+133
-143
lines changed

‎src/__phutil_library_map__.php‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1718,6 +1718,7 @@
17181718
'PhabricatorOAuthServerAuthController' => 'applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php',
17191719
'PhabricatorOAuthServerAuthorizationCode' => 'applications/oauthserver/storage/PhabricatorOAuthServerAuthorizationCode.php',
17201720
'PhabricatorOAuthServerAuthorizationsSettingsPanel' => 'applications/oauthserver/panel/PhabricatorOAuthServerAuthorizationsSettingsPanel.php',
1721+
'PhabricatorOAuthServerCapabilityCreateClients' => 'applications/oauthserver/capability/PhabricatorOAuthServerCapabilityCreateClients.php',
17211722
'PhabricatorOAuthServerClient' => 'applications/oauthserver/storage/PhabricatorOAuthServerClient.php',
17221723
'PhabricatorOAuthServerClientQuery' => 'applications/oauthserver/query/PhabricatorOAuthServerClientQuery.php',
17231724
'PhabricatorOAuthServerConsoleController' => 'applications/oauthserver/controller/PhabricatorOAuthServerConsoleController.php',
@@ -4471,6 +4472,7 @@
44714472
'PhabricatorOAuthServerAuthController' => 'PhabricatorAuthController',
44724473
'PhabricatorOAuthServerAuthorizationCode' => 'PhabricatorOAuthServerDAO',
44734474
'PhabricatorOAuthServerAuthorizationsSettingsPanel' => 'PhabricatorSettingsPanel',
4475+
'PhabricatorOAuthServerCapabilityCreateClients' => 'PhabricatorPolicyCapability',
44744476
'PhabricatorOAuthServerClient' =>
44754477
array(
44764478
0 => 'PhabricatorOAuthServerDAO',

‎src/applications/oauthserver/application/PhabricatorApplicationOAuthServer.php‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,4 +48,12 @@ public function getRoutes() {
4848
);
4949
}
5050

51+
protected function getCustomCapabilities() {
52+
return array(
53+
PhabricatorOAuthServerCapabilityCreateClients::CAPABILITY => array(
54+
'default' => PhabricatorPolicies::POLICY_ADMIN,
55+
),
56+
);
57+
}
58+
5159
}

‎src/applications/oauthserver/capability/PhabricatorOAuthServerCapabilityCreateClients.php‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
<?php
2+
3+
final class PhabricatorOAuthServerCapabilityCreateClients
4+
extends PhabricatorPolicyCapability {
5+
6+
const CAPABILITY = 'oauthserver.create';
7+
8+
public function getCapabilityKey() {
9+
return self::CAPABILITY;
10+
}
11+
12+
public function getCapabilityName() {
13+
return pht('Can Create OAuth Applications');
14+
}
15+
16+
public function describeCapabilityRejection() {
17+
return pht('You do not have permission to create OAuth applications.');
18+
}
19+
20+
}

‎src/applications/oauthserver/controller/PhabricatorOAuthServerController.php‎

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

33
abstract class PhabricatorOAuthServerController
4-
extends PhabricatorController {
4+
extends PhabricatorController {
55

66
public function buildStandardPageResponse($view, array $data) {
77
$user = $this->getRequest()->getUser();

‎src/applications/oauthserver/controller/client/PhabricatorOAuthClientBaseController.php‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,14 @@
11
<?php
22

3-
/**
4-
* @group oauthserver
5-
*/
63
abstract class PhabricatorOAuthClientBaseController
7-
extends PhabricatorOAuthServerController {
4+
extends PhabricatorOAuthServerController {
85

96
private $clientPHID;
7+
108
protected function getClientPHID() {
119
return $this->clientPHID;
1210
}
11+
1312
private function setClientPHID($phid) {
1413
$this->clientPHID = $phid;
1514
return $this;

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

Lines changed: 82 additions & 138 deletions
Original file line numberDiff line numberDiff line change
@@ -1,173 +1,117 @@
11
<?php
22

3-
/**
4-
* @group oauthserver
5-
*/
63
final class PhabricatorOAuthClientEditController
7-
extends PhabricatorOAuthClientBaseController {
8-
9-
private $isEdit;
10-
protected function isClientEdit() {
11-
return $this->isEdit;
12-
}
13-
private function setIsClientEdit($is_edit) {
14-
$this->isEdit = (bool) $is_edit;
15-
return $this;
16-
}
17-
18-
protected function getExtraClientFilters() {
19-
if ($this->isClientEdit()) {
20-
$filters = array(
21-
array('url' => $this->getFilter(),
22-
'label' => 'Edit Client')
23-
);
24-
} else {
25-
$filters = array();
26-
}
27-
return $filters;
28-
}
29-
30-
public function getFilter() {
31-
if ($this->isClientEdit()) {
32-
$filter = 'client/edit/'.$this->getClientPHID();
33-
} else {
34-
$filter = 'client/create';
35-
}
36-
return $filter;
37-
}
4+
extends PhabricatorOAuthClientBaseController {
385

396
public function processRequest() {
40-
$request = $this->getRequest();
41-
$current_user = $request->getUser();
42-
$error = null;
43-
$bad_redirect = false;
44-
$phid = $this->getClientPHID();
45-
// if we have a phid, then we're editing
46-
$this->setIsClientEdit($phid);
7+
$request = $this->getRequest();
8+
$viewer = $request->getUser();
479

48-
if ($this->isClientEdit()) {
49-
$client = id(new PhabricatorOAuthServerClient())
50-
->loadOneWhere('phid = %s',
51-
$phid);
52-
$title = 'Edit OAuth Client';
53-
// validate the client
54-
if (empty($client)) {
10+
$phid = $this->getClientPHID();
11+
if ($phid) {
12+
$client = id(new PhabricatorOAuthServerClientQuery())
13+
->setViewer($viewer)
14+
->withPHIDs(array($phid))
15+
->requireCapabilities(
16+
array(
17+
PhabricatorPolicyCapability::CAN_VIEW,
18+
PhabricatorPolicyCapability::CAN_EDIT,
19+
))
20+
->executeOne();
21+
if (!$client) {
5522
return new Aphront404Response();
5623
}
57-
if ($client->getCreatorPHID() != $current_user->getPHID()) {
58-
$message = 'Access denied to edit client with id '.$phid.'. '.
59-
'Only the user who created the client has permission to '.
60-
'edit the client.';
61-
return id(new Aphront403Response())
62-
->setForbiddenText($message);
63-
}
64-
$submit_button = 'Save OAuth Client';
65-
$secret = null;
66-
// new client - much simpler
24+
25+
$title = pht('Edit OAuth Application: %s', $client->getName());
26+
$submit_button = pht('Save Application');
27+
$crumb_text = pht('Edit');
28+
$cancel_uri = $client->getViewURI();
29+
$is_new = false;
6730
} else {
68-
$client = new PhabricatorOAuthServerClient();
69-
$title = 'Create OAuth Client';
70-
$submit_button = 'Create OAuth Client';
71-
$secret = Filesystem::readRandomCharacters(32);
31+
$this->requireApplicationCapability(
32+
PhabricatorOAuthServerCapabilityCreateClients::CAPABILITY);
33+
34+
$client = PhabricatorOAuthServerClient::initializeNewClient($viewer);
35+
36+
$title = pht('Create OAuth Application');
37+
$submit_button = pht('Create Application');
38+
$crumb_text = pht('Create Application');
39+
$cancel_uri = $this->getApplicationURI();
40+
$is_new = true;
7241
}
7342

43+
$errors = array();
44+
$e_redirect = true;
45+
$e_name = true;
7446
if ($request->isFormPost()) {
7547
$redirect_uri = $request->getStr('redirect_uri');
7648
$client->setName($request->getStr('name'));
7749
$client->setRedirectURI($redirect_uri);
78-
if ($secret) {
79-
$client->setSecret($secret);
50+
51+
if (!strlen($client->getName())) {
52+
$errors[] = pht('You must choose a name for this OAuth application.');
53+
$e_name = pht('Required');
8054
}
81-
$client->setCreatorPHID($current_user->getPHID());
82-
$uri = new PhutilURI($redirect_uri);
55+
8356
$server = new PhabricatorOAuthServer();
57+
$uri = new PhutilURI($redirect_uri);
8458
if (!$server->validateRedirectURI($uri)) {
85-
$error = new AphrontErrorView();
86-
$error->setSeverity(AphrontErrorView::SEVERITY_ERROR);
87-
$error->setTitle(
59+
$errors[] = pht(
8860
'Redirect URI must be a fully qualified domain name '.
89-
'with no fragments. See '.
90-
'http://tools.ietf.org/html/draft-ietf-oauth-v2-23#section-3.1.2 '.
91-
'for more information on the correct format.');
92-
$bad_redirect = true;
93-
} else {
94-
$client->save();
95-
// refresh the phid in case its a create
96-
$phid = $client->getPHID();
97-
if ($this->isClientEdit()) {
98-
return id(new AphrontRedirectResponse())
99-
->setURI('/oauthserver/client/?edited='.$phid);
100-
} else {
101-
return id(new AphrontRedirectResponse())
102-
->setURI('/oauthserver/client/?new='.$phid);
103-
}
61+
'with no fragments. See %s for more information on the correct '.
62+
'format.',
63+
'http://tools.ietf.org/html/draft-ietf-oauth-v2-23#section-3.1.2');
64+
$e_redirect = pht('Invalid');
10465
}
105-
}
10666

107-
$panel = new AphrontPanelView();
108-
if ($this->isClientEdit()) {
109-
$delete_button = phutil_tag(
110-
'a',
111-
array(
112-
'href' => $client->getDeleteURI(),
113-
'class' => 'grey button',
114-
),
115-
'Delete OAuth Client');
116-
$panel->addButton($delete_button);
67+
if (!$errors) {
68+
$client->save();
69+
$view_uri = $client->getViewURI();
70+
return id(new AphrontRedirectResponse())->setURI($view_uri);
71+
}
11772
}
118-
$panel->setHeader($title);
11973

12074
$form = id(new AphrontFormView())
121-
->setUser($current_user)
75+
->setUser($viewer)
12276
->appendChild(
12377
id(new AphrontFormTextControl())
124-
->setLabel('Name')
125-
->setName('name')
126-
->setValue($client->getName()));
127-
if ($this->isClientEdit()) {
128-
$form
129-
->appendChild(
130-
id(new AphrontFormTextControl())
131-
->setLabel('ID')
132-
->setValue($phid))
133-
->appendChild(
134-
id(new AphrontFormStaticControl())
135-
->setLabel('Secret')
136-
->setValue($client->getSecret()));
137-
}
138-
$form
78+
->setLabel('Name')
79+
->setName('name')
80+
->setValue($client->getName())
81+
->setError($e_name))
13982
->appendChild(
14083
id(new AphrontFormTextControl())
141-
->setLabel('Redirect URI')
142-
->setName('redirect_uri')
143-
->setValue($client->getRedirectURI())
144-
->setError($bad_redirect));
145-
if ($this->isClientEdit()) {
146-
$created = phabricator_datetime($client->getDateCreated(),
147-
$current_user);
148-
$updated = phabricator_datetime($client->getDateModified(),
149-
$current_user);
150-
$form
151-
->appendChild(
152-
id(new AphrontFormStaticControl())
153-
->setLabel('Created')
154-
->setValue($created))
155-
->appendChild(
156-
id(new AphrontFormStaticControl())
157-
->setLabel('Last Updated')
158-
->setValue($updated));
159-
}
160-
$form
84+
->setLabel('Redirect URI')
85+
->setName('redirect_uri')
86+
->setValue($client->getRedirectURI())
87+
->setError($e_redirect))
16188
->appendChild(
16289
id(new AphrontFormSubmitControl())
163-
->setValue($submit_button));
90+
->addCancelButton($cancel_uri)
91+
->setValue($submit_button));
92+
93+
$crumbs = $this->buildApplicationCrumbs();
94+
if (!$is_new) {
95+
$crumbs->addTextCrumb(
96+
$client->getName(),
97+
$client->getViewURI());
98+
}
99+
$crumbs->addTextCrumb($crumb_text);
100+
101+
$box = id(new PHUIObjectBoxView())
102+
->setHeaderText($title)
103+
->setFormErrors($errors)
104+
->setForm($form);
164105

165-
$panel->appendChild($form);
166-
return $this->buildStandardPageResponse(
167-
array($error,
168-
$panel
106+
return $this->buildApplicationPage(
107+
array(
108+
$crumbs,
109+
$box,
169110
),
170-
array('title' => $title));
111+
array(
112+
'title' => $title,
113+
'device' => true,
114+
));
171115
}
172116

173117
}

‎src/applications/oauthserver/storage/PhabricatorOAuthServerClient.php‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,12 @@ public function getDeleteURI() {
2121
return '/oauthserver/client/delete/'.$this->getPHID().'/';
2222
}
2323

24+
public static function initializeNewClient(PhabricatorUser $actor) {
25+
return id(new PhabricatorOAuthServerClient())
26+
->setCreatorPHID($actor->getPHID())
27+
->setSecret(Filesystem::readRandomCharacters(32));
28+
}
29+
2430
public function getConfiguration() {
2531
return array(
2632
self::CONFIG_AUX_PHID => true,
@@ -39,21 +45,32 @@ public function generatePHID() {
3945
public function getCapabilities() {
4046
return array(
4147
PhabricatorPolicyCapability::CAN_VIEW,
48+
PhabricatorPolicyCapability::CAN_EDIT,
4249
);
4350
}
4451

4552
public function getPolicy($capability) {
4653
switch ($capability) {
4754
case PhabricatorPolicyCapability::CAN_VIEW:
4855
return PhabricatorPolicies::POLICY_USER;
56+
case PhabricatorPolicyCapability::CAN_EDIT:
57+
return PhabricatorPolicies::POLICY_NOONE;
4958
}
5059
}
5160

5261
public function hasAutomaticCapability($capability, PhabricatorUser $viewer) {
62+
switch ($capability) {
63+
case PhabricatorPolicyCapability::CAN_EDIT:
64+
return ($viewer->getPHID() == $this->getCreatorPHID());
65+
}
5366
return false;
5467
}
5568

5669
public function describeAutomaticCapability($capability) {
70+
switch ($capability) {
71+
case PhabricatorPolicyCapability::CAN_EDIT:
72+
return pht("Only an application's creator can edit it.");
73+
}
5774
return null;
5875
}
5976

0 commit comments

Comments
 (0)