@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.)
hq.recaptime.dev/wiki/Phorge
phorge
phabricator
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
+77
-11
+1
src/__phutil_library_map__.php
+1
src/__phutil_library_map__.php
···
713
713
'PhabricatorOAuthFailureView' => 'applications/auth/view/oauthfailure',
714
714
'PhabricatorOAuthLoginController' => 'applications/auth/controller/oauth',
715
715
'PhabricatorOAuthProvider' => 'applications/auth/oauth/provider/base',
716
+
'PhabricatorOAuthProviderException' => 'applications/auth/oauth/provider/exception',
716
717
'PhabricatorOAuthProviderFacebook' => 'applications/auth/oauth/provider/facebook',
717
718
'PhabricatorOAuthProviderGitHub' => 'applications/auth/oauth/provider/github',
718
719
'PhabricatorOAuthProviderGoogle' => 'applications/auth/oauth/provider/google',
+6
-2
src/applications/auth/controller/oauth/PhabricatorOAuthLoginController.php
+6
-2
src/applications/auth/controller/oauth/PhabricatorOAuthLoginController.php
···
64
64
'access_token' => $this->accessToken,
65
65
));
66
66
67
-
$user_data = @file_get_contents($userinfo_uri);
68
-
$provider->setUserData($user_data);
67
+
try {
68
+
$user_data = @file_get_contents($userinfo_uri);
69
+
$provider->setUserData($user_data);
70
+
} catch (PhabricatorOAuthProviderException $e) {
71
+
return $this->buildErrorResponse(new PhabricatorOAuthFailureView());
72
+
}
69
73
$provider->setAccessToken($this->accessToken);
70
74
71
75
$user_id = $provider->retrieveUserID();
+13
src/applications/auth/oauth/provider/base/PhabricatorOAuthProvider.php
+13
src/applications/auth/oauth/provider/base/PhabricatorOAuthProvider.php
···
98
98
return $this->accessToken;
99
99
}
100
100
101
+
/**
102
+
* Often used within setUserData to make sure $data is not completely
103
+
* junk. More granular validations of data might be necessary depending on
104
+
* the provider and are generally encouraged.
105
+
*/
106
+
final protected function validateUserData($data) {
107
+
if (empty($data) || !is_array($data)) {
108
+
throw new PhabricatorOAuthProviderException();
109
+
}
110
+
111
+
return true;
112
+
}
113
+
101
114
public static function newProvider($which) {
102
115
switch ($which) {
103
116
case self::PROVIDER_FACEBOOK:
+1
src/applications/auth/oauth/provider/base/__init__.php
+1
src/applications/auth/oauth/provider/base/__init__.php
+20
src/applications/auth/oauth/provider/exception/PhabricatorOAuthProviderException.php
+20
src/applications/auth/oauth/provider/exception/PhabricatorOAuthProviderException.php
···
1
+
<?php
2
+
3
+
/*
4
+
* Copyright 2012 Facebook, Inc.
5
+
*
6
+
* Licensed under the Apache License, Version 2.0 (the "License");
7
+
* you may not use this file except in compliance with the License.
8
+
* You may obtain a copy of the License at
9
+
*
10
+
* http://www.apache.org/licenses/LICENSE-2.0
11
+
*
12
+
* Unless required by applicable law or agreed to in writing, software
13
+
* distributed under the License is distributed on an "AS IS" BASIS,
14
+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15
+
* See the License for the specific language governing permissions and
16
+
* limitations under the License.
17
+
*/
18
+
19
+
final class PhabricatorOAuthProviderException extends Exception {
20
+
}
+10
src/applications/auth/oauth/provider/exception/__init__.php
+10
src/applications/auth/oauth/provider/exception/__init__.php
+3
-1
src/applications/auth/oauth/provider/facebook/PhabricatorOAuthProviderFacebook.php
+3
-1
src/applications/auth/oauth/provider/facebook/PhabricatorOAuthProviderFacebook.php
+3
-1
src/applications/auth/oauth/provider/github/PhabricatorOAuthProviderGitHub.php
+3
-1
src/applications/auth/oauth/provider/github/PhabricatorOAuthProviderGitHub.php
+8
-1
src/applications/auth/oauth/provider/google/PhabricatorOAuthProviderGoogle.php
+8
-1
src/applications/auth/oauth/provider/google/PhabricatorOAuthProviderGoogle.php
···
87
87
}
88
88
89
89
public function setUserData($data) {
90
-
$xml = new SimpleXMLElement($data);
90
+
// SimpleXMLElement will throw if $data is unusably malformed, which to
91
+
// us is just a provider issue
92
+
try {
93
+
$xml = new SimpleXMLElement($data);
94
+
} catch (Exception $e) {
95
+
throw new PhabricatorOAuthProviderException();
96
+
}
97
+
91
98
$id = (string)$xml->id;
92
99
$this->userData = array(
93
100
'id' => $id,
+1
src/applications/auth/oauth/provider/google/__init__.php
+1
src/applications/auth/oauth/provider/google/__init__.php
···
7
7
8
8
9
9
phutil_require_module('phabricator', 'applications/auth/oauth/provider/base');
10
+
phutil_require_module('phabricator', 'applications/auth/oauth/provider/exception');
10
11
phutil_require_module('phabricator', 'infrastructure/env');
11
12
12
13
phutil_require_module('phutil', 'utils');
+3
-5
src/applications/auth/oauth/provider/phabricator/PhabricatorOAuthProviderPhabricator.php
+3
-5
src/applications/auth/oauth/provider/phabricator/PhabricatorOAuthProviderPhabricator.php
···
102
102
public function setUserData($data) {
103
103
// need to strip the javascript shield from conduit
104
104
$data = substr($data, 8);
105
-
$data = json_decode($data, true);
106
-
if (!is_array($data)) {
107
-
throw new Exception('Invalid user data.');
108
-
}
109
-
$this->userData = $data['result'];
105
+
$data = idx(json_decode($data, true), 'result');
106
+
$this->validateUserData($data);
107
+
$this->userData = $data;
110
108
return $this;
111
109
}
112
110
+2
src/applications/auth/oauth/provider/phabricator/__init__.php
+2
src/applications/auth/oauth/provider/phabricator/__init__.php
+6
-1
src/applications/people/controller/settings/panels/oauth/PhabricatorUserOAuthSettingsPanelController.php
+6
-1
src/applications/people/controller/settings/panels/oauth/PhabricatorUserOAuthSettingsPanelController.php
···
226
226
$error = 'Unable to retrieve image.';
227
227
}
228
228
} catch (Exception $e) {
229
-
$error = 'Unable to save image.';
229
+
if ($e instanceof PhabricatorOAuthProviderException) {
230
+
$error = sprintf('Unable to retrieve image from %s',
231
+
$provider->getProviderName());
232
+
} else {
233
+
$error = 'Unable to save image.';
234
+
}
230
235
}
231
236
$notice = new AphrontErrorView();
232
237
if ($error) {