From 8111dc74bf89085931ebf7d05493b9825482475c Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 14 Jun 2013 07:04:41 -0700 Subject: [PATCH] Migrate the OAuthInfo table to the ExternalAccount table Summary: Ref T1536. Migrates the OAuthInfo table to ExternalAccount, and makes `PhabricatorUserOAuthInfo` a wrapper for an ExternalAccount. Test Plan: Logged in with OAuth, registered with OAuth, linked/unlinked OAuth accounts, checked OAuth status screen, deleted an account with related OAuth. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T1536 Differential Revision: https://secure.phabricator.com/D6172 --- .../sql/patches/20130611.migrateoauth.php | 66 ++++++++ src/__phutil_library_map__.php | 1 - .../PhabricatorOAuthLoginController.php | 13 +- ...atorOAuthDefaultRegistrationController.php | 1 - .../people/editor/PhabricatorUserEditor.php | 7 - .../storage/PhabricatorUserOAuthInfo.php | 152 +++++++++++++++--- .../patch/PhabricatorBuiltinPatchList.php | 4 + 7 files changed, 205 insertions(+), 39 deletions(-) create mode 100644 resources/sql/patches/20130611.migrateoauth.php diff --git a/resources/sql/patches/20130611.migrateoauth.php b/resources/sql/patches/20130611.migrateoauth.php new file mode 100644 index 0000000000..468ee78ff4 --- /dev/null +++ b/resources/sql/patches/20130611.migrateoauth.php @@ -0,0 +1,66 @@ +establishConnection('w'); + +$xaccount = new PhabricatorExternalAccount(); + +echo "Migrating OAuth to ExternalAccount...\n"; + +$domain_map = array( + 'disqus' => 'disqus.com', + 'facebook' => 'facebook.com', + 'github' => 'github.com', + 'google' => 'google.com', +); + +try { + $phabricator_oauth_uri = new PhutilURI( + PhabricatorEnv::getEnvConfig('phabricator.oauth-uri')); + $domain_map['phabricator'] = $phabricator_oauth_uri->getDomain(); +} catch (Exception $ex) { + // Ignore; this likely indicates that we have removed `phabricator.oauth-uri` + // in some future diff. +} + +$rows = queryfx_all( + $conn_w, + 'SELECT * FROM user_oauthinfo'); +foreach ($rows as $row) { + echo "Migrating row ID #".$row['id'].".\n"; + $user = id(new PhabricatorUser())->loadOneWhere( + 'id = %d', + $row['userID']); + if (!$user) { + echo "Bad user ID!\n"; + continue; + } + + $domain = idx($domain_map, $row['oauthProvider']); + if (empty($domain)) { + echo "Unknown OAuth provider!\n"; + continue; + } + + + $xaccount = id(new PhabricatorExternalAccount()) + ->setUserPHID($user->getPHID()) + ->setAccountType($row['oauthProvider']) + ->setAccountDomain($domain) + ->setAccountID($row['oauthUID']) + ->setAccountURI($row['accountURI']) + ->setUsername($row['accountName']) + ->setDateCreated($row['dateCreated']); + + try { + $xaccount->save(); + } catch (Exception $ex) { + phlog($ex); + } +} + +echo "Done.\n"; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index bdd0a5af9e..d81b7bec6c 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3405,7 +3405,6 @@ phutil_register_library_map(array( 'PhabricatorUserEmail' => 'PhabricatorUserDAO', 'PhabricatorUserLDAPInfo' => 'PhabricatorUserDAO', 'PhabricatorUserLog' => 'PhabricatorUserDAO', - 'PhabricatorUserOAuthInfo' => 'PhabricatorUserDAO', 'PhabricatorUserPreferences' => 'PhabricatorUserDAO', 'PhabricatorUserProfile' => 'PhabricatorUserDAO', 'PhabricatorUserProfileEditor' => 'PhabricatorApplicationTransactionEditor', diff --git a/src/applications/auth/controller/PhabricatorOAuthLoginController.php b/src/applications/auth/controller/PhabricatorOAuthLoginController.php index 8604f8dfe6..0feea708b8 100644 --- a/src/applications/auth/controller/PhabricatorOAuthLoginController.php +++ b/src/applications/auth/controller/PhabricatorOAuthLoginController.php @@ -120,7 +120,6 @@ final class PhabricatorOAuthLoginController $provider_name))); $dialog->addHiddenInput('confirm_token', $provider->getAccessToken()); $dialog->addHiddenInput('state', $this->oauthState); - $dialog->addHiddenInput('scope', $oauth_info->getTokenScope()); $dialog->addSubmitButton('Link Accounts'); $dialog->addCancelButton($provider->getSettingsPanelURI()); @@ -284,24 +283,16 @@ final class PhabricatorOAuthLoginController $provider->getProviderKey(), $provider->retrieveUserID()); - $scope = $this->getRequest()->getStr('scope'); - if (!$oauth_info) { - $oauth_info = new PhabricatorUserOAuthInfo(); + $oauth_info = new PhabricatorUserOAuthInfo( + new PhabricatorExternalAccount()); $oauth_info->setOAuthProvider($provider->getProviderKey()); $oauth_info->setOAuthUID($provider->retrieveUserID()); - // some providers don't tell you what scope you got, so default - // to the minimum Phabricator requires rather than assuming no scope - if (!$scope) { - $scope = $provider->getMinimumScope(); - } } $oauth_info->setAccountURI($provider->retrieveUserAccountURI()); $oauth_info->setAccountName($provider->retrieveUserAccountName()); $oauth_info->setToken($provider->getAccessToken()); - $oauth_info->setTokenStatus('unused'); - $oauth_info->setTokenScope($scope); return $oauth_info; } diff --git a/src/applications/auth/controller/oauthregistration/PhabricatorOAuthDefaultRegistrationController.php b/src/applications/auth/controller/oauthregistration/PhabricatorOAuthDefaultRegistrationController.php index 6b81024bf7..7a18362a1a 100644 --- a/src/applications/auth/controller/oauthregistration/PhabricatorOAuthDefaultRegistrationController.php +++ b/src/applications/auth/controller/oauthregistration/PhabricatorOAuthDefaultRegistrationController.php @@ -170,7 +170,6 @@ final class PhabricatorOAuthDefaultRegistrationController $form = new AphrontFormView(); $form ->addHiddenInput('confirm_token', $provider->getAccessToken()) - ->addHiddenInput('expires', $oauth_info->getTokenExpires()) ->addHiddenInput('state', $this->getOAuthState()) ->setUser($request->getUser()) ->setAction($action_path) diff --git a/src/applications/people/editor/PhabricatorUserEditor.php b/src/applications/people/editor/PhabricatorUserEditor.php index 6ed36f0139..29d081bed4 100644 --- a/src/applications/people/editor/PhabricatorUserEditor.php +++ b/src/applications/people/editor/PhabricatorUserEditor.php @@ -310,13 +310,6 @@ final class PhabricatorUserEditor extends PhabricatorEditor { $ldap->delete(); } - $oauths = id(new PhabricatorUserOAuthInfo())->loadAllWhere( - 'userID = %d', - $user->getID()); - foreach ($oauths as $oauth) { - $oauth->delete(); - } - $externals = id(new PhabricatorExternalAccount())->loadAllWhere( 'userPHID = %s', $user->getPHID()); diff --git a/src/applications/people/storage/PhabricatorUserOAuthInfo.php b/src/applications/people/storage/PhabricatorUserOAuthInfo.php index b1efb259d9..25a9192455 100644 --- a/src/applications/people/storage/PhabricatorUserOAuthInfo.php +++ b/src/applications/people/storage/PhabricatorUserOAuthInfo.php @@ -1,46 +1,160 @@ account->getID(); + } - protected $token; - protected $tokenExpires = 0; - protected $tokenScope = ''; - protected $tokenStatus = 'unused'; + public function setToken($token) { + $this->token = $token; + return $this; + } + + public function getToken() { + return $this->token; + } + + public function __construct(PhabricatorExternalAccount $account) { + $this->account = $account; + } + + public function setAccountURI($value) { + $this->account->setAccountURI($value); + return $this; + } + + public function getAccountURI() { + return $this->account->getAccountURI(); + } + + public function setAccountName($account_name) { + $this->account->setUsername($account_name); + return $this; + } + + public function getAccountName() { + return $this->account->getUsername(); + } + + public function setUserID($user_id) { + $user = id(new PhabricatorUser())->loadOneWhere('id = %d', $user_id); + if (!$user) { + throw new Exception("No such user with given ID!"); + } + $this->account->setUserPHID($user->getPHID()); + return $this; + } + + public function getUserID() { + $phid = $this->account->getUserPHID(); + if (!$phid) { + return null; + } + + $user = id(new PhabricatorUser())->loadOneWhere('phid = %s', $phid); + if (!$user) { + throw new Exception("No such user with given PHID!"); + } + + return $user->getID(); + } + + public function setOAuthUID($oauth_uid) { + $this->account->setAccountID($oauth_uid); + return $this; + } + + public function getOAuthUID() { + return $this->account->getAccountID(); + } + + public function setOAuthProvider($oauth_provider) { + $domain = self::getDomainForProvider($oauth_provider); + $this->account->setAccountType($oauth_provider); + $this->account->setAccountDomain($domain); + + return $this; + } + + public function getOAuthProvider() { + return $this->account->getAccountType(); + } public static function loadOneByUserAndProviderKey( PhabricatorUser $user, $provider_key) { - return id(new PhabricatorUserOAuthInfo())->loadOneWhere( - 'userID = %d AND oauthProvider = %s', - $user->getID(), - $provider_key); + $account = id(new PhabricatorExternalAccount())->loadOneWhere( + 'userPHID = %s AND accountType = %s AND accountDomain = %s', + $user->getPHID(), + $provider_key, + self::getDomainForProvider($provider_key)); + + if (!$account) { + return null; + } + + return new PhabricatorUserOAuthInfo($account); } public static function loadAllOAuthProvidersByUser( PhabricatorUser $user) { - return id(new PhabricatorUserOAuthInfo())->loadAllWhere( - 'userID = %d', - $user->getID()); + $accounts = id(new PhabricatorExternalAccount())->loadAllWhere( + 'userPHID = %s', + $user->getPHID()); + + $results = array(); + foreach ($accounts as $account) { + $results[] = new PhabricatorUserOAuthInfo($account); + } + + return $results; } public static function loadOneByProviderKeyAndAccountID( $provider_key, $account_id) { - return id(new PhabricatorUserOAuthInfo())->loadOneWhere( - 'oauthProvider = %s and oauthUID = %s', + $account = id(new PhabricatorExternalAccount())->loadOneWhere( + 'accountType = %s AND accountDomain = %s AND accountID = %s', $provider_key, + self::getDomainForProvider($provider_key), $account_id); + + if (!$account) { + return null; + } + + return new PhabricatorUserOAuthInfo($account); } + public function save() { + $this->account->save(); + return $this; + } + + private static function getDomainForProvider($provider_key) { + $domain_map = array( + 'disqus' => 'disqus.com', + 'facebook' => 'facebook.com', + 'github' => 'github.com', + 'google' => 'google.com', + ); + + try { + $phabricator_oauth_uri = new PhutilURI( + PhabricatorEnv::getEnvConfig('phabricator.oauth-uri')); + $domain_map['phabricator'] = $phabricator_oauth_uri->getDomain(); + } catch (Exception $ex) { + // Ignore. + } + + return idx($domain_map, $provider_key); + } } diff --git a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php index eb677ba048..d635d8ce47 100644 --- a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php +++ b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php @@ -1358,6 +1358,10 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList { 'type' => 'sql', 'name' => $this->getPatchPath('20130607.xaccount.sql'), ), + '20130611.migrateoauth.php' => array( + 'type' => 'php', + 'name' => $this->getPatchPath('20130611.migrateoauth.php'), + ), ); } }