I have two modules which implement hook_openid() to add/alter request parameters.

What I'm seeing is that when both modules set the same request parameter, the resulting value after module_invoke_all() completes is an array rather than a single value. This is to be expected given the call to array_merge_recursive() but it means that the resulting request is of invalid format and is rejected by IdP's.

Seems to me the solution is to "flatten" arrays like that prior to calling openid_redirect() by taking the last value when the value is an array.

EXAMPLE:
========

Output of hook_openid from module1:
--------------------------------
Array
(
[schmoopie] => foo
)

Output of hook_openid from module2:
--------------------------------
Array
(
[schmoopie] => bar
)

Request as returned by openid.module:openid_authentication_request()
------------------------------------------------------------
Array
(
[schmoopie] => Array
(
[0] => foo
[1] => bar
)
)

Response from verisign
-------------------
ns:http://openid.net/signon/2.0
mode:error
error:BAD_REQUEST

Files: 
CommentFileSizeAuthor
#14 flatten-request-1379056-14.patch1.3 KBBarrett
PASSED: [[SimpleTest]]: [MySQL] 39,741 pass(es).
[ View ]
#11 flatten-request-1379056-11.patch1.08 KBBarrett
PASSED: [[SimpleTest]]: [MySQL] 39,065 pass(es).
[ View ]
#4 flatten-request-1379056-4.patch1.08 KBBarrett
PASSED: [[SimpleTest]]: [MySQL] 39,049 pass(es).
[ View ]
#1 flatten-request-1379056.patch1.11 KBBarrett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch flatten-request-1379056.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new1.11 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch flatten-request-1379056.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

The attached patch adds a simple flattening function and invokes it prior to returning the request array from openid_authentication_request().

Version:6.22» 8.x-dev
Status:Needs review» Needs work

+1 for this being a bug, and your proposed solution to it as well.

The OpenID 2.0 Final specification defines messages as single value key=value pairs. My Emphasis in bold:

4.1. Protocol Messages

The OpenID Authentication protocol messages are mappings of plain-text keys to plain-text values. The keys and values permit the full Unicode character set (UCS). When the keys and values need to be converted to/from bytes, they MUST be encoded using UTF-8 [RFC3629].

Messages MUST NOT contain multiple parameters with the same name.

see: http://openid.net/specs/openid-authentication-2_0.html#anchor3

Yet Drupal 6 openid.module is treating a value that may be an Array() as a scalar, in both openid_redirect() and openid_redirect_http().

openid_redirect_http(), openid.inc line 30:

<?php
 
foreach ($message as $key => $val) {
   
$query[] = $key .'='. urlencode($val);    // BUG: If two modules answered hook_openid(), this $val may be an array()!
 
}
?>

and function openid_redirect_form, openid.inc line 55: (this provides the Javascript form for OpenID 2.0 redirects)

<?php
 
foreach ($message as $key => $value) {
   
$form[$key] = array(
     
'#type' => 'hidden',
     
'#name' => $key,
     
'#value' => $value,   // BUG:  $value may contain an array()
   
);
?>

So as can be clearly seen from inspecting the code, it's easy to un-intentionally violate the protocol when two modules both set the same request parameter from hook_openid().

I agree with Barrett that the openid module should take steps to prevent this. The correct behaviour should be that the module with the highest weight that answers hook_openid() last will "win". This is accomplished with the patch from #1.

Finally, this behaviour is present in HEAD as well. Line 776 of 8.x core/modules/openid/openid.module also contains:

<?php
$request
= array_merge($request, module_invoke_all('openid', 'request', $request));
?>

I'm therefore marking this as 8.x. We should fix it there, then backport the bugfix to D7 and D6.

I will come up with a simpletest we can use to replicate the problem and verify the fix. It might as well test the entire request object and make sure it conforms to the specification of unique key names with scalar (single) values.

Meanwhile, can someone offer some feedback on Barrett's solution of flattening the array? Perhaps there is a better way or a more Drupal way?

Version:8.x-dev» 7.x-dev

The bug does not exist in D8. It was fixed in #765984: Convert hook_openid('request', ...) to hook_openid_request_alter(). But that issue changes the API, so we cannot backport it to D7 as is.

I think the idea about just using the last value is fine. AFAICT the values in $request are either strings or arrays of strings (assuming that modules are well-behaved and only return arrays of strings), so there is no need to for _flatten_request_element() to call itself recursively(). Let's just keep everything inside openid_authentication_request().

Status:Needs work» Needs review
StatusFileSize
new1.08 KB
PASSED: [[SimpleTest]]: [MySQL] 39,049 pass(es).
[ View ]

The attached patch is a re-roll of the patch in comment 1 against current D7 and in light of input in comment #3.

#4: flatten-request-1379056-4.patch queued for re-testing.

#1: flatten-request-1379056.patch queued for re-testing.

Couldnt apply both patches. The output being 'patch corrupt' and apparantly it does have some errors which are critical.
For example in http://drupal.org/node/1379056#pift-results-1379056 at line:40 comments are not closed and on line: 39 the '}' should follow '+' and it didnt.

#4: flatten-request-1379056-4.patch queued for re-testing.

@bobbyaldol: you shouldn't need to apply both patches, only the most recent one which is attached to #4.

root@bob:/var/www/drupal/drupal# git apply flatten-request-1379056-4.patch
flatten-request-1379056-4.patch:30: trailing whitespace.
error: patch failed: modules/openid/openid.module:787
error: modules/openid/openid.module: patch does not apply

This is the error I am having while applying the patch(the most recent one as you said).

StatusFileSize
new1.08 KB
PASSED: [[SimpleTest]]: [MySQL] 39,065 pass(es).
[ View ]

*grumbles about trailing whitespace*

+  // Make sure the request only has simple key=>value pairs, not arrays
I think this comment needs a bit of elaboration to stress that arrays appears when two modules specify the same parameter, i.e. not only if some broken module returns an array. I suggest you look into the comment in rdf_get_namespaces() for inspiration. Don't forget the trailing period.

Apart from this nit, I think this patch is ready to fly.

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new1.3 KB
PASSED: [[SimpleTest]]: [MySQL] 39,741 pass(es).
[ View ]

Improved comments in attached patch.

Status:Needs review» Reviewed & tested by the community

Title:if more than one hook_openid implementation alters a given parameter, the resulting value is an array and request is invalidif more than one hook_openid implementation returns a given parameter, the resulting value is an array and request is invalid
Priority:Normal» Major
Status:Reviewed & tested by the community» Fixed
Issue tags:+7.22 release notes

Hm, as far as I can tell, this bug was actually worse - if you followed the hook_openid() documentation example and returned the whole $request object (rather than just the parameters you're adding) this would occur any time two modules both implemented the hook, rather than only if they happened to alter the same parameter.

I'm a little concerned there's no test coverage for this (or explicit hook documentation about the behavior), but then again there is no existing test coverage for the hook and it might be hard to test...

So I committed this in the interest of getting the bug fixed, and because it seemed straightforward and worked as expected on manual testing. Thanks! http://drupalcode.org/project/drupal.git/commit/0bcf41a

Maybe there can be a followup for tests and/or documentation?

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