Skip to content

Commit 1620bce

Browse files
author
epriestley
committed
Add Google as an OAuth2 provider (BETA)
Summary: This is pretty straightforward, except: - We need to request read/write access to the address book to get the account ID (which we MUST have) and real name, email and account name (which we'd like to have). This is way more access than we should need, but there's apparently no "get_loggedin_user_basic_information" type of call in the Google API suite (or, at least, I couldn't find one). - We can't get the profile picture or profile URI since there's no Plus API access and Google users don't have meaningful public pages otherwise. - Google doesn't save the fact that you've authorized the app, so every time you want to login you need to reaffirm that you want to give us silly amounts of access. Phabricator sessions are pretty long-duration though so this shouldn't be a major issue. Test Plan: - Registered, logged out, and logged in with Google. - Registered, logged out, and logged in with Facebook / Github to make sure I didn't break anything. - Linked / unlinked Google accounts. Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran Reviewed By: aran CC: aran, epriestley, Makinde Differential Revision: 916
1 parent 4da43b3 commit 1620bce

File tree

12 files changed

+263
-20
lines changed

12 files changed

+263
-20
lines changed

conf/default.conf.php

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -335,6 +335,24 @@
335335
'github.application-secret' => null,
336336

337337

338+
// -- Google ---------------------------------------------------------------- //
339+
340+
// Can users use Google credentials to login to Phabricator?
341+
'google.auth-enabled' => false,
342+
343+
// Can users use Google credentials to create new Phabricator accounts?
344+
'google.registration-enabled' => true,
345+
346+
// Are Google accounts permanently linked to Phabricator accounts, or can
347+
// the user unlink them?
348+
'google.auth-permanent' => false,
349+
350+
// The Google "Client ID" to use for Google API access.
351+
'google.application-id' => null,
352+
353+
// The Google "Client Secret" to use for Google API access.
354+
'google.application-secret' => null,
355+
338356
// -- Recaptcha ------------------------------------------------------------- //
339357

340358
// Is Recaptcha enabled? If disabled, captchas will not appear.

src/__phutil_library_map__.php

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -486,6 +486,7 @@
486486
'PhabricatorOAuthProvider' => 'applications/auth/oauth/provider/base',
487487
'PhabricatorOAuthProviderFacebook' => 'applications/auth/oauth/provider/facebook',
488488
'PhabricatorOAuthProviderGithub' => 'applications/auth/oauth/provider/github',
489+
'PhabricatorOAuthProviderGoogle' => 'applications/auth/oauth/provider/google',
489490
'PhabricatorOAuthRegistrationController' => 'applications/auth/controller/oauthregistration/base',
490491
'PhabricatorOAuthUnlinkController' => 'applications/auth/controller/unlink',
491492
'PhabricatorObjectGraph' => 'applications/phid/graph',
@@ -1090,6 +1091,7 @@
10901091
'PhabricatorOAuthLoginController' => 'PhabricatorAuthController',
10911092
'PhabricatorOAuthProviderFacebook' => 'PhabricatorOAuthProvider',
10921093
'PhabricatorOAuthProviderGithub' => 'PhabricatorOAuthProvider',
1094+
'PhabricatorOAuthProviderGoogle' => 'PhabricatorOAuthProvider',
10931095
'PhabricatorOAuthRegistrationController' => 'PhabricatorAuthController',
10941096
'PhabricatorOAuthUnlinkController' => 'PhabricatorAuthController',
10951097
'PhabricatorObjectGraph' => 'AbstractDirectedGraph',

src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@ public function getURIMap() {
138138
'/logout/$' => 'PhabricatorLogoutController',
139139

140140
'/oauth/' => array(
141-
'(?P<provider>github|facebook)/' => array(
141+
'(?P<provider>\w+)/' => array(
142142
'login/$' => 'PhabricatorOAuthLoginController',
143143
'diagnose/$' => 'PhabricatorOAuthDiagnosticsController',
144144
'unlink/$' => 'PhabricatorOAuthUnlinkController',

src/applications/auth/controller/login/PhabricatorLoginController.php

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -121,13 +121,8 @@ public function processRequest() {
121121
$forms['Phabricator Login'] = $form;
122122
}
123123

124-
$providers = array(
125-
PhabricatorOAuthProvider::PROVIDER_FACEBOOK,
126-
PhabricatorOAuthProvider::PROVIDER_GITHUB,
127-
);
128-
foreach ($providers as $provider_key) {
129-
$provider = PhabricatorOAuthProvider::newProvider($provider_key);
130-
124+
$providers = PhabricatorOAuthProvider::getAllProviders();
125+
foreach ($providers as $provider) {
131126
$enabled = $provider->isProviderEnabled();
132127
if (!$enabled) {
133128
continue;
@@ -138,6 +133,7 @@ public function processRequest() {
138133
$client_id = $provider->getClientID();
139134
$provider_name = $provider->getProviderName();
140135
$minimum_scope = $provider->getMinimumScope();
136+
$extra_auth = $provider->getExtraAuthParameters();
141137

142138
// TODO: In theory we should use 'state' to prevent CSRF, but the total
143139
// effect of the CSRF attack is that an attacker can cause a user to login
@@ -163,7 +159,13 @@ public function processRequest() {
163159
->setAction($auth_uri)
164160
->addHiddenInput('client_id', $client_id)
165161
->addHiddenInput('redirect_uri', $redirect_uri)
166-
->addHiddenInput('scope', $minimum_scope)
162+
->addHiddenInput('scope', $minimum_scope);
163+
164+
foreach ($extra_auth as $key => $value) {
165+
$auth_form->addHiddenInput($key, $value);
166+
}
167+
168+
$auth_form
167169
->setUser($request->getUser())
168170
->setMethod('GET')
169171
->appendChild(

src/applications/auth/controller/oauth/PhabricatorOAuthLoginController.php

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -63,9 +63,7 @@ public function processRequest() {
6363
'access_token' => $this->accessToken,
6464
));
6565

66-
$user_json = @file_get_contents($userinfo_uri);
67-
$user_data = json_decode($user_json, true);
68-
66+
$user_data = @file_get_contents($userinfo_uri);
6967
$provider->setUserData($user_data);
7068
$provider->setAccessToken($this->accessToken);
7169

@@ -240,7 +238,7 @@ private function retrieveAccessToken(PhabricatorOAuthProvider $provider) {
240238
'client_secret' => $client_secret,
241239
'redirect_uri' => $redirect_uri,
242240
'code' => $code,
243-
);
241+
) + $provider->getExtraTokenParameters();
244242

245243
$post_data = http_build_query($query_data);
246244
$post_length = strlen($post_data);
@@ -270,8 +268,7 @@ private function retrieveAccessToken(PhabricatorOAuthProvider $provider) {
270268
return $this->buildErrorResponse(new PhabricatorOAuthFailureView());
271269
}
272270

273-
$data = array();
274-
parse_str($response, $data);
271+
$data = $provider->decodeTokenResponse($response);
275272

276273
$token = idx($data, 'access_token');
277274
if (!$token) {

src/applications/auth/oauth/provider/base/PhabricatorOAuthProvider.php

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ abstract class PhabricatorOAuthProvider {
2020

2121
const PROVIDER_FACEBOOK = 'facebook';
2222
const PROVIDER_GITHUB = 'github';
23+
const PROVIDER_GOOGLE = 'google';
2324

2425
private $accessToken;
2526

@@ -32,7 +33,25 @@ abstract public function getRedirectURI();
3233
abstract public function getClientID();
3334
abstract public function getClientSecret();
3435
abstract public function getAuthURI();
36+
37+
/**
38+
* If the provider needs extra stuff in the auth request, return it here.
39+
* For example, Google needs a response_type parameter.
40+
*/
41+
public function getExtraAuthParameters() {
42+
return array();
43+
}
44+
3545
abstract public function getTokenURI();
46+
47+
/**
48+
* If the provider needs extra stuff in the token request, return it here.
49+
* For example, Google needs a grant_type parameter.
50+
*/
51+
public function getExtraTokenParameters() {
52+
return array();
53+
}
54+
3655
abstract public function getUserInfoURI();
3756
abstract public function getMinimumScope();
3857

@@ -44,10 +63,21 @@ abstract public function retrieveUserProfileImage();
4463
abstract public function retrieveUserAccountURI();
4564
abstract public function retrieveUserRealName();
4665

66+
/**
67+
* Override this if the provider returns the token response as, e.g., JSON
68+
* or XML.
69+
*/
70+
public function decodeTokenResponse($response) {
71+
$data = null;
72+
parse_str($response, $data);
73+
return $data;
74+
}
75+
4776
public function __construct() {
4877

4978
}
5079

80+
5181
final public function setAccessToken($access_token) {
5282
$this->accessToken = $access_token;
5383
return $this;
@@ -65,6 +95,9 @@ public static function newProvider($which) {
6595
case self::PROVIDER_GITHUB:
6696
$class = 'PhabricatorOAuthProviderGithub';
6797
break;
98+
case self::PROVIDER_GOOGLE:
99+
$class = 'PhabricatorOAuthProviderGoogle';
100+
break;
68101
default:
69102
throw new Exception('Unknown OAuth provider.');
70103
}
@@ -76,6 +109,7 @@ public static function getAllProviders() {
76109
$all = array(
77110
self::PROVIDER_FACEBOOK,
78111
self::PROVIDER_GITHUB,
112+
self::PROVIDER_GOOGLE,
79113
);
80114
$providers = array();
81115
foreach ($all as $provider) {

src/applications/auth/oauth/provider/facebook/PhabricatorOAuthProviderFacebook.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ public function getMinimumScope() {
6969
}
7070

7171
public function setUserData($data) {
72-
$this->userData = $data;
72+
$this->userData = json_decode($data, true);
7373
return $this;
7474
}
7575

src/applications/auth/oauth/provider/github/PhabricatorOAuthProviderGithub.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ public function getMinimumScope() {
6969
}
7070

7171
public function setUserData($data) {
72-
$this->userData = $data['user'];
72+
$this->userData = idx(json_decode($data, true), 'user');
7373
return $this;
7474
}
7575

Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
1+
<?php
2+
3+
/*
4+
* Copyright 2011 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+
class PhabricatorOAuthProviderGoogle extends PhabricatorOAuthProvider {
20+
21+
private $userData;
22+
23+
public function getProviderKey() {
24+
return self::PROVIDER_GOOGLE;
25+
}
26+
27+
public function getProviderName() {
28+
return 'Google (BETA)';
29+
}
30+
31+
public function isProviderEnabled() {
32+
return PhabricatorEnv::getEnvConfig('google.auth-enabled');
33+
}
34+
35+
public function isProviderLinkPermanent() {
36+
return PhabricatorEnv::getEnvConfig('google.auth-permanent');
37+
}
38+
39+
public function isProviderRegistrationEnabled() {
40+
return PhabricatorEnv::getEnvConfig('google.registration-enabled');
41+
}
42+
43+
public function getRedirectURI() {
44+
return PhabricatorEnv::getURI('/oauth/google/login/');
45+
}
46+
47+
public function getClientID() {
48+
return PhabricatorEnv::getEnvConfig('google.application-id');
49+
}
50+
51+
public function getClientSecret() {
52+
return PhabricatorEnv::getEnvConfig('google.application-secret');
53+
}
54+
55+
public function getAuthURI() {
56+
return 'https://accounts.google.com/o/oauth2/auth';
57+
}
58+
59+
public function getTokenURI() {
60+
return 'https://accounts.google.com/o/oauth2/token';
61+
}
62+
63+
public function getUserInfoURI() {
64+
return 'https://www.google.com/m8/feeds/contacts/default/full';
65+
}
66+
67+
public function getMinimumScope() {
68+
// This is the Google contacts API, which is apparently the best way to get
69+
// the user ID / login / email since Google doesn't apparently have a
70+
// more generic "user.info" sort of call (or, if it does, I couldn't find
71+
// it). This is sort of terrifying since it lets Phabricator read your whole
72+
// address book and possibly your physical address and such, so it would
73+
// be really nice to find a way to restrict this scope to something less
74+
// crazily permissive. But users will click anything and the dialog isn't
75+
// very scary, so whatever.
76+
return 'https://www.google.com/m8/feeds';
77+
}
78+
79+
public function setUserData($data) {
80+
$xml = new SimpleXMLElement($data);
81+
$id = (string)$xml->id;
82+
$this->userData = array(
83+
'id' => $id,
84+
'email' => (string)$xml->author[0]->email,
85+
'real' => (string)$xml->author[0]->name,
86+
87+
// Guess account name from email address, this is just a hint anyway.
88+
'account' => head(explode('@', $id)),
89+
);
90+
return $this;
91+
}
92+
93+
public function retrieveUserID() {
94+
return $this->userData['id'];
95+
}
96+
97+
public function retrieveUserEmail() {
98+
return $this->userData['email'];
99+
}
100+
101+
public function retrieveUserAccountName() {
102+
return $this->userData['account'];
103+
}
104+
105+
public function retrieveUserProfileImage() {
106+
// No apparent API access to Plus yet.
107+
return null;
108+
}
109+
110+
public function retrieveUserAccountURI() {
111+
// No apparent API access to Plus yet.
112+
return null;
113+
}
114+
115+
public function retrieveUserRealName() {
116+
return $this->userData['real'];
117+
}
118+
119+
public function getExtraAuthParameters() {
120+
return array(
121+
'response_type' => 'code',
122+
);
123+
}
124+
125+
public function getExtraTokenParameters() {
126+
return array(
127+
'grant_type' => 'authorization_code',
128+
);
129+
130+
}
131+
132+
public function decodeTokenResponse($response) {
133+
return json_decode($response, true);
134+
}
135+
136+
}
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
<?php
2+
/**
3+
* This file is automatically generated. Lint this module to rebuild it.
4+
* @generated
5+
*/
6+
7+
8+
9+
phutil_require_module('phabricator', 'applications/auth/oauth/provider/base');
10+
phutil_require_module('phabricator', 'infrastructure/env');
11+
12+
phutil_require_module('phutil', 'utils');
13+
14+
15+
phutil_require_source('PhabricatorOAuthProviderGoogle.php');

0 commit comments

Comments
 (0)