openid.module needs tests

I need to re-evaluate this, but I couldn't write tests before because of this http://drupal.org/node/244652.

claiming this one

Bump. We really do need some sort of baseline tests for this functionality ASAP. It's causing regressions, like for example at #287178: Use hook_form_FORM_ID_alter() where possible.

This patch adds a dummy OpenID provider that is used for testing, as well as some unit tests for the internal functions. The code coverage is not 100%, but at least it's now more than 0% :-) The dummy provider probably needs to be extended a bit in order to support all aspects of the OpenID protocol.

We should include chx in on this dicussed as we have discussed this numerous times.

Now also tests account registration using OpenID. More tests should probably be added in #216101: OpenID fails to auto-register account: Username invalid, email required.

I haven't included a test for account registration when user_email_verificication is TRUE, because this currently doesn't work properly due to #395340: Email verification not enforced with OpenID auto-registration.

The patch currently fails testing due to regressions introduced by #347250: Multiple load users and #322344: (notice) Form improvements from Views.

Wow. Let's hurry up and get these in so we stop doing that. :)

Patch updated to use the new user_load_by_name(). Still waiting for #322344: (notice) Form improvements from Views to land.

Reroll. chx helped on IRC with the regression from #322344.

This is a very impressive patch, c960657. Nothing to add. A very good base on the road to have a fully tested OpenID implementation ;)

Congratulations!

You wrote an openid test provider in this few lines of code, unbelievable!

Oh. My. God. c960657, you are my *hero*!

So I really really really really want this in so we can stop breaking OpenID every 2 months, but because it's a very tweaky area that few (as in like you and walkah :P) understand, and because this test doubles as wonderful documentation about how it's supposed to work, I'm going to be a bit harsh on documentation requirements, etc. This is not to be a pain in the ass -- well ok, maybe a little bit ;) -- but because I care. :)

*cracks knuckles* (note: I read patches I don't understand backwards; sorry)

My main thing is let's get a summary line of comments for every 4-5 lines of patch. This makes the code more scannable to people who don't care about (or would be lost in) the details and just want an overview of what's going on. For example:

+/**
+ * OpenID endpoint; handle authenticate request.
+ */
+function _openid_test_endpoint_authenticate() {
+  global $base_url;
+
+  module_load_include('inc', 'openid');
+
+  // Explain what in the hell a "nonce" is. :)
+  $nonce = _openid_nonce();
+
+  // Build response structure for verification or whatever the point of doing this is.
+  $response = array(
+    'openid.ns' => 'http://specs.openid.net/auth/2.0',
+    'openid.mode' => 'id_res',
+    'openid.op_endpoint' => $base_url . url('openid/provider'),
+    'openid.claimed_id' => $_REQUEST['openid_claimed_id'],
+    'openid.identity' => $_REQUEST['openid_identity'],
+    'openid.return_to' => $_REQUEST['openid_return_to'],
+    'openid.response_nonce' => $nonce,
+    'openid.assoc_handle' => 'openid-test',
+    'openid.sreg.email' => 'johndoe@example.com',
+    'openid.sreg.nickname' => 'johndoe',
+    'openid.signed' => 'op_endpoint,claimed_id,identity,return_to,response_nonce,assoc_handle',
+  );
+  
+  // Send the whoozie whatsit to the snargaflooga.
+  $association = new stdClass;
+  $association->mac_key = variable_get('mac_key');
+  $keys_to_sign = explode(',', $response['openid.signed']);
+  $response['openid.sig'] = _openid_signature($association, $response, $keys_to_sign);
+
+  // Send them off to somewhere over the rainbow.
+  drupal_set_header('Content-Type', 'text/plain');
+  header('Location: ' . url($_REQUEST['openid_return_to'], array('query' => http_build_query($response, '', '&'), 'external' => TRUE)));
+}
+

(replace the silly things with actual things :P)

The goal is definitely NOT to repeat the OpenID spec verbatim, but merely to provide enough details that someone can follow along with the code. Right now, I'm completely lost, which means other people will be too. If we can document this to the point that an idiot like me can understand it, suddenly we could end up with many more people who could help with OpenID patches.

Functions that need this treatment include:
- _openid_test_endpoint_associate()
- _openid_test_endpoint_authenticate()
- testOpenidSignature()
- testCreateAccountWithoutEmailVerification()
- testLogin()
- testDeleteIdentity() - and should that just be testDelete() to match testLogin()?

+  return 'Lorem ipsum';

That's cute and all, but is this part of the spec? If not, could we make this something more descriptive?

+/**
+ * @file
+ * Dummy OpenID Provider used with SimpleTest.
+ */

Please, please can we expand this out with an overview of what the heck is going on in this file? I would love 2-3 paragraphs of text that summarize "This module exposes blargdy flargs to in order to test the smurfing and the smooching of the OpenID smarfleblatz. A typical request goes like... etc."

+++ modules/openid/openid_test.install	24 Apr 2009 15:50:10 -0000
@@ -0,0 +1,12 @@
+<?php
+
+// $Id: openid_test.install,v 1.4 2008/12/23 07:48:24 foo Exp $
+

Minor: that $Id$ line should be immediately after the php tag.

There's also no @file declaration here, but I notice we're lacking that on other .install files as well, so we'll clean that up and make them all consistent in a separate patch. Same thing with openid.test.

+    // Note that all of the following refer to the same endpoint, so only the
+    // first will trigger an associate request in openid_association().

What a wonderful opportunity to explain to some poor schmuck what in the heck a Yardis is. :)

On another note, it looks like there's also some trailing whitespace that's introduced in this patch; Let's get rid of that as well.

Thanks for the praise, everybody :-)

This patch adds a lot of comments explaining what is going on with references to proper parts of the relevant specs. I hope this makes it easier to understand what is going on, though I have looked too much at the code today to tell whether I missed something fundamental.

Ouch.

Doxygen comments adjusted to follow coding guidelines (I was just made aware of these in another issue).

<blink><marquee><font style="font-size: 98794893.8em">WOW!</font></marquee></blink> ;)

The comments in here are actually way more detailed than I was asking for, but that's not a bad thing at all. I'm fairly confident that with this, someone with OpenID spec knowledge but without Drupal knowledge could get a good sense of what's going on in here to add additional tests, as well as someone with Drupal knowledge but without OpenID spec knowledge could know where to look for more information if a piece of code they wrote broke tests in a particular area. I know I certainly understand this stuff much better now, than I did before I read this patch. :)

I noticed a couple of minor things reading through, and then I see no reason not to commit this. Sorry, this skips around quite a bit:

+ * The provider simply responds positively to all authentication request. In

should be "requests"

+ * OpenID endpoint; handle "associate" requests (see OpenID Authentication 2.0, section 8).

Doesn't wrap at 80 chars. The (see ..) part could be moved to the block below.

+   * Test creation of users using OpenID auto-registration with email
+   * verification disabled.

First line should only be 80 chars. Could be shortened to just "Test openID auto-registration with email verification disabled."

Incidentally, after a quick grep, it turns out we use also 'e-mail' and not 'email' in documentation.

+            <URI>'  . url('openid-test/endpoint', array('absolute' => TRUE)) . '</URI>

There are two spaces rather than one before that first concatenation operator.

There are a couple of lines that could use additional clarification, such as:

+    $this->user = $this->drupalCreateUser(array());

"User doesn't need special permissions; only the ability to log in."

+    $this->drupalGet('');

"Load the front page to get the user login block."

(alternately, maybe this could drupalGet('user/login') to make it more clear what's happening, unless your specific intent is to test the login block.)

+  protected $user;

Hm. I wonder if it's possible for that to conflict with the global $user variable ever? Should we call it web_user or something for consistency with other tests, and to prevent an unfortunate accidental namespace collision?

+    $this->assertRaw('<script type="text/javascript">document.getElementById("openid-redirect-form").submit();</script>', t('JavaScript form submission found.'));

Hm. That seems overly brittle. There's no kind of menu callback we can throw a drupalPost against?

I've re-rolled addressing webchick's points above.

Changed

+    $this->assertRaw('<script type="text/javascript">document.getElementById("openid-redirect-form").submit();</script>', t('JavaScript form submission found.'));

to

+    $this->assertTitle(t('OpenID redirect'), t('OpenID redirect page was displayed.'));

This is awesome in every aspect. Great code, great documentation, much needed. Committed to CVS HEAD. Thanks! Stunning work, c960657.

The openid_test.module and related files should be placed in the tests/ folder under openid, per #383600: Move tests into subdirectory.

This patch moves the files.

Committed. Thanks.