From 7379f95ee590c79b4838f7896fd46faf700c7b08 Mon Sep 17 00:00:00 2001
From: webchick <webchick@24967.no-reply.drupal.org>
Date: Mon, 5 Sep 2011 12:05:36 -0700
Subject: [PATCH] Issue #575810 by wojtha, Heine, vzima: Fixed OpenID discovery
spec violation - follow redirects.
---
modules/openid/openid.api.php | 10 ++-
modules/openid/openid.module | 107 ++++++++++++++++++------
modules/openid/openid.test | 57 +++++++++++++
modules/openid/tests/openid_test.module | 35 ++++++++
4 files changed, 180 insertions(+), 29 deletions(-)
diff --git a/modules/openid/openid.api.php b/modules/openid/openid.api.php
index 11faa71efa89..5e3d15d94cbe 100644
--- a/modules/openid/openid.api.php
+++ b/modules/openid/openid.api.php
@@ -49,8 +49,13 @@ function hook_openid_response($response, $account) {
* Allow modules to declare OpenID discovery methods.
*
* The discovery function callbacks will be called in turn with an unique
- * parameter, the claimed identifier. They have to return an array of services,
- * in the same form returned by openid_discover().
+ * parameter, the claimed identifier. They have to return an associative array
+ * with array of services and claimed identifier in the same form as returned by
+ * openid_discover(). The resulting array must contain following keys:
+ * - 'services' (required) an array of discovered services (including OpenID
+ * version, endpoint URI, etc).
+ * - 'claimed_id' (optional) new claimed identifer, found by following HTTP
+ * redirects during the services discovery.
*
* The first discovery method that succeed (return at least one services) will
* stop the discovery process.
@@ -58,6 +63,7 @@ function hook_openid_response($response, $account) {
* @return
* An associative array which keys are the name of the discovery methods and
* values are function callbacks.
+ *
* @see hook_openid_discovery_method_info_alter()
*/
function hook_openid_discovery_method_info() {
diff --git a/modules/openid/openid.module b/modules/openid/openid.module
index 6d4b1d7ff78f..bb6ad712bebb 100644
--- a/modules/openid/openid.module
+++ b/modules/openid/openid.module
@@ -256,16 +256,25 @@ function openid_login_validate($form, &$form_state) {
function openid_begin($claimed_id, $return_to = '', $form_values = array()) {
module_load_include('inc', 'openid');
+ $service = NULL;
$claimed_id = openid_normalize($claimed_id);
+ $discovery = openid_discovery($claimed_id);
- $services = openid_discovery($claimed_id);
- $service = _openid_select_service($services);
+ if (!empty($discovery['services'])) {
+ $service = _openid_select_service($discovery['services']);
+ }
- if (!$service) {
+ // Quit if the discovery result was empty or if we can't select any service.
+ if (!$discovery || !$service) {
form_set_error('openid_identifier', t('Sorry, that is not a valid OpenID. Ensure you have spelled your ID correctly.'));
return;
}
+ // Set claimed id from discovery.
+ if (!empty($discovery['claimed_id'])) {
+ $claimed_id = $discovery['claimed_id'];
+ }
+
// Store discovered information in the users' session so we don't have to rediscover.
$_SESSION['openid']['service'] = $service;
// Store the claimed id
@@ -352,11 +361,13 @@ function openid_complete($response = array()) {
// identififer to make sure that the provider is authorized to
// respond on behalf of this.
if ($response_claimed_id != $claimed_id) {
- $services = openid_discovery($response_claimed_id);
- $uris = array();
- foreach ($services as $discovered_service) {
- if (in_array('http://specs.openid.net/auth/2.0/server', $discovered_service['types']) || in_array('http://specs.openid.net/auth/2.0/signon', $discovered_service['types'])) {
- $uris[] = $discovered_service['uri'];
+ $discovery = openid_discovery($response['openid.claimed_id']);
+ if ($discovery && !empty($discovery['services'])) {
+ $uris = array();
+ foreach ($discovery['services'] as $discovered_service) {
+ if (in_array('http://specs.openid.net/auth/2.0/server', $discovered_service['types']) || in_array('http://specs.openid.net/auth/2.0/signon', $discovered_service['types'])) {
+ $uris[] = $discovered_service['uri'];
+ }
}
}
if (!in_array($service['uri'], $uris)) {
@@ -378,10 +389,21 @@ function openid_complete($response = array()) {
/**
* Perform discovery on a claimed ID to determine the OpenID provider endpoint.
*
- * @param $claimed_id The OpenID URL to perform discovery on.
+ * Discovery methods are provided by the hook_openid_discovery_method_info and
+ * could be further altered using the hook_openid_discovery_method_info_alter.
*
- * @return Array of services discovered (including OpenID version, endpoint
- * URI, etc).
+ * @param $claimed_id
+ * The OpenID URL to perform discovery on.
+ *
+ * @return
+ * The resulting discovery array from the first successful discovery method,
+ * which must contain following keys:
+ * - 'services' (required) an array of discovered services (including OpenID
+ * version, endpoint URI, etc).
+ * - 'claimed_id' (optional) new claimed identifer, found by following HTTP
+ * redirects during the services discovery.
+ * If all the discovery method fails or if no appropriate discovery method is
+ * found, FALSE is returned.
*/
function openid_discovery($claimed_id) {
module_load_include('inc', 'openid');
@@ -389,15 +411,15 @@ function openid_discovery($claimed_id) {
$methods = module_invoke_all('openid_discovery_method_info');
drupal_alter('openid_discovery_method_info', $methods);
- // Execute each method in turn.
+ // Execute each method in turn and return first successful discovery.
foreach ($methods as $method) {
- $discovered_services = $method($claimed_id);
- if (!empty($discovered_services)) {
- return $discovered_services;
+ $discovery = $method($claimed_id);
+ if (!empty($discovery)) {
+ return $discovery;
}
}
- return array();
+ return FALSE;
}
/**
@@ -421,24 +443,33 @@ function openid_openid_discovery_method_info() {
*
* @see http://openid.net/specs/openid-authentication-2_0.html#discovery
* @see hook_openid_discovery_method_info()
+ * @see openid_discovery()
+ *
+ * @return
+ * An array of discovered services and claimed identifier or NULL. See
+ * openid_discovery() for more specific information.
*/
function _openid_xri_discovery($claimed_id) {
if (_openid_is_xri($claimed_id)) {
// Resolve XRI using a proxy resolver (Extensible Resource Identifier (XRI)
// Resolution Version 2.0, section 11.2 and 14.3).
$xrds_url = variable_get('xri_proxy_resolver', 'http://xri.net/') . rawurlencode($claimed_id) . '?_xrd_r=application/xrds+xml';
- $services = _openid_xrds_discovery($xrds_url);
- foreach ($services as $i => &$service) {
- $status = $service['xrd']->children(OPENID_NS_XRD)->Status;
- if ($status && $status->attributes()->cid == 'verified') {
- $service['claimed_id'] = openid_normalize((string)$service['xrd']->children(OPENID_NS_XRD)->CanonicalID);
+ $discovery = _openid_xrds_discovery($xrds_url);
+ if (!empty($discovery['services']) && is_array($discovery['services'])) {
+ foreach ($discovery['services'] as $i => &$service) {
+ $status = $service['xrd']->children(OPENID_NS_XRD)->Status;
+ if ($status && $status->attributes()->cid == 'verified') {
+ $service['claimed_id'] = openid_normalize((string)$service['xrd']->children(OPENID_NS_XRD)->CanonicalID);
+ }
+ else {
+ // Ignore service if the Canonical ID could not be verified.
+ unset($discovery['services'][$i]);
+ }
}
- else {
- // Ignore service if CanonicalID could not be verified.
- unset($services[$i]);
+ if (!empty($discovery['services'])) {
+ return $discovery;
}
}
- return $services;
}
}
@@ -447,6 +478,11 @@ function _openid_xri_discovery($claimed_id) {
*
* @see http://openid.net/specs/openid-authentication-2_0.html#discovery
* @see hook_openid_discovery_method_info()
+ * @see openid_discovery()
+ *
+ * @return
+ * An array of discovered services and claimed identifier or NULL. See
+ * openid_discovery() for more specific information.
*/
function _openid_xrds_discovery($claimed_id) {
$services = array();
@@ -458,7 +494,18 @@ function _openid_xrds_discovery($claimed_id) {
$headers = array('Accept' => 'application/xrds+xml');
$result = drupal_http_request($xrds_url, array('headers' => $headers));
- if (!isset($result->error)) {
+ // Check for HTTP error and make sure, that we reach the target. If the
+ // maximum allowed redirects are exhausted, final destination URL isn't
+ // reached, but drupal_http_request() doesn't return any error.
+ // @todo Remove the check for 200 HTTP result code after the following issue
+ // will be fixed: http://drupal.org/node/1096890.
+ if (!isset($result->error) && $result->code == 200) {
+
+ // Replace the user-entered claimed_id if we received a redirect.
+ if (!empty($result->redirect_url)) {
+ $claimed_id = openid_normalize($result->redirect_url);
+ }
+
if (isset($result->headers['content-type']) && preg_match("/application\/xrds\+xml/", $result->headers['content-type'])) {
// Parse XML document to find URL
$services = _openid_xrds_parse($result->data);
@@ -504,7 +551,13 @@ function _openid_xrds_discovery($claimed_id) {
}
}
}
- return $services;
+
+ if (!empty($services)) {
+ return array(
+ 'services' => $services,
+ 'claimed_id' => $claimed_id,
+ );
+ }
}
/**
diff --git a/modules/openid/openid.test b/modules/openid/openid.test
index 09632ba1417c..6e2528e66af2 100644
--- a/modules/openid/openid.test
+++ b/modules/openid/openid.test
@@ -124,6 +124,28 @@ class OpenIDFunctionalTestCase extends OpenIDWebTestCase {
// OpenID Authentication 2.0, section 7.3.3:
$this->addIdentity(url('openid-test/html/openid2', array('absolute' => TRUE)), 2, 'http://example.com/html-openid2');
+
+ // OpenID Authentication 2.0, section 7.2.4:
+ // URL Identifiers MUST then be further normalized by both (1) following
+ // redirects when retrieving their content and finally (2) applying the
+ // rules in Section 6 of RFC3986 to the final destination URL. This final
+ // URL MUST be noted by the Relying Party as the Claimed Identifier and be
+ // used when requesting authentication.
+
+ // Single redirect.
+ $identity = $expected_claimed_id = url('openid-test/redirected/yadis/xrds/1', array('absolute' => TRUE));
+ $this->addRedirectedIdentity($identity, 2, 'http://example.com/xrds', $expected_claimed_id, 0);
+
+ // Exact 3 redirects (default value for the 'max_redirects' option in
+ // drupal_http_request()).
+ $identity = $expected_claimed_id = url('openid-test/redirected/yadis/xrds/2', array('absolute' => TRUE));
+ $this->addRedirectedIdentity($identity, 2, 'http://example.com/xrds', $expected_claimed_id, 2);
+
+ // Fails because there are more than 3 redirects (default value for the
+ // 'max_redirects' option in drupal_http_request()).
+ $identity = url('openid-test/redirected/yadis/xrds/3', array('absolute' => TRUE));
+ $expected_claimed_id = FALSE;
+ $this->addRedirectedIdentity($identity, 2, 'http://example.com/xrds', $expected_claimed_id, 3);
}
/**
@@ -279,6 +301,41 @@ class OpenIDFunctionalTestCase extends OpenIDWebTestCase {
$this->assertRaw(t('Successfully added %identity', array('%identity' => $claimed_id)), t('Identity %identity was added.', array('%identity' => $identity)));
}
+ /**
+ * Add OpenID identity, changed by the following redirects, to user's profile.
+ *
+ * According to OpenID Authentication 2.0, section 7.2.4, URL Identifiers MUST
+ * be further normalized by following redirects when retrieving their content
+ * and this final URL MUST be noted by the Relying Party as the Claimed
+ * Identifier and be used when requesting authentication.
+ *
+ * @param $identity
+ * The User-supplied Identifier.
+ * @param $version
+ * The protocol version used by the service.
+ * @param $local_id
+ * The expected OP-Local Identifier found during discovery.
+ * @param $claimed_id
+ * The expected Claimed Identifier returned by the OpenID Provider, or FALSE
+ * if the discovery is expected to fail.
+ * @param $redirects
+ * The number of redirects.
+ */
+ function addRedirectedIdentity($identity, $version = 2, $local_id = 'http://example.com/xrds', $claimed_id = NULL, $redirects = 0) {
+ // Set the final destination URL which is the same as the Claimed
+ // Identifier, we insert the same identifier also to the provider response,
+ // but provider could futher change the Claimed ID actually (e.g. it could
+ // add unique fragment).
+ variable_set('openid_test_redirect_url', $identity);
+ variable_set('openid_test_response', array('openid.claimed_id' => $identity));
+
+ $this->addIdentity(url('openid-test/redirect/' . $redirects, array('absolute' => TRUE)), $version, $local_id, $claimed_id);
+
+ // Clean up.
+ variable_del('openid_test_redirect_url');
+ variable_del('openid_test_response');
+ }
+
/**
* Tests that openid.signed is verified.
*/
diff --git a/modules/openid/tests/openid_test.module b/modules/openid/tests/openid_test.module
index bad1184a311a..629dcd3356a5 100644
--- a/modules/openid/tests/openid_test.module
+++ b/modules/openid/tests/openid_test.module
@@ -60,6 +60,19 @@ function openid_test_menu() {
'access callback' => TRUE,
'type' => MENU_CALLBACK,
);
+ $items['openid-test/redirect'] = array(
+ 'title' => 'OpenID Provider Redirection Point',
+ 'page callback' => 'openid_test_redirect',
+ 'access callback' => TRUE,
+ 'type' => MENU_CALLBACK,
+ );
+ $items['openid-test/redirected/%/%'] = array(
+ 'title' => 'OpenID Provider Final URL',
+ 'page callback' => 'openid_test_redirected_method',
+ 'page arguments' => array(2, 3),
+ 'access callback' => TRUE,
+ 'type' => MENU_CALLBACK,
+ );
return $items;
}
@@ -212,6 +225,28 @@ function openid_test_endpoint() {
}
}
+/**
+ * Menu callback; redirect during Normalization/Discovery.
+ */
+function openid_test_redirect($count = 0) {
+ if ($count == 0) {
+ $url = variable_get('openid_test_redirect_url', '');
+ }
+ else {
+ $url = url('openid-test/redirect/' . --$count, array('absolute' => TRUE));
+ }
+ $http_response_code = variable_get('openid_test_redirect_http_reponse_code', 301);
+ header('Location: ' . $url, TRUE, $http_response_code);
+ exit();
+}
+
+/**
+ * Menu callback; respond with appropriate callback.
+ */
+function openid_test_redirected_method($method1, $method2) {
+ return call_user_func('openid_test_' . $method1 . '_' . $method2);
+}
+
/**
* OpenID endpoint; handle "associate" requests (see OpenID Authentication 2.0,
* section 8).
--
GitLab