[project @ Auth_OpenID_Consumer: OpenID 2 draft 12 fragment handling]

2 files changed, 74 insertions, 18 deletions

diff --git a/Auth/OpenID/Consumer.php b/Auth/OpenID/Consumer.php
index 93bd929..52fb100 100644
--- a/Auth/OpenID/Consumer.php
+++ b/Auth/OpenID/Consumer.php
@@ -986,10 +986,16 @@ class Auth_OpenID_GenericConsumer {
             }
         }
 
-        if ($to_match->claimed_id != $endpoint->claimed_id) {
+        // Fragments do not influence discovery, so we can't compare a
+        // claimed identifier with a fragment to discovered
+        // information.
+        list($defragged_claimed_id, $_) =
+            Auth_OpenID::urldefrag($to_match->claimed_id);
+
+        if ($defragged_claimed_id != $endpoint->claimed_id) {
             return new Auth_OpenID_FailureResponse($endpoint,
               sprintf('Claimed ID does not match (different subjects!), ' .
-                      'Expected %s, got %s', $to_match->claimed_id,
+                      'Expected %s, got %s', $defragged_claimed_id,
                       $endpoint->claimed_id));
         }
 
@@ -1045,42 +1051,58 @@ class Auth_OpenID_GenericConsumer {
             ($to_match->local_id !== null)) {
             return new Auth_OpenID_FailureResponse($endpoint,
               'openid.identity is present without openid.claimed_id');
-        } else if (($to_match->claimed_id !== null) &&
-                   ($to_match->local_id === null)) {
+        }
+
+        if (($to_match->claimed_id !== null) &&
+            ($to_match->local_id === null)) {
             return new Auth_OpenID_FailureResponse($endpoint,
               'openid.claimed_id is present without openid.identity');
-        } else if ($to_match->claimed_id === null) {
+        }
+
+        if ($to_match->claimed_id === null) {
             // This is a response without identifiers, so there's
             // really no checking that we can do, so return an
             // endpoint that's for the specified `openid.op_endpoint'
             return Auth_OpenID_ServiceEndpoint::fromOPEndpointURL(
                                                 $to_match->server_url);
-        } else if (!$endpoint) {
+        }
+
+        // Fragments do not influence discovery, so we can't compare a
+        // claimed identifier with a fragment to discovered
+        // information.
+        list($defragged_claimed_id, $_) =
+            Auth_OpenID::urldefrag($to_match->claimed_id);
+
+        if (!$endpoint) {
             // The claimed ID doesn't match, so we have to do
             // discovery again. This covers not using sessions, OP
             // identifier endpoints and responses that didn't match
             // the original request.
             // oidutil.log('No pre-discovered information supplied.')
             return $this->_discoverAndVerify($to_match);
-        } else if ($to_match->claimed_id != $endpoint->claimed_id) {
+        } else if ($defragged_claimed_id != $endpoint->claimed_id) {
             // oidutil.log('Mismatched pre-discovered session data. '
             //             'Claimed ID in session=%s, in assertion=%s' %
             //             (endpoint.claimed_id, to_match.claimed_id))
             return $this->_discoverAndVerify($to_match);
-        } else {
-            // The claimed ID matches, so we use the endpoint that we
-            // discovered in initiation. This should be the most
-            // common case.
-            $result = $this->_verifyDiscoverySingle($endpoint, $to_match);
+        }
 
-            if (Auth_OpenID::isFailure($result)) {
-                return $result;
-            }
+        // The claimed ID matches, so we use the endpoint that we
+        // discovered in initiation. This should be the most common
+        // case.
+        $result = $this->_verifyDiscoverySingle($endpoint, $to_match);
 
-            return $endpoint;
+        if (Auth_OpenID::isFailure($result)) {
+            return $result;
+        }
+
+        // The endpoint we return should have the claimed ID from the
+        // message we just verified, fragment and all.
+        if ($endpoint->claimed_id != $to_match->claimed_id) {
+            $endpoint->claimed_id = $to_match->claimed_id;
         }
 
-        // Never reached.
+        return $endpoint;
     }
 
     /**
@@ -1092,6 +1114,7 @@ class Auth_OpenID_GenericConsumer {
         list($unused, $services) = call_user_func($this->discoverMethod,
                                                   $to_match->claimed_id,
                                                   $this->fetcher);
+
         if (!$services) {
             return new Auth_OpenID_FailureResponse(null,
               sprintf("No OpenID information found at %s",
diff --git a/Tests/Auth/OpenID/VerifyDisco.php b/Tests/Auth/OpenID/VerifyDisco.php
index a3a6277..a95a4d3 100644
--- a/Tests/Auth/OpenID/VerifyDisco.php
+++ b/Tests/Auth/OpenID/VerifyDisco.php
@@ -150,6 +150,33 @@ class Tests_Auth_OpenID_VerifyDisco extends OpenIDTestMixin {
         $this->failUnlessProtocolError(
             $this->consumer->_verifyDiscoveryResults($msg, $endpoint));
     }
+
+    function test_openid2Fragment()
+    {
+        $claimed_id = "http://unittest.invalid/";
+        $claimed_id_frag = $claimed_id . "#fragment";
+
+        $endpoint = new Auth_OpenID_ServiceEndpoint();
+        $endpoint->local_id = 'my identity';
+        $endpoint->claimed_id = $claimed_id;
+        $endpoint->server_url = 'Phone Home';
+        $endpoint->type_uris = array(Auth_OpenID_TYPE_2_0);
+
+        $msg = Auth_OpenID_Message::fromOpenIDArgs(
+                    array('ns' => Auth_OpenID_OPENID2_NS,
+                          'identity' => $endpoint->local_id,
+                          'claimed_id' => $claimed_id_frag,
+                          'op_endpoint' => $endpoint->server_url));
+        $result = $this->consumer->_verifyDiscoveryResults($msg, $endpoint);
+
+        $this->assertEquals($result->local_id, $endpoint->local_id);
+        $this->assertEquals($result->server_url, $endpoint->server_url);
+
+        $this->assertEquals($result->type_uris, $endpoint->type_uris);
+
+        $this->assertEquals($result->claimed_id, $claimed_id_frag);
+    }
+
 }
 
 // XXX: test the implementation of _discoverAndVerify
@@ -178,7 +205,9 @@ class Tests_openID2NoEndpointDoesDisco extends Tests_Auth_OpenID_VerifyDisco {
     function test_openID2NoEndpointDoesDisco()
     {
         $op_endpoint = 'Phone Home';
-        $sentinel = 'thing';
+        $this->consumer->sentinel = new Auth_OpenID_ServiceEndpoint();
+        $this->consumer->sentinel->claimed_id = 'monkeysoft';
+
         $msg = Auth_OpenID_Message::fromOpenIDArgs(
             array('ns' => Auth_OpenID_OPENID2_NS,
                   'identity' => 'sour grapes',
@@ -199,6 +228,10 @@ class Tests_openID2MismatchedDoesDisco extends Tests_Auth_OpenID_VerifyDisco {
         $mismatched->identity = 'nothing special, but different';
         $mismatched->local_id = 'green cheese';
 
+        $sentinel = new Auth_OpenID_ServiceEndpoint();
+        $sentinel->claimed_id = 'monkeysoft';
+        $this->consumer->sentinel = $sentinel;
+
         $op_endpoint = 'Phone Home';
 
         $msg = Auth_OpenID_Message::fromOpenIDArgs(