Issue #575810 by wojtha, Heine, vzima: Fixed OpenID discovery spec violation - follow redirects.

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,
+ * parameter, the claimed identifier. They have to return an associative array
- * in the same form returned by openid_discover().
+ * 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() {

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);
+  if (!empty($discovery['services'])) {
-  $service = _openid_select_service($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);
+              $discovery = openid_discovery($response['openid.claimed_id']);
-              $uris = array();
+              if ($discovery && !empty($discovery['services'])) {
-              foreach ($services as $discovered_service) {
+                $uris = array();
-                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'])) {
+                foreach ($discovery['services'] as $discovered_service) {
-                  $uris[] = $discovered_service['uri'];
+                  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
+ * @param $claimed_id
- * URI, etc).
+ *   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);
+    $discovery = $method($claimed_id);
-    if (!empty($discovered_services)) {
+    if (!empty($discovery)) {
-      return $discovered_services;
+      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);
+    $discovery = _openid_xrds_discovery($xrds_url);
-    foreach ($services as $i => &$service) {
+    if (!empty($discovery['services']) && is_array($discovery['services'])) {
-      $status = $service['xrd']->children(OPENID_NS_XRD)->Status;
+      foreach ($discovery['services'] as $i => &$service) {
-      if ($status && $status->attributes()->cid == 'verified') {
+        $status = $service['xrd']->children(OPENID_NS_XRD)->Status;
-        $service['claimed_id'] = openid_normalize((string)$service['xrd']->children(OPENID_NS_XRD)->CanonicalID);
+        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 {
+      if (!empty($discovery['services'])) {
-        // Ignore service if CanonicalID could not be verified.
+        return $discovery;
-        unset($services[$i]);
       }
     }
-    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,
+    );
+  }
 }
 
 /**

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.
    */

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).