Page MenuHomePhabricator
Files F15461640

D15593.id.diff
No OneTemporary
Actions

D15593.id.diff
View Options

diff --git a/src/applications/conduit/call/ConduitCall.php b/src/applications/conduit/call/ConduitCall.php
--- a/src/applications/conduit/call/ConduitCall.php
+++ b/src/applications/conduit/call/ConduitCall.php
@@ -65,10 +65,6 @@
return $this->handler->shouldAllowUnguardedWrites();
}
- public function getRequiredScope() {
- return $this->handler->getRequiredScope();
- }
-
public function getErrorDescription($code) {
return $this->handler->getErrorDescription($code);
}
diff --git a/src/applications/conduit/controller/PhabricatorConduitAPIController.php b/src/applications/conduit/controller/PhabricatorConduitAPIController.php
--- a/src/applications/conduit/controller/PhabricatorConduitAPIController.php
+++ b/src/applications/conduit/controller/PhabricatorConduitAPIController.php
@@ -45,7 +45,6 @@
$auth_error = null;
$conduit_username = '-';
if ($call->shouldRequireAuthentication()) {
- $metadata['scope'] = $call->getRequiredScope();
$auth_error = $this->authenticateUser($api_request, $metadata, $method);
// If we've explicitly authenticated the user here and either done
// CSRF validation or are using a non-web authentication mechanism.
@@ -185,11 +184,6 @@
// First, verify the signature.
try {
$protocol_data = $metadata;
-
- // TODO: We should stop writing this into the protocol data when
- // processing a request.
- unset($protocol_data['scope']);
-
ConduitClient::verifySignature(
$method,
$api_request->getAllParameters(),
@@ -362,11 +356,8 @@
$user);
}
- // handle oauth
$access_token = idx($metadata, 'access_token');
- $method_scope = idx($metadata, 'scope');
- if ($access_token &&
- $method_scope != PhabricatorOAuthServerScope::SCOPE_NOT_ACCESSIBLE) {
+ if ($access_token) {
$token = id(new PhabricatorOAuthServerAccessToken())
->loadOneWhere('token = %s', $access_token);
if (!$token) {
@@ -377,25 +368,33 @@
}
$oauth_server = new PhabricatorOAuthServer();
- $valid = $oauth_server->validateAccessToken($token,
- $method_scope);
- if (!$valid) {
+ $authorization = $oauth_server->authorizeToken($token);
+ if (!$authorization) {
return array(
'ERR-INVALID-AUTH',
- pht('Access token is invalid.'),
+ pht('Access token is invalid or expired.'),
);
}
- // valid token, so let's log in the user!
- $user_phid = $token->getUserPHID();
- $user = id(new PhabricatorUser())
- ->loadOneWhere('phid = %s', $user_phid);
+ $user = id(new PhabricatorPeopleQuery())
+ ->setViewer(PhabricatorUser::getOmnipotentUser())
+ ->withPHIDs(array($token->getUserPHID()))
+ ->executeOne();
if (!$user) {
return array(
'ERR-INVALID-AUTH',
pht('Access token is for invalid user.'),
);
}
+
+ $ok = $this->authorizeOAuthMethodAccess($authorization, $method);
+ if (!$ok) {
+ return array(
+ 'ERR-OAUTH-ACCESS',
+ pht('You do not have authorization to call this method.'),
+ );
+ }
+
return $this->validateAuthenticatedUser(
$api_request,
$user);
@@ -642,4 +641,30 @@
return array($metadata, $params);
}
+ private function authorizeOAuthMethodAccess(
+ PhabricatorOAuthClientAuthorization $authorization,
+ $method_name) {
+
+ $method = ConduitAPIMethod::getConduitMethod($method_name);
+ if (!$method) {
+ return false;
+ }
+
+ $required_scope = $method->getRequiredScope();
+ switch ($required_scope) {
+ case ConduitAPIMethod::SCOPE_ALWAYS:
+ return true;
+ case ConduitAPIMethod::SCOPE_NEVER:
+ return false;
+ }
+
+ $authorization_scope = $authorization->getScope();
+ if (!empty($authorization_scope[$required_scope])) {
+ return true;
+ }
+
+ return false;
+ }
+
+
}
diff --git a/src/applications/conduit/controller/PhabricatorConduitConsoleController.php b/src/applications/conduit/controller/PhabricatorConduitConsoleController.php
--- a/src/applications/conduit/controller/PhabricatorConduitConsoleController.php
+++ b/src/applications/conduit/controller/PhabricatorConduitConsoleController.php
@@ -142,6 +142,36 @@
pht('Errors'),
$error_description);
+
+ $scope = $method->getRequiredScope();
+ switch ($scope) {
+ case ConduitAPIMethod::SCOPE_ALWAYS:
+ $oauth_icon = 'fa-globe green';
+ $oauth_description = pht(
+ 'OAuth clients may always call this method.');
+ break;
+ case ConduitAPIMethod::SCOPE_NEVER:
+ $oauth_icon = 'fa-ban red';
+ $oauth_description = pht(
+ 'OAuth clients may never call this method.');
+ break;
+ default:
+ $oauth_icon = 'fa-unlock-alt blue';
+ $oauth_description = pht(
+ 'OAuth clients may call this method after requesting access to '.
+ 'the "%s" scope.',
+ $scope);
+ break;
+ }
+
+ $view->addProperty(
+ pht('OAuth Scope'),
+ array(
+ id(new PHUIIconView())->setIcon($oauth_icon),
+ ' ',
+ $oauth_description,
+ ));
+
$view->addSectionHeader(
pht('Description'), PHUIPropertyListView::ICON_SUMMARY);
$view->addTextContent(
diff --git a/src/applications/conduit/method/ConduitAPIMethod.php b/src/applications/conduit/method/ConduitAPIMethod.php
--- a/src/applications/conduit/method/ConduitAPIMethod.php
+++ b/src/applications/conduit/method/ConduitAPIMethod.php
@@ -15,6 +15,8 @@
const METHOD_STATUS_UNSTABLE = 'unstable';
const METHOD_STATUS_DEPRECATED = 'deprecated';
+ const SCOPE_NEVER = 'scope.never';
+ const SCOPE_ALWAYS = 'scope.always';
/**
* Get a short, human-readable text summary of the method.
@@ -108,8 +110,7 @@
}
public function getRequiredScope() {
- // by default, conduit methods are not accessible via OAuth
- return PhabricatorOAuthServerScope::SCOPE_NOT_ACCESSIBLE;
+ return self::SCOPE_NEVER;
}
public function executeMethod(ConduitAPIRequest $request) {
diff --git a/src/applications/conduit/method/ConduitGetCapabilitiesConduitAPIMethod.php b/src/applications/conduit/method/ConduitGetCapabilitiesConduitAPIMethod.php
--- a/src/applications/conduit/method/ConduitGetCapabilitiesConduitAPIMethod.php
+++ b/src/applications/conduit/method/ConduitGetCapabilitiesConduitAPIMethod.php
@@ -24,6 +24,10 @@
return 'dict<string, any>';
}
+ public function getRequiredScope() {
+ return self::SCOPE_ALWAYS;
+ }
+
protected function execute(ConduitAPIRequest $request) {
$authentication = array(
'token',
diff --git a/src/applications/conduit/method/ConduitQueryConduitAPIMethod.php b/src/applications/conduit/method/ConduitQueryConduitAPIMethod.php
--- a/src/applications/conduit/method/ConduitQueryConduitAPIMethod.php
+++ b/src/applications/conduit/method/ConduitQueryConduitAPIMethod.php
@@ -18,6 +18,10 @@
return 'dict<dict>';
}
+ public function getRequiredScope() {
+ return self::SCOPE_ALWAYS;
+ }
+
protected function execute(ConduitAPIRequest $request) {
$methods = id(new PhabricatorConduitMethodQuery())
->setViewer($request->getUser())
diff --git a/src/applications/oauthserver/PhabricatorOAuthServer.php b/src/applications/oauthserver/PhabricatorOAuthServer.php
--- a/src/applications/oauthserver/PhabricatorOAuthServer.php
+++ b/src/applications/oauthserver/PhabricatorOAuthServer.php
@@ -29,7 +29,6 @@
final class PhabricatorOAuthServer extends Phobject {
const AUTHORIZATION_CODE_TIMEOUT = 300;
- const ACCESS_TOKEN_TIMEOUT = 3600;
private $user;
private $client;
@@ -158,39 +157,35 @@
/**
* @task token
*/
- public function validateAccessToken(
- PhabricatorOAuthServerAccessToken $token,
- $required_scope) {
-
- $created_time = $token->getDateCreated();
- $must_be_used_by = $created_time + self::ACCESS_TOKEN_TIMEOUT;
- $expired = time() > $must_be_used_by;
- $authorization = id(new PhabricatorOAuthClientAuthorization())
- ->loadOneWhere(
- 'userPHID = %s AND clientPHID = %s',
- $token->getUserPHID(),
- $token->getClientPHID());
+ public function authorizeToken(
+ PhabricatorOAuthServerAccessToken $token) {
+ $user_phid = $token->getUserPHID();
+ $client_phid = $token->getClientPHID();
+
+ $authorization = id(new PhabricatorOAuthClientAuthorizationQuery())
+ ->setViewer(PhabricatorUser::getOmnipotentUser())
+ ->withUserPHIDs(array($user_phid))
+ ->withClientPHIDs(array($client_phid))
+ ->executeOne();
if (!$authorization) {
- return false;
- }
- $token_scope = $authorization->getScope();
- if (!isset($token_scope[$required_scope])) {
- return false;
+ return null;
}
- $valid = true;
- if ($expired) {
- $valid = false;
- // check if the scope includes "offline_access", which makes the
- // token valid despite being expired
- if (isset(
- $token_scope[PhabricatorOAuthServerScope::SCOPE_OFFLINE_ACCESS])) {
- $valid = true;
+ // TODO: This should probably be reworked; expiration should be an
+ // exclusive property of the token. For now, this logic reads: tokens for
+ // authorizations with "offline_access" never expire.
+
+ $is_expired = $token->isExpired();
+ if ($is_expired) {
+ $offline_access = PhabricatorOAuthServerScope::SCOPE_OFFLINE_ACCESS;
+ $authorization_scope = $authorization->getScope();
+ if (empty($authorization_scope[$offline_access])) {
+ return null;
}
}
- return $valid;
+ return $authorization;
}
/**
diff --git a/src/applications/oauthserver/PhabricatorOAuthServerScope.php b/src/applications/oauthserver/PhabricatorOAuthServerScope.php
--- a/src/applications/oauthserver/PhabricatorOAuthServerScope.php
+++ b/src/applications/oauthserver/PhabricatorOAuthServerScope.php
@@ -4,13 +4,7 @@
const SCOPE_OFFLINE_ACCESS = 'offline_access';
const SCOPE_WHOAMI = 'whoami';
- const SCOPE_NOT_ACCESSIBLE = 'not_accessible';
- /*
- * Note this does not contain SCOPE_NOT_ACCESSIBLE which is magic
- * used to simplify code for data that is not currently accessible
- * via OAuth.
- */
public static function getScopesDict() {
return array(
self::SCOPE_OFFLINE_ACCESS => 1,
diff --git a/src/applications/oauthserver/controller/PhabricatorOAuthServerTokenController.php b/src/applications/oauthserver/controller/PhabricatorOAuthServerTokenController.php
--- a/src/applications/oauthserver/controller/PhabricatorOAuthServerTokenController.php
+++ b/src/applications/oauthserver/controller/PhabricatorOAuthServerTokenController.php
@@ -144,7 +144,7 @@
$result = array(
'access_token' => $access_token->getToken(),
'token_type' => 'Bearer',
- 'expires_in' => PhabricatorOAuthServer::ACCESS_TOKEN_TIMEOUT,
+ 'expires_in' => $access_token->getExpiresDuration(),
);
return $response->setContent($result);
} catch (Exception $e) {
diff --git a/src/applications/oauthserver/query/PhabricatorOAuthClientAuthorizationQuery.php b/src/applications/oauthserver/query/PhabricatorOAuthClientAuthorizationQuery.php
--- a/src/applications/oauthserver/query/PhabricatorOAuthClientAuthorizationQuery.php
+++ b/src/applications/oauthserver/query/PhabricatorOAuthClientAuthorizationQuery.php
@@ -7,7 +7,7 @@
private $userPHIDs;
private $clientPHIDs;
- public function witHPHIDs(array $phids) {
+ public function withPHIDs(array $phids) {
$this->phids = $phids;
return $this;
}
@@ -22,19 +22,12 @@
return $this;
}
+ public function newResultObject() {
+ return new PhabricatorOAuthClientAuthorization();
+ }
+
protected function loadPage() {
- $table = new PhabricatorOAuthClientAuthorization();
- $conn_r = $table->establishConnection('r');
-
- $data = queryfx_all(
- $conn_r,
- 'SELECT * FROM %T auth %Q %Q %Q',
- $table->getTableName(),
- $this->buildWhereClause($conn_r),
- $this->buildOrderClause($conn_r),
- $this->buildLimitClause($conn_r));
-
- return $table->loadAllFromArray($data);
+ return $this->loadStandardPage($this->newResultObject());
}
protected function willFilterPage(array $authorizations) {
@@ -49,43 +42,44 @@
foreach ($authorizations as $key => $authorization) {
$client = idx($clients, $authorization->getClientPHID());
+
if (!$client) {
+ $this->didRejectResult($authorization);
unset($authorizations[$key]);
continue;
}
+
$authorization->attachClient($client);
}
return $authorizations;
}
- protected function buildWhereClause(AphrontDatabaseConnection $conn_r) {
- $where = array();
+ protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) {
+ $where = parent::buildWhereClauseParts($conn);
- if ($this->phids) {
+ if ($this->phids !== null) {
$where[] = qsprintf(
- $conn_r,
+ $conn,
'phid IN (%Ls)',
$this->phids);
}
- if ($this->userPHIDs) {
+ if ($this->userPHIDs !== null) {
$where[] = qsprintf(
- $conn_r,
+ $conn,
'userPHID IN (%Ls)',
$this->userPHIDs);
}
- if ($this->clientPHIDs) {
+ if ($this->clientPHIDs !== null) {
$where[] = qsprintf(
- $conn_r,
+ $conn,
'clientPHID IN (%Ls)',
$this->clientPHIDs);
}
- $where[] = $this->buildPagingClause($conn_r);
-
- return $this->formatWhereClause($where);
+ return $where;
}
public function getQueryApplicationClass() {
diff --git a/src/applications/oauthserver/storage/PhabricatorOAuthServerAccessToken.php b/src/applications/oauthserver/storage/PhabricatorOAuthServerAccessToken.php
--- a/src/applications/oauthserver/storage/PhabricatorOAuthServerAccessToken.php
+++ b/src/applications/oauthserver/storage/PhabricatorOAuthServerAccessToken.php
@@ -22,4 +22,18 @@
) + parent::getConfiguration();
}
+ public function isExpired() {
+ $now = PhabricatorTime::getNow();
+ $expires_epoch = $this->getExpiresEpoch();
+ return ($now > $expires_epoch);
+ }
+
+ public function getExpiresEpoch() {
+ return $this->getDateCreated() + 3600;
+ }
+
+ public function getExpiresDuration() {
+ return PhabricatorTime::getNow() - $this->getExpiresEpoch();
+ }
+
}

File Metadata

Mime Type
text/plain
Expires
Wed, Apr 2, 9:46 AM (1 w, 3 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7223436
Default Alt Text
D15593.id.diff (14 KB)

Event Timeline

Phabricator · Built by Phacility