OpenID Consumer/Relying Party support in core for 6.x

nice ... subscribing.

FYI, this fakeadd script seems to work for creating diffs that include new files

No patch. :(

rerolled without cruft

variable_get keeps a static, so there is no point in maintaining another one IMO.

woops. wrong issue.

@webcheck - read walkah's description for how to get the proposed patch. i think we can safely call this 'in review'.

There's a $5000 bounty for putting this in core...
Let's test this ;)

any progess here? HEAD has changed a fair amount so it is likelywe need a reroll.

Subscribing

Subscribe ;)

With DrupalCamp in Toronto next weekend, there might be time for a bit of a mini-sprint on this...

Still no patch.

Because I can not add directories via diff/patch (and there are 5 or so files), I ask reviewers to please just check out the HEAD version of the module from Drupal's contrib CVS.

It's here: http://drupal.org/project/openid or if you prefer here: http://cvs.drupal.org/viewcvs/drupal/contributions/modules/openid/

And subscribing.

I have heard Dries commented on the implementation in other channels already. I probably represent the openid clueless developer community quite well, so here are my few comments:

openid.module:

- no @file comment in the module code
- closing bracket with wrong indentation in hook_menu, form_alter() and elsewhere
- more code comments in form_alter() would help, so it is clear, why do you bypass the built in validation (I see, but it is better documented)
- indenting is fubar in form_alter() 'openid_link'
- why 'what is this?' for the input field description, it does not describe anything (but it should)
- why "openid.return_to" with the dot when other fields are with underscores
- "Unable to login. OpenID Discovery failed" dot missing at the end
- documentation is very sparse on what happens in the steps in openid_login_submit()
- what is the "op" in "op_endpoint"?
- $assoc_handle should be $association IMHO
- "OpenID Association failed" dot missing at the end
- "request authentication from the IdP", be a bit more verbose here please
- document why there are two redirections
- no code comments for most functions... like it is very hard to tell what this would do by the signature: openid_association_request($public)
- is openid_authentication_page() an OpenID provider page? an openid server? not easy to tell from just the code
- it is not clear what the two user_save()s in openid_authentication() do, why they are required.. I also would just use the array() syntax for building up the $edit array, but multiple single line values
- "Unable to create the account" no dot at the end (huh, am I mistaken that error messages should have a dot?)

openid.install:

- "OpenID needs the bcmath extension for encryption." should note that this is a PHP extension to install, not a Drupal module
- indenting in openid_install() is never right, and at the same time inconsistent in itself
- I have not seen backticks used in the CREATE TABLEs elsewhere, it might be against the coding standards too

openid.js:

- four spaces used for indentation

xrds.inc:

- no @file comment, no code comments on functions
- our xmlrpc implementation does not use global variables but uses a getter/setter pair of functions, so this is not really consistent

openid.inc:

- @file comment text is there, but not @file... line wrapped too early
- "From Appendix B: Diffie-Hellman Key Exchange Default Value" what appendix? from a book?
- constants and functions miss phpdoc
- concatenation coding style problems in openid_redirect_http()
- the arrays in openid_redirect_form() should have a comma at the end, and closing parenthesis on a newline
- lots of magic functions, like _openid_create_message(), where I have no clue what is happening without docs, why aren't the three if()s with same return values or-ed?
- hurray, docs for _openid_signature(), but, erm, not phpdoc syntax
- "7.4.4.1. Integer Representations" what does the 7... mean? a book section?

So I mostly miss documentation. Doc comments are hardly seen in this code. Also if this module serves two purposes: authentication against another openid provider and being a provider itself, the two roles should be separated in the code as far as I see. Mark functions belonging to either of the camps as such.

subscribing

Any update on this? I would really like to see this in Drupal 6.

Now that Schema is in I have a few cycles to think about something else. Like OpenID. :-) Some initial comments:

1. I could do without the animation and fade-in on the user login form.

2. I entered the openid "fake". I got the error message "Unable authenticate. OpenID Discovery failed." That should be friendlier.

3. I mistyped my openid as http://bjaspan.openid.com instead of bjaspan.myopenid.com. I got sent back to the username/password form. Since typos are likely, users should be sent back to the openid form in this case.

4. This got a bit confusing: My admin account has my email address (addr 1). My myopenid account has my email address (addr 1). When I tried to log in via openid, pressed "Allow Once" at myopenid.org, and got sent back to Drupal, Drupal gave me an error because the email address was in use, and it dropped me into the normal create-account form. Fine, I expected an error here. I entered a new username and a different email address (addr 2) and pressed Submit. The Drupal site said it emailed me instructions but I haven't gotten them (though that might be a local site config error). However, I then tried *again* to log in with that same openid, and this time it let me, as the account I manually created with a username/email address. However, I never did anything to confirm that account.

So, if the openid module is clever enough to say "that email addr is taken, but give us another one and we'll create an account based on your openid anyway," that's great, but it needs to say so instead of saying "instructions have been emailed to you" if I do not need to use those instructions.

5. I have not ported the Persistent Login module to HEAD yet, but we should make sure they interoperate. I do NOT want to have to type "bjaspan.myopenid.com" every time I visit a site to log in.

6. For that matter, if OpenID is going into core, Persistent Login should, too; both make Drupal easier to log in to and more secure.

Sub

How about something more like: "Sorry, that is not a valid OpenID. Please ensure you have spelled your ID correctly." ?

Yes, that is much better. My point was only that "OpenID discovery failed" isn't particularly useful; a user might think that signals some kind of back-end failure or whatever. Anything that says "invalid OpenID" is good.

Re: the scenario where I tried to log in for the first time via an OpenID that has the same email address as an existing account. I need to understand how OpenID in general works and how you've tied it into Drupal better before I am qualified to have an opinion.

Re: persistent login. I'm not suggesting blocking openid. Again, I need to look at the details of how openid+drupal works, particularly how the Drupal PHP session is handled, how the redirects to the openid server happen, and when the actual login occurs.

bcmath is an extremely slow library. GMP is far faster. Please consider bcmath only as an alternative if GMP is note available.

RE: speed of bcmath vs GMP -- in many distros, the GMP libraries are compiled incorrectly -- from the home page of the GMP site:

GMP is very often miscompiled! We are seeing ever increasing problems with miscompilations of the GMP code. It has now come to the point where a compiler should be assumed to miscompile GMP. Please never use your newly compiled libgmp.a or libgmp.so without first running make check

Practically speaking, this can bite you if you are installing on a box with faulty libraries because things won't work -- We experienced many hours of troubleshooting as a result of this problem using the JanRain OpenID libraries -- obviously, there was nothing wrong with the JR libraries, and everything worked fine once we recompiled the GMP libraries, but it's something that requires root access, thus making it beyond the reach of many shared hosting customers.

@bonobo You are correct that GMP can be a challenge to compile, but you are incorrect that it requires root access. The PHP and GMP running GetOpenID.com are compiled and run entirely in user mode.

http://drupal.org/node/23256 has been marked duplicate of this.

subscribing

Subscribing.

I just committed a few minor changes for new submit/validate handler argument order and for typo in openid.schema. Hope thats OK, walkah.

Unfortunately, I got 'login failed' when i tried to use my AOL OpenID. FYI, that URL is http://openid.aol.com/. that might be a result of finding a collision with the drupal username. not sure.

I tried with my new MyOenID ID and got the confusing misdirection to registration screen that barry mentioned. Then once I picked an unused nickname and email i got back to drupal home page with no message at all. i looked in users table and indeed my new user exists in users table. nice. i did expect to be logged in at this point. thats the user experience i'd like to see. i did notice that my site was requiring admins to approve accounts. i didn't ask for that - might be a core bug that is affecting this module.

anyway, i will keep on testing versions until this gets in.

@moshe weitzman If you need details from testing, you can try my company's OpenID server at getopenid.com. The server keeps pretty excessive logs of everything, including all requests and responses.

@walkah That's fine. I agree the GMP is not a block-worthy issue.

Impelement => Implement . When you have an array, the ); is indented to the variable not to contents of the array. Where is the JS fallback? Seeing a # is a sign of lacking one. l(t('What is OpenID?'), 'http://openid.net/') misses external=>TRUE. elseif( needs a space. $return_to = url('', array('absolute' => TRUE)); maybe <front> . if (function_exists('bcadd')) { if my understanding is correct, this should be a requirement not a runtime check.

Attempt to create a shared secrete <= secret.

user_save('', $form_state['values']); I would be happier with a NULL than a ''

In openid_authentication the last drupal_goto() maybe should be drupal_goto($form_state['redirect'])

Use $realm = $return_to ? $base_url : '' in the beginning of openid_authentication_request

openid.inc , instead of === null, use isset(), it's faster.

} else { is a coding style error.

false needs to be uppercase

doxygen comments everywhere -- comments are on a separate line not on the @param line

Fix PHP's braindead handling of POST data. Sure. But what braindeadness you are fixing? That keys can't contain a dot? If yes, then comment so.

// Provide bcpowmod support for PHP4 -- don't provide. Let this be our PHP5 poster child. Or at least warn that with PHP4 openid will be slow.

My only objection at this point is that my company might have to pay up as one of the OpenID bounty sponsors. :-)

Array closes were indented bad, I committed a fix for them. And I added a proper form redirect instead of just drupal_goto.

We still have function_exists('bcadd') this should not hold up the patch but I still would like to still this check moved to .install hook_requirements. But then again, I might be totally wrong and this module might be usable w/o bcmath -- I do not say I completely understand this fantastic module.

Unless I am mistaken, the actual "log the user in to Drupal, set global $user" occurs in openid_authentication() on line 383 in rev 1.20. I observe that (1) user_login_validate() is not called so blocked/denied users can still get in and (2) user_login_submit() is not called so the login is not watchdog'ed, database login time is not updated, hook_user('login') is not invoked (which is, I presume, why Persistent Login does not work with this module), and sess_regenerate() is not called.

Incidentally, Persistent Login handles calling user_login_submit() but not user_login_validate(). I definitely recall considering using user_login_validate() in PL for exactly the reasons listed above but decided against it and I do not recall why. If no one can think of a good reason we should *not* be applying user_login_validate()s rules, then we clearly should be.

So. I think that openid_authentication() should call drupal_execute('user_login', values_array) instead of just setting global $user. However, the _validate and _submit logic a bit tricky:

1. user_login_validate() checks for blocked/denied users. Then, only if $form_values['pass'] is set, it tries user_authenticate() and sets global $user. openid doesn't want that last bit, so just don't put a 'pass' in form_values.

2. user_login_submit() assumes that global $user is already set if the authentication succeeded.

I see three approaches to make openid work with this workflow:

1. Add a #validate handler to the user_login form that sets global $user but *only* when the form is generated by the drupal_execute() call in openid_authenticate(). AFAIK, this will require setting a global variable to tell openid_form_alter() when to add the #validate handler. Kinda ugly.

2. Set global $user before calling drupal_exeucte('user_login') then, if (form_get_errors()), validation failed (user is blocked) so reset global $user to anonymous.

3. Add a function to user.module to do it all nice and easily. OpenID and Persistent Login are both doing the same thing here: providing an alternative authentication and login path. They should both do the same thing. Centralizing it in user.module makes it more likely to be done right.

Or am I just completely missing that openid.module actually gets this all right?

Here is an example of the function I propose adding to user.module (note that I haven't even tried compiling this code much less tested it (I just noticed I'm passing an object instead of an array as the third arg to form functions, so that is at least one bug)):

/**
 * Perform standard Drupal login operations for a user object.  The
 * user object must already be authenticated (such as by OpenID or
 * Persistent Login).  This function verifies that the user account is
 * not blocked/denied and then logs the login, updates the login
 * timestamp in the database, invokes hook_user('login'), regenerates
 * the session, etc.
 *
 * @param $account
 *    An authenticated user object to be set as the currently logged
 *    in user.
 * @return
 *    TRUE if the login succeeds, FALSE otherwise.
 */
function user_external_login($account) {
  $form = drupal_get_form('user_login');
  $state = array();
  user_login_validate($form, $state, $account);
  if (form_get_errors()) {
    return FALSE;
  }
  global $user;
  $user = $account;
  user_login_submit($form, $state, $account);
  return TRUE;
}

openid_authentication() would then have logic like:

    if (!variable_get('user_email_verification', TRUE) || $account->access) {
      // used to be: $user = $account;
      user_external_login($account);
    }

Two new issues:

1. There is a link for "Log in using OpenID" but, once you click it, no link for going back to the user/pass form.

2. On my system (Intel CoreDuo, lots of RAM, Apache 2.2.3, PHP 5.2.0), trying to use an OpenID login sends Apache into a CPU spin cycle. It did not do this when I first tried it last week.

I knew about the problems in my user_external_login(), your fixes are fine, I'll follow up there.

Ooops! Another nit... the OpenID logo is currently a transparent PNG which displays poorly on IE6. I don't think alpha blending is really needed here so a transparent GIF should be fine instead. Or you could use the awful IE6-transparent-png CSS hack. A GIF is be better, IMHO.

Attached new png image with index transparency which should work better for IE6.

walkah and i did some real time QA and fixing on this patch. Here is my list of items which were still outstanding at the end. they may or may not be valid. this patch is very close.

[5:30pm] moshe_work: so, we have no mapping that will help if a the username or email already exist in drupal?
[5:30pm] moshe_work: "The name moweitzman is already taken.
[5:30pm] moshe_work: The e-mail address moweitzman@tejasa.com is already registered."
[5:34pm] moshe_work: and then i land on the user/register page which is a bit odd

[5:36pm] moshe_work: tabbing doesn't work right after you switch to openID mode on user/login

[5:44pm] moshe_work: user/x/openid could use some help text. how about "You may login to this site using an OpenID. You may add your OpenId URLs below, and also see a list of any OpenIDs which have already been added."

[5:45pm] moshe_work: notice: Undefined property: stdClass::$data in /Users/mw/htd/dr6c/sites/all/modules/openid/openid.module on line 503.
[5:45pm] moshe_work: notice: Undefined index: is_valid in /Users/mw/htd/dr6c/sites/all/modules/openid/openid.module on line 504.
[5:45pm] moshe_work: this happenned when i was logged in an tried to add an OpenID from my profile page
[5:46pm] moshe_work: the add did not work. looks like a bug to me
[5:46pm] moshe_work: user/10/openid page

james - the attached patch is now needed for openid: "add core drupal version to openid.info. is a new requirement in drupal6".

would be great to get an update on the few small items i posted.

Tested and committed.

Waiting for James' YouTube movie where he dances on his desk.

openid.module prevents any other module from form_alter()ing the login form and getting the results. See http://drupal.org/node/148419#comment-262273.

At Moshe's suggestion, I'm combining my openid.module patch for this problem with the user.module patch since they are so closely related. Both are posted at http://drupal.org/node/148419.

hook_openid() was never documented. Please fix.

once hook_openid is documented please update #148410: Actions in core as well

Still no docs. The hook appears in openid_authentication_request() in openid.module

Followup for the undocumented hook is in #345376: Document missing hook_openid() so we can stop pinging everyone in this issue with every update. Setting status back to the original status.