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?