user_get_authmaps: respect proper key-value ordering as in description

Ooops. Forgot --no-prefix last time. Updated patch.

Attached is a unit test routine. Won't pass until the patch in #1 is applied.

----
For the curious, the bug comes from commit c407765a4dae4ab103f5a7b13ceaf9237efbe4a2
Author: dries
Date: Thu Apr 30 16:10:10 2009 +0000

- Patch #394594: DBTNG: User module by Berdir: additional conversion to the new database abstraction layer plus clean-up.
----

Beware: this is my first stab at writing a unit test, so it's probably not the prettiest. For example the assertEqual( ... , 0) should probably be assertFalse(...). Etc.

The unit test in #2 passes after application of the patch in #1 on my system. Changing status to needs review.

Combined patch is attached. Description in the patch follows:

955414: (user_get_authmaps) Fix key value pair ordering.

The comments for user_get_authmaps describe the function as
returning a key-value pair '%module' => '%authname'. This agrees
with functionality in past releases.

In #394594: DBTNG: User module, the key value pair ordering was (inadvertently)
reversed to be '%authname' => '%module'.

Included also is a unit test for user_get_authmaps and
user_set_authmaps which verifies that authmaps can be set and
removed, and their output has the proper key-value pair ordering.

moshe: I discovered the bug while working to update the CAS module to D7. I assume using authmaps is still the preferred way to interface with external authentication systems.

(Changing priority to major since this completely breaks external authentication schemes.)

Thanks moshe. The OpenID module in core does make use of the core authmap table, but uses it's own SQL code to query it (and hence is unaffected by this bug).

A follow up to patch in #7, this time replacing the static strings in the test routine with ones generated by $this->randomName().

I'm posting a summary of the problem in hopes of getting some action on this. The bug and patch are straightforward.

Summary of the bug:

The function user_get_authmaps claims to return "an associative array with module as key and username as value." However the current code returns an associative array with the username as key and module as the value, effectively reversing the the key/value pair.

This key/value reversal problem stems from a mistake in #394594: DBTNG: User module. You can clearly see the bug introduced in the diff

--- /cvs/drupal/drupal/modules/user/user.module	2009/04/29 08:04:24	1.981
+++ /cvs/drupal/drupal/modules/user/user.module	2009/04/30 16:10:10	1.982
@@ -1520,14 +1524,8 @@
  *   An associative array with module as key and username as value.
  */
 function user_get_authmaps($authname = NULL) {
-  $result = db_query("SELECT authname, module FROM {authmap} WHERE authname = :authname", array(':authname' => $authname));
-  $authmaps = array();
-  $has_rows = FALSE;
-  foreach ($result as $authmap) {
-    $authmaps[$authmap->module] = $authmap->authname;
-    $has_rows = TRUE;
-  }
-  return $has_rows ? $authmaps : 0;
+  $authmaps = db_query("SELECT authname, module FROM {authmap} WHERE authname = :authname", array(':authname' => $authname))->fetchAllKeyed();
+  return count($authmaps) ? $authmaps : 0;
 }
 
 /**

Recommended solution:

The fix is to replace fetchAllKeyed() with fetchAllKeyed(1,0).

The proposed patch in #11 fixes the key/value ordering and provides a unit test to ensure that user_get_authmaps and user_set_authmaps work as advertised. I'm uploading a rebased version of this patch since user.module has seen some changes in the last week.

Carlos8f and I reworked the unit test at this morning's coding session at BAD Camp.

carlos8f: You're right about removing the setUp() and $admin_user parts as they are not used.

Updated patch attached, which is #15 plus the changes carlos8f suggested in #16 and #17.

Edit: Okay that was weird... ended up with a double post some how.

#18, re-uploaded. I'm scratching my head about how the file just disappeared, but whatever.

And changing the status to needs review, to trigger the bot. The file seems to actually exist this time... ;)

Since there seems to be minimal interest in fixing this regression, here's a patch which rips out all of the external authentication functions ala moshe in #9... Why ship them in a broken state when you can just remove them instead?

(obviously, I'd prefer to just have #22 committed, but I'm a bit frustrated).

@rfay: I don't think this needs to be announced, as it is rarely used and a regression from D6 in the DBTNG conversion. Also, any module developer is likely to just mimic openid's implementation which never even calls the function.