Remove dependency of current_user on request and authentication manager

Could we do something like this? Not tested.

EDIT: Meant to actually remove that onKernelRequestDetermineSiteStatus method.

This really looks critical to me, bumping priority.

Just looked at book_node_load(), which got me to https://api.drupal.org/api/drupal/core%21modules%21book%21src%21BookMana...

Since we have entity caching in core now, book module should absolutely not be doing anything access-dependent in book_node_load() - so I think the test fail is actually exposing a bug in book module and we should change book module instead.

Probably the quickest place to move that logic would be hook_entity_prepare_view().

So, the book code should be using the node access system to optimize these checks and never be calling access on a single object, normally.

But I guess this is using the really old and crappy node links API. Can we defer the access check to render time? Maybe a question for Wim?

Not sure how to make sure we don't see this general problem again in the future, or it will just break so devs will fix it?

So, I think the approach here is wrong, or we need to re-think when upcasting happens.

In general, we don't want to go to the trouble of loading objects (upcasting) if authentication is going to fail.

The bug in this case seems to be that's it's using hook_node_load() to essentially attach render information.

Maybe should be using hook_entity_prepare_view() instead?

Oops - no - I take all that back.

Book module is basically trying to load a data field. We should not be checking access at all during that - if access is denied for the node, you won't see the attached field anyhow.

I tried to write a beta evaluation for this issue:

However, the conclusion that such a use case would require authentication to be performed separately in a subrequest is wrong in my opinion. I'd like to point out that user authentication should not be confused with masquerading. The latter can be performed independently of the former and does not require separate authentication.

User authentication means checking the credentials sent along with the initial HTTP request. This step should be performed after routing and before access-control checks are fired. Not before and not afterwards.

This itself sounds not like a justification for this being a critical? Is the current way of doing it (on the fly) so brittle at the moment?

The patch here has exposed some very dodgy assumptions elsewhere like #2380615: Result of book_node_load() should not vary depending on user permissions. I think we need to evaluate all of the issues on #2371629: [meta] Finalize Session and User Authentication API to figure out which if any are release blocking etc. not sure if this one is or how much of the other work it blocks.

I do think #2286591: [meta] Security audit the Authentication component needs to be done before release - authentication providers are pretty broken at the moment and it's a very security sensitive API. Tthat can only happen once the session and auth APIs are reviewable, which doesn't feel like the case without some of this range of issues being fixed.

Do we have to expect any drawbacks when moving language negotiation after routing (and upcasting)?

You can't do it after upcasting, as upcasting behaves different depending on the language.

reroll

Per #41

1. Make upcasting language-agnostic.
2. Move authentication into a route enhancer which runs with a higher priority than upcasting.
3. Decouple upcasting from routing and execute it after authentication and language negotiation but before access checking (i.e. in AccessAwareRouter::matchRequest().

#41.2 looks like the the only option that can be accomodated in the current issue scope, so I will try out that option.

Awesome @znerol :) You've already done great work in this area, and I'd be happy to review.

It would be still great to outline in the issue summary the following points. Its hard to understand what this issue is exactly about, beside of the state of HEAD

a) What is problematic in triggering authentication multiple times. The only thing I can read in the summary is: "in my opinion its wrong"
b) How can you ever resolve the problem that you need the current user to do path inbound processing which comes before routing
c) What is the problem in running authentication again once you have more information due to basic auth?

@znerol
... this is for everyone trying to understand the issue, please be fair and give people an easy life when they want to participate in any kind of way in this issue.

@dawehner: #52.b: Good question. We need to figure this one out. Updated IS.

@znerol, when I was stepping through the code, I found the PathProcessorLanguage ran before the RouterListener. Maybe you could check?

Straight re-roll

Authentication providers need some of the $request attributes to be injected early. Let's see how many fails we have now.

Okay. Looks like the Cookie auth provider is not doing that much in this patch - it's definitely not authenticating and this patch expects it to. Drupal cookie auth is being handled in SessionHandler.

This shouldn't be the case and should be moved to \Drupal\Core\Authentication\Provider\Cookie. #2228393: Decouple session from cookie based user authentication may need to go in first before we continue on this issue.

Pulled in some ideas from #2228393: Decouple session from cookie based user authentication (nice work @znerol) to separate out cookie auth from the SessionHandler into the Cookie auth provider. Also applied some fixes from #2328645: Remove remaining global $user (btw, it would help to commit that issue before this one).

This makes the patch much larger, but it's good to test that it will work. Afterwards the Cookie auth part (the interdiff mostly) can be pulled out to #2228393: Decouple session from cookie based user authentication and finished there.

Let's see how many tests fail now.

1. +++ b/core/lib/Drupal/Core/Authentication/Provider/Cookie.php
   @@ -28,13 +49,49 @@ public function applies(Request $request) {
   +      $values = $this->connection->query("SELECT u.*, s.* FROM {users_field_data} u INNER JOIN {sessions} s ON u.uid = s.uid WHERE u.default_langcode = 1 AND s.sid = :sid", array(
   

   suppose better to query sessions first, then join user data, also please left only required columns

2. +++ b/core/lib/Drupal/Core/Authentication/Provider/Cookie.php
   @@ -28,13 +49,49 @@ public function applies(Request $request) {
   +      if ($values && $values['uid'] > 0 && $values['status'] == 1) {
   ...
   +        $rids = $this->connection->query("SELECT ur.roles_target_id as rid FROM {user__roles} ur WHERE ur.entity_id = :uid", array(
   ...
   +        /** @var \Drupal\user\UserStorageInterface $storage */
   +        $storage = \Drupal::entityManager()->getStorage('user');
   +        $storage->updateLastAccessTimestamp($user, REQUEST_TIME);
   

   Not good for Core to know storage related details of user module.

   probably better to use entity query service (properly injected)

   Also entity manager should be injected

3. +++ b/core/lib/Drupal/Core/Session/SessionHandler.php
   @@ -64,57 +74,30 @@ public function open($save_path, $name) {
   +    $record = $this->connection->select('sessions', 's')
   ...
   -    $values = $this->connection->query("SELECT u.*, s.* FROM {users_field_data} u INNER JOIN {sessions} s ON u.uid = s.uid WHERE u.default_langcode = 1 AND s.sid = :sid", array(
   

   now this will execute a module handler for alter query hook

It looks like these tests are failing because when you login with \Drupal\simpletest\WebTestBase::drupalLogin that it's not sticking. The login works, but then when the browser requests node/add/book, the session isn't found so you can't access the page, can't fill out the form, can't create a new node, and the tests start failing.

Also curious. After you run tests, with this patch applied, you're logged out of your session.

Not quite sure what is broken yet, but posting these notes in case it helps someone else see what is broken.

Couple more things.

+++ b/core/authorize.php
@@ -50,8 +50,10 @@
+function authorize_access_allowed(Request $request) {

This patch adds a parameter to the authorize_access_allowed() function, but doesn't add the corresponding @docblock

+++ b/core/lib/Drupal/Core/Routing/Enhancer/AuthenticationEnhancer.php
@@ -7,72 +7,73 @@
+ * Authenticates the incomming request.

Typo, "incomming" should be "incoming"

+++ b/core/lib/Drupal/Core/Routing/Enhancer/AuthenticationEnhancer.php
@@ -7,72 +7,73 @@
+    // incomming master request.

"incomming" should be "incoming"

Thanks for the reviews @andypost and @eojthebrave. Will tackle them after fixing the test fails.

$user = $this->drupalCreateUser(['appropriate', 'permissions']);
$this->drupalLogin($user);
$this->drupalGet('node/add/book'); // This one will work - 200 OK
$this->drupalGet('node/add/book'); // This one will fail - 403 Forbidden

The first one will work, the second one will fail with access denied. That's why ::drupalPostForm('node/add/book', ...) fails because the post is done on the second call.

Interestingly, this doesn't happen for all tests. I wrote a spanking new custom module with a test and copied BookTest::createBookNode() into it. And that module's version of ::createBookNode() worked, BookTest's version failed. Crazy...

Still working through the code...

So I think I found the offending line of code:

+++ b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
@@ -86,12 +97,22 @@ public function getContext() {
+    catch (\Exception $e) {
+      try {
+        $this->accountProxy->setAccount($this->authenticationProvider->authenticate($request));
+      }
+      catch (\Exception $e) {
+        // Ignore exception thrown while trying to authenticate inside the
+        // enclosing catch clause.
+      }
+
+      throw $e;

Attempting to catch and re-authenticate after an exception in the AccessAwareRouter::matchRequest() fails, leading to the current user (as read from the session) being effectively logged out again. This is the cause of most of the test failures - sending to testbot to confirm.

The exceptions being thrown are of the nature:
None of the routers in the chain matched this request GET /node HTTP/1.1 Accept: text/html Accept-Charset: ISO-8859-1,...

I'm now drilling down to understand why authentication fails after these exceptions are thrown in the AccessAwareRouter::matchRequest() (and perhaps also why the route matching fails in the first place)

Is there any particular reason why we need to re-authenticate after the matcher fails to match? If there is a session already and the user is authenticated from the session, need we bother to re-authenticate after the route match fails?

I think the nested try/catch was added in #62, so perhaps the reason is explained in the comments and changes around #60 - #70 ?

Drilled a bit more into the exceptions thrown by AccessAwareRouter and why authentication fails afterwards. Found that the PathBasedBreadcrumbBuilder is using the AccessAwareRouter to find matching routes as it is building the breadcrumb off of the current path. Unfortunately, the $request object passed in is incomplete (no route parameters) resulting in the exceptions as well as the auth failures.

Another undesirable side effect of this is that authentication is re-run multiple times as the breadcrumb links are being built.
A way to fix this would be to changing the PathBasedBreadcrumbBuilder to use a route matcher (e.g. NestedMatcher) instead of a router to build its links.

Removed authentication from AccessAwareRouter. Now auth is only on the AuthenticationEnhancer.

Meh

Found that the PathBasedBreadcrumbBuilder is using the AccessAwareRouter to find matching routes as it is building the breadcrumb off of the current path. Unfortunately, the $request object passed in is incomplete (no route parameters) resulting in the exceptions as well as the auth failures.

I think this needs own issue to solve.

#84 shows same failures are not caused by that

The really clean way to do authentication would have been to do it in a kernel event subscriber that runs before routing. We would avoid all of this route enhancer messiness. AFAICT the only thing making this hard is the _auth routing option. Maybe we can figure out a different approach to specifying the authentication provider that doesn't rely on the routing system.

I think this needs own issue to solve.

Raised #2428609: Refactor PathBasedBreadcrumbBuilder to not use AccessAwareRouter for building links

I do not think #73 was a good idea

Can u explain why? I was just trying out what could be possible causes of the test fails. We can move that change back to #2228393: Decouple session from cookie based user authentication and finish it there, I have no problem with that. But we need that change for authentication to work as it should.

Just in general I'm wondering whether we can decouple the routing system from the authentication system by using a path
level override.

looks as solid idea, +1

+1 to this proposal. Some questions @znerol:

1. has cookie, has ba credentials
   cookie authentication provider applies, authentication succeeds, 200

   Will basic auth trump over cookie auth in this case if the order of priority was 1) Basic Auth, 2) Cookie Auth?

2. How would we distinguish a rest route from a non-rest route?

This is a much more elegant approach than what we were doing before. Thanks.

1. -   * Filters a list of providers and only return those allowed on the request.
   -   *
   -   * @param \Drupal\Core\Authentication\AuthenticationProviderInterface[] $providers
   -   *   An array of authentication provider objects.
   -   * @param Request $request
   -   *   The request object.
   -   *
   -   * @return \Drupal\Core\Authentication\AuthenticationProviderInterface[]
   -   *   The filtered array authentication provider objects.
   -   */
   -  protected function filterProviders(array $providers, Request $request) {
   -    $route = RouteMatch::createFromRequest($request)->getRouteObject();
   -    $allowed_providers = array();
   -    if ($route && $route->hasOption('_auth')) {
   -      $allowed_providers = $route->getOption('_auth');
   -    }
   -    elseif ($default_provider = $this->defaultProviderId()) {
   -      // @todo Mirrors the defective behavior of AuthenticationEnhancer and
   -      // restricts the list of allowed providers to the default provider if no
   -      // _auth was specified on the current route.
   -      //
   -      // This restriction will be removed by https://www.drupal.org/node/2286971
   -      // See also https://www.drupal.org/node/2283637
   -      $allowed_providers = array($default_provider);
   -    }
   -
   -    return array_intersect_key($providers, array_flip($allowed_providers));
   -  }
   -
   -  /**
   

   +1

2.    public function applies(Request $request) {
   -    return $request->hasSession();
   +    return $request->hasSession() && $this->sessionConfiguration->hasSession($request);
      }
   

   These two look like they are doing the same thing, it's only after checking SessionConfiguration::hasSession() docs that we see that sessionConfiguration is actually checking for a session cookie. Not related, but it would be clearer if SessionConfiguration::hasSession() was called ::hasSessionCookie()

3. +  /**
   +   * Authenticates user on request.
   +   *
   +   * @see \Drupal\Core\Authentication\AuthenticationProviderInterface::authenticate()
   +   */
   +  public function onKernelRequestAuthenticate(GetResponseEvent $event) {
   +    if ($event->getRequestType() == HttpKernelInterface::MASTER_REQUEST) {
   +      $request = $event->getRequest();
   +      $account = $this->authenticationProvider->authenticate($request);
   +      $this->accountProxy->setAccount($account);
   +    }
      }
   

   <3 Now that's what I'm talking 'bout.

4. --- a/core/lib/Drupal/Core/Routing/Enhancer/AuthenticationEnhancer.php
   +++ /dev/null
   @@ -1,86 +0,0 @@
   -<?php
   -
   

   +1. Bye-bye authentication enhancer.

5.    public function handleException(GetResponseForExceptionEvent $event) {
        $exception = $event->getException();
   -    if ($GLOBALS['user']->isAnonymous() && $exception instanceof AccessDeniedHttpException) {
   -      if (!$this->applies($event->getRequest())) {
   +    $request = $event->getRequest();
   +    if ($GLOBALS['user']->isAnonymous() && $exception instanceof AccessDeniedHttpException && !$this->hasCredentials($request)) {
   +      $route = RouteMatch::createFromRequest($request)->getRouteObject();
   

   We shouldn't be using global $user anymore after this issue considering #2328645: Remove remaining global $user will remove the last remaining uses that are not in this issue.

6. @@ -50,11 +50,6 @@ public function testBasicAuth() {
        $this->drupalGet('admin');
        $this->assertResponse('403', 'No authentication prompt for routes not explicitly defining authentication providers.');
    
   -    $account = $this->drupalCreateUser(array('access administration pages'));
   -
   -    $this->basicAuthGet(Url::fromRoute('system.admin'), $account->getUsername(), $account->pass_raw);
   -    $this->assertNoLink('Log out', 0, 'User is not logged in');
   -    $this->assertResponse('403', 'No basic authentication for routes not explicitly defining authentication providers.');
   

   Looks like BasicAuth tests need to be updated to reflect the expectations in #92. Maybe in a follow up.

Is there any use case where we would want to disable Cookie Authentication completely?

I agree with @almaudoh, apart from the need to remove the call to $GLOBALS[user], this looks great, so +1 from me too. I will test it as soon as it gets to Needs Review again.

Is there any use case where we would want to disable Cookie Authentication completely?

Don't know if it's relevant here but I don't use it when we authenticate our REST calls.

Long story short, we need to restore current_user after a container rebuild.

Regrettably that is not as easy as it seems. This is mainly because the entity.manager service needs some additional setup/cache-clearing before it can be used after modules are updated.

We could use the UserSession to restore the user account instead of trying to load the User entity and also creating an additional dependency on entity manager.

FWIW I'm not comfortable with making current_user container aware in #108.

Actually, I believe the preferred approach to setting the current user should be to use either new AnonymousUserSession() or new UserSession($user_id), so we don't have to load the huge User entity if we don't need to.

RE #111:

we need to either remove the container awareness of AccountProxy and instead use a static call to Drupal::entityManager() or figure out is trying to serialize the proxy and whether this is justifiable (i bet not).

Can we do both?

This is looking much better now. Just a few nits:

1. +      // Save the id of the currently logged in user.
   +      if ($this->container->initialized('current_user')) {
   +        $current_user_id = $this->container->get('current_user')->id();
   +      }
   +
   

   Maybe we can expand on the comment to explain why we are saving the currently logged in user.

2. +
   +    if (!empty($current_user_id)) {
   +      $this->container->get('current_user')->setInitialAccountId($current_user_id);
   +    }
   +
   .
   .
   .
   +  /**
   +   * Sets the id of the initial account.
   +   *
   +   * Never use this method, its sole purpose is to work around weird effects
   +   * during mid-request container rebuilds.
   +   *
   +   * @param int $account_id
   +   *   The id of the initial account.
   +   */
   +  public function setInitialAccountId($account_id);
   

   This is the only not-so-nice piece of this otherwise great patch. In a follow-up, we may want to look at how this can be improved.

3. index 80f7796..4d622a7 100644
   --- a/core/lib/Drupal/Core/EventSubscriber/AuthenticationSubscriber.php
   +++ b/core/lib/Drupal/Core/EventSubscriber/AuthenticationSubscriber.php
   @@ -8,8 +8,10 @@
   +  /**
       * Keep authentication manager as private variable.
       *
   -   * @param AuthenticationProviderInterface $authentication_manager
   +   * @param \Drupal\Core\Authentication\AuthenticationProviderInterface $authentication_manager
       *   The authentication manager.
   -  public function __construct(AuthenticationProviderInterface $authentication_provider) {
   +  public function __construct(AuthenticationProviderInterface $authentication_provider, AccountProxyInterface $account_proxy) {
   

   s/manager/provider/. Match docs to function signature. Also the constructor first line doc needs to be updated.

Updated IS and added related issue #2228393: Decouple session from cookie based user authentication because cookie auth is still being done in SessionHandler

Hm, I'm not sure it can be this easy.

This makes every authentication method available on every part of Drupal. Take something like https://www.drupal.org/project/services_token_access, you don't want that to allow you to access every page?

I also think it is wrong to hardcode basic auth to rest routes, and you're not actually doing that yet, you're still relying on the _auth option. custom services/API's will want to use basic auth as well, and want to get a login prompt, somehow?

I'm not sure why the following is not an option, I've not read every comment/patch here:

1. Do the authentication like done here, the first one that matches wins, don't worry about routes.
2. When matching the route, compare the method that was used with the requirements. Possibly improve improve the fallback to a configurable list of default methods instead of that hardcoded last-in-list-wins assumption. (This should be able to deal with the "but I want to allow client cert and cookie, or only client cert" requirement)
3. If it doesn't match, access denied. No re-trying or anything, just bail out.

So then you try to use basic auth to access /user. With HEAD, you get a normal page, as anonymous, now you get a 403, so what? IMHO, that's even better. What worse can happen, how is that a problem?

I don't know if access checker or built-in, but as written, we whould have to keep the current behavior more or less that by default, only the default method(s) are allowed?

A method on that interface might work, but we also need a way to set it, we can say that we only have that on actual implementations, but user entity is tricky, there we need it on the interface, which would actually be a bit weird?)

For our REST interfaces we use our own custom provider, which passes an OAuth2 token to an external authentication server. For "normal" access we use Cookie Authentication.

I am not sure what is being suggested here, but it would make sense to me that I can configure a route to use whatever provider I want.

The latest patch removes that option, and I agree with you that is not an option, including the part that not specifying anything on a route needs to use a (limited, somehow configurable) list, not all.

HEAD is doing some strange things right now and tries to re-authenticate if you e.g. use basic auth for a route that does not allow it, so it switches to the cookie authentication, which then provides the default anon user. My suggestion was to simply drop that part and in that case, return a 403 and be done with it.

I think that makes sense, and I think that functionality belongs into the domain of access checking. I'm not yet sure whether it should be implemented as a *real* access checker automatically added to each route or just be plugged directly into the access manager.

I'm not sure, access checking is also applied to all displayed links, and that's a pretty pointless thing to run those?

Regarding the getAuthenticationMethod() on the user entity, that could be implemented to just return NULL (or FALSE or whatever we want to return for unauthenticated). Only the UserSession and AccountProxy actually need to return something useful. That said, it is also weird to have all those *session* methods on the user entity.

Uhm, no? basic auth already returns loaded entities, and so would #2345611: Load user entity in Cookie AuthenticationProvider instead of using manual queries? And those need to return something too?

That's also not my point, I've no issue with adding getAuthenticationMethod(). I'm asking how basic_auth would tell the loaded $user entity that it was loaded through basic_auth. For that, we need "setAuthenticationMethod()" on UserInterface, and that's the weird part :)

- Removed, wrong issue -

Right, we could possibly do AccountProxyInterface::getAccountMethod(), but that's not what you wrote :)

Ignore comment #145, wrong issue o.0

Maybe I'm not following the discussions well here, but there is still the first question: how do you associate _auth method with specific routes? And how is AuthenticationSubscriber aware of this since it is running before routing? Will AuthenticationManager build and maintain it's own mapping of route -> _auth when routes are being built?

Also, I don't think it makes sense to authenticate successfully with cookie auth, for example, only to 403 again just because the route specified basic auth since cookie auth never gave basic auth the chance to authenticate the user (cookie auth applied first).

Maybe I'm not following the discussions well here, but there is still the first question: how do you associate _auth method with specific routes? And how is AuthenticationSubscriber aware of this since it is running before routing? Will AuthenticationManager build and maintain it's own mapping of route -> _auth when routes are being built?

No mapping, no knowing.

We authenticate first, and know user X, with authentication method Y.

When we check access to the route, we look for the _auth option and deny access if we have a mismatch.

Maybe I'm missing something, because it almost seems too easy ;)

Also, I don't think it makes sense to authenticate successfully with cookie auth, for example, only to 403 again just because the route specified basic auth since cookie auth never gave basic auth the chance to authenticate the user (cookie auth applied first).

No, basic_auth runs first. It looks like it doesn't in HEAD, but that is because we filter it out when we don't know yet what will be allowed. This is exactly what would change, removing that filtering.

It might not allow for every imaginable scenario, but I can currently not think of any that would be an actual problem.

I think what we should do is convert the comparison from #92 into a table with HEAD | znerol's idea | my idea and extend it with at least one more use case: trying to access /user with basic authentication.

I don't have time to do that right now, if someone can start that, that would be great, I'm happy to complete/review it later.

+++ b/core/modules/basic_auth/src/Access/NoBasicAuthCheck.php
@@ -0,0 +1,69 @@
+  protected function hasCredentials(Request $request) {

+++ b/core/modules/basic_auth/src/Authentication/Provider/BasicAuth.php
@@ -152,4 +158,32 @@ public function handleException(GetResponseForExceptionEvent $event) {
+  protected function hasCredentials(Request $request) {

Could go in a trait to avoid duplication?

I'm confused why you add a custom check for basic auth? We want a generic solution, that is also applied if there is no _auth option, which means it is applied to all routes. And that would imho just cause unexpected troubles with ANY, so it should not be an access check?

But it isn't possible :)

It will apply to every route and return TRUE or FALSE. This will completely break the ANY/or mode, because those will always allow access.

Not sure what you mean. ANY means that at least one check needs to return TRUE. If we add an access check that applies to all routes, and always returns TRUE, then the routes that use ANY always result in TRUE, because at least one check is always going to do so.

That's not screwed? That's ANY, by design. And the access checker knowing about it or not doesn't change anything?

Mh? For CSRF you always want to KILL. Other cases might want to consider other access checkers as well.

Few review comments/questions:

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -731,6 +737,12 @@ protected function initializeContainer($rebuild = FALSE) {
   +    if (!empty($current_user_id)) {
   

   so, we are trying to set account and auth_method only for authenticated user here?

2. +++ b/core/lib/Drupal/Core/EventSubscriber/AuthenticationMethodCheckSubscriber.php
   @@ -0,0 +1,76 @@
   +   * Constructs a new authentictaino method check.
   

   Minor: typo

3. +++ b/core/lib/Drupal/Core/EventSubscriber/AuthenticationMethodCheckSubscriber.php
   @@ -0,0 +1,76 @@
   +    // @todo: Collect list of providers in a compiler pass and inject them into
   

   Do we need a follow up for this todo? if there is one, can we update the d.o URL?

4. +++ b/core/lib/Drupal/Core/EventSubscriber/AuthenticationMethodCheckSubscriber.php
   @@ -0,0 +1,76 @@
   +   * Checks whether authentication was performed with a method allowed on the route.
   

   More than 80 chars?

5. +++ b/core/modules/basic_auth/src/Authentication/Provider/BasicAuth.php
   @@ -152,4 +158,32 @@ public function handleException(GetResponseForExceptionEvent $event) {
   +  protected function hasCredentials(Request $request) {
   

   Why isn't this part of applies()? do we really need a new method here?

Re #171: I still think the AnonymousUserSession / UserSession should be used for authentication instead of the User entity. This decouples the authentication system from the entity system and keeps code less complex.

We could re-purpose the (Anonymous)UserSession into value objects to represent sessions as the name implies. We don't have to pollute the AccountInterface with getAuthenticationMethod/setAuthenticationMethod since it's not needed there, just extend a UserSessionInterface from it to hold those methods, then AuthenticationProviderInterface::authenticate() would return an instance of UserSessionInterface

1. +++ b/core/lib/Drupal/Core/Authentication/AuthenticationProviderAccessInterface.php
   @@ -0,0 +1,37 @@
   +   *
   +   * @var \Symfony\Component\HttpFoundation\Request
   +   *   The request.
   +   *
   

   This should be @param.

2. +++ b/core/lib/Drupal/Core/Authentication/AuthenticationProviderAccessInterface.php
   @@ -0,0 +1,37 @@
   +  /**
   +   * Checks whether the authentication method is allowed on a given route/path.
   +   *
   +   * While authentication itself is run before routing, this method is called
   +   * after routing, hence RouteMatch is available and can be used to inspect
   +   * route options.
   +   *
   +   * @var \Symfony\Component\HttpFoundation\Request
   +   *   The request.
   +   *
   +   * @return bool
   ...
   +   *   FALSE.
   +   */
   +  public function access(Request $request);
   

   I'm not sure if "access" is a good name here. Maybe "isAllowed()" instead?

   @return should say "TRUE if this authentication method is allowed ..."

3. +++ b/core/lib/Drupal/Core/Session/AccountProxy.php
   @@ -75,10 +52,17 @@ public function setAccount(AccountInterface $account) {
   +        // After the container is rebuilt, DrupalKernel sets the initial
   +        // account to the id of the logged in user. This is necessary in order
   +        // to refresh the user account reference here.
   +        $this->account = \Drupal::entityManager()->getStorage('user')->load($this->initialAccountId);
   

   Could you add a comment here why we cannot inject the user storage here and must call the global \Drupal?

Otherwise looks like an improvement to me and the changes to the REST tests look fine.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/AuthenticationSubscriber.php
   @@ -7,71 +7,139 @@
   +    // The priority for authentictaion must be higher than the highest event
   

   typo.

2. +++ b/core/modules/basic_auth/src/Authentication/Provider/BasicAuth.php
   @@ -131,25 +132,14 @@ public function authenticate(Request $request) {
   +    global $base_url;
   

   you should be able to get this from the request context, getCompleteBaseUrl()

This is hard to follow. The tables seem to have different labels and structure.

Can you bold the items that demonstrate the differences / bugs?

Here's my attempt at displaying the results.

Note, I misunderstood first, so explanation of the columns:

access: allow = anyone, auth = only when logged in, deny = nobody is allowed to access
_auth: what the route defines as the _auth option
cred: credentials the request sends, (both means both cookie and basic auth in the same request)
response: the response status code

Output is a "diff -up head.txt patch.txt", so the changes are as the patch changes it:

--- head.txt	2015-02-23 18:53:45.301784493 +0100
+++ patch.txt	2015-02-23 18:54:10.129958403 +0100
@@ -2,13 +2,13 @@ No. access  _auth   cred    response
  1. allow   none    none    200
  2. allow   none    cookie  200
  3. allow   none    basic   200
- 4. allow   none    both    200
+ 4. allow   none    both    403
  5. allow   cookie  none    200
  6. allow   cookie  cookie  200
  7. allow   cookie  basic   200
- 8. allow   cookie  both    200
+ 8. allow   cookie  both    403
  9. allow   basic   none    200
-10. allow   basic   cookie  200
+10. allow   basic   cookie  403
 11. allow   basic   basic   200
 12. allow   basic   both    200
 13. allow   both    none    200
@@ -18,13 +18,13 @@ No. access  _auth   cred    response
 17. auth    none    none    403
 18. auth    none    cookie  200
 19. auth    none    basic   403
-20. auth    none    both    200
+20. auth    none    both    403
 21. auth    cookie  none    403
 22. auth    cookie  cookie  200
 23. auth    cookie  basic   403
-24. auth    cookie  both    200
+24. auth    cookie  both    403
 25. auth    basic   none    401
-26. auth    basic   cookie  401
+26. auth    basic   cookie  403
 27. auth    basic   basic   200
 28. auth    basic   both    200
 29. auth    both    none    401
@@ -40,7 +40,7 @@ No. access  _auth   cred    response
 39. deny    cookie  basic   403
 40. deny    cookie  both    403
 41. deny    basic   none    401
-42. deny    basic   cookie  401
+42. deny    basic   cookie  403
 43. deny    basic   basic   403
 44. deny    basic   both    403
 45. deny    both    none    401

The differences are easy to explain:
4: sends both, basic auth is checked first, authenticated, then denied (because non means the default aka cookie). This is exactly the change that I mentioned above, IMHO OK and actually expected.
8: same except it explicitly wants cookie
10: This is an interesting change, I think this makes sense, wrong auth method should not allow, even if the route allows anyone access (that combination does not make sense, but it's also not relevant that it would allow anyone, just that it does if you're authenticated). No idea why it allowed that before?

20 & 24 are the same as 4 & 8, 26 is the same as 10 (it makes no difference for all of those if only auth or all are allowed)

42. is the same as 10 really, we would have allowed access but then basic auth would have kicked in with the challenge I think. Now we deny access.

So all those changes are IMHO valid, and I like the new Authentication subscriber :)

Confirmed, sorry. Looks like the page cache interfered locally, I think this is the security issue that was recently closed and has yet to be fixed in 8.x (aka, basic auth requests must not be page cached, ever).

@znerol uploading patches at superspeed - like The Flash :)

Some reviews and some comments:

1. -   * @var string
   +   * @var \Drupal\Core\Authentication\AuthenticationAccessProviderInterface[]
       */
   -  protected $triggeredProviderId = '';
   +  protected $filters;
   

   Doc should be AuthenticationProviderFilterInterface.

2. +   * List of providers which implement the challenge interface.
   +   *
   +   * @var \Drupal\Core\Authentication\AuthenticationChallengeProviderInterface[]
   +   */
   +  protected $challengers;
   +
   

   The name $challengers threw me off at first, and I was wondering "what are they challenging". Couldn't figure out from the name until I read the code - challenging the AccessDeniedException? Maybe a more descriptive name?

3. +   * @see https://www.drupal.org/node/2432585
   +   */
   +  public function __construct($global_providers = ['cookie' => TRUE]) {
   +    $this->globalProviders = $global_providers;
   +  }
   

   Hard-coding cookie auth, what if there's a use case where cookie auth is not wanted? Guess we can discuss that more at #2432585: Improve authentication manager service construction to support custom global service providers

4. @@ -72,139 +97,175 @@ public function addProvider(AuthenticationProviderInterface $provider, $id, $pri
   +    if ($provider instanceof AuthenticationProviderFilterInterface) {
   +      $this->filters[$id] = $provider;
   +    }
   +    if ($provider instanceof AuthenticationProviderChallengeInterface) {
   +      $this->challengers[$id] = $provider;
   +    }
   .
   .
   .
   index 80f7796..3619e2b 100644
   --- a/core/lib/Drupal/Core/EventSubscriber/AuthenticationSubscriber.php
   +++ b/core/lib/Drupal/Core/EventSubscriber/AuthenticationSubscriber.php
   @@ -7,71 +7,139 @@
   +  public function __construct(AuthenticationProviderInterface $authentication_provider, AccountProxyInterface $account_proxy) {
        $this->authenticationProvider = $authentication_provider;
   +    $this->filter = ($authentication_provider instanceof AuthenticationProviderFilterInterface) ? $authentication_provider : NULL;
   +    $this->challengeProvider = ($authentication_provider instanceof AuthenticationProviderChallengeInterface) ? $authentication_provider : NULL;
   

   Wonder if we couldn't just inject these as separate arguments since we have separate interfaces, this makes the dependencies more explicit. Of course, this implies @authentication would be repeated three times in the service definition arguments. But this just makes it clearer that AuthenticationManager is playing a triple role.

5. +   * @return string|NULL
   +   *   The id of the first authentication provider which applies to the request.
   +   *   If no application detects appropriate credentials, then NULL is returned.
       */
   .
   .
   +  protected function getChallenger($request) {
   +    if (!empty($this->challengers)) {
   

   @return doc doesn't match method.

Overall, these are great changes.

If basic is not among the default auth providers, it shouldn't be the one to authenticate in the first place, even though it is higher in priority.

This is my concern and will cause problems in a use case I have right now.

[Edit: completed sentence]

Re #193 & #194: The behavior in 4 & 8 is not satisfactory. Without specifying an _auth method, basic authenticates and then denies. Moreover, the route / path is allowed for all users. That is not proper behavior IMHO

I respectfully disagree ;)

Note that it is *not* possible authenticate with basic auth on HEAD if no _auth option is provided either. What happens instead is that basic_auth fails to authenticate in the first place*, we fall back to session, "authenticate" as anon user and are granted access.

* To clarify even more: basic auth *always* fails at that point. We throw away the result and repeat it after we get the route, and only then basic auth can authenticate.

The fact that all users would be allowed to access the page is a detail that we don't know yet at that point.

This is also visible in the fact that those cases are no longer different in second group of checks, where we actually require a user. There both deny access, if for a different reason.

If basic is not among the default auth providers, it shouldn't be the one to authenticate in the first place, even though it is higher in priority.

This is my concern and will cause problems in a use case I have right now.

Care to share your use case for this? I've trouble imaging a use case that relies on this.

Also, you +1'd the suggestion from @znerol in #92, which would have removed the concept of a limited list of default auth providers completely, so basic auth would have allowed access to all routes.

We just can't have it both ways. We can't require the authenticated user before routing and make authentication depending on the matched route.

See also #104, which explains possible issues with the current behavior. This issue IMHO solves that, because we only have one authentication process and we can rely on the result, even if we end up denying access because of the used method.

In the hope that the discussion has completed, I reviewed & tested the code, and using the test module from #188, I believe I have been able to reproduced the results in #193.

But I don't feel confident enough about what is going on to mark this RTBC. I wonder if some sort of flow chart, maybe something like https://www.drupal.org/node/2212069#comment-9651765 would be helpful ?

Re #204: We want to see the new authentication system as something that should be flexible enough to apply to other (possibly not yet envisioned) use cases. In this patch we are basing it on the specific use cases that assume basic auth would run before cookie auth - I realize this is based on what we already have in HEAD.

While this patch is a big improvement on what we have in HEAD, we do have to acknowledge that the use of the _auth parameter in routing is the key limitation in implementing a more flexible and robust authentication system. Because we should only do authentication once, with the appropriately selected auth method, prior to routing.

In order not to hold up this patch, which I would not want to do, we can continue this line of reasoning in a separate issue.

Also, you +1'd the suggestion from @znerol in #92, which would have removed the concept of a limited list of default auth providers completely, so basic auth would have allowed access to all routes.

I actually saw that as an intermediate step, after which we could figure out how to limit auth methods to routes without using the _auth routing parameter (mentioned in #89). But the ideas took a different direction, which is why I got confused thereafter.

We just can't have it both ways. We can't require the authenticated user before routing and make authentication depending on the matched route.

Will experiment with other different possibilities in a separate issue.

Edit: removed the comment

@almaudoh, #207

While this patch is a big improvement on what we have in HEAD, we do have to acknowledge that the use of the _auth parameter in routing is the key limitation in implementing a more flexible and robust authentication system.

I think flexibility & robustness are essential. I find the current system & proposed changes difficult to understand. The 45 step manual check in #193 illustrates how complex the system is. So some kind of picture would be very helpful - thanks for offering.