During Yadis service discovery OpenID v1 OP service with lower priority is chosen instead OpenID v2 Claimed id service.
Example XRDS:
<?xml version="1.0" encoding="UTF-8"?>
<xrds:XRDS xmlns:xrds="xri://$xrds" xmlns:openid="http://openid.net/xmlns/1.0" xmlns="xri://$xrd*($v*2.0)">
<XRD>
<Service priority="0">
<Type>http://specs.openid.net/auth/2.0/signon</Type>
<Type>http://specs.openid.net/extensions/pape/1.0</Type>
<URI>https://mojeid.cz/endpoint/</URI>
<LocalID>https://mojeid.cz/id/oDjCDzoP4u/</LocalID>
</Service>
<Service priority="10">
<Type>http://openid.net/signon/1.1</Type>
<Type>http://specs.openid.net/extensions/pape/1.0</Type>
<URI>https://mojeid.cz/endpoint/</URI>
<openid:Delegate>https://mojeid.cz/id/oDjCDzoP4u/</openid:Delegate>
</Service>
<Service priority="20">
<Type>http://openid.net/signon/1.0</Type>
<Type>http://specs.openid.net/extensions/pape/1.0</Type>
<URI>https://mojeid.cz/endpoint/</URI>
<openid:Delegate>https://mojeid.cz/id/oDjCDzoP4u/</openid:Delegate>
</Service>
</XRD>
</xrds:XRDS>
mojeID.cz provider doesn't provide Openid OP v2 service, instead of that it provides identified by Claimed Identifier Element as the main preferred service. However current Openid implementation check in first loop only for service identified by OP Identifier Element. So it founds v1 service as a preferred one instead of the v2 service, because OP Identifier Element is searched only if no OP service is found and <Service priority="0">
doesn't apply in this case.
This bug appears in D7 only, D6 behavior is ok - D6 Openid implementation is using the preferred service (http://specs.openid.net/auth/2.0/signon
) .
Comment | File | Size | Author |
---|---|---|---|
#6 | 1076414-openid-selected-service-2.patch | 3.2 KB | c960657 |
#5 | 1076414-openid-selected-service-1.patch | 3.62 KB | wojtha |
#3 | 1076414-openid-selected-service.patch | 3.16 KB | c960657 |
#1 | openid_preferred_service_1076414.patch | 1.12 KB | wojtha |
Comments
Comment #1
wojtha CreditAttribution: wojtha commentedComment #2
wojtha CreditAttribution: wojtha commentedTest passed, but drupal.org doesn't respond this time because of network trouble, so test status wasn't updated.
Comment #3
c960657 CreditAttribution: c960657 commentedThis patch does almost the same but I think the resulting code is cleaner because it only uses one loop and makes the priorities of each type more obvious. What do you think?
Comment #4
wojtha CreditAttribution: wojtha commentedI left the two loops as they were, because I wanted to avoid this long IF statement, any chance to simplify it?
Selected type priority isn't used further in the code.. Edit: Sorry, this is alright. forgot that it is in the loop, so it is used in the long condition ^^^.But one loop less is definetely better :-)
Powered by Dreditor.
Comment #5
wojtha CreditAttribution: wojtha commentedI try to reformat it using intendation and parentheses to make it more readabe, I checked http://www.php.net/manual/en/language.operators.precedence.php to make me sure couple of times :-)
Please check if I added the parentheses right... I know that parentheses is not required in this case, but the operators precendece is tricky sometimes, especially in long conditions like this one. Added parentheses makes the conditions more readable and easy understandable without some hard thinking. Now it is clear on the first sight whats going on IMO.
Good work!
Comment #6
c960657 CreditAttribution: c960657 commentedYou didn't :-) I have added some parenthesis to my original patch. The indentation is unusual in core, but it seems “compatible” with the few occurrences of multi-line if conditions.
Comment #7
wojtha CreditAttribution: wojtha commentedYou see!? :-)
Cool, RTBC from me... but as an "idea author" of the patch I let someone uninvolved to change the status.
Comment #8
wojtha CreditAttribution: wojtha commentedBumping to major since it affects all http://specs.openid.net/auth/2.0/signon providers.
Comment #9
wojtha CreditAttribution: wojtha commentedBumping to D8.
Comment #10
wojtha CreditAttribution: wojtha commented#6: 1076414-openid-selected-service-2.patch queued for re-testing.
Comment #11
Heine CreditAttribution: Heine commentedI've confirmed that this selects a service according to both specs. A potential optimization would be to break on finding an OP identifier element, but I doubt we'll notice in practice.
Comment #12
c960657 CreditAttribution: c960657 commentedI'm not sure I understand your suggestion. We must iterate over all Service elements in order to find the one with the highest (numerically lowest) priority, so we cannot break early.
Comment #13
Heine CreditAttribution: Heine commentedYou are, of coure, right.
Comment #14
Dries CreditAttribution: Dries commentedCommitted to 7.x and 8.x. Thanks wojtha, c960657 et al.