Redirecting fails when not logged in on the provider site

I have this problem when testing http://openidenabled.com/resources/openid-test/diagnose-server/

However, in my case, it fails to redirect when I am logged in.

The first test works: Associate (DH-SHA1 session)

The second test begins to work, I get this screen on my drupal:

OpenID login

You are being logged into http://openidenabled.com/resources/openid-test/diagnose-server/TestCheckidSetup/, would you like to continue?

I click Yes and rather than redirect me back to the source page, it returns me to my drupal authorization page with this broken message:

OpenID login

You are being logged into , would you like to continue?

I'm seeing exactly the same as ninjay. Running Drupal 6.14, with all modules up to date.

I tried with Drupal 6.13, and it works there. Seems like something has changed in Drupal core somewhere between Drupal 6.13 and 6.14.
Looking at the change logs, it seems that several vulnerabilities in the OpenID modules has been fixed, obviously one of them broke the OpenID provider module.

I didn't check the changelook, but some tracing. When I stepped it through, I found _openid_provider_sign(...) does not return openid.redirect_to anymore. I'm not deep into OpenID enough to understand this correctly yet.

Anybody did some other research?

subscribe

Subscribe

Problem

The form in question that is malfunctioning is openid_provider_form().

Analyzing openid_provider_form_submit() I found that $form_state['storage'] is empty when a user comes from the login form and not directly from the Relying Party. This is the case when the user does not have a valid session on the OpenID Provider: upon being redirected from the Relying Party to the Provider, the Provider redirects the user to the login form for authentication and then - on success - back to the OpenID endpoint where openid_provider_form is presented to her.

And here is the culprit:

#302240: button broken due to fix various problems when using form storage
http://drupalcode.org/viewvc/drupal/drupal/includes/form.inc?r1=1.265.2....

What used to be:

  if (!empty($form_state['storage']) || !empty($form_state['rebuild'])) {
    $form = drupal_rebuild_form($form_id, $form_state, $args);
  }

is now

  if ((!empty($form_state['storage']) || !empty($form_state['rebuild'])) && !empty($form_state['submitted']) && !form_get_errors()) {
    $form = drupal_rebuild_form($form_id, $form_state, $args);
  }

Up to Drupal 6.13 drupal_rebuild_form() would be executed as soon as there was a $form_state['storage'] present, with 6.14, the form needs to be submitted in order to invoke drupal_rebuild_form(). drupal_rebuild_form() in turn invokes form_set_cache(), thus the form and form state are cached for subsequent page loads.

OpenID Provider relies on this behavior for shuttling $response and $realm from openid_provider_form() to openid_provider_form_submit(), take a quick look at both functions and you will see that openid_provider_form() sticks $response and $realm into $form_state['storage'] where openid_provider_form_submit() expects them.

So the question is actually: how did this form work at all since Drupal 6.14?

Shouldn't openid_provider_form_submit() have failed in all cases? I mean if drupal_rebuild_form() was never executed, the form never cached, how could openid_provider_form have ever worked since 6.14?

The answer is: In one case, drupal_rebuild_form() is actually being executed.

This one case is when a user is already authenticated on the OpenID Provider. The user is being redirected from a form on the Relying Party to the OpenID Provider, so when the Provider endpoint openid/provider is hit, $_POST is set. The user is authenticated, so the page callback on openid/provider displays openid_provider_form to the user in the same page load.

Now, when FormAPI builds the form in drupal_get_form(), it sticks $_POST (the one sent by Relying Party) into $form['#post'] without any further validation - the validation happens later, for now it assumes that this POST data was submitted against the current form. This mistake will later lead FormAPI to assume that the form was submitted and thus it will execute openid_provider_form() and cache the form.

Here is how: Before the form is displayed, drupal_process_form() is executed which in turn invokes form_builder(). form_builder() makes sure a $form array is complete. It will execute _form_builder_handle_input_element() which will test for $form['#post'] to decide whether or not to attach a 'button' array to $form_state:

function _form_builder_handle_input_element() {
// ...
  if (!empty($form['#post']) && isset($form['#executes_submit_callback'])) {
    // ...
    $form_state['buttons'][$button_type][] = $form;
// ...

Later in form_builder() _form_builder_ie_cleanup() is invoked, which will use the presence of $form_state['buttons'] to set $form_state['submitted'] = TRUE;:

function _form_builder_ie_cleanup($form, &$form_state) {
  if (!empty($form['#type']) && $form['#type'] == 'form') {
    if (empty($form_state['submitted']) && !empty($form_state['buttons']['submit']) && empty($form_state['buttons']['button'])) {
      // ...
      $form_state['submitted'] = TRUE;
// ...

And BANG - when drupal_get_form() hits the test for $form_state['submitted'] it will evaluate to true and call drupal_rebuild_form() which in turn calls form_set_cache().

Don't ask me how I found out.

Solution:

The actual problems on the FormAPI level will need to be repaired, but independent from that, OpenID Provider will have to fix its use of FormAPI (after all it was relying on a bug to work).

The adjustment for OpenID Provider seems surprisingly simple:

1) unset $_POST before form is beingn built
2) set $form['#cache'] = TRUE to force FormAPI to cache the form regardless of submitted stage (see drupal_get_form()

Needs review.

Warning: I haven't read this issue in detail, nor researched its implications.

Nevertheless, this would explain the bug that was recently reported in the HS issue queue: #662266: Add submits forms after adding another item to a CCK field OR with JS disabled. How is it possible that the API got changed in a stable version?
Just thinking about going through all of FAPI again to find a way to get HS back to work makes me want to vomit. The fact that that FAPI is so complex to work with in advanced cases, says enough. It needs to be rewritten. FAPI makes me hate Drupal — a lot.
Yes, I know, don't talk, but do. But you've got to have the time. And there have to be enough people annoyed with the current state to leverage the willingness to spend a lot of time rewriting FAPI. So far, I've only encountered people who agree that a rewrite would be nice, but virtually nobody feels very strong about it. I hope this is the drop that makes the difference :)

Just stumbled upon this on Twitter.

+++ openid_provider.inc
@@ -142,6 +142,9 @@ function openid_provider_authentication_response($request) {
+    // Unset global post variable, otherwise FAPI will assume it has been 
+    // submitted against openid_provider_form.
+    unset($_POST);
     return drupal_get_form('openid_provider_form', $response, $realm);

I'm not familiar with this module, but if you don't want form processing to happen at all, then you may want to consider to not use drupal_get_form() in the first place. Use the internal Form API functions instead - most probably drupal_retrieve_form() in this case.

This review is powered by Dreditor.

Form processing is needed, unset($_POST); is in place to avoid the (accidental IMO) $form_state['submitted'] = TRUE; set by the IE cleanup function based on the presence of $_POST (details in #7, I know it's a long writeup...)

*Excellent* FormAPI research, Alex. Committed - thanks!

I see that this patch was committed on January 7, 2010:

http://drupal.org/cvs?commit=311138

Diffs:

- http://drupalcode.org/viewvc/drupal/contributions/modules/openid_provide...
- http://drupalcode.org/viewvc/drupal/contributions/modules/openid_provide...

But appears to be have been reverted (accidentally?) on February 14, 2010:

http://drupal.org/cvs?commit=328618

Diffs:

- http://drupalcode.org/viewvc/drupal/contributions/modules/openid_provide...
- http://drupalcode.org/viewvc/drupal/contributions/modules/openid_provide...

I've rerolled the patch against DRUPAL-6--1.

I was having the problem as described above. I applied the patch in #13 and it now works just fine.

Working here, too on two different deployments.

RTBC!

13 worked for me, too.

#13 works for me.

#13 worked for me too.

Kinda handy as I am about to demo it at http://drupal.org.uk/event/drupal-focus-enterprise/26-may-2010

Thank you! :)

We're running 3 production deployments on this patch atm. walkah - do you just want to commit this?

subscribe

ok. i applied the patch and now get redirected back to the correct site. but: i'm not logged in there!

both are drupal sites, one with openid_provider module and one with openid core module activiated.

i am using the url http://mydomain.net/user/1/identity as openid identity...

#13 works for me

nilsja: do you already have a working association? #306078: Using OpenID provider gives "Validation error" is a separate issue which might also impact you

helmo: at the openid provider under /user/1/openid-sites the openid-site where i want to login (btw, hot do you call this? openid client?) is listed. do you mean that with association? i don't get any error messages, just not logged in after successful redirecting back.

do you know any possibility how to better diagnose the problem? i didn't change anything at the core openid module. but on another drupal installation opeinid login works fine. driving me nuts.

@nilsja: Yes, thats what I meant with association. I'm at a loss myself for ways to better diagnose this. Unfortunately the diagnostics site from #1 is down, and single stepping a login is quite tricky.

Patch in #13 also fixed my redirection problems. Would love this to be commited.

committed to DRUPAL-6--1 ... sorry for the mixup, I knew I'd applied that before.