Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#31 | openid-tests-803294-31.patch | 2.19 KB | oriol_e9g |
#28 | 803294-28_openid_tests_https_site-D7.patch | 2.19 KB | c960657 |
#28 | 803294-28_openid_tests_https_site.patch | 2.21 KB | c960657 |
#27 | 803294-24_openid_tests_https_site-D7.patch | 2.41 KB | c960657 |
#24 | 803294-24_openid_tests_https_site.patch | 2.19 KB | wojtha |
Comments
Comment #1
c960657 CreditAttribution: c960657 commentedYou 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.
Comment #2
mfboops good point :p Also revised comments as suggested
Comment #3
c960657 CreditAttribution: c960657 commentedLooks good.
Comment #4
webchickSorry, this fell WAY off my radar.
And no longer applies. :(
Comment #5
Heine CreditAttribution: Heine commentedNeeded 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?
Comment #6
Heine CreditAttribution: Heine commentedplease ignore the patch in #5. I did not intend to upload it as it is bogus.
Comment #7
mfbAccording 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."
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.
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.
Comment #8
Heine CreditAttribution: Heine commentedI 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.
Comment #9
c960657 CreditAttribution: c960657 commentedOpenID 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?
Comment #10
mfbI'd like to see tests passing on HTTPS so I actually tried this out. Seems even uglier to me but here's a patch.
Comment #11
mfbI 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).
Comment #12
colanSubscribing.
Comment #13
c960657 CreditAttribution: c960657 commentedI 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().
Comment #14
wojtha CreditAttribution: wojtha commentedSince D8 was kicked out, this needs to be fixed in D8 first. Rerolled last mfb's patch againtst D8.
Comment #15
c960657 CreditAttribution: c960657 commentedComment #16
catch.
Comment #17
Dries CreditAttribution: Dries commented#11: 803294-openid-https.patch queued for re-testing.
Comment #18
Dries CreditAttribution: Dries commentedSorry, tried to re-test the wrong patch. The patch in #14 does not seem to apply against 8.x. Marking as 'needs work'.
Comment #19
wojtha CreditAttribution: wojtha commentedComment #21
c960657 CreditAttribution: c960657 commentedThis is wrong (the question mark should be removed). The previous patch had it right.
Comment #22
wojtha CreditAttribution: wojtha commented@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... :-)
Comment #24
wojtha CreditAttribution: wojtha commentedWell, 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.
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:
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:
And than after redirects (which Drupal doesn't respect that time...)
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.
Comment #25
c960657 CreditAttribution: c960657 commentedComment #26
webchickHm. This applies to 8.x, but not to 7.x. Could I get a 7.x patch as well?
Comment #27
c960657 CreditAttribution: c960657 commentedHere is a patch for 7.x.
Comment #28
c960657 CreditAttribution: c960657 commentedUpdated patched for D7 and D8.
Comment #29
sunLooks good to me, and OpenID maintainers seem to agree.
Comment #30
catchCommitted/pushed to 8.x.
Moving back to 7.x.
Comment #31
oriol_e9gComment #32
c960657 CreditAttribution: c960657 commentedComment #33
webchickCommitted and pushed to 7.x. Thanks!