Remove basic_auth

basic_auth got into core with #1890878: Add modular authentication system, including Http Basic; deprecate global $user.

I think we should not move it to contrib, as it is very useful for working with REST services in D8. If we only have session cookie authentication in core then it is a lot harder for people to test and implement web service consumers that need to authenticate. When you perform write operations using session cookies you need to supply an X-CSRF-Token header with a security token which is tedious.

Having this in core means that the developer experience to get started with REST is a lot better, since people don't have to search for a contributed module first to get started faster.

Otherwise this is a tiny module that does not do any harm and we have people that are volunteering to maintain it.

"Won't be needed on many sites" is true for about 2/3 of the modules in core. We want to have a non-user-in-a-browser-auth option in core out of the box, if for no other reason than to ensure we don't break the auth system; "3 implementations" and all that. It's also, as Klausi said, good to give developers something to build off of very easily.

I suspect Basic Auth will be more used in practice than PostgreSQL support, but we still have that in core. :-)

Needs more discussion. Specifically #2160021: Basic auth has no login flood control reminded me of this.

@Crell, thanks for the reminder, had been meaning to open that issue for a couple of months at least #2160433: [policy, no patch] Move PostgreSQL driver support into contrib.

Flood protection is on it's way. Should we close this issue?

No, this issue needs to remain open. Have people actually tried to use the module in different scenarios? Here is a scenario where it fails:

1. Create new d8 site
2. Log in
3. Enable basic_auth module
4. Log out
5. Put d8 site behind AuthBasic password protection (because people do that especially for dev sites) - this usually consists of some username/password pair that is not likely to correspond with a user/password on the site.
6. Go to site (will need to enter AuthBasic credentials)
7. Log in

If you followed these steps correctly, you will be greeted with the following:
Fatal error: Call to undefined function drupal_session_regenerate() in /path/to/drupal8/core/modules/user/user.module on line 1024. If you refresh the page, you will once again see the user login form as you have 'not logged' in.

(The line number may change depending on the changes to user.module)

Why this occurs:

With both authentication systems running, basic_auth gets first shot at validating user; it fails. Cookie plugin tries to validate user and passes (and redirects). System is now stuck in limbo since basic_auth takes over page after the validation/redirect, it sees that credentials were passed for AuthBasic user. user_login_finalize calls on drupal_session_regenerate but since session.inc was not included, it dies.

Since basic_auth is running, it doesn't really make sense for a session to be created for authentication (it is after all getting credentials from the AuthBasic format. So then either user module should not be generating the session (though I have no idea on that subsystem) or basic_auth contains critical flaws that it really should not be a part of core.

I think we should not move it to contrib, as it is very useful for working with REST services in D8. If we only have session cookie authentication in core then it is a lot harder for people to test and implement web service consumers that need to authenticate.

I'm not really understanding this reasoning. Having something in core that could easily break your site is far worse than having advanced functionality for "ease of development" (people using services are more advanced users than those that are not and this module with basic_auth makes it sound like a person is going to get httpbasic protection). Modules like this should either end up in contrib or, if it is for a learning experience, in examples module.

I should have also mentioned on a followup note: including session.inc might solve the fatal error. But the user (on a site protected by htpasswd) still doesn't get logged in (due to their htpasswd credentials not matching any user that might be in the site). Thinking this through more, if the module is to remain in core, it needs one (or more) of the following:

• Implement httpauth into the module (see Shield for what I mean.)
• Implement the ability for basic auth to only run on certain endpoints (though again, this is not solving the real issue described in #9)

If you're not going to do this:

• Move to contrib.
• Move to examples

And since this was brought up:

We want to have a non-user-in-a-browser-auth option in core out of the box, if for no other reason than to ensure we don't break the auth system; "3 implementations" and all that.

Hide the module from being enabled in drupal core then. Implement basic_auth_test (which is essentially the module as-is with function name changes and the hidden flag set to true in the yml file) so you don't break the auth system.

Having something in core that could easily break your site

I think trying to fix this is better than just removing it, no? :)

afaik, the routing system already supports changing the auth type per route anyway - so that is covered. We just need to worry about fixing the issue you have outlined in #9.

Fixing the issue outlined in #9 means one of the possible options outlined in #10. Changing the auth type per route (I did not know about this and looked up the router test) is interesting but doesn't solve #9.

trying to fix this is better than just removing it, no? :)

The issue would also be that fixing this may not be an option.

The problem is basically on ANY site that authenticates and passes on PHP_AUTH_USER and PHP_AUTH_PW credentials (see http://www.php.net/manual/en/features.http-auth.php). So if, for example, my dev site is password protected by AuthBasicProvider, it is sharing the same credentials as what basic_auth expects. basic_auth takes over the page since it sees this credentials exist and will always try to validate against them. And this means possibly (a) no live user can log into the site (this can be partially resolved by only availing the provider on certain paths) and definitely (b) services endpoints powered by basic_auth behind a password protected site will never work.

Adding the configuration for this to protect your site in apache/nginx is easy. Any site/server managed by plesk allows you to password protect sites through a UI. Drupal service providers like Pantheon allow you to lock environment access using web server authentication (guessing AuthBasic). This is a very basic setting that is easy to implemented (and provided by many providers) that having a module like this in core *is* an issue.

While I am not blaming anyone as it is not generally the type of thing a person tests when on software still in beta, I would argue that the initial provider plugin should have been actually tested in a browser in various scenarios before making its way into core. This issue would certainly come up once people actively develop their site on D8 (that's how this problem was discovered). If you look at the history of the module, the module is in core SOLELY so it an always-on provider plugin and enabled by default (and breaking pw protected sites).

I agree that fixing this in core is better than removing it though some may not like my approach. I think that the best way to salvage what is there is to convert it into a module that is solely for testing purposes. Attached patch moves the module to system/tests/modules and is renamed to basic_auth_test. I'm not exactly sure what is supposed to be done in rest.settings.yml for the supported_auth section but everything else should be sound.

Radical idea: How about if you know you are using basic_auth (on whatever level) you don't use both? We could pretty much say that about any sort of environment configuration that could break your site. So if you are setting this up at the webserver level, why would you also have this configured at the application level? That seems non-sensical.

This is not a required module, so just don't use it in that case?

I agree that it is non-sensical. But none of the docs anywhere (in the module, on the docs page) mention anything that this is an issue. The few reasons someone would turn it on at the application level:

• Want to try developing services with it (which is not recommended anyways since it is insecure) without knowing it conflicts with httpd basic auth. If someone has found flood control issues, does it mean they are thinking of using the module in a serious manner?
• Think it works like shield and install the module. Realize it didn't do anything and then try to set up httpd basic auth without uninstalling the module.
• Its so easy.

The last reason is, to me, the biggest issue. I can see many areas where a dev site gets set up by their systems manager (who turns on basic auth) and gets handed off to a set of developers after to work on it. They don't realize the ramifications of the module (after all, it is a part of core so unless they explicitly mention it, there shouldn't be any issue - people generally feel very comfortable install core modules) and maybe develop stuff around it (or forget to turn it off). Or someone turns on the module in dev by mistake. I can keep listing other reasons (including other reasons I have seen - yes, I saw the first one) but then this becomes another TLDR post :(

Setting up basic auth on the server generally requires some level of knowledge/configuration (in plesk, you have to add the users; in pantheon you have to write out the username/pass that will be used to put the site under httpbasic authentication and you have to dig around a bit to get to them in either case. Writing out basic auth on the server requires time and knowledge to do that). This requires visiting the module page and turning it on. No configurations, no warnings, no double checks - its just on.

Imagine already being behind basic auth (from the server) and you turn it on - you're logged out and you have no way of getting back into the site. No way to turn it off outside using drush (which requires additional knowledge and access to the server). Its a horrible user experience. Even docs aren't really enough for this. Unless you want to add directly to the info file in the name attribute 'learning/testing module - do not use for production', the fact that its so easy is the reason it should be revised and/or moved (and we have the capability as a community to do both - #12 solves one of them).

Its an extra module and doesn't need to be enabled...but ease of installation is something important to think about. The PHP module was a part of core for a very long time - it was a great day when it finally moved out into contrib and committed out of core. I'm not equating this with the crazy level of insecurity and bad things people could do that PHP module offered. But that module was also easy to install (and again, part of core which made people feel extra comfortable just enabling it). Something to consider.

Ok. Fair.

I think we could solve some of that by changing how we deal with auth providers on routes. I.e. how we manage the default behaviour for applicable providers.

I also don't think its drastically insecure if used in conjunction with SSL. That could be a docs issue, but also could be difficult to get across to people.

(Sorry, on my mobile phone. So short reply:))

basic_auth is not just a testing module; as damian says, it can be used safely in the right situation (with SSL, behind a firewall, etc.). Having multiple auth mechanisms in core, though, is important to ensure we don't break it by accident. Frankly I think it's far more useful than our vestigial support for multiple theme engines. :-)

That said, one thing I don't understand is how we even could differentiate between Drupal-targeted basic requests and Apache-targeted basic requests. Both are going to look the same to the browser; is there a way for basic_auth to detect that Apache already took care of it, so it should ignore it? Does that even make sense to do? I'm not sure. This is a legit concern, and one we didn't consider when writing the module as that is *not* what it was intended for at all. I can see where some might think it is, though; at minimum we can improve the docs/messaging around it, but if we can do more we should. Even if it's not in core, this problem would still remain so it needs to be solved either way.

That said, one thing I don't understand is how we even could differentiate between Drupal-targeted basic requests and Apache-targeted basic requests.

I could be wrong about this, but I think you can use $_SERVER['AUTH_TYPE'] and $_SERVER['REMOTE_USER'] to determine if Apache handled the authentication by itself or not.

See: http://www.php.net/manual/en/features.http-auth.php and http://www.php.net/manual/en/reserved.variables.server.php

That being said, I have the same questions that you do regarding how basic_auth should respond to Apache already taking care of the authentication.

Having multiple auth mechanisms in core, though, is important to ensure we don't break it by accident. Frankly I think it's far more useful than our vestigial support for multiple theme engines. :-)

This isn't a good argument for keeping the feature. The same argument was used for keeping trigger module in core, and we don't ship non-test implementations of systems like lock/cache either.

The current authentication system in drupal leaves much to be demanded. basic_auth module fails in another circumstance that I found out. I files a separate issue for it. #2283637: Provide test coverage to prove that an AuthenticationProvider can initiate a session

If authentication system in core is shoddy, can contrib correct it without wacky gyrations? This is a key pillar of the whole concept of "REST framework", it needs to be smooth regardless of where it lives.

@Grayside...Well, aside from the authentication issue having its set of problems, it is, as you said, a key part of our REST suite. However, basic authentication does not need to be in core. If we're getting into the argument of it needing to be smooth regardless of where it lives, then it should first live in contrib. Get cleaned up. Become smooth. Verified. *Prove* that it should to be in core and won't cause issues that are so simple to replicate.

Core needs to offer some sort of authentication for REST module other than cookies. We also have API freeze to deal with. If the current model is insufficient, we need to figure out a better one.

I am open to suggestions, because I don't have a better one off the top of my head. :-/

Suggestions have been offered in previous posts - you are clearly not open. So long as we get that checkbox for 'multiple options' right?

And given that inflexibility, I'm no longer willing to be flexible - other issues can sort this out.

Closing issue since this is not going anywhere.