backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 39 forks source link

[DX] Contrib modules broken because of the removal of the authmap table. #2377

Open LeeteqXV opened 7 years ago

LeeteqXV commented 7 years ago

Yubikey as 2FA: (Yubikey 7.x-2.1)

I only replaced the core = 7.x line with backdrop = 1.x in yubikey.info, and then the activation of that module went ok ONLY when I extracted the files manually into the modules folder (upload/install module in the BDC gui did not work).

I entered the Yubikey settings and saved the settings page. Then when trying to access the yubikey user page to add a Yubikey, I got an error and that page could not be accessed. The error log revealed that the authmap table did not exist, so I exported a working and already active (for 2 uids) authmap table from a Drupal 7.x install, and imported it into the database using phpmyadmin.

Now Yubikey default settings with username + password + (optional) yubikey works with BDC 1.5.

Should the authmap table exist already in default BDC setup?

(I think it is used also for TFA / TFA_basic and others too. A previous test with TFA failed, but I did not try to enter the authmap table manually then. Will try that one of the coming days and see if also TFA module might work "out of the box".)

herbdool commented 7 years ago

authmap was deprecated early on in the development of Drupal 8: https://www.drupal.org/node/1863874 (Backdrop forked a little while after this so it inherited this).

The change record encourages module developers to come up with their own solution for managing external auth.

klonos commented 7 years ago

The change record encourages module developers to come up with their own solution for managing external auth.

Not sure if that should be our official policy on such matters. One of our promises to D7 users considering to move to Backdrop instead of D8 is backwards compatibility and this change certainly breaks contrib.

jlfranklin commented 7 years ago

Yesh. That will break a lot of other very useful modules, including LDAP and OAuth.

herbdool commented 7 years ago

This probably isn't a high priority for adding into core, mostly because I believe the core maintainers might see it as not necessary for the needs of the 80% of users. But this doesn't mean that someone can't add a contrib module where all it does is set up the authmap table and API (if there is any) so that these other modules can work. There's a similar solution in Drupal 8 from the looks of it.

klonos commented 7 years ago

@herbdool I think that backwards compatibility comes first before the 80/20 rule. See https://backdropcms.org/philosophy where we state that:

Backdrop values contrib developers over core developers.

Core developers should fight the urge to refactor, because small changes in core may result in large changes for contrib. ...

Also, on the same page you can see that in the list of our principles, "Backwards compatibility is important." is number 1, while "Write code for the majority." comes second.

klonos commented 7 years ago

...especially if all it takes is just the addition of a single table to our db schema in order for contrib to work with minimal change.

herbdool commented 7 years ago

I think this is an unusual case.

This change was not made in Backdrop; it was made in the early version of Drupal 8. So Backdrop isn't breaking compatibility with the immediate ancestor of the fork. This issue isn't about refraining from breaking something, but about doing actual work to re-add something that was in Drupal 7.

I can think of a few other drastic changes that were made in the early version of Drupal 8, which Backdrop inherited. Just one example which impacts my firm is the removal of xmlrpc. I doubt that xmlrpc would be a good candidate for re-adding into Backdrop core even if it's removal "broke" modules like Production Monitor. So we'll just need to find a workaround.

herbdool commented 7 years ago

Do you know for sure it was just a single table?

klonos commented 7 years ago

This change was not made in Backdrop; it was made in the early version of Drupal 8. So Backdrop isn't breaking compatibility with the immediate ancestor of the fork. This issue isn't about refraining from breaking something, but about doing actual work to re-add something that was in Drupal 7.

Yes, but you are missing the point here. We are "promising" or "advertising" backwards compatibility with Drupal 7. You might have already heard either @quicksketch or @jenlampton state in their Backdrop presentations that although the project was forked from the D8 branch, this was done at a very early stage and that we should essentially consider Backdrop a fork of Drupal 7. Also, our contrib states that when porting a project over from d.org, you should start off the 7.x branch.

Do you know for sure it was just a single table?

Nope, ...and from the change record, it seems that other things like some functions were removed too. This is the patch that was committed.

jlfranklin commented 7 years ago

Reaching back to when I did some work on the Simple LDAP module and some client SSL auth, I'm 99% sure it was just a single table in core. Simple LDAP added its own auth table, and its own user auth functions. All external auth modules needed to create replacement functions to authenticate users.

I can't say I blame D8 for removing it or Backdrop for leaving it out. External auth was always clunky at best, and adding more than one external auth mechanism was difficult. It would be nice to have a clean and well-defined external auth framework in Backdrop, something that is easy to register external auth systems, add auth and identity functions, map profile fields, handle two-factor, seamless single sign-on (not just common user databases), hybrids (think: twitter login + Salesforce profile), etc.

Doing it right is a lot of work. We could just put back the authmap table, but that would just encourage the old baggage. I doubt external auth would cross the 80% threshold, but it could easily cross 50%. Logging in via Facebook, Twitter and/or Google is a pretty popular feature on the modern web. Salesforce, LDAP, and Exchange integration is not just for enterprise-class companies anymore. Non-profits love to keep their customer data in Salesforce, and SaaS offerings like Office 365 or Google Apps are how most small businesses manage their IT until they grow up enough to need in-house servers.

herbdool commented 7 years ago

Thanks @jlfranklin. Sounds like there are some good reasons to leave it out of core until there's a better approach.

@klonos Backdrop lists the core modules that it removed: https://api.backdropcms.org/node/28538 (or that the early fork removed). It doesn't list xmlrpc or authmap. Some, like forum, now exist as a contrib project. From what I understand most will not be added back to core even though their removal breaks compatibility for those people who were using them.

LeeteqXV commented 7 years ago

On 29/12/16 20:32, John Franklin wrote:

Doing it right is a lot of work. We /could/ just put back the authmap table, but that would just encourage the old baggage. I doubt external auth would cross the 80% threshold, but it could easily cross 50%. Logging in via Facebook, Twitter and/or Google is a pretty popular feature on the modern web. We need something that works already now, not a new idealistic future framework that might be in -alpha state the next 2+ years. We already have something that works. It is not a problem if it is not elegant or ideal, which is for the next stages of BackDrop to handle. But we need a functional starting point...

What already works in D7 is:

a) the TFA/TFA basic modules (read: TOTP/GoogleAuthenticator) b) Yubikey module c) several others as well (no point listing them all)

Current state of affairs..: The world is about to realize 3 things:

  1. None of the big operators are secure, they are getting hacked one by one, so the wish to connect and trust Twitter logins or the like TO the BackDrop site (not only to connect the other way) is diminishing, at least until both those external sites AND we in our end both have secure 2FA solutions in place.

  2. Since both small and large sites gets hacked - actually on a weekly basis these days - (regardless of budget), we need to make sure that our accounts are secure in a way that prevents anyone from using login credentials from other services to log in to our web sites. A practical problem here is that it is very hard - or impossible - to make people stop using the same password/email on several sites. 2FA is a perfect if not the only viable solution to this situation.

  3. TOTP and any such low-level security addons that only deals with (short) numbers are not secure enough. (Even 2FA is not secure enough; we soon need Multi-Factor, and the world has already started moving in that direction.) For now, we can use Yubikey's own OTP when we want strong 2FA, but soon during 2017 we will see more support for the new, mature, international FIDO U2F open standard, (which the new Yubikey models also support, btw) and then we will have a really great 2FA/Multi-Factor base.

@LeeteqXV

herbdool commented 7 years ago

The only way this is likely to move this forward is for someone here to make a PR so the core maintainers can review it and decide if it should go into core or if it should be in contrib.

jenlampton commented 7 years ago

One of our promises to D7 users considering to move to Backdrop instead of D8 is backwards compatibility

That's not true, there's no promise of 100% backwards compatibility - our philosophy merely states that it's important to us (where it may not be in Drupal 8). The goal is 80% backwards compatibility, and this is part of the 20% that changed. (In hindsight, we may not have chosen to change this had we forked from D7 head - but this is where we are today...)

This probably isn't a high priority for adding into core, mostly because I believe the core maintainers might see it as not necessary for the needs of the 80% of users. But this doesn't mean that someone can't add a contrib module where all it does is set up the authmap table and API (if there is any) so that these other modules can work.

I agree with this, 100%.

The only way this is likely to move this forward is for someone here to make a PR so the core maintainers can review it and decide if it should go into core or if it should be in contrib.

I'd rather not see developer time wasted on a PR for core in this case - if there is a problem that needs to be solved today, I'd rather see a contrib module that replaces the missing functionality from core, or better yet, moves that functionality directly into LDAP or OpenID or the module that needs it. We can look at our usage stats for Backdrop 2.x and see if it's something that truly does need to be moved back into core or not.

herbdool commented 7 years ago

Sounds like a separate contrib project is the way to go. For whoever wants to take this one, the relevant commits to adapt are likely: http://cgit.drupalcode.org/drupal/commit/?h=8.0.x&id=881b8a6a17ccac75bd0f8d94ee97439fea36ad38 and https://www.drupal.org/files/817118-109-Remove-authmap-and-migrate-OpenID.patch for the code that's removed and would need to be recreated under a contrib module namespace, http://cgit.drupalcode.org/drupal/commit/?h=8.0.x&id=56a7c639e311d6a2c4a420048c3a73dd13070003 for tests that can be adapted for Backdrop and https://www.drupal.org/node/2634840 for adding indices to the authmap table (from Drupal 7).

It looks like Drupal 8 didn't remove the authmap table in the end, just deprecated it and told developers to put their data into their own tables: http://cgit.drupalcode.org/drupal/tree/core/modules/user/user.install?h=8.0.x&id=881b8a6a17ccac75bd0f8d94ee97439fea36ad38. This doesn't change the fact that none of it is included in Backdrop right now and the easiest route to get this functionality back is to create a contrib module for adding back the API.

jenlampton commented 7 years ago

the easiest route to get this functionality back is to create a contrib module for adding back the API.

There was no good reason to share this database table across multiple auth sources, and thus no need for an API either. The easiest route to get things working is just to add the table that's needed to the module that needs it. See facebook OAuth for an example.

herbdool commented 7 years ago

There was no good reason to share this database table across multiple auth sources, and thus no need for an API either.

Even better.

herbdool commented 7 years ago

So this is by design, not a bug per se. I'm marking it as needing a change record.

VasasA commented 3 years ago

The ported LDAP module uses its own ldap_authmap table. It is declared by ldap_user.install

joelsteidl commented 2 years ago

I'm creating a module called "authmap" that adds the database table back and some of the removed functions get_authmaps, external_load etc.

I'll post a link here once it is ready.

joelsteidl commented 2 years ago

I'm not sure if I'm missing deprecated functions, but I created a generic module that creates the authmap database table and replaces the user_ functions that I know about.

Does anyone know of a list of user_ functions that were deprecated?

https://github.com/backdrop-contrib/authmap

jenlampton commented 2 years ago

Did you already look through these change records? https://docs.backdropcms.org/change-records?field_impacts_value=Module+developers&keys=user_

klonos commented 2 years ago

@joelsteidl which function specifically are you having issues with?

joelsteidl commented 2 years ago

@klonos No issues. Just wondering if I included all of D7's functions that have to do with the authmap table. See https://github.com/backdrop-contrib/authmap/blob/1.x-1.x/authmap.module

I included all the ones I needed for porting SimpleSAML PHP, but if there are others that I'm missing, I wanted to go ahead and include them.

I found the changelog where the authmap table was removed from the install file, but that's it.

klonos commented 2 years ago

Well @joelsteidl the table was removed from early versions of Drupal 8, and Backdrop inherited the changes. The change record for D8 in https://www.drupal.org/node/1863874 mentions only the 4 same functions that you seem to have already added in the module (going by what's being mentioned in the README.md file).

joelsteidl commented 2 years ago

Thanks @klonos! I updated the authmap module to make it more clear that it was removed from Drupal 8.x.