OpenID discovery and login tests fail on HTTPS sites, because openid.test strips the scheme from the identity URL and then normalizes by adding on "http://".

In this patch, "http://" is stripped and "https://" is preserved, which allows normalization to work correctly in both cases.

Files: 
CommentFileSizeAuthor
#31 openid-tests-803294-31.patch2.19 KBoriol_e9g
PASSED: [[SimpleTest]]: [MySQL] 37,292 pass(es).
[ View ]
#28 803294-28_openid_tests_https_site-D7.patch2.19 KBc960657
FAILED: [[SimpleTest]]: [MySQL] 39,332 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#28 803294-28_openid_tests_https_site.patch2.21 KBc960657
PASSED: [[SimpleTest]]: [MySQL] 34,031 pass(es).
[ View ]
#27 803294-24_openid_tests_https_site-D7.patch2.41 KBc960657
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 803294-24_openid_tests_https_site-D7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#24 803294-24_openid_tests_https_site.patch2.19 KBwojtha
PASSED: [[SimpleTest]]: [MySQL] 29,863 pass(es).
[ View ]
#22 803294-22_openid_tests_https_site.patch2.45 KBwojtha
FAILED: [[SimpleTest]]: [MySQL] 29,789 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#19 803294-19_openid_tests_https_site.patch2.45 KBwojtha
FAILED: [[SimpleTest]]: [MySQL] 29,773 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#14 803294-14-openid_https_D8.patch2.41 KBwojtha
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 803294-14-openid_https_D8_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#11 803294-openid-https.patch2.55 KBmfb
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 803294-openid-https.patch.
[ View ]
#10 https-openid.patch2.54 KBmfb
PASSED: [[SimpleTest]]: [MySQL] 25,594 pass(es).
[ View ]
#5 do803294-force-http.patch772 bytesHeine
PASSED: [[SimpleTest]]: [MySQL] 25,577 pass(es).
[ View ]
#2 openid-https.patch2.25 KBmfb
PASSED: [[SimpleTest]]: [MySQL] 20,427 pass(es).
[ View ]
openid-https.patch1.53 KBmfb
PASSED: [[SimpleTest]]: [MySQL] 20,424 pass(es).
[ View ]

Comments

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new2.25 KB
PASSED: [[SimpleTest]]: [MySQL] 20,427 pass(es).
[ View ]

oops good point :p Also revised comments as suggested

Status:Needs review» Reviewed & tested by the community

Looks good.

Status:Reviewed & tested by the community» Needs work

Sorry, this fell WAY off my radar.

And no longer applies. :(

StatusFileSize
new772 bytes
PASSED: [[SimpleTest]]: [MySQL] 25,577 pass(es).
[ View ]

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.

What is the rationale to _skip_ testing normalization of the provided identifier when on HTTPS?

According to OpenID specs, if the URL is missing a scheme, it should be normalized by prepending the "http://" scheme. If a HTTPS Identifier URL is desired, "it is RECOMMENDED" to issue a redirect from HTTP to HTTPS so that if the user leaves off the scheme and URL is normalized to HTTP, then HTTPS Identifier URL will still end up being used. A URL that does have a scheme should not be normalized to HTTP, because "Relying Parties MUST differentiate between URL Identifiers that have different schemes."

Shouldn't we just force the tested URL to be http (via url's https option) instead?

It wouldn't be valid to force the URL to be HTTP. The site might only be available via HTTPS, and it might not have the recommended redirects in place from HTTP to HTTPS. And HTTPS URLs should be supported directly.

Or test normalization in openid_begin in a dedicated function?

I do think it would be a good idea to have unit tests for normalization, rather than relying on the test environment being available on a HTTP URL so that the scheme can be left off and the URL normalized to HTTP.

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?

StatusFileSize
new2.54 KB
PASSED: [[SimpleTest]]: [MySQL] 25,594 pass(es).
[ View ]

I'd like to see tests passing on HTTPS so I actually tried this out. Seems even uglier to me but here's a patch.

Status:Needs work» Needs review
StatusFileSize
new2.55 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 803294-openid-https.patch.
[ View ]

I also rerolled the original patch for consideration, given that there are already some unit tests for URL normalization, and already some tests that don't run on HTTPS test environments (see common.test and session.test).

Subscribing.

Status:Needs review» Reviewed & tested by the community

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

Version:7.x-dev» 8.x-dev
Status:Reviewed & tested by the community» Needs review
StatusFileSize
new2.41 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 803294-14-openid_https_D8_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

Status:Needs review» Reviewed & tested by the community

Issue tags:+needs backport to D7

.

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

Status:Reviewed & tested by the community» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new2.45 KB
FAILED: [[SimpleTest]]: [MySQL] 29,773 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 803294-19_openid_tests_https_site.patch, failed testing.

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

Status:Needs work» Needs review
StatusFileSize
new2.45 KB
FAILED: [[SimpleTest]]: [MySQL] 29,789 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 803294-22_openid_tests_https_site.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.19 KB
PASSED: [[SimpleTest]]: [MySQL] 29,863 pass(es).
[ View ]

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.

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new2.41 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 803294-24_openid_tests_https_site-D7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Here is a patch for 7.x.

StatusFileSize
new2.21 KB
PASSED: [[SimpleTest]]: [MySQL] 34,031 pass(es).
[ View ]
new2.19 KB
FAILED: [[SimpleTest]]: [MySQL] 39,332 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Updated patched for D7 and D8.

Status:Needs review» Reviewed & tested by the community

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

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Committed/pushed to 8.x.

Moving back to 7.x.

Status:Patch (to be ported)» Needs review
StatusFileSize
new2.19 KB
PASSED: [[SimpleTest]]: [MySQL] 37,292 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 7.x. Thanks!

Status:Fixed» Closed (fixed)
Issue tags:-needs backport to D7

Automatically closed -- issue fixed for 2 weeks with no activity.