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
Comment | File | Size | Author |
---|---|---|---|
#14 | flatten-request-1379056-14.patch | 1.3 KB | Barrett |
#11 | flatten-request-1379056-11.patch | 1.08 KB | Barrett |
#4 | flatten-request-1379056-4.patch | 1.08 KB | Barrett |
#1 | flatten-request-1379056.patch | 1.11 KB | Barrett |
Comments
Comment #1
Barrett CreditAttribution: Barrett commentedThe attached patch adds a simple flattening function and invokes it prior to returning the request array from openid_authentication_request().
Comment #2
damien_vancouver CreditAttribution: damien_vancouver commented+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:
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:
and function openid_redirect_form, openid.inc line 55: (this provides the Javascript form for OpenID 2.0 redirects)
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:
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?
Comment #3
c960657 CreditAttribution: c960657 commentedThe 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().
Comment #4
Barrett CreditAttribution: Barrett commentedThe attached patch is a re-roll of the patch in comment 1 against current D7 and in light of input in comment #3.
Comment #5
bobbyaldol CreditAttribution: bobbyaldol commented#4: flatten-request-1379056-4.patch queued for re-testing.
Comment #6
bobbyaldol CreditAttribution: bobbyaldol commented#1: flatten-request-1379056.patch queued for re-testing.
Comment #7
bobbyaldol CreditAttribution: bobbyaldol commentedCouldnt 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.
Comment #8
bobbyaldol CreditAttribution: bobbyaldol commented#4: flatten-request-1379056-4.patch queued for re-testing.
Comment #9
Barrett CreditAttribution: Barrett commented@bobbyaldol: you shouldn't need to apply both patches, only the most recent one which is attached to #4.
Comment #10
bobbyaldol CreditAttribution: bobbyaldol commentedThis is the error I am having while applying the patch(the most recent one as you said).
Comment #11
Barrett CreditAttribution: Barrett commented*grumbles about trailing whitespace*
Comment #12
c960657 CreditAttribution: c960657 commentedI 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.
Comment #13
c960657 CreditAttribution: c960657 commentedComment #14
Barrett CreditAttribution: Barrett commentedImproved comments in attached patch.
Comment #15
c960657 CreditAttribution: c960657 commentedComment #16
David_Rothstein CreditAttribution: David_Rothstein commentedHm, 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?