OpenID discovery and login tests fail on HTTPS site

-    $this->addIdentity(preg_replace('@^https?://@', '', $identity), 2, $identity);
+    $this->addIdentity(preg_replace('@^http?://@', '', $identity), 2, $identity);

You should remove the ? as well - otherwise you'll match either "http://" or "htt://".

I suggest you change the comment from referring to “The URL scheme” to “The "http://" prefix”, and add a mention that this has no effect when the tests are run on an HTTPS site.

Looks good.

Sorry, this fell WAY off my radar.

And no longer applies. :(

Needed work anyway IMO.

What is the rationale to _skip_ testing normalization of the provided identifier when on HTTPS? Shouldn't we just force the tested URL to be http (via url's https option) instead? Or test normalization in openid_begin in a dedicated function?

please ignore the patch in #5. I did not intend to upload it as it is bogus.

I don't see how the OpenID specs are relevant here, my question was in regard to skipping testing; Tests should not be depend on or be skipped depending on site configuration. Coverage should be complete regardless of the environment. A unit test seems unavoidable.

OpenID involves a lot of HTTP-foo back and forth between client, server and OpenID Provider, so it's hard to test HTTP-only stuff if the test environment only runs HTTPS, as long as we cannot hook into the HTTP client (this feature is proposed in #786074: Allow hooks for drupal_http_request() request and responses).

But I wonder whether we could use hook_openid_normalization_method_info_alter() in this particular case to force the identifier back to https for the sake of testing?

Subscribing.

I think the patch in #11 is the right approach considering that OpenIDFunctionalTestCase::testDiscovery() is a functional test and not a unit test. We already have unit tests for normalization in OpenIDUnitTest::testOpenidNormalize().

Since D8 was kicked out, this needs to be fixed in D8 first. Rerolled last mfb's patch againtst D8.

.

#11: 803294-openid-https.patch queued for re-testing.

Sorry, tried to re-test the wrong patch. The patch in #14 does not seem to apply against 8.x. Marking as 'needs work'.

deimos:drupal-head dries$ git apply -p1 ../f.p 
error: patch failed: modules/openid/openid.test:78
error: modules/openid/openid.test: patch does not apply

-    $this->addIdentity(preg_replace('@^https?://@', '', $identity), 2, 'http://example.com/xrds', $identity);
+    $this->addIdentity(preg_replace('@^http?://@', '', $identity), 2, 'http://example.com/xrds', $identity);

This is wrong (the question mark should be removed). The previous patch had it right.

@c960657 you are right. My bad. Wrong 3-way merge. However I don't think this was the reason, why the test failed.... So I expect that it fails again... :-)

Well, the test failed in the following part. It is not working in D8 now because #1076366: OpenID discovery spec violation - fragments are removed from claimed id was commited.

+++ b/modules/openid/openid.testundefined
@@ -92,9 +92,11 @@ class OpenIDFunctionalTestCase extends OpenIDWebTestCase {
-    // Tell openid_test.module to respond with this identifier. We test if
-    // openid_complete() processes it right.
-    variable_set('openid_test_response', array('openid.claimed_id' => $identity));
+    // Tell openid_test.module to respond with this identifier. On HTTP test
+    // environments, the URL scheme is stripped in order to test that the
+    // returned identifier is normalized in openid_complete(). If fragment part
+    // is present in the identifier, it should be retained.

(11.2) If the Claimed Identifier was not previously discovered by the Relying Party (the "openid.identity" in the request was "http://specs.openid.net/auth/2.0/identifier_select" or a different Identifier, or if the OP is sending an unsolicited positive assertion), the Relying Party MUST perform discovery on the Claimed Identifier in the response to make sure that the OP is authorized to make assertions about the Claimed Identifier.

Only thing which we are doing here is stripping the fragment, but only for the purposes of assertion verification, we do not do any changes to Claimed Id here:

(11.2) If the Claimed Identifier in the assertion is a URL and contains a fragment, the fragment part and the fragment delimiter character "#" MUST NOT be used for the purposes of verifying the discovered information.

There is nothing about normalization during verification of the assertion which should somehow modify the authenticated identifier. Normalization should be provided only at the beginning, when normalizing the user input - ID which they claim to own - to the URL Identifier:

(7.2) The end user's input MUST be normalized into an Identifier, as follows: ...

And than after redirects (which Drupal doesn't respect that time...)

(7.2) URL Identifiers MUST then be further normalized by both following redirects when retrieving their content and finally 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.

So I stripped the last test alteration, the first two are ok and makes perfect sense. If the third one will be still a problem for the HTTPS testing environments we will need to think out other way how to fix it.

Powered by Dreditor.

Hm. This applies to 8.x, but not to 7.x. Could I get a 7.x patch as well?

Here is a patch for 7.x.

Updated patched for D7 and D8.

Looks good to me, and OpenID maintainers seem to agree.

Committed/pushed to 8.x.

Moving back to 7.x.

Committed and pushed to 7.x. Thanks!