[feature/oauth] Changes due to code review

PHPBB3-11673

phpBB/includes/ucp/ucp_auth_link.php

@@ -17,8 +17,17 @@ if (!defined('IN_PHPBB'))
 
 class ucp_auth_link
 {
+	/**
+	* @var string
+	*/
 	public $u_action;
 
+	/**
+	* Generates the ucp_auth_link page and handles the auth link process
+	*
+	* @param	int		$id
+	* @param	string	$mode
+	*/
 	public function main($id, $mode)
 	{
 		global $config, $request, $template, $phpbb_container, $user;
@@ -72,7 +81,7 @@ class ucp_auth_link
 			}
 		}
 
-		// In some cases, an request to an external server may be required in
+		// In some cases, a request to an external server may be required. In
 		// these cases, the GET parameter 'link' should exist and should be true
 		if ($request->variable('link', false))
 		{
@@ -114,8 +123,11 @@ class ucp_auth_link
 
 		$s_hidden_fields = build_hidden_fields($s_hidden_fields);
 
+		// Replace "error" strings with their real, localised form
+		$error = array_map(array($user, 'lang'), $error);
+
 		$template->assign_vars(array(
-			'ERROR'	=> $this->build_error_text($error),
+			'ERROR'	=> $error,
 
 			'PROVIDER_TEMPLATE_FILE'	=> $provider_data['TEMPLATE_FILE'],
 
@@ -126,20 +138,4 @@ class ucp_auth_link
 		$this->tpl_name = 'ucp_auth_link';
 		$this->page_title = 'UCP_AUTH_LINK';
 	}
-
-	private function build_error_text(array $errors)
-	{
-		global $user;
-
-		// Replace all errors that are language constants
-		foreach ($errors as $key => $error)
-		{
-			if (isset($user->lang[$error]))
-			{
-				$errors[$key] = $user->lang($error);
-			}
-		}
-
-		return implode('<br />', $errors);
-	}
 }

phpBB/includes/ucp/ucp_login_link.php

@@ -91,7 +91,9 @@ class ucp_login_link
 					if ($result)
 					{
 						$login_link_error = $user->lang[$result];
-					} else {
+					}
+					else
+					{
 						// Finish login
 						$result = $user->session_create($login_result['user_row']['user_id'], false, false, true);
 

phpBB/language/en/common.php

@@ -90,6 +90,7 @@ $lang = array_merge($lang, array(
 	'AUTH_NO_PROFILE_CREATED'		=> 'The creation of a user profile was unsuccessful.',
 	'AUTH_PROVIDER_OAUTH_ERROR_INVALID_ENTRY'				=> 'Invalid database entry.',
 	'AUTH_PROVIDER_OAUTH_ERROR_INVALID_SERVICE_TYPE'		=> 'Invalid service type provided to OAuth service handler.',
+	'AUTH_PROVIDER_OAUTH_ERROR_SERVICE_NOT_CREATED'			=> 'OAuth service not created',
 	'AUTH_PROVIDER_OAUTH_SERVICE_BITLY'						=> 'Bitly',
 	'AUTH_PROVIDER_OAUTH_SERVICE_FACEBOOK'					=> 'Facebook',
 	'AUTH_PROVIDER_OAUTH_SERVICE_GOOGLE'					=> 'Google',

phpBB/language/en/ucp.php

@@ -270,7 +270,7 @@ $lang = array_merge($lang, array(
 	'LINK_REMOTE_SIZE'			=> 'Avatar dimensions',
 	'LINK_REMOTE_SIZE_EXPLAIN'	=> 'Specify the width and height of the avatar, leave blank to attempt automatic verification.',
 	'LOGIN_EXPLAIN_UCP'			=> 'Please login in order to access the User Control Panel.',
-	'LOGIN_LINK'					=> 'Link or Register Your External Account with This Board',
+	'LOGIN_LINK'					=> 'Link or register your account on an external service with your board account',
 	'LOGIN_LINK_EXPLAIN'			=> 'You have attempted to login with an external service that is not yet connected to an account on this board. You must now either link this account to an existing account or create a new account.',
 	'LOGIN_LINK_MISSING_DATA'		=> 'Data that is necessary to link your account with an external service is not available. Please restart the login process.',
 	'LOGIN_LINK_NO_DATA_PROVIDED'	=> 'No data has been provided to this page to link an external account to a forum account. Please contact the board administrator if you continue to experience problems.',
@@ -480,18 +480,18 @@ $lang = array_merge($lang, array(
 	'UCP_AIM'					=> 'AOL Instant Messenger',
 	'UCP_ATTACHMENTS'			=> 'Attachments',
 	'UCP_AUTH_LINK'				=> 'External accounts',
-	'UCP_AUTH_LINK_ASK'			=> 'You currently have no account tied to this external service. Click the button below to link your board account to an account with this external service.',
+	'UCP_AUTH_LINK_ASK'			=> 'You currently have no account associated with this external service. Click the button below to link your board account to an account with this external service.',
 	'UCP_AUTH_LINK_ID'			=> 'Unique identifier',
 	'UCP_AUTH_LINK_LINK'		=> 'Link',
-	'UCP_AUTH_LINK_MANAGE'		=> 'Manage external accounts',
+	'UCP_AUTH_LINK_MANAGE'		=> 'Manage external account associations',
-	'UCP_AUTH_LINK_TITLE'		=> 'Manage your external accounts',
+	'UCP_AUTH_LINK_TITLE'		=> 'Manage your external account associations',
 	'UCP_AUTH_LINK_UNLINK'		=> 'Unlink',
 	'UCP_COPPA_BEFORE'			=> 'Before %s',
 	'UCP_COPPA_ON_AFTER'		=> 'On or after %s',
 	'UCP_EMAIL_ACTIVATE'		=> 'Please note that you will need to enter a valid email address before your account is activated. You will receive an email at the address you provide that contains an account activation link.',
 	'UCP_ICQ'					=> 'ICQ number',
 	'UCP_JABBER'				=> 'Jabber address',
-	'UCP_LOGIN_LINK'			=> 'Set up an external account',
+	'UCP_LOGIN_LINK'			=> 'Set up an external account association',
 
 	'UCP_MAIN'					=> 'Overview',
 	'UCP_MAIN_ATTACHMENTS'		=> 'Manage attachments',

phpBB/phpbb/auth/provider/interface.php

@@ -45,9 +45,9 @@ interface phpbb_auth_provider_interface
 	 *							'error_msg' => string
 	 *							'user_row' => array
 	 *						)
-	 *					A fourth key of the array may be present 'redirect_data'
+	 *					A fourth key of the array may be present:
-	 *					This key is only used when 'status' is equal to
+	 *					'redirect_data'	This key is only used when 'status' is
-	 *					LOGIN_SUCCESS_LINK_PROFILE and it's value is an
+	 *					equal to LOGIN_SUCCESS_LINK_PROFILE and its value is an
 	 *					associative array that is turned into GET variables on
 	 *					the redirect url.
 	 */
@@ -89,7 +89,7 @@ interface phpbb_auth_provider_interface
 	 *							array: 'BLOCK_VAR_NAME'. If this is present,
 	 *							then its value should be a string that is used
 	 *							to designate the name of the loop used in the
-	 *							ACP template file. In addition to this, an
+	 *							ACP template file. When this is present, an
 	 *							additional key named 'BLOCK_VARS' is required.
 	 *							This must be an array containing at least one
 	 *							array of variables that will be assigned during

phpBB/phpbb/auth/provider/oauth/oauth.php

@@ -211,8 +211,8 @@ class phpbb_auth_provider_oauth extends phpbb_auth_provider_base
 
 			// Retrieve the user's account
 			$sql = 'SELECT user_id, username, user_password, user_passchg, user_pass_convert, user_email, user_type, user_login_attempts
-			FROM ' . $this->users_table . "
+			FROM ' . $this->users_table . '
-			WHERE user_id = '" . $this->db->sql_escape($row['user_id']) . "'";
+			WHERE user_id = ' . (int) $row['user_id'];
 			$result = $this->db->sql_query($sql);
 			$row = $this->db->sql_fetchrow($result);
 			$this->db->sql_freeresult($result);
@@ -231,7 +231,9 @@ class phpbb_auth_provider_oauth extends phpbb_auth_provider_base
 				'error_msg'		=> false,
 				'user_row'		=> $row,
 			);
-		} else {
+		}
+		else
+		{
 			$url = $service->getAuthorizationUri();
 			header('Location: ' . $url);
 		}
@@ -291,8 +293,7 @@ class phpbb_auth_provider_oauth extends phpbb_auth_provider_base
 
 		if (!$service)
 		{
-			// Update to an actual error message
+			throw new Exception('AUTH_PROVIDER_OAUTH_ERROR_SERVICE_NOT_CREATED');
-			throw new Exception('Service not created: ' . $service_name);
 		}
 
 		return $service;
@@ -474,7 +475,7 @@ class phpbb_auth_provider_oauth extends phpbb_auth_provider_base
 	}
 
 	/**
-	* Performs the account linking for login_link
+	* Performs the account linking for auth_link
 	*
 	* @param	array	$link_data		The same variable given to {@see phpbb_auth_provider_interface::link_account}
 	* @param	string	$service_name	The name of the service being used in
@@ -503,7 +504,9 @@ class phpbb_auth_provider_oauth extends phpbb_auth_provider_base
 			);
 
 			$this->link_account_perform_link($data);
-		} else {
+		}
+		else
+		{
 			$url = $service->getAuthorizationUri();
 			header('Location: ' . $url);
 		}

phpBB/phpbb/auth/provider/oauth/service/facebook.php

@@ -66,7 +66,6 @@ class phpbb_auth_provider_oauth_service_facebook extends phpbb_auth_provider_oau
 	{
 		if (!($this->service_provider instanceof \OAuth\OAuth2\Service\Facebook))
 		{
-			// TODO: make exception class and use language constant
 			throw new Exception('AUTH_PROVIDER_OAUTH_ERROR_INVALID_SERVICE_TYPE');
 		}
 
@@ -87,8 +86,7 @@ class phpbb_auth_provider_oauth_service_facebook extends phpbb_auth_provider_oau
 	{
 		if (!($this->service_provider instanceof \OAuth\OAuth2\Service\Facebook))
 		{
-			// TODO: make exception class and use language constant
+			throw new Exception('AUTH_PROVIDER_OAUTH_ERROR_INVALID_SERVICE_TYPE');
-			throw new Exception('Invalid service provider type');
 		}
 
 		// Send a request with it

phpBB/phpbb/auth/provider/oauth/token_storage.php

@@ -83,7 +83,8 @@ class phpbb_auth_provider_oauth_token_storage implements TokenStorageInterface
 	*/
 	public function retrieveAccessToken()
 	{
-		if( $this->cachedToken instanceOf TokenInterface ) {
+		if ($this->cachedToken instanceOf TokenInterface)
+		{
 			return $this->cachedToken;
 		}
 
@@ -92,7 +93,7 @@ class phpbb_auth_provider_oauth_token_storage implements TokenStorageInterface
 			'provider'	=> $this->service_name,
 		);
 
-		if ($this->user->data['user_id'] == ANONYMOUS)
+		if ($this->user->data['user_id'] === ANONYMOUS)
 		{
 			$data['session_id']	= $this->user->data['session_id'];
 		}
@@ -133,7 +134,7 @@ class phpbb_auth_provider_oauth_token_storage implements TokenStorageInterface
 			'provider'	=> $this->service_name,
 		);
 
-		if ($this->user->data['user_id'] == ANONYMOUS)
+		if ($this->user->data['user_id'] === ANONYMOUS)
 		{
 			$data['session_id']	= $this->user->data['session_id'];
 		}
@@ -149,12 +150,12 @@ class phpbb_auth_provider_oauth_token_storage implements TokenStorageInterface
 		$this->cachedToken = null;
 
 		$sql = 'DELETE FROM ' . $this->auth_provider_oauth_table . '
-			WHERE user_id = ' . $this->user->data['user_id'] . '
+			WHERE user_id = ' . $this->user->data['user_id'] . "
-				AND provider = \'' . $this->db->sql_escape($this->service_name) . '\'';
+				AND provider = '" . $this->db->sql_escape($this->service_name) . "'";
 
-		if ($this->user->data['user_id'] == ANONYMOUS)
+		if ($this->user->data['user_id'] === ANONYMOUS)
 		{
-			$sql .= ' AND session_id = \'' . $this->user->data['session_id'] . '\'';
+			$sql .= " AND session_id = '" . $this->user->data['session_id'] . "'";
 		}
 
 		$this->db->sql_query($sql);
@@ -176,8 +177,8 @@ class phpbb_auth_provider_oauth_token_storage implements TokenStorageInterface
 			SET ' . $this->db->sql_build_array('UPDATE', array(
 					'user_id' => (int) $user_id
 				)) . '
-				WHERE user_id = ' . $this->user->data['user_id'] . '
+				WHERE user_id = ' . $this->user->data['user_id'] . "
-					AND session_id = \'' . $this->user->data['session_id'] . '\'';
+					AND session_id = '" . $this->user->data['session_id'] . "'";
 		$this->db->sql_query($sql);
 	}
 
@@ -188,7 +189,8 @@ class phpbb_auth_provider_oauth_token_storage implements TokenStorageInterface
 	*/
 	public function has_access_token_by_session()
 	{
-		if( $this->cachedToken ) {
+		if ($this->cachedToken)
+		{
 			return true;
 		}
 
@@ -208,14 +210,7 @@ class phpbb_auth_provider_oauth_token_storage implements TokenStorageInterface
 	*/
 	protected function _has_acess_token($data)
 	{
-		$row = $this->get_access_token_row($data);
+		return (bool) $this->get_access_token_row($data);
-
-		if (!$row)
-		{
-			return false;
-		}
-
-		return true;
 	}
 
 	public function retrieve_access_token_by_session()