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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

wojtha’s picture

Status: Active » Needs review
FileSize
1.12 KB
wojtha’s picture

Test passed, but drupal.org doesn't respond this time because of network trouble, so test status wasn't updated.

c960657’s picture

This 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?

wojtha’s picture

Status: Needs review » Needs work
+++ b/modules/openid/openid.inc
@@ -188,32 +188,30 @@ function _openid_select_service(array $services) {
+      if ($type_priority && (!$selected_service || $type_priority < $selected_type_priority || $type_priority == $selected_type_priority && $service['priority'] < $selected_service['priority'])) {

I left the two loops as they were, because I wanted to avoid this long IF statement, any chance to simplify it?

+++ b/modules/openid/openid.inc
@@ -188,32 +188,30 @@ function _openid_select_service(array $services) {
+        $selected_type_priority = $type_priority;

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.

wojtha’s picture

Status: Needs work » Needs review
FileSize
3.62 KB

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

       if (($type_priority && !$selected_service)
          || ($type_priority < $selected_type_priority)
          || ($type_priority == $selected_type_priority) && ($service['priority'] < $selected_service['priority'])) {
        $selected_service = $service;
        $selected_type_priority = $type_priority;
      }

Good work!

c960657’s picture

Please check if I added the parentheses right...

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

wojtha’s picture

You didn't :-) ...

You see!? :-)

Cool, RTBC from me... but as an "idea author" of the patch I let someone uninvolved to change the status.

wojtha’s picture

Priority: Normal » Major

Bumping to major since it affects all http://specs.openid.net/auth/2.0/signon providers.

wojtha’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7

Bumping to D8.

wojtha’s picture

Heine’s picture

Status: Needs review » Reviewed & tested by the community

I'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.

c960657’s picture

I'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.

Heine’s picture

You are, of coure, right.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x and 8.x. Thanks wojtha, c960657 et al.

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

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