From 8988667dccee1f0ffe044f30d4ef8297d94d57a0 Mon Sep 17 00:00:00 2001 From: Bob Trahan Date: Tue, 1 May 2012 11:51:40 +0200 Subject: [PATCH] Make Oauth-registration flows a bit more resilient to failures from the providers Summary: basically by validating we have good user data when we set the user data. Test Plan: simulated a failure from a phabricator on phabricator oauth scenario. viewed ui that correctly told me there was an error with the provider and to try again. Reviewers: epriestley Reviewed By: epriestley CC: aran, Koolvin Maniphest Tasks: T1077 Differential Revision: https://secure.phabricator.com/D2337 --- src/__phutil_library_map__.php | 1 + .../oauth/PhabricatorOAuthLoginController.php | 8 ++++++-- .../base/PhabricatorOAuthProvider.php | 13 ++++++++++++ .../auth/oauth/provider/base/__init__.php | 1 + .../PhabricatorOAuthProviderException.php | 20 +++++++++++++++++++ .../oauth/provider/exception/__init__.php | 10 ++++++++++ .../PhabricatorOAuthProviderFacebook.php | 4 +++- .../github/PhabricatorOAuthProviderGitHub.php | 4 +++- .../google/PhabricatorOAuthProviderGoogle.php | 9 ++++++++- .../auth/oauth/provider/google/__init__.php | 1 + .../PhabricatorOAuthProviderPhabricator.php | 8 +++----- .../oauth/provider/phabricator/__init__.php | 2 ++ ...icatorUserOAuthSettingsPanelController.php | 7 ++++++- 13 files changed, 77 insertions(+), 11 deletions(-) create mode 100644 src/applications/auth/oauth/provider/exception/PhabricatorOAuthProviderException.php create mode 100644 src/applications/auth/oauth/provider/exception/__init__.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 7f46fddf6f..da2e6b243f 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -713,6 +713,7 @@ phutil_register_library_map(array( 'PhabricatorOAuthFailureView' => 'applications/auth/view/oauthfailure', 'PhabricatorOAuthLoginController' => 'applications/auth/controller/oauth', 'PhabricatorOAuthProvider' => 'applications/auth/oauth/provider/base', + 'PhabricatorOAuthProviderException' => 'applications/auth/oauth/provider/exception', 'PhabricatorOAuthProviderFacebook' => 'applications/auth/oauth/provider/facebook', 'PhabricatorOAuthProviderGitHub' => 'applications/auth/oauth/provider/github', 'PhabricatorOAuthProviderGoogle' => 'applications/auth/oauth/provider/google', diff --git a/src/applications/auth/controller/oauth/PhabricatorOAuthLoginController.php b/src/applications/auth/controller/oauth/PhabricatorOAuthLoginController.php index 21697101e7..8c07c25bed 100644 --- a/src/applications/auth/controller/oauth/PhabricatorOAuthLoginController.php +++ b/src/applications/auth/controller/oauth/PhabricatorOAuthLoginController.php @@ -64,8 +64,12 @@ final class PhabricatorOAuthLoginController 'access_token' => $this->accessToken, )); - $user_data = @file_get_contents($userinfo_uri); - $provider->setUserData($user_data); + try { + $user_data = @file_get_contents($userinfo_uri); + $provider->setUserData($user_data); + } catch (PhabricatorOAuthProviderException $e) { + return $this->buildErrorResponse(new PhabricatorOAuthFailureView()); + } $provider->setAccessToken($this->accessToken); $user_id = $provider->retrieveUserID(); diff --git a/src/applications/auth/oauth/provider/base/PhabricatorOAuthProvider.php b/src/applications/auth/oauth/provider/base/PhabricatorOAuthProvider.php index 1c6f8b3913..d7d0263d5e 100644 --- a/src/applications/auth/oauth/provider/base/PhabricatorOAuthProvider.php +++ b/src/applications/auth/oauth/provider/base/PhabricatorOAuthProvider.php @@ -98,6 +98,19 @@ abstract class PhabricatorOAuthProvider { return $this->accessToken; } + /** + * Often used within setUserData to make sure $data is not completely + * junk. More granular validations of data might be necessary depending on + * the provider and are generally encouraged. + */ + final protected function validateUserData($data) { + if (empty($data) || !is_array($data)) { + throw new PhabricatorOAuthProviderException(); + } + + return true; + } + public static function newProvider($which) { switch ($which) { case self::PROVIDER_FACEBOOK: diff --git a/src/applications/auth/oauth/provider/base/__init__.php b/src/applications/auth/oauth/provider/base/__init__.php index fcc5fa3984..68a2a7894b 100644 --- a/src/applications/auth/oauth/provider/base/__init__.php +++ b/src/applications/auth/oauth/provider/base/__init__.php @@ -6,6 +6,7 @@ +phutil_require_module('phabricator', 'applications/auth/oauth/provider/exception'); phutil_require_module('phabricator', 'infrastructure/env'); phutil_require_module('phutil', 'symbols'); diff --git a/src/applications/auth/oauth/provider/exception/PhabricatorOAuthProviderException.php b/src/applications/auth/oauth/provider/exception/PhabricatorOAuthProviderException.php new file mode 100644 index 0000000000..f3d75c4fb5 --- /dev/null +++ b/src/applications/auth/oauth/provider/exception/PhabricatorOAuthProviderException.php @@ -0,0 +1,20 @@ +userData = json_decode($data, true); + $data = json_decode($data, true); + $this->validateUserData($data); + $this->userData = $data; return $this; } diff --git a/src/applications/auth/oauth/provider/github/PhabricatorOAuthProviderGitHub.php b/src/applications/auth/oauth/provider/github/PhabricatorOAuthProviderGitHub.php index a0532a5fe2..6369fdc7c1 100644 --- a/src/applications/auth/oauth/provider/github/PhabricatorOAuthProviderGitHub.php +++ b/src/applications/auth/oauth/provider/github/PhabricatorOAuthProviderGitHub.php @@ -79,7 +79,9 @@ final class PhabricatorOAuthProviderGitHub extends PhabricatorOAuthProvider { } public function setUserData($data) { - $this->userData = idx(json_decode($data, true), 'user'); + $data = idx(json_decode($data, true), 'user'); + $this->validateUserData($data); + $this->userData = $data; return $this; } diff --git a/src/applications/auth/oauth/provider/google/PhabricatorOAuthProviderGoogle.php b/src/applications/auth/oauth/provider/google/PhabricatorOAuthProviderGoogle.php index 5fb3e4e6c1..65ef259234 100644 --- a/src/applications/auth/oauth/provider/google/PhabricatorOAuthProviderGoogle.php +++ b/src/applications/auth/oauth/provider/google/PhabricatorOAuthProviderGoogle.php @@ -87,7 +87,14 @@ final class PhabricatorOAuthProviderGoogle extends PhabricatorOAuthProvider { } public function setUserData($data) { - $xml = new SimpleXMLElement($data); + // SimpleXMLElement will throw if $data is unusably malformed, which to + // us is just a provider issue + try { + $xml = new SimpleXMLElement($data); + } catch (Exception $e) { + throw new PhabricatorOAuthProviderException(); + } + $id = (string)$xml->id; $this->userData = array( 'id' => $id, diff --git a/src/applications/auth/oauth/provider/google/__init__.php b/src/applications/auth/oauth/provider/google/__init__.php index 92eeccc3bc..8d9c08d931 100644 --- a/src/applications/auth/oauth/provider/google/__init__.php +++ b/src/applications/auth/oauth/provider/google/__init__.php @@ -7,6 +7,7 @@ phutil_require_module('phabricator', 'applications/auth/oauth/provider/base'); +phutil_require_module('phabricator', 'applications/auth/oauth/provider/exception'); phutil_require_module('phabricator', 'infrastructure/env'); phutil_require_module('phutil', 'utils'); diff --git a/src/applications/auth/oauth/provider/phabricator/PhabricatorOAuthProviderPhabricator.php b/src/applications/auth/oauth/provider/phabricator/PhabricatorOAuthProviderPhabricator.php index d5954d27e3..bd0dc222c0 100644 --- a/src/applications/auth/oauth/provider/phabricator/PhabricatorOAuthProviderPhabricator.php +++ b/src/applications/auth/oauth/provider/phabricator/PhabricatorOAuthProviderPhabricator.php @@ -102,11 +102,9 @@ extends PhabricatorOAuthProvider { public function setUserData($data) { // need to strip the javascript shield from conduit $data = substr($data, 8); - $data = json_decode($data, true); - if (!is_array($data)) { - throw new Exception('Invalid user data.'); - } - $this->userData = $data['result']; + $data = idx(json_decode($data, true), 'result'); + $this->validateUserData($data); + $this->userData = $data; return $this; } diff --git a/src/applications/auth/oauth/provider/phabricator/__init__.php b/src/applications/auth/oauth/provider/phabricator/__init__.php index 323a785603..df9d7b6135 100644 --- a/src/applications/auth/oauth/provider/phabricator/__init__.php +++ b/src/applications/auth/oauth/provider/phabricator/__init__.php @@ -9,5 +9,7 @@ phutil_require_module('phabricator', 'applications/auth/oauth/provider/base'); phutil_require_module('phabricator', 'infrastructure/env'); +phutil_require_module('phutil', 'utils'); + phutil_require_source('PhabricatorOAuthProviderPhabricator.php'); diff --git a/src/applications/people/controller/settings/panels/oauth/PhabricatorUserOAuthSettingsPanelController.php b/src/applications/people/controller/settings/panels/oauth/PhabricatorUserOAuthSettingsPanelController.php index d3c0ce4b54..37502edbfc 100644 --- a/src/applications/people/controller/settings/panels/oauth/PhabricatorUserOAuthSettingsPanelController.php +++ b/src/applications/people/controller/settings/panels/oauth/PhabricatorUserOAuthSettingsPanelController.php @@ -226,7 +226,12 @@ final class PhabricatorUserOAuthSettingsPanelController $error = 'Unable to retrieve image.'; } } catch (Exception $e) { - $error = 'Unable to save image.'; + if ($e instanceof PhabricatorOAuthProviderException) { + $error = sprintf('Unable to retrieve image from %s', + $provider->getProviderName()); + } else { + $error = 'Unable to save image.'; + } } $notice = new AphrontErrorView(); if ($error) {