Add modular authentication system, including Http Basic; deprecate global $user

restws also has code we can borrow here.

this is a few lines of code. i really question the wisdom of making it depend on the new Session stuff. sometimes it feels like we are fishing for things to depend on.

1. Digest authentication may be a good alternative since it obviates the need for SSL; raw password is not transmitted at all
2. Excellent SO discussion about authentication for REST APIs

First thing working perfectly: you get a 403 response when not authorised :-)

So that REST part is handled.

First step then will be to return a 401 to indicate that authentication will be required.

This is what you expect to see for a 401 response:

  HTTP/1.1 401 Authorization Required
  Date: Fri, 08 Feb 2013 23:23:23 GMT
  Server: Apache/2.2.14 (Ubuntu)
  WWW-Authenticate: Digest realm="XXXXX", nonce="/Ml+2D7VBAA=766517372151fe63803ceadc0bebaa24b6c0460c", algorithm=MD5, domain="/", qop="auth"
  Last-Modified: Wed, 16 Dec 2009 03:32:21 GMT
  Accept-Ranges: bytes
  Content-Length: 336
  Vary: Accept-Encoding
  Connection: close
  Content-Type: text/html

When trying to figure out if I could override the REST stuff to have a 401 just on the REST stuff, my interpretation of this is that this was not possible.

In core/modules/rest/lib/Drupal/rest/Plugin/ResourceBase.php it seems permissions are set in _permissions, but this is simply a Drupal permission a user has or not.

So we need to return a 401 at the Drupal core level itself, so obviously that has more impact than just a REST services patch.

Crell suggested we don't do it global, but try to keep it to REST only for now.

And after some discussion, this is the consensus on how to approach this:

1. We must clearly distinguish between authentication and authorisation.
2. We return a 401 when authentication is needed, a 403 when authorisation fails.
3. We cannot return a 401 until we have determined that authorisation is needed.
4. We only ask for authentication if a user is not logged it. Because if we have a uid > 0, we have authentication, so the authorisation correctly failed.

What we therefore will do is catch the 403 exception handler, and return a 401 there, if and only if someone is not logged in.

Crell on IRC:

So catch a 403, if the request is one that's HTTP_Digest-able (by some definition) set a 401 response.

Then we have a request listener that checks for an auth attempt, and if it finds one it logs the user in. And that happens pre-routing.

Finally back to this: forgot actually that digest authentication is not easy to do with Drupal, as the password is never transmitted with digest authentication.

To make such things work you would need to store the md5 digest of the username, realm (site name), and plain text password in the database. Yes, that means if you change the site name, all your passwords become invalid...

So we can only do basic authentication. But I think it will be a good idea to disallow basic authentication unless https is enabled as the password goes in plain text over the wire (just like your good old Drupal login). So perhaps we should like to let the user tick a box to enable http?

On the other hand, ssl encryption isn't as good as you think it is, as a man-in-the-middle attack is much easier than most people realise.

So working on basic authentication right now. It's better than cookies, but just as problemetic from a security point of view.

I've got a primitive basic authentication patch ready, proof of concept. However, got stuck when running this with the latest Drupal 8 + new entity patch #1818556: Convert nodes to the new Entity Field API. So waiting for that be resolved.

Here is a concept patch. It's really basic, so should be quick to understand and review.

1. Patch ExceptionController.php, and return 401 on 403.
2. Patch CoreBundle to inject a handle that logs in a user if the HTTP Authorisation header has been passed in.
3. And a new EventSubscriber for basic authentication which sits just before the LegacyAccessSubscriber and AccessManager.

What I'm looking for is a reviewer who can give me some guidance on how to proceed and make this a patch that can be accepted:

1. The ExceptionController.php is obviously ugly: we only want to return a 401 depending on some condition. I.e. only for REST services, or perhaps only when the route is marked that a 401 is OK.
2. Is injecting a HTTP authentication handler the right way? Does this need to be a pluggable? Obviously the event stuff is already pluggable, so injecting a digest handler should be quite possible.

All suggestions most appreciated!

I forget: this patch is on top of the entity-ng patch #1818556: Convert nodes to the new Entity Field API, you need that first.

Furthermore, on how to do digest authentication. I've come up with 2 approaches:

1. We add a realm config value to REST services, and add a table with tuples (uid, digest hash). See below for the digest hash calculation.

Advantage: new users, and when you update your password, can use http digest authentication.

Disadvantages: when enabled to an existing site, existing users cannot use digest authentication until they have updated their password. So we need a list showing what users do not have a password for authentication yet. Also when you change the realm, all passwords become invalid.

The digest hash is calculated as follows: md5 (username, ':', realm, ':', plain text password)

Or from the command-line:

echo -n 'myname:My Realm:password' | md5sum

Obviously md5 passwords can be calculated quickly. So another disadvantage could be that if your database is stolen, the passwords could be calculated (which you can do now too, but perhaps slightly slower). If a hacker has access to your db, you probably have bigger worries.

The second method gives every user immediate access, if you ask them not to use their password, but to use their hassed pass or a variant hereof.

I.e. when using digest authentication you do not give your plain text password to the browser, but the "pass" value as stored in Drupal or an md5 version or such thereof. I.e. to login with wget you do:

wget --user admin --password '$S$EZLEJ0YCPdMUb7lFtpGfI/2AxsPqJ/oBk7FoxKByh06cLW.2Xmuc' http://localhost/entity/node/1

The disadvantage is obviously that this is not intuitive, and you have to explain that to people.

PS: the reason we need to make this more complex is that the password is never sent over the wire, so we need to use a "password" that is known at both ends. So either we precalculate (first option) when we know the plain text password, or we use the hashed password (or the hash of that as the password).

According to this apache page, it only allows one format for digest authentication, md5.

http://httpd.apache.org/docs/2.2/misc/password_encryptions.html#digest

Apache recognises one format for digest-authentication passwords - the MD5 hash of the string user:realm:password as a 32-character string of hexadecimal digits.

But I don;t see anywhere in the spec that says it must be md5.

http://tools.ietf.org/html/rfc2617#section-4.13

Upon receiving the Authorization header, the server may check its
validity by looking up the password that corresponds to the submitted
username. Then, the server must perform the same digest operation
(e.g., MD5) performed by the client, and compare the result to the
given request-digest value.

kim.pepper, there is an algorithm parameter, but most clients only support md5/md5-sess/ntlm.

Crell> Does digest mandate md5?

See previous reply.

Crell> No space between get and (.

Yes, will clean up for final. I'm just used to write code like writing text. Will compress and densify it when done :-)

Crell> We definitely need some way to flag a route as supporting HTTP auth

Working on that.

We definitely need some way to flag a route as supporting HTTP auth, since presumably most sites won't want to do this on all pages. I think the request is available to us in the Exception Controller, or can be, so we should be able to retrieve the route and check its options array.

We have the request, but it does not contain what route objected as far as I can see. Debugging.

Not meaning to derail this issue, but has anyone looked at HMAC, which can be used in combination with md5 or the more robust sha1? see http://rc3.org/2011/12/02/using-hmac-to-authenticate-web-service-requests/

Edit: apparently Facebook uses HMAC (updated link).

We discussed this and decided that MD5 is not a problem here. With Symfony, we added back in a bunch of MD5 related code and we want to be compliant with clients.

I have not looked into HMAC.

HMAC is not standard, so works only with custom clients. So should be in addition to more widely supported approaches.

This is simply updating patch #12 against the latest greatest. Nothing new.

This always returns NULL in the exception handler.

I propose to add an attribute to route object that defines what authentication mechanism should be triggered as we will need to have others as well like oauth etc.

When subscriber sees that this attribute is available it should presave current user, authenticate the according to its rules and after execution of the controller to restore user that was active before authentication. This is how Services in D7 work. This will allow administrator, who is session authenticated to try authentication calls and not get logged off.

Probably we should have authentication plugins to support different types of authentication. Each plugin will have its own key for route attribute.

After extensive discussion at codesprint the plan is:
- add listener that will authenticate user with any possible authentication provider. If request has information about authentication for more than one method -- we throw 400 error response (Bad Authentication Request). In case of successful authentication we store information about the way user authenticated in request object.
- we add access check that will ensure that route allows to use authentication provider that user authenticated with. If not we throw 401 Not Authorized.

This is looking good. Do you plan to work on a second auth provider?

Ideally we add a test for the second provider. I think the standard login done by all the tests is adequate testing of the cookie provider.

Thanks for working on this.

This will of course break page caching. Depending on the used authentication mechanisms in a request (cookie headers, HTTP basic auth headers, oauth headers etc.) you need to vary the page cache key or bypass page caching. But page caching is broken anyway right now, I'm not even sure which issue to link here. Examples: #1973206: ViewSubscriber with page caching - Denial of Service (SA-CONTRIB-2013-042), #1855260: Page caching broken by accept header-based routing, #1597696: Consider whether HttpCache offers any significant benefit over the existing page cache

Sorry for silly question, but does page caching even applicable to REST? I can see some situations when we do want to cache REST requests, but it is really difficult to control(decide) what to cache and what not to cache in case of REST, so my assumption would be that Drupal is not going to cache any of REST request.

@atchijov - Caching can be respected using the Cache-Control header. This allows the client to call the APIs with various levels of caching, i.e. "no-cache" to bypass the cache, "max-age" to prevent stale cache, "No-store" to prevent writing to the cache, etc. - http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.9

Although exactly what Drupal should cache is outside of my knowledge, but I imagine some things can and should be cached for REST requests

Finally have something working (still waiting for all tests to run).

This patch still needs:
- comments in the code
- I do not like authentications providers have to register in container with 'authentication.XXX' key. But I do not have idea how to change this.
- I do not like to include sessions.inc in AuthenticationSubscriber::onKernelRequestAuthenticate because of FinishResponseSubscriber. This opens session (sets a cookies) even in case we do a call with basic authentication only. Can we remove drupal_session_commit() from FinishResponseSubscriber? Or maybe check if authentication is done with 'cookie' authentication provider and only then run drupal_session_commit()?

Reroll of the patch.

Whoa! I just read the issue and I have to admit I understood little of it. For what I understand, we are building a system that lets other authentication systems such as OAuth to subscribe to an Authentication manager, right? Therefore, we could do two-legged authentication for REST endpoints as we currently do with Services + OAuth in Drupal 7.

Since I started maintaining the OAuth I have just done support. I do not know the OAuth protocol in detail, but I could port the module to Drupal 8 so it uses this system. Thoughts?

By the way, the OAuth module uses OAuth1. @bojanz (pinged to chime in here through personal contact form) created recently an OAuth2 Server module.

#38: core-1890878-authentication-providers-38.patch queued for re-testing.

Move drupal_commit_session() from FinishResponseSubscriber to AuthenticationSubscriber listener (running through cleanup() method of authentication provider). Lets see how tests will pass.

Also I have done changes in simpletest/TestBase.php to avoid calling drupal_save_session() if this function does not exist.

+    $username = $request->server->get('PHP_AUTH_USER');
+    $password = $request->server->get('PHP_AUTH_PW');

I never really understood what those special variables are actually for... but I'm pretty sure we should read ->headers->get('Authorization') directly instead.

Also, wouldn't it be waaaay simpler to use the Symfony Security component instead of trying to come up with our own, broken, security framework?

The Symfony security component is a large and complex beast. The ripple effects of trying to integrate that are huge at this stage of the dev cycle; we haven't even managed to get Symfony sessions in properly yet. :-(

Just browsing through the component, I do not actually think that's true. The Firewall part of the component seems fairly self-supporting and is basically what this patch is doing.

We can and should start small. The only thing we want to leverage right now is the Firewall class (which is basically what AuthenticationManager in this patch does), and the basic authentication tools (BasicAuthenticationEntryPoint, BasicAuthenticationListener).

In turn, we are going to need a SecurityContextInterface: we can reuse the default SecurityContext implementation, but that requires a AuthenticationManagerInterface and a AccessDecisionManagerInterface.

For the AuthenticationManagerInterface, we can just go with a Drupal-specific implementation for now, and hardcode it to check the user database.

For the AccessDecisionManagerInterface, we can just go with a Drupal-specific check of permissions (ie. wrap user_access()).

No need to touch anything else in the Security component for now. We are going to need a FirewallMap to initialize the Firewall, but I assume that's just glue code to the router.

Then, once we have this basic infrastructure in place, we can move on to adopt more elements of the Security framework.

Just clarifying: Digest auth is not on the table. Digest was designed in 1997/1999, at a time where it was though as a good idea to store the plaintext password (or a MD5 of username - realm - password) on the server side. There are other advanced authentication schemes (Negotiate, SASL, etc.), but nothing that is broadly adopted and has value against just using OAuth.

Rerolled patch plus:
- added code comments
- removed AuthenticationManagerInterface in favour of AuthenticationProviderInterface
- changed from using $request->server->get('PHP_AUTH_USER') to $request->headers->get('PHP_AUTH_USER') like implemented in Security component

I personally do not think we are able to adapt Security component at the moment as it needs more efforts and we have limited time before code freeze.

Hi ygerasimov,

Looking good.

I haven't followed the thread closely, so I'm not sure why we are removing DRUPAL_BOOTSTRAP_SESSION as part of this patch?

These are some minor codestyle/docs issues I picked up:

+++ b/core/core.services.ymlundefined
@@ -427,3 +427,22 @@ services:
+      - { name: access_check }

Needs newline at end of file.

+++ b/core/lib/Drupal/Core/Access/AuthenticationProviderAccessCheck.phpundefined
@@ -0,0 +1,40 @@
+}

Needs newline at end of file.

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.phpundefined
@@ -0,0 +1,73 @@
+
+

Extra blank lines should be removed.

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.phpundefined
@@ -0,0 +1,73 @@
+  public function cleanup(Request $request) {

Needs docs.

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationProviderInterface.phpundefined
@@ -0,0 +1,38 @@
+interface AuthenticationProviderInterface {

Needs docs.

+++ b/core/lib/Drupal/Core/Authentication/Provider/Cookie.phpundefined
@@ -0,0 +1,34 @@
+class Cookie implements AuthenticationProviderInterface {

Needs docs.

+++ b/core/lib/Drupal/Core/Authentication/Provider/Cookie.phpundefined
@@ -0,0 +1,34 @@
+}

Needs newline at end of file.

+++ b/core/lib/Drupal/Core/Authentication/Provider/HttpBasic.phpundefined
@@ -0,0 +1,36 @@
+class HttpBasic implements AuthenticationProviderInterface {

Needs docs.

+++ b/core/modules/system/lib/Drupal/system/Tests/Authentication/HttpBasicTest.phpundefined
@@ -0,0 +1,72 @@
+  public function testHttpBasic() {

Needs docs.

+++ b/core/modules/system/tests/modules/router_test/router_test.routing.ymlundefined
@@ -60,3 +60,10 @@ router_test_10:
+    _content: '\Drupal\router_test\TestContent::test11'

Needs newline at end of file.

Kim, thank you very much for the review.

I have fixed these issues plus now less tests should fail.

Reroll of the patch.

Rerolling the patch. Checking tests.

Just clarifying: Digest auth is not on the table. Digest was designed in 1997/1999, at a time where it was though as a good idea to store the plaintext password (or a MD5 of username - realm - password) on the server side. There are other advanced authentication schemes (Negotiate, SASL, etc.), but nothing that is broadly adopted and has value against just using OAuth.

Not sure I'm getting you. You don't want to support any HTTP authentication method?

OAuth is barely supported, compared to the enormous range of http clients that support http authentication. Digest is infinitely more secure than Drupal sending plain text passwords over the wire, but it's complicated to get this in.

But having basic authentication over ssl is definitely something one must support to reach a wide audience.

@berenddeboer: please re-read. "Digest" authentication cannot be used, because it forces us to store the password (or a digest of it) on the server side. "Basic" is fine. The most future-proof solution is OAuth.

Patch reroll. Trying to fix tests.

Another reroll. Add initializing of session during install.

AccessCheckInterface::access() Is expected to return NULL if the implementation is not responsible. Changing the implementation of AuthenticationProviderAccessCheck seems to fix most of the tests. The errors I have seen so far are related to language negotiation.

By my opinion just wrapping the session handling with the cookie class , will give us a lot of headache. The session handling should be more decoupled. Sadly I do not have an idea how. Note that there is an issue trying to make use of symphony for the authentication (#1858196: [meta] Leverage Symfony Session components).

And this is list of some more issues that might interfere with what is happening here.

• #1157310: base authentication class to make multiple and external authn methods easier to implement
• #27972: Make authentication pluggable
• #1806514: Unify anonymous and registered users

Triggering testbot.

We are getting closer with simpletest! Another round.

Joggling around some event subscriber and dealing with an edge case in autorize.php striped down most of the remaining test failures. Let's see how much is remaining.

Oh, sorry. Did not see the previous patch.

Claim that for now.

That one should give it the rest ;)

This patch fixed the coding styles mentioned in the comment above.

Why not just break; from the foreach loop on the first provider we find? Do we need to keep going to force only one to work?

I thought the same but I do not have any real arguments for one or the other solution. My gut tells my to do a break and forget about the double credentials.

The main thing that is missing that I see is how to force authentication with HTTP Basic. I am no expert on it, but my understanding is that in case of a Basic Auth failure an HTTP 401 should be returned […]

Currently if no provider feels responsible the global user is populated with an anonymous account. This will give us a 404. We probably can trigger a 401 respons instead and skip any further page processing (how can that be done by now?).

Currently if no provider feels responsible the global user is populated
with an anonymous account. This will give us a 404. We probably can
trigger a 401 respons instead and skip any further page processing (how
can that be done by now?).

No, it does not. It continues with the process and then autorization decides if the anon user has access to that route.

What @Crell meant is that in case of having authentication information but it being wrong, it should abort, even if the anon user would have access to that page. Not sure if we can already throw an AccessDeniedException at this point?

Handle the least prioritized provider as fallback and make sure its clean up gets triggered.

No, it does not. It continues with the process and then autorization decides if the anon user has access to that route.

That was what I meant.

Amazing work, I like what I see here! This seems to be almost ready, so I'm starting the nitpicks:

+++ b/core/lib/Drupal/Core/Access/AuthenticationProviderAccessCheck.php
@@ -0,0 +1,47 @@
+
+class AuthenticationProviderAccessCheck implements AccessCheckInterface {

missing doc block for the class. Also elsewhere, please check all your classes.

+++ b/core/lib/Drupal/Core/Access/AuthenticationProviderAccessCheck.php
@@ -0,0 +1,47 @@
+  /**
+   * This access check applies to every route.
+   *
+   * @param Route $route
+   *
+   * @return bool

@param and @return description must not be empty. Better use @inheritdoc here and add the comment under it. Also elsewhere.

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.php
@@ -0,0 +1,145 @@
+  /**
+   *  Priority array all registered authentication providers.

2 white spaces before "Priority".

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.php
@@ -0,0 +1,145 @@
+    foreach ($this->providers as $provider_id => $provider) {
+      $account = $provider->authenticate($request);
+
+      if ($account !== NULL) {
+        // We do not allow request to have information to authenticate
+        // with two methods at the same time.
+        if (!empty($this->triggeredProvider)) {
+          throw new BadRequestHttpException(t('Multiple authentication methods are not allowed.'));
+        }
+        $this->account = $account;
+        $this->triggeredProvider = $provider_id;
+      }

if one provider successfully authenticated we should break the loop, right? There is no point in asking the other providers then?

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.php
@@ -0,0 +1,145 @@
+  /**
+   * Does clean up by running cleanup() of provider that authenticated the user.
+   *
+   * @param Request $request
+   *   The request object.
+   */
+  public function cleanup(Request $request) {
+    if (empty($this->triggeredProvider)) {
+      return;
+    }
+    $provider = $this->providers[$this->triggeredProvider];
+    $provider->cleanup($request);

Why and what does a provider have to cleanup? Is this a common pattern for authentication providers? To me it looks like session cookie authentication is messy and needs this ... but anyone else?

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationProviderInterface.php
@@ -0,0 +1,39 @@
+  /**
+   * Do cleanup.
+   *
+   * @param Request $request
+   *   The request object.
+   */
+  public function cleanup(Request $request);

What cleanup? That comment should give an example and refer to the session cookie cleanup mess. I find it ugly that we have this method on the interface, can we avoid it?

+++ b/core/lib/Drupal/Core/Authentication/Provider/Cookie.php
@@ -0,0 +1,37 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function cleanup(Request $request) {
+    drupal_session_commit();

I really think the cookie auth provider should cleanup on its own, maybe by registering as subscriber to a response kernel event and invoking drupal_session_commit() there? That way we could avoid the cleanup() method on the interface.

+++ b/core/lib/Drupal/Core/EventSubscriber/AuthenticationSubscriber.php
@@ -0,0 +1,72 @@
+  /**
+   * Trigger clean up.
+   */
+  public function onRespond(FilterResponseEvent $event) {
+    if ($event->getRequestType() == HttpKernelInterface::MASTER_REQUEST) {
+      $request = $event->getRequest();
+
+      $this->authenticationManager->cleanup($request);
+    }

Yep, this should go to the session cookie provider, or is there a compelling reason other authentication providers want to cleanup?

+++ b/core/modules/system/lib/Drupal/system/Tests/Authentication/HttpBasicTest.php
@@ -0,0 +1,84 @@
+    $this->basicAuthGet('router_test/test11', $account->name, $account->pass_raw);
+    $this->assertText($account->name, 'Account name is displayed.');
+    $this->curlClose();
+
+    $this->basicAuthGet('router_test/test11', $account->name, $this->randomName());
+    $this->assertNoText($account->name, 'Bad basic auth credentials do not authenticate the user.');

You should also assert the HTTP response status codes here.

Ok, the ugliness of cleanup() should not hold up this issue. Please document it properly on the interface with a reference to the session cookie implementation. I would argue that we should have an abstract base class that implements an empty cleanup(), but that is also a minor detail and not blocking this effort.

As a starter, here comes a lot of documentation work.

As of the 401/403, I will have a look at it once I have talked with @Berdir tomorrow in the office.

I also feel the need to do more tests. Suggestions on how to improve them are welcome.

Regarding 401, in case user is not authenticated we can run through all providers and run some method on them. After first provider that returns not NULL (means that provider *did* something, in case of basic authentication it sets up 401) we stop.

So in case of basic authentication has higher priority than any others we will get 401 with a challenge.

Throwing exceptions is fun. Now authentication returns 401 and 403 as expected.

I am unsure what the WWW-Authenticate header should look like nor if this is actually the way to go. I have not done anything with the test for I wanted to have some feedback first.

No. I do not think patch #90 is the way to go. We need to set 401 and proper headers with realm only in case if user get access denied and user is anonymous and basic auth is enabled for this call.

I will work on this patch tomorrow.

Here is patch that makes 401 in case basic authentication is enabled for the route and access denied exception thrown.

The problem that still exists is that I haven't found the way to set WWW-Authenticate header. Maybe it needs some fresh look.

Added a redirect to the login form int the cookie exception handler and improved the http basic chalenge as well and som coding style improvements.

Besides that I am stuck here too.

In AuthenticationManager::handleException(), if I remove the checks for the route, I get the 401 with the expected chalenge. I also manage to get the redirect to the user form for cookie.

The problem is that the route is not set by the time the exception handling happens and I have not found any other means to get a route object.

I tried to set a rout variable on the AuthenticationManager during access check in AuthenticationProviderAccessCheck but for some reason the execution does not com that far.

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationProviderInterface.phpundefined
@@ -23,7 +23,7 @@
-   * @return Drupal\Core\Session\AccountInterface|FALSE|NULL
+   * @return \Drupal\Core\Session\AccountInterface|FALSE|NULL

Should be lowercase false/null I think.

As discussed, I don't really like an API that can return an object, false or NULL from the same method. That feels like its doing too much.

Maybe there should be, similar to access checks, a separate method for a provider today if he applies for that request that can return true/false and an authenticate method that can return either an AccountInterface and or null if authentication failed?

+++ b/core/lib/Drupal/Core/Authentication/Provider/Cookie.phpundefined
@@ -40,6 +42,11 @@ public function cleanup(Request $request) {
+    $exception = $event->getException();
+    if (user_is_anonymous() && get_class($exception) == 'Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException') {
+      $event->setResponse(new RedirectResponse(url('user/login', array('absolute' => TRUE))));
+      return TRUE;
+    }
+    return FALSE;

Not sure about this, that seems like a considerable behavior change? And hardcoding this seems.. questionable. It should at least have a destination redirect back to this url?

+++ b/core/lib/Drupal/Core/Authentication/Provider/HttpBasic.phpundefined
@@ -44,7 +44,14 @@ public function cleanup(Request $request) {}
+      $site_name = \Drupal::config('system.site')->get('name');
+      global $base_url;

Should inject the config factory (@config) into this. Not sure if the fallback is necessary, I think we usually assume that the site name is set?

+++ b/core/lib/Drupal/Core/Authentication/Provider/HttpBasic.phpundefined
@@ -44,7 +44,14 @@ public function cleanup(Request $request) {}
+      $challenge = format_string('Basic realm="@realm"', array(

I guess this should use \Drupal\Component\Utility\String::format()?

+++ b/core/authorize.phpundefined
@@ -48,10 +48,12 @@ function authorize_access_denied_page() {
 function authorize_access_allowed() {
+  require_once DRUPAL_ROOT . '/' . settings()->get('session_inc', 'core/includes/session.inc');
+  drupal_session_initialize();
   return settings()->get('allow_authorize_operations', TRUE) && user_access('administer software updates');

What should we do about session.inc? What actually prevents us from using the container (or a special container with hardcoded services) in here, update.php and install.php?

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.phpundefined
@@ -0,0 +1,191 @@
+   * @var int[]

We're currently using array for these cases, not [].

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.phpundefined
@@ -0,0 +1,191 @@
+   * The authenticated user.

Authenticated in Drupal usually means a user/account with the authenticated role, which excludes anonymous users, but they still can have a valid session and be... identified?

That also makes the whole naming here.. a bit problematic.

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.phpundefined
@@ -0,0 +1,191 @@
+   * @param AuthenticationProviderInterface $provider

Missing namespace.

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.phpundefined
@@ -0,0 +1,191 @@
+   * @return int
+   *   0  - If both providers have the same priority.
+   *   1  - If provider A has lower priority than B.
+   *   -1 - If provider A has higher priority than A.

not sure, we usually don't seem to bother to document the return value of a sort callback?

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.phpundefined
@@ -0,0 +1,191 @@
+    // accessed in Drupal\Core\Access\AuthenticationProviderAccessCheck.

Missing \

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.phpundefined
@@ -0,0 +1,191 @@
+   * Do clean up.

Reads strange...

"Cleans up"?

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.phpundefined
@@ -0,0 +1,191 @@
+   * @param Request $request

Missing namespace.

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationProviderInterface.phpundefined
@@ -0,0 +1,60 @@
+ * Contains Drupal\Core\Authentication\AuthenticationProviderInterface.

\, Same for all other files.

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationProviderInterface.phpundefined
@@ -0,0 +1,60 @@
+   * This method called early on KernelEvents::REQUEST event.

is called, also namespace. Not sure if this is even relevant to be documented here? It's called with a request, it doesn't need to know when?

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationProviderInterface.phpundefined
@@ -0,0 +1,60 @@
+   * @param GetResponseForExceptionEvent $event

namespace.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.phpundefined
@@ -876,6 +876,7 @@ protected function prepareEnvironment() {
+    require_once DRUPAL_ROOT . '/' . settings()->get('session_inc', 'core/includes/session.inc');
     drupal_save_session(FALSE);

This seems unnecessary? We either have to assume that by now, this has been loaded or actually, it should probably become a method on the authentication manager if we want to assume that we might be running test with basic auth :)

+++ b/core/modules/system/lib/Drupal/system/Tests/Authentication/HttpBasicTest.phpundefined
@@ -0,0 +1,86 @@
+   * Dos HTTP basic auth request.

Does.

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationProviderInterface.phpundefined
@@ -23,7 +23,7 @@
-   * @return Drupal\Core\Session\AccountInterface|FALSE|NULL
+   * @return \Drupal\Core\Session\AccountInterface|FALSE|NULL

Should be lowercase false/null I think.

As discussed, I don't really like an API that can return an object, false or NULL from the same method. That feels like one thing too much :)

Maybe there should be, similar to access checks, a separate method for a provider today if he applies for that request that can return true/false and an authenticate method that can return either an AccountInterface and or null if authentication failed?

+++ b/core/modules/system/lib/Drupal/system/Tests/Authentication/HttpBasicTest.phpundefined
@@ -0,0 +1,86 @@
+   * We do not use drupalGet because we need to set curl settings for basic

namespace\class::drupalGet(). Deja-vu :)

In general, this still feels a bit backwards. We first try all providers that we have only to then later on check if the one that responded (if any did), is actually valid for this request?

Similar to other services there is now a applies() method on the AuthenticationProviderInterface.

As of the login redirect. We probably can make this configurable. For now I added the destination query.

The configuration is no injected to HttpBasic. I hope I did that the right way.

What should we do about session.inc? What actually prevents us from using the container (or a special container with hardcoded services) in here, update.php and install.php?

I guess it is update and install. I have not touched it so far, because I wanted to have the rest working.

This seems unnecessary? We either have to assume that by now, this has been loaded or actually, it should probably become a method on the authentication manager if we want to assume that we might be running test with basic auth :)

Things will go really bad if session.inc is not loaded there.

namespace\class::drupalGet(). Deja-vu :)

That was obvios :p

In general, this still feels a bit backwards. We first try all providers that we have only to then later on check if the one that responded (if any did), is actually valid for this request?

Agreed. Yet i have no Idea how to do this different.

Then lets see what I gan do with all those test fails.

Having the cookie provider doing a redirect on access denied was a nobel thought but this brakes most of the test for this returns 200 and not 403. I removed it for now.

Also took care about the warnings.

This should fixes the Basic Auth test.

The interdiff is against #101.

This should also fix the unit tests.

Interdiff still against #101.

#110: drupal-1890878-110.patch queued for re-testing.

Very very cool! We are getting really close to RTBC!

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.phpundefined
@@ -0,0 +1,209 @@
+    // While any provider may authenticate a request, only the first listed
+    // by a route may give a challenge for a failed request to allow the user
+    // to login.
+    $allowed_providers = array_intersect($auth_providers, array_keys($this->providers));
+
+    if ($allowed_providers) {
+      $this->providers[current($allowed_providers)]->handleException($event);
+    }

I do not quite agree with this. We should let all allowed providers to handle exception and to break once it is handled. For example top priority may be provider that does not handle exception at all (so default behavior will be expected).

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationProviderInterface.phpundefined
@@ -0,0 +1,67 @@
+   * @return bool
+   *   TRUE - exception handled. No need to run through other providers.
+   *   FALSE - no actions have been done. Run through other providers.
+   */
+  public function handleException(GetResponseForExceptionEvent $event);

This is about my top comment. Here is documentation about what handleException() returns. So in case FALSE is returned we go through other providers.

+++ b/core/lib/Drupal/Core/Authentication/Provider/HttpBasic.phpundefined
@@ -0,0 +1,86 @@
+      }
+      return TRUE;;
+    }

One semicolon should be removed.

+++ b/core/update.phpundefined
@@ -427,7 +427,9 @@ function update_check_requirements($skip_warnings = FALSE) {
+require_once DRUPAL_ROOT . '/' . settings()->get('session_inc', 'core/includes/session.inc');
+drupal_session_initialize();

We use these two lines pretty often in different places. Maybe lets have a method in Cookie provider and then using container execute that method. In this way we will have control over initializing session in one place. Maybe some other other place or even simple function will do the job as well.

Technically we should be using RouteObjectInterface::ROUTE_OBJECT, which is the constant for _route_object. But yes, going to $defaults is better.

It wasn't my choice to switch. The RouteObjectInterface::ROUTE_OBJECT is just not set at that moment of the execution cycle.

I do not quite agree with this. We should let all allowed providers to handle exception and to break once it is handled. For example top priority may be provider that does not handle exception at all (so default behavior will be expected).

You will have a pretty hard time to determine if a provider (e.g basic auth) is to handle the exception or not.

Given we cokkie is the one that is responsible and we ended with an anonymous user. Basic will kick in for there are no basic credentials and the user is anonymous. The other way around, Cookie has no way to determine if it is responsible or not or it is at least pretty hard by the look of the code in session.inc.

Two questions here from my side:

1. In which order shall the providers be iterated on error handling: highest or lowest prioritized first?
2. What should be the conditions for the providers (mainly basic auth)?

We use these two lines pretty often in different places. Maybe lets have a method in Cookie provider and then using container execute that method. In this way we will have control over initializing session in one place. Maybe some other other place or even simple function will do the job as well.

This concernes install.php and update.php. Do we have a fully fledged container there? I fear that we won't be able to do so but that is just a feeling and I have not checked it.

install.php and update.php have containers, install.php just has a manually set up container for early phases and update.php some overrides. So you just need to make sure to manually register the services in install.core.inc I think.

Readded provider iteration on error handling.

+++ b/core/lib/Drupal/Core/Routing/Enhancer/AuthenticationEnhancer.phpundefined
@@ -0,0 +1,44 @@
+      $auth_providers = ($route && $route->getOption('_auth')) ? $route->getOption('_auth') : array();
+      if (!empty($auth_providers)) {
+        // If the request was authenticated with a non-permitted provider,
+        // force the user back to anonymous.
+        if (!in_array($auth_provider_triggered, $auth_providers)) {
+          $request->attributes->set('session', drupal_anonymous_user());
+        }
+      }

Instead of array(), $auth_providers should be assigned to default provider. And then if user was authenticated with not default provider -- we set him back to anonymous.

Lets add another test. To try to open /admin page while trying to login using basic authentication as administrator. This is to ensure we can't login to legacy route with basic authentication and get 403.

Let's give this a try.

+++ b/core/lib/Drupal/Core/Routing/Enhancer/AuthenticationEnhancer.phpundefined
@@ -0,0 +1,54 @@
+
+      $auth_providers = ($route && $route->getOption('_auth')) ? $route->getOption('_auth') : array($this->manger->defaultProviderId());
+      if (!empty($auth_providers)) {
+        // If the request was authenticated with a non-permitted provider,
+        // force the user back to anonymous.
+        if (!in_array($auth_provider_triggered, $auth_providers)) {
+          $request->attributes->set('session', drupal_anonymous_user());
+        }
+      }
+    }

No need for "if (!empty($auth_providers)) {" as it always will be non-empty.

+++ b/core/modules/system/lib/Drupal/system/Tests/Authentication/HttpBasicTest.phpundefined
@@ -0,0 +1,93 @@
+  public function testHttpBasic() {
+    $account = $this->drupalCreateUser();

User should have 'access administration pages' permission for last check.

Everything else looks good to me. Lets wait for tests to pass.

Now for legacy stuff only cookie authentication is allowed.

My bad, wrong condition. Added additional check.

This all looks great. Here's some very minor nits. If a follow up is opened for them (one follow up for all the remaining nits is fine, doesn't need to be one per nit), I'd be fine with this being RTBC'd as-is. I haven't read all the issue comments though, so don't know if there remains any other unaddressed feedback or remaining concerns.

+++ b/core/includes/bootstrap.inc
@@ -158,11 +158,6 @@
-const DRUPAL_BOOTSTRAP_SESSION = 5;

Should we decrement the higher constants accordingly?

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.php
@@ -0,0 +1,209 @@
+ * On each request, let all authentication providers try to authenticate the
+ * user. The provider are iterated according to their priority and the first
+ * provider detecting credentials for his method will become the triggered
+ * provider. No further provider will get triggered.
+ *
+ * If no provider felt responsible the manager assumes that the least
+ * prioritized should have and therefore is the triggered provider and the user
+ * is set to anonymous.

This could use a little grammar polish (e.g., singular/plural, his/its, etc.).

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.php
@@ -0,0 +1,209 @@
+   * Add provider to the array of registered providers.

s/Add/Adds a/

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.php
@@ -0,0 +1,209 @@
+   * Authenticate user.
+   *
+   * Iterate the available providers according th their priority.
+   *
+   * @param \Symfony\Component\HttpFoundation\Request $request
+   *   The request object.
+   *
+   * @return \Drupal\Core\Session\AccountInterface
+   *   The account interface of the authenticated user. Defaults to a anonymous.

Should this just be an {@inheritdoc}? If not, then could use a couple spelling and grammar fixes.

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.php
@@ -0,0 +1,209 @@
+        // Provider felt responsible for this request – trigger authentication.

Code doesn't "feel".

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.php
@@ -0,0 +1,209 @@
+    // No provider felt responsible – assume the one with the least priority
+    // should have.

Code doesn't "feel".

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.php
@@ -0,0 +1,209 @@
+    // Save the authenticated account for later access. The global $user object
+    // is included for backward compatibility only and should be considered
+    // deprecated.
+    $user = $account;
+    $request->attributes->set('session', $account);

Let's split this into two parts then. First the part that isn't deprecated. Then the part that is. And add a @todo for that part.

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationProviderInterface.php
@@ -0,0 +1,67 @@
+   * Do cleanup.

Could use a more informative short sentence.

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationProviderInterface.php
@@ -0,0 +1,67 @@
+   * Handle exception.

Handles an exception.

+++ b/core/lib/Drupal/Core/EventSubscriber/LegacyAccessSubscriber.php
@@ -23,12 +23,25 @@ class LegacyAccessSubscriber implements EventSubscriberInterface {
+    if (isset($router_item['access']) && $provider && $provider != 'cookie') {

Why are we checking $router_item['access']? Should we instead check $router_item itself?

+++ b/core/lib/Drupal/Core/Routing/Enhancer/AuthenticationEnhancer.php
@@ -0,0 +1,52 @@
+      $route = isset($defaults['_route_object']) ? $defaults['_route_object'] : NULL;

Should we use the ROUTE_OBJECT constant here instead?

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.phpundefined
@@ -0,0 +1,202 @@
+    $request->attributes->set('session', $account);

Not completely happy with this being called session.

Discussed this with @corvus_ch for a while last week, it doesn't make much sense to have a session thing when you're doing basic authentication and it's actually your user/account.

Wondering how hard it would be at this point to treat the session data and the account object as different things (remove session methods completely from AccountInterface, I added them because it's there right now but that can change) and then this would be account and session would be added separately by the cookie provider.... somehow.

All that might or might not be possible in 8.x. All I'm sying here is, shouldn't this be called 'account'? :)

Not completely happy with this being called session.

Discussed this with @corvus_ch for a while last week, it doesn't make much sense to have a session thing when you're doing basic authentication and it's actually your user/account.

[…]

All that might or might not be possible in 8.x. All I'm sying here is, shouldn't this be called 'account'? :)

The naming of this was not introduced by this patch. I tried to change it and a lot of stuff broke. Changing the naming of this should be handled in a follow up.

i agree that the attribute key should be named account.
Right now it is confusing moreover because the Request object has a getSession() method..which it gives you the session service from request's property..So even if we dont have a session service, lets give this the correct name

Session is now renamed to account.

The naming of this was not introduced by this patch.

Well I really thought it was already there. :)

#132: drupal-1890878-132.patch queued for re-testing.

Back to RTBC then.

#132 didn't include the changes from #127. This just applies the #132 interdiff on top of #127. Leaving at RTBC.

#136: drupal-1890878-136.patch queued for re-testing.

In Symfony Security component seems to just call that "user" pretty much like Drupal 7 which make some sort of sense. "Account" is a good term too since our user entities are named "User". The name "identity" seems good too (if I remember well I saw this term used in ZF1).

Identity works for me.

I am implementing an OAuthProvider that plugs into the AuthenticationManager at #2027863: Make Oauth module an AuthenticationProvider.

I know this is RTBC but I couldn't resist.

Even though it turns this into a ContainerAware service I think we should use the same pattern we also use in the AccessManager and not hand references into the addProvider() method directly so that we do not needlessly instantiate all of the providers for every request.

Fixes some other minor issues too.

I find defaultProvider() a bit weird and especially the AuthenticationManager::__construct() is a bit suspicious with that substr() on the service id. Hmm...

This certainly has some improvements. Thanks.

Certainly not dozens. And even then... we iterate through all of them anyway so there would not possibly be a performance benefit. As such, ContainerAware is unnecessary and superfluous.

Okay, agreed...

#136 is fine as is and still RTBC. The changes from #144 are not OK.

The other changes are still valid though (code style/documentation, missing interface, incorrect naming of variables in #136).

Will post a re-roll without ContainerAware asap.

This is probably better than. Still not too happy with defaultProviderId(). In fact i'd prefer it if we could live without such a thing and through that also make the ManagerInterface obsolute. But if we have it (public) and call it from outside we have to have that interface.

I reviewed this patch and it looks good. I like the architecture -- pretty easy to follow. However, when it came to committing this patch, it no longer applied because of the changes to CoreBundle.php. I'm afraid this requires a reroll.

On it.

Rerolled to lates HEAD.

Once again.

Committed aa9c2cc and pushed to 8.x. Thanks!

Re: patch workflows, should I open a separate issue to change "Eighth" to "Sixth"? : )

A critical typo! I'm on it. Committed to 8.x. ;-)

Back to active - needs change notice

I have created a change notice https://drupal.org/node/2031999 please correct it for language and add missing pieces. It is only about module authentication system.

I changed paragraph on the authentication sequence from

Each of providers are run on every request (in case their "applies" method returns TRUE). After any provider returns account we stop the loop of running through remaining providers.

to

On each request the applies method of each provider will be called in the order of their priority. The first provider returning TRUE gets the change to authenticate the user.

And I did some gramar improvements. I am not a native englisch speaker, so those changes might be wrong.

I added #2032115: Remove session.inc and move code to the cookie authentication provider as a follow up.

I have closed #27972: Make authentication pluggable as a duplicate of this one. For #1157310: base authentication class to make multiple and external authn methods easier to implement I am not sure if it is a duplicate or not.

We need another change notice that global $user is deprecated.

Follow-up: #2032553: The _account attribute on the Request is not available during web tests.

Untagging.