From a837c3d73eaf8089265da097eef185a1148005fc Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 16 Mar 2016 05:17:47 -0700 Subject: [PATCH] Make temporary token storage/schema more flexible Summary: Ref T10603. This makes minor updates to temporary tokens: - Rename `objectPHID` (which is sometimes used to store some other kind of identifier instead of a PHID) to `tokenResource` (i.e., which resource does this token permit access to?). - Add a `userPHID` column. For LFS tokens and some other types of tokens, I want to bind the token to both a resource (like a repository) and a user. - Add a `properties` column. This makes tokens more flexible and supports custom behavior (like scoping LFS tokens even more tightly). Test Plan: - Ran `bin/storage upgrade -f`, got a clean upgrade. - Viewed one-time tokens. - Revoked one token. - Revoked all tokens. - Performed a one-time login. - Performed a password reset. - Added an MFA token. - Removed an MFA token. - Used a file token to view a file. - Verified file token was removed after viewing file. - Linked my account to an OAuth1 account (Twitter). Reviewers: chad Reviewed By: chad Maniphest Tasks: T10603 Differential Revision: https://secure.phabricator.com/D15478 --- .../20160316.lfs.01.token.resource.sql | 2 + .../20160316.lfs.02.token.user.sql | 2 + .../20160316.lfs.03.token.properties.sql | 2 + .../20160316.lfs.04.token.default.sql | 2 + .../PhabricatorAuthOneTimeLoginController.php | 2 +- .../PhabricatorAuthRevokeTokenController.php | 2 +- .../engine/PhabricatorAuthSessionEngine.php | 4 +- .../auth/factor/PhabricatorTOTPAuthFactor.php | 4 +- .../PhabricatorOAuth1AuthProvider.php | 6 +- .../PhabricatorAuthTemporaryTokenQuery.php | 62 ++++++++++--------- .../storage/PhabricatorAuthTemporaryToken.php | 32 +++++++--- .../files/storage/PhabricatorFile.php | 4 +- .../PhabricatorPasswordSettingsPanel.php | 2 +- .../panel/PhabricatorTokensSettingsPanel.php | 2 +- 14 files changed, 79 insertions(+), 49 deletions(-) create mode 100644 resources/sql/autopatches/20160316.lfs.01.token.resource.sql create mode 100644 resources/sql/autopatches/20160316.lfs.02.token.user.sql create mode 100644 resources/sql/autopatches/20160316.lfs.03.token.properties.sql create mode 100644 resources/sql/autopatches/20160316.lfs.04.token.default.sql diff --git a/resources/sql/autopatches/20160316.lfs.01.token.resource.sql b/resources/sql/autopatches/20160316.lfs.01.token.resource.sql new file mode 100644 index 0000000000..7be5bbda54 --- /dev/null +++ b/resources/sql/autopatches/20160316.lfs.01.token.resource.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_auth.auth_temporarytoken + CHANGE objectPHID tokenResource VARBINARY(64) NOT NULL; diff --git a/resources/sql/autopatches/20160316.lfs.02.token.user.sql b/resources/sql/autopatches/20160316.lfs.02.token.user.sql new file mode 100644 index 0000000000..72174d6fe8 --- /dev/null +++ b/resources/sql/autopatches/20160316.lfs.02.token.user.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_auth.auth_temporarytoken + ADD userPHID VARBINARY(64); diff --git a/resources/sql/autopatches/20160316.lfs.03.token.properties.sql b/resources/sql/autopatches/20160316.lfs.03.token.properties.sql new file mode 100644 index 0000000000..2cb4449d73 --- /dev/null +++ b/resources/sql/autopatches/20160316.lfs.03.token.properties.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_auth.auth_temporarytoken + ADD properties LONGTEXT NOT NULL COLLATE {$COLLATE_TEXT}; diff --git a/resources/sql/autopatches/20160316.lfs.04.token.default.sql b/resources/sql/autopatches/20160316.lfs.04.token.default.sql new file mode 100644 index 0000000000..0f0ce4abc4 --- /dev/null +++ b/resources/sql/autopatches/20160316.lfs.04.token.default.sql @@ -0,0 +1,2 @@ +UPDATE {$NAMESPACE}_auth.auth_temporarytoken + SET properties = '{}' WHERE properties = ''; diff --git a/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php b/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php index 9ecea74d6a..d98879d0ed 100644 --- a/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php +++ b/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php @@ -132,7 +132,7 @@ final class PhabricatorAuthOneTimeLoginController $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); id(new PhabricatorAuthTemporaryToken()) - ->setObjectPHID($target_user->getPHID()) + ->setTokenResource($target_user->getPHID()) ->setTokenType($password_type) ->setTokenExpires(time() + phutil_units('1 hour in seconds')) ->setTokenCode(PhabricatorHash::digest($key)) diff --git a/src/applications/auth/controller/PhabricatorAuthRevokeTokenController.php b/src/applications/auth/controller/PhabricatorAuthRevokeTokenController.php index c1f0c21cb1..6d516916eb 100644 --- a/src/applications/auth/controller/PhabricatorAuthRevokeTokenController.php +++ b/src/applications/auth/controller/PhabricatorAuthRevokeTokenController.php @@ -11,7 +11,7 @@ final class PhabricatorAuthRevokeTokenController $query = id(new PhabricatorAuthTemporaryTokenQuery()) ->setViewer($viewer) - ->withObjectPHIDs(array($viewer->getPHID())); + ->withTokenResources(array($viewer->getPHID())); if (!$is_all) { $query->withIDs(array($id)); } diff --git a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php index e6289926dd..98b6a63b5a 100644 --- a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php +++ b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php @@ -635,7 +635,7 @@ final class PhabricatorAuthSessionEngine extends Phobject { $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); id(new PhabricatorAuthTemporaryToken()) - ->setObjectPHID($user->getPHID()) + ->setTokenResource($user->getPHID()) ->setTokenType($onetime_type) ->setTokenExpires(time() + phutil_units('1 day in seconds')) ->setTokenCode($key_hash) @@ -679,7 +679,7 @@ final class PhabricatorAuthSessionEngine extends Phobject { return id(new PhabricatorAuthTemporaryTokenQuery()) ->setViewer($user) - ->withObjectPHIDs(array($user->getPHID())) + ->withTokenResources(array($user->getPHID())) ->withTokenTypes(array($onetime_type)) ->withTokenCodes(array($key_hash)) ->withExpired(false) diff --git a/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php b/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php index 8c29eb7d14..418270de63 100644 --- a/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php +++ b/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php @@ -36,7 +36,7 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor { $temporary_token = id(new PhabricatorAuthTemporaryTokenQuery()) ->setViewer($user) - ->withObjectPHIDs(array($user->getPHID())) + ->withTokenResources(array($user->getPHID())) ->withTokenTypes(array(self::TEMPORARY_TOKEN_TYPE)) ->withExpired(false) ->withTokenCodes(array(PhabricatorHash::digest($key))) @@ -55,7 +55,7 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor { $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); id(new PhabricatorAuthTemporaryToken()) - ->setObjectPHID($user->getPHID()) + ->setTokenResource($user->getPHID()) ->setTokenType(self::TEMPORARY_TOKEN_TYPE) ->setTokenExpires(time() + phutil_units('1 hour in seconds')) ->setTokenCode(PhabricatorHash::digest($key)) diff --git a/src/applications/auth/provider/PhabricatorOAuth1AuthProvider.php b/src/applications/auth/provider/PhabricatorOAuth1AuthProvider.php index a22707b6cc..bc9572061f 100644 --- a/src/applications/auth/provider/PhabricatorOAuth1AuthProvider.php +++ b/src/applications/auth/provider/PhabricatorOAuth1AuthProvider.php @@ -221,7 +221,7 @@ abstract class PhabricatorOAuth1AuthProvider // Wipe out an existing token, if one exists. $token = id(new PhabricatorAuthTemporaryTokenQuery()) ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withObjectPHIDs(array($key)) + ->withTokenResources(array($key)) ->withTokenTypes(array($type)) ->executeOne(); if ($token) { @@ -230,7 +230,7 @@ abstract class PhabricatorOAuth1AuthProvider // Save the new secret. id(new PhabricatorAuthTemporaryToken()) - ->setObjectPHID($key) + ->setTokenResource($key) ->setTokenType($type) ->setTokenExpires(time() + phutil_units('1 hour in seconds')) ->setTokenCode($secret) @@ -243,7 +243,7 @@ abstract class PhabricatorOAuth1AuthProvider $token = id(new PhabricatorAuthTemporaryTokenQuery()) ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withObjectPHIDs(array($key)) + ->withTokenResources(array($key)) ->withTokenTypes(array($type)) ->withExpired(false) ->executeOne(); diff --git a/src/applications/auth/query/PhabricatorAuthTemporaryTokenQuery.php b/src/applications/auth/query/PhabricatorAuthTemporaryTokenQuery.php index aecd92a4a7..72141f75f0 100644 --- a/src/applications/auth/query/PhabricatorAuthTemporaryTokenQuery.php +++ b/src/applications/auth/query/PhabricatorAuthTemporaryTokenQuery.php @@ -4,8 +4,9 @@ final class PhabricatorAuthTemporaryTokenQuery extends PhabricatorCursorPagedPolicyAwareQuery { private $ids; - private $objectPHIDs; + private $tokenResources; private $tokenTypes; + private $userPHIDs; private $expired; private $tokenCodes; @@ -14,8 +15,8 @@ final class PhabricatorAuthTemporaryTokenQuery return $this; } - public function withObjectPHIDs(array $object_phids) { - $this->objectPHIDs = $object_phids; + public function withTokenResources(array $resources) { + $this->tokenResources = $resources; return $this; } @@ -34,41 +35,39 @@ final class PhabricatorAuthTemporaryTokenQuery return $this; } - protected function loadPage() { - $table = new PhabricatorAuthTemporaryToken(); - $conn_r = $table->establishConnection('r'); - - $data = queryfx_all( - $conn_r, - 'SELECT * FROM %T %Q %Q %Q', - $table->getTableName(), - $this->buildWhereClause($conn_r), - $this->buildOrderClause($conn_r), - $this->buildLimitClause($conn_r)); - - return $table->loadAllFromArray($data); + public function withUserPHIDs(array $phids) { + $this->userPHIDs = $phids; + return $this; } - protected function buildWhereClause(AphrontDatabaseConnection $conn_r) { - $where = array(); + public function newResultObject() { + return new PhabricatorAuthTemporaryToken(); + } + + protected function loadPage() { + return $this->loadStandardPage($this->newResultObject()); + } + + protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) { + $where = parent::buildWhereClauseParts($conn); if ($this->ids !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'id IN (%Ld)', $this->ids); } - if ($this->objectPHIDs !== null) { + if ($this->tokenResources !== null) { $where[] = qsprintf( - $conn_r, - 'objectPHID IN (%Ls)', - $this->objectPHIDs); + $conn, + 'tokenResource IN (%Ls)', + $this->tokenResources); } if ($this->tokenTypes !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'tokenType IN (%Ls)', $this->tokenTypes); } @@ -76,12 +75,12 @@ final class PhabricatorAuthTemporaryTokenQuery if ($this->expired !== null) { if ($this->expired) { $where[] = qsprintf( - $conn_r, + $conn, 'tokenExpires <= %d', time()); } else { $where[] = qsprintf( - $conn_r, + $conn, 'tokenExpires > %d', time()); } @@ -89,14 +88,19 @@ final class PhabricatorAuthTemporaryTokenQuery if ($this->tokenCodes !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'tokenCode IN (%Ls)', $this->tokenCodes); } - $where[] = $this->buildPagingClause($conn_r); + if ($this->userPHIDs !== null) { + $where[] = qsprintf( + $conn, + 'userPHID IN (%Ls)', + $this->userPHIDs); + } - return $this->formatWhereClause($where); + return $where; } public function getQueryApplicationClass() { diff --git a/src/applications/auth/storage/PhabricatorAuthTemporaryToken.php b/src/applications/auth/storage/PhabricatorAuthTemporaryToken.php index 93e491bdb0..be08f5db2d 100644 --- a/src/applications/auth/storage/PhabricatorAuthTemporaryToken.php +++ b/src/applications/auth/storage/PhabricatorAuthTemporaryToken.php @@ -3,30 +3,39 @@ final class PhabricatorAuthTemporaryToken extends PhabricatorAuthDAO implements PhabricatorPolicyInterface { - // TODO: OAuth1 stores a client identifier here, which is not a real PHID. - // At some point, we should rename this column to be a little more generic. - protected $objectPHID; - + // NOTE: This is usually a PHID, but may be some other kind of resource + // identifier for some token types. + protected $tokenResource; protected $tokenType; protected $tokenExpires; protected $tokenCode; + protected $userPHID; + protected $properties; protected function getConfiguration() { return array( self::CONFIG_TIMESTAMPS => false, + self::CONFIG_SERIALIZATION => array( + 'properties' => self::SERIALIZATION_JSON, + ), self::CONFIG_COLUMN_SCHEMA => array( + 'tokenResource' => 'phid', 'tokenType' => 'text64', 'tokenExpires' => 'epoch', 'tokenCode' => 'text64', + 'userPHID' => 'phid?', ), self::CONFIG_KEY_SCHEMA => array( 'key_token' => array( - 'columns' => array('objectPHID', 'tokenType', 'tokenCode'), + 'columns' => array('tokenResource', 'tokenType', 'tokenCode'), 'unique' => true, ), 'key_expires' => array( 'columns' => array('tokenExpires'), ), + 'key_user' => array( + 'columns' => array('userPHID'), + ), ), ) + parent::getConfiguration(); } @@ -73,12 +82,12 @@ final class PhabricatorAuthTemporaryToken extends PhabricatorAuthDAO public static function revokeTokens( PhabricatorUser $viewer, - array $object_phids, + array $token_resources, array $token_types) { $tokens = id(new PhabricatorAuthTemporaryTokenQuery()) ->setViewer($viewer) - ->withObjectPHIDs($object_phids) + ->withTokenResources($token_resources) ->withTokenTypes($token_types) ->withExpired(false) ->execute(); @@ -88,6 +97,15 @@ final class PhabricatorAuthTemporaryToken extends PhabricatorAuthDAO } } + public function getTemporaryTokenProperty($key, $default = null) { + return idx($this->properties, $key, $default); + } + + public function setTemporaryTokenProperty($key, $value) { + $this->properties[$key] = $value; + return $this; + } + /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php index 28328796ce..58cfe346c4 100644 --- a/src/applications/files/storage/PhabricatorFile.php +++ b/src/applications/files/storage/PhabricatorFile.php @@ -1123,7 +1123,7 @@ final class PhabricatorFile extends PhabricatorFileDAO // Save the new secret. $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); $token = id(new PhabricatorAuthTemporaryToken()) - ->setObjectPHID($this->getPHID()) + ->setTokenResource($this->getPHID()) ->setTokenType(self::ONETIME_TEMPORARY_TOKEN_TYPE) ->setTokenExpires(time() + phutil_units('1 hour in seconds')) ->setTokenCode(PhabricatorHash::digest($key)) @@ -1136,7 +1136,7 @@ final class PhabricatorFile extends PhabricatorFileDAO public function validateOneTimeToken($token_code) { $token = id(new PhabricatorAuthTemporaryTokenQuery()) ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withObjectPHIDs(array($this->getPHID())) + ->withTokenResources(array($this->getPHID())) ->withTokenTypes(array(self::ONETIME_TEMPORARY_TOKEN_TYPE)) ->withExpired(false) ->withTokenCodes(array(PhabricatorHash::digest($token_code))) diff --git a/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php b/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php index dd26890fc2..f2db373945 100644 --- a/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php +++ b/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php @@ -46,7 +46,7 @@ final class PhabricatorPasswordSettingsPanel extends PhabricatorSettingsPanel { if ($key) { $token = id(new PhabricatorAuthTemporaryTokenQuery()) ->setViewer($user) - ->withObjectPHIDs(array($user->getPHID())) + ->withTokenResources(array($user->getPHID())) ->withTokenTypes(array($password_type)) ->withTokenCodes(array(PhabricatorHash::digest($key))) ->withExpired(false) diff --git a/src/applications/settings/panel/PhabricatorTokensSettingsPanel.php b/src/applications/settings/panel/PhabricatorTokensSettingsPanel.php index a92026333a..030ad37116 100644 --- a/src/applications/settings/panel/PhabricatorTokensSettingsPanel.php +++ b/src/applications/settings/panel/PhabricatorTokensSettingsPanel.php @@ -23,7 +23,7 @@ final class PhabricatorTokensSettingsPanel extends PhabricatorSettingsPanel { $tokens = id(new PhabricatorAuthTemporaryTokenQuery()) ->setViewer($viewer) - ->withObjectPHIDs(array($viewer->getPHID())) + ->withTokenResources(array($viewer->getPHID())) ->execute(); $rows = array();