Remove {authmap} and migrate OpenID entries to their own table

+1

I don't think it would hurt to require the module parameter.

Somewhat related: I still believe user_external_login_register is completely whack, but I was unable to convince anyone else. #765222: user_external_login_register does not look right

{authmap}.authname is a unique key, so I'm unsure what you are trying to fix here.

What the heck? Is that a throwback to the old days before there was a module column?

I changed my module, which used to have authname's like "1234@facebook", to authname's that are simply "1234" because it makes the queries in my module much more efficient. Now only in a comment dismissing this issue do I learn that I've made my module to be incompatible with other modules which use authmap!

Seriously, something has to change... documentation or code. Why add the module column if all it does is add overhead?

@Dave Cohen: indeed, you need to use 1234@facebook, not 1234. The module column is there to all modules to quickly find all the authentication maps that belong to them (OpenID uses that, for example, to display the list of identifiers you have mapped to your account).

This stuff is mostly undocumented, I believe, so starting by documenting it would be great. As far as I know, there are no performance penalty either way, so I see no compelling reason to change that in D7, which is long API frozen.

Back in the D5 days, I used "1234@facebook.com" because that's what another module (facebook auth) used, and I wanted to be compatible with that module.

Now that I understand the module column, I realize that I can no longer use that convention for authmaps, because it is taken by another module. So because the module column was added, but not part of the unique key, I have to ensure that my convention for authnames is truly globally unique.

On the bright side, simply using "1234" should work fine. Since no one else will use just a number. It pays to be first! :)

I realize it's too late for D7, but I suggest in D8 we remove the unique key 'authname' and replace it with a unique key 'authname, module'. And then make the module a required parameter for user_external_load() and all other user_external... functions.

Any objection to that change in D8?

What do you guys think of this solution ? #920908: {authmap}.authname should not be unique.

Instead of having a unique key 'authname, module', having a module which implement a schema: aid, uid, authname. Then the authname column fit perfectly with the external provider and is not a varchar(128) anymore, and we can have this unique key.

So back to this D7 issue -- waht are you saying should be documented, and where should it be documented? Is this a change from D6? Or should we close this issue as "won't fix"?

I've split the documentation needs for D7 into #1118422: Document unique key on authname in {authmap}, so that we can use this issue for discussing {authmap} in D8.

Assuming we aren't sticking with the status quo, there are several options:

1. Require 'authname, module' everywhere instead of just 'authname':
   From #817118-11: Remove {authmap} and migrate OpenID entries to their own table, Sylvain Lecoy suggests:

   Instead of having a unique key 'authname, module', having a module which implement a schema: aid, uid, authname. Then the authname column fit perfectly with the external provider and is not a varchar(128) anymore, and we can have this unique key.

2. Remove {authmap} entirely:
   From #955414-9: user_get_authmaps: respect proper key-value ordering as in description, moshe claims:

   Thanks. I'd actually recommend maintaining your own map table. These last vestiges of dist auth will be removed in D8. I don't really see much benefit in using this core table.

   and later in #955414-13: user_get_authmaps: respect proper key-value ordering as in description:

   Yes, I think that OpenId should gets its own map table as well in D8. There is no longer any benefit to centralization of mappings since login never consults this table. Contrib modules and OpenID need to inject custom validators into the login form and do login check there. Actually, some contrib modules may choose to forego the map table if they know that drupal username will always match external username.

Drupal for Facebook uses its own table now. So I have no objection to removing authmap entirely.

Central Authentication Services also uses its own table, due to these annoying limitations with {authmap}.

Let's go removing the authmap completely.

Here's a first attempt at removing {authmap} and moving OpenID entries to their own table.

Subscribing.

Fixed the errors.

One kink still to work out is that openID will need to provide the equivalent of user_external_load(). For the moment, I've named it openid_external_load(), but that name should be improved. openid_user_load() is obviously out of the question due to the conflict with hook_user_load().

We need to provide an unified way to authenticate with external tools. Not re-copying openid.module routines in each new external auth module.

In http://drupal.org/node/990800#comment-3794804 Damien suggest reimplementing the openid routines, but i think we should find a better way to handle that and avoid code duplication.

Sylvain: would you recommend then, some sort of Authentication class which modules could subclass to implement specific details related to their implementation?

Authentication class, or hooks as we are used in drupal yes, but more importantly an api for handling module interaction with user module.

Registration is a good example, with external provider there is thousand ways of registering your new user with drupal, with different information (certain providers doesn't give the user email address for instance, other do, with birthdate and so forth). But at the end, we just want to register our new user in the user database, that mean with user.module routines.

So we need a low level API function to handle that. Our new module can alter the registration form by adding extra fields and saving information in its own table, but the final step should be handle by user module. See my reply #4 here: #990800: user_external_login_register should explicitely return operation performed, do you see the slight difference between my oauth implementation of openid_authentication ? Don't you feel like there is a bit of code duplication ? If our modules will be loosely coupled they won't have high cohesion due to code duplication and not reusing common methods.

We all know that a good component oriented software is made of two ingredients: loose coupling and high cohesion. The patch is good, but we need more work or open a new ticket to have helpers function for registering, logging in users, (and logout as well ?).

There is a related issue here: #556380: Remove OpenID from core

ldap for d7 uses authmap. doesn't need to. the user.module provides most of the hooks and functions of need to ldap authentication. user_external_login_register could use a little improvement and the bug #1022362: \Drupal\user\Form\UserLoginForm::validateAuthentication function does not follow user module validation rules made it difficult to use more than one external authentication at the same time.

There is room for improvement to user_external_login_register for sure.

The authmap table and related functions need to go. They were in core for 10 years (my first patch!) and they never got used in a significant way. Contrib should make own tables and own CRUD.

I confirm in my OAuth consumer project that I can't use the authmap at all.

As soon as you get more than one externel account per user, the schema and methods to retrieve user auth can't be used. Authmap works fine for a 1-1 user relationship. If you want to map multiple external accounts for the same user, you can't use the user_set_authmaps. See: #927812: user_set_authmaps() using uid+module_name pair as a key in db_merge() call

If we want to keep the authmap, we need to allow multiple accounts per user:

• user_set_authmaps should be rethinked to not use anymore db_merge. uid + module name pair as a key prevents saving multiple account for the same uid / module. Using db_insert and return the aid can greatly help modules which rely on aid.
• module column allows only one application per module. In the case you have multiple applications for the same module (staging, dev, and prod for instance) you can't rely neither on the authmap. This should be a "per application" authname. That is, an entity_id column.
• A live example is coming here: http://drupal.org/sandbox/SylvainLecoy/1127118

PK: aid, eid
FK: uid

aid : application ID (Drupal entity)
eid : external ID (ensured unique by the provider)
uid : user ID

The two former composes the primary key. The primary key is made of an external ID, for a local application. This reflect the reality and allow multiple external id, for a same user, represented by an application (Drupal entity).

Sylvain: So do you think we should rehab {authmap} or just get rid of it?

Authmap is closely related to users, it allows authentication. User is a core module of drupal.

If we look closely at how authmap is used, there is only 6 matches for the word authmap in OpenID module, and about 16 matches in the user module (not including tests). They are dispatched between those functions: user_set_authmap, user_get_authmap, user_external_login_register (note that this function is considered deprecated by the community).

If OpenID remain in core, then we should keep the authmap. We need then redesign it, this is how I did for OAuth:

Suppose you have an "OAuth Application" entity into drupal:

Then, you can have an account into Twitter (admin) that allows read/write operations and Twitter that doesn't allows it. Moreover, because you are an admin, you have multiple account for Twitter (admin) that you need to use to authenticate your main Drupal user. To do that, the proposed authmap should be:

PK: aid
FK: uid
FK: oid
FK: authname (provider)

Unique constraint: oid, authname

aid : Primary Key: Unique authmap ID.
uid : User's {users}.uid.
oid : Application {oauth}.oid which is controlling the authentication.
authname : Unique authentication name.

The two first records are for the same application, but with two different users. This was precisely not possible with the previous authmap system due to unicity over the key "authname". By moving the unicity per application based, you remove this limitation. The two later are a same user, for a different application. This was possible but only per-type application in the previous design.

If we want to keep the authmap, the system need a few changes, in particular I believe that we need an application system. Having application entity in core is another debate, which is commented here: #1255674: [meta] Make core maintainable

In conclusion: In my opinion, if we want an application system (which is basically just add the support of a new Entity "Application" in core) then we should rehab the authmap. If we want to delegate such a system to contrib, then we should remove authmap from user, and let OpenID inherits from the previous authmap.

subscribe

I was about to open a new issue but I remember having posting this somewhere.

What do you guys think of re-hab the authmap on a "per-entity" basis instead of a "per-module" basis ?

We could either provide the "application" framework for registering new applications, or let the abstract eid column for people implementing their own solutions. @see #31

This would let the dev defining multiple entities (such as application) per module: for instance staging, production, admin, vip, etc.

Personally I think either core needs a complete framework for handling external authentication, one which overcomes all of the difficulties you describe in #31 and one that is easily extensible to new frameworks; or each external provider should provide their own "authmap"-esque table. As you describe above, many modules have to provide their own "authmap" table currently just to overcome the overly restrictive unique key issue.

I think what you suggest in #31 is insufficient. It's a step in the right direction, but I think we either need to go all the way with this or stop pretending that we provide this service at all.

right. authmap has been in its current state for over 10 years. it isn't getting better. kill it and bring it back if contrib ever makes a better system.

So here we are then.. !

Need review.

Looking good.

Do we usually declare a new table twice? Seems like the hook_update_n could call into openid_schema() to get the table definition it needs.

No, it can't. If we add another column, then the first update function will already create the table with the new column and then the follow-up update function will try to re-add it.

There are some tricks to avoid it, views.module for example as a long history of numbering hook_schema() too and always return the latest one, then update functions can call into one specifically but core currently isn't doing that.

Let's try this again.

+++ b/core/modules/user/user.installundefined
@@ -628,5 +585,25 @@
+
+/**
+ * Implements hook_update_dependencies().
+ */
+function user_update_dependencies() {
+  // user_update_8011() first need the openid_update_8002() to ensure migration
+  // has been done.
+  $dependencies['user'][8011] = array(
+    'openid' => 8002,
+  );
+
+  return $dependencies;

I aussme this is breaking the update tests. It should be the other way round. This should be defined in openid.install so that it only runs when openid was ever enabled. Otherwise, this blocks user.module from updating I think.

Oh, that's a smart change, I was about to implement a test hock with a module_exists but your solution is more interesting :)

Another option would be to leave the table on upgrades. Because it is possible that someone out there *is* having data in there and we don't want to remove that. That's what we'll have to do with the variables table for example. Maybe rename it somehow. #347988: Move $user->data into own table is doing something similar for the user.data column and moves it into a prefixed table.

Yes, the table definitely has to be kept.

We usually rename tables that hold backup data for other modules to migrate, but normally, that goes hand in hand with a new table schema for the original table, which does not happen here. In this case, I think we can safely retain the table as-is.

So here a patch that keep the table and adds a deprecated warning.

go testbot!

Feel free to take over the patch and fix the typo about the authmap table, add comment, or any piece that are missing :)

Any reviewers ?

Looks good to me, but then again I've been wanting to strip out the authmap table for a long time. Nice to see those tests I wrote a few years ago finally be removed!

Feel free to put as RTBC ;)

Thanks so much for the work. Let me ask, why is this not a text field on the user entity?

re: #56: I don't think a field would be appropriate, because, it's not like the auth data would be optional or configurable, and it also applies to a single entity type only — if you enable OpenID module, then you expect it to work for all users without further configuration.

What I don't really understand with this patch is the end result with regard to multiple external authentication providers.

Currently, we're able to support multiple providers and authmap sort of works like a "registry" of which providers are associated with a particular account. This e.g. allows easy lookups but potentially also migrations from one provider to another (which obviously depends on the provider, but nevertheless, possible).

Furthermore, the authmap table works just fine as-is for various authentication provider modules. E.g., I'm co-maintaining the vBulletin bridge module, which (also) leverages the authmap table to register users that have been "imported" from vB. (The module is doing more, but that's the most basic part of it.)

I thought we already discussed this and my proposition to rehab the authmap was denied after #31.

That's why we completely removes it, contribs modules can implements their own authmap. However, I am in favor of rehab the authmap and I have ideas for this, but right now we have to decide because I can't go back and forth writing patches as the wind direction change.

What I don't really understand with this patch is the end result with regard to multiple external authentication providers.

Currently, we're able to support multiple providers and authmap sort of works like a "registry" of which providers are associated with a particular account. This e.g. allows easy lookups but potentially also migrations from one provider to another (which obviously depends on the provider, but nevertheless, possible).

Our point of integration for multiple external auth providers is the login form validation. Modules each have a shot at setting authenticated=TRUE if one of them does, then the form is validated and login proceeds. Thats all that core should offer IMO. We tried to provide the "registry" for 10 years and noone worked on this system. I have some basis for stating this. The authmap system was my first contribution to Drupal in 2001.

If folks have ideas for a registry, then lets make registry in Contrib and propose it for core in D9. Authmap in core is not working as a growing ecosystem.

I'm with Moshe. I tried to use authmap in Drupal for Facebook. It couldn't be made to work well, so now that module creates in own table mapping uids to external ids. And it's fine that way. I won't miss authmap and I'd rather see all this Drupal brain power solving other problems. Just sayin'.

I don't want to hold this up. I haven't fully reviewed the patch in depth though, so I can't tell whether this is ready.

Just a couple minor points then this is RTBC ....

+++ b/core/modules/openid/openid.install
@@ -133,3 +173,62 @@
+  // Migrate entries from {authmap} to {openid_identities}.
+  $query = db_select('authmap', 'a')
+    ->condition('module', 'openid');
+  $query->addField('a', 'uid');
+  $query->addField('a', 'authname', 'identifier');
+  db_insert('openid_identities')
+    ->from($query)
+    ->execute();
+
+  // Remove old entries in {authmap}.
+  db_delete('authmap')
+    ->condition('module', 'openid')
+    ->execute();
+}

Lets only execute the delete if the db_insert() succeeded. I think a little safety here is wise.

+++ b/core/modules/openid/openid.module
@@ -88,6 +88,32 @@
+/**
+ * Implements hook_user_delete().
+ */
+function openid_user_delete($account) {
+  db_delete('openid_identities')
+    ->condition('uid', $account->uid)
+    ->execute();
+}
+

I think we usually type hint hook implementations like this. So add a type hint for User. Also applies to other hook implementations.

Actually, db_insert() throws an exception on error, not return FALSE so nothing to do there. I think we just need to add type hinting and then RTBC.

What do you mean by type hinting, in the core, there is no hook_user_delete(User $account) or something; Even in user.api.php the param $account is neither type hinted in the doc or in the code. Also, I am not sure this belong to this issue actually.

I think we haven't updated most of the user hooks and neither the api documentation, unlike nodes for example. So let's make it a follow-up task to do it everywhere?

OK, type hinting in a follow-up as per #65.

#1800174: Add missing type hinting to User module docblocks is already open to address type hinting in User module.

Reviewed this patch and it looks good. However, it doesn't seem to apply. Asking for a re-test.

#50: 1801176-50-Remove-authmap-and-migrate-OpenID.patch queued for re-testing.

#50: 1801176-50-Remove-authmap-and-migrate-OpenID.patch queued for re-testing.

#50: 1801176-50-Remove-authmap-and-migrate-OpenID.patch queued for re-testing.

"failed to checkout from [git://git.drupal.org/project/drupal.git]".

I see its cnflicting with new changes introduced, I don't have time to reroll this after head. If someone wants to chip in...

Can someone take over this issue ? I don't have time to reroll to purchase HEAD right now. I think the changes are minors enough to be handled by anyone.

Oops wrong tag

Rebased the RTBC patch in #50 on the current 8.x branch, fixing the two conflicts. No other changes.

Thanks bfroehle.

#79: 1801176-79-Remove-authmap-and-migrate-OpenID.patch queued for re-testing.

This still looks good (re #68) and applies now. Committed to 8.x. Thanks for the re-roll.

Will need a change notification.

Also there doesn't seem to be upgrade path tests added here?

Also, the table wasn't actually removed from the schema. I totally agree with not *deleting* it on upgrade, but it has no place in a fresh new D8 install. (Can't believe chx didn't notice this ;-))

What need to be tested exactly ? I'll write those.

Would you suggest to remove the table once d8 ship ?

Well, I wouldn't actually remove the table for existing tables at all (until D9). But currently each new site that you install still creates an "authmap" table which doesn't get used at all, but just sits there cluttering up your SHOW TABLES :-). That's just pointless and also Contrib modules that are using authmap now will have no reason to not use it in D8, if it's still sitting around there anyway.

I opened #1860986: Drop left-over tables from 8.x for removing 8.x tables, have added {authmap} there. However tstoeckler is right it should be removed from user_schema() now so it doesn't get created for new installs. This will also speed up test runs a little.

For tests, we'd add some dummy data to the 7.x dump in the {authmap} table, this ensures the insert query isn't a no-op. Then an added bonus would be to assert that the data in the new table is valid.

Ok i'll catch it in a follow-up:

• Remove the {authmap} from the user_schema() definition so it does not get installed on fresh new site.
• Ensure dummy data is migrated from {authmap} to {openid_identities}.

Should I open a new issue for this or just provide a patch here ?

Also, how is the process of writing a change notice ?

You can post the patch here directly.
Writing a change notice is as easy (or hard) as filling out: http://drupal.org/node/add/changenotice

I like this kind of test as I find it amusing to write, however, I am not sure about the usefulness of this one as it will introduce overhead in the - already-long - drupal testing queue.

I couldn't test on local (I got a problem when I execute test on zend server + windows via the Simpletest interface) but it passes with run-test.sh. By the way if someone here can help me fix that, I can't manage to run a single test from the Admin Interface screen :(

Here is a draft for change notice: http://drupal.org/node/1863874

Still cannot test on the admin interface, still passing on command line... so wierd.

Ahhh you guys changed the way we activate modules :))

Any tips on how to activate the 'openid' module ?

You need to simulate it already being installed by adding a corresponding row into the system table in drupal-7.authmap.database.php. Also, the comment in that file talks about user data.

OpenID updates functions aren't supposed to install this table when I call performUpgrades() ?

The openid_identities tables is not supposed to exist in a Drupal 7 setup. Are you sure I need to add the schema ? On OpenID module installation, the schema should exist normally, so what is wrong ?

EDIT: Oh you mean that $modules is a list of modules already installed, and are not actually installed, so that's why it need to be simulated through drupal7.authmap.database.php ?

No, the update function are supposed to add the new schema, but they are not executed in your test because openid.module is not enabled in drupal-7.bare.minimal.database.php.

I just checked and it is enabled in drupal-7.bare.standard_all.database.php, so it should work if you use that one.

Ok I'm going to still use the drupal-7.bare.minimal.database.php dump, and copy the openid related schema in the drupal-7.openid.database.php.

Could not test this in local :( so let's see what the bot says.

And now the table already exists, meaning that the update function is called :-)

Removed the openid_identities from the dump..!

Really painful not to be able to test in local :(

This would definitely pass.

+++ b/core/modules/openid/lib/Drupal/openid/Tests/Upgrade/OpenIDAuthmapUpgradePathTest.php
@@ -0,0 +1,68 @@
+
+    foreach ($expected_identities as $aid => $expected_identity) {
+      $this->assertEqual($expected_identity, $db_identities[$aid]);
+    }

This way we are not testing that the third entry in authmap, which is not by openid.module does not end up in the openid_association table (or however it's called). I think we should eather loop directly over $db_entities (which would result in a "Index does not exist error") or additionally assert that count($expected...) == count($db...). I'm not sure which one I prefer.

+++ b/core/modules/openid/tests/upgrade/drupal-7.openid.database.php
@@ -0,0 +1,94 @@
+ * This dump enable the openid module, the drupal-7.bare.minimal.database.php.gz

Should be "This dump enables openid module", i.e. "enable" -> "enables" and "the" can be dropped.

Marking "needs work" at least for the first item.

Sure, here we are!

Looks good!

Thanks! Committed/pushed to 8.x.

This removed user_external_load() without a lick of documentation.

Nevermind, I found the change record it just doesn't show up for user_external_load. Now it does. Sorry for the noise.

Removing spurious tags - https://www.drupal.org/node/1207020