Add Authentication Collector

maybe it needs add related issues from contrib and tag as contrib blocker (console at least)

+++ b/core/modules/rest/rest.services.yml
@@ -31,3 +31,7 @@ services:
     arguments: ['rest']
+  rest.authentication_collector:
+    class: Drupal\rest\RestAuthenticationCollector
+    tags:
+    - { name: service_collector, tag: authentication_provider, call: addProvider }

I'm curious, does that really belong into rest module and not into core itself?

It would be great to have some sort of test coverage.

@dawehner regarding #4

it exists in core - today

core.services.yml
under the name 'authentication'

it does vacuum up all the authentication_provider's
but the results in the form of AuthenticationManager::getSortedProviders() are protected.

I am curious to know why it is protected... I can't see any security implications.

[ Side note - to be helpful - the todo above AuthenticationManager::getSortedProviders() is stale - the issue is fixed, but the comment just has not been removed ]

Re #4 and #5: Looks like we went round in a really big circle here.

See #2456303-23: AuthenticationManager needs interface and ::getProviderKeys where @znerol explained the rationale for making AuthenticationManager::getSortedProviders() protected and the suggested solution #2456303-16: AuthenticationManager needs interface and ::getProviderKeys, which led to #2456283-4: AuthenticationManager::getSortedProviders is protected, which led to this issue.

So maybe we need to review the rationale again though, even though we could do without the duplication of code.

So in summary

There were three options, in #2456303: AuthenticationManager needs interface and ::getProviderKeys

1) Revert the visibility of the getSortedProviders() to public.
2) Add Authentication Collector to Rest - which spawned this issue
3) The third option is just provide a new method, and I am just going to quote @znerol

I think we should have gone with something like option 3

@znerol from this comment https://www.drupal.org/node/2456303#comment-9844563

However, I do no think that we simply should revert the visibility of getSortedProviders(). Rather let's add a method which returns metadata records keyed by provider key, such that the result looks something like this:

$result = [
  'basic_auth' => [
    'service_id' => 'basic_auth.authentication.basic_auth',
    'priority' => 100,
    'global' => FALSE,
  ],
  'cookie' => [
    'service_id' => 'authentication.cookie',
    'priority' => 0,
    'global' => TRUE,
  ],
];

I can do something about the stale todo from #6

#2505655: Remove stale todo in AuthenticationManager

If we go with the original approach but have a core AuthenticationCollector. What do people think about that? Something like this (Could remove AuthenticationManagerInterface also).

I am not apposed to the metadata idea either, but seems like that shouldn't live on the AuthenticationManager, and if it didn't then it would need to container aware to be able to load the providers?

I like that approach, given that its also similar to the approach done in \Drupal\Core\Access\AccessManager

1) Refactored AuthenticationManagerTest to reflect the new way of doing things

2) Fixed a gaggle of minor stray cs errors. - nothing major.

+++ b/core/tests/Drupal/Tests/Core/Authentication/AuthenticationManagerTest.php
@@ -28,9 +29,10 @@ class AuthenticationManagerTest extends UnitTestCase {
+    $auth_collector = new AuthenticationCollector();

@@ -48,14 +50,16 @@ public function testDefaultFilter($applies, $has_route, $auth_option, $provider_
+    $authentication_collector = new AuthenticationCollector();

+1 to not add a mock! Mocking IMHO mostly makes sense if you are outside of your domain, this is still as part of it so that is totally fine.

Do we really need dedicated tests for this collector? I mean it can't be bad, but we have a lot of implicit test coverage already.

+1 on the code in general.

I would be inclined to add a unit test anyway, wont take long to do. I can do that in the morning if no one else does first.

We can still remove the AuthenticationManagerInterface too.

Wow!! I really like this approach - really clean. Just some nitpicks mostly:

1. +++ b/core/lib/Drupal/Core/Authentication/AuthenticationCollector.php
   @@ -0,0 +1,90 @@
   +/**
   

   Nit: newline

2. +++ b/core/lib/Drupal/Core/Authentication/AuthenticationCollector.php
   @@ -0,0 +1,90 @@
   +    // Force the builders to be re-sorted.
   ...
   +      // Sort the builders according to priority.
   

   I know this is c/p code but I always hoped for an opportunity to change 'builders' to 'providers'

3. +++ b/core/lib/Drupal/Core/Authentication/AuthenticationCollectorInterface.php
   @@ -0,0 +1,62 @@
   +   *   THe provider ID.
   

   THe

4. +++ b/core/lib/Drupal/Core/Authentication/AuthenticationCollectorInterface.php
   @@ -0,0 +1,62 @@
   +   * @todo Replace with a list of providers sorted during compile time in
   +   *   https://www.drupal.org/node/2432585.
   +   *
   

   This was removed in #2505655: Remove stale todo in AuthenticationManager

5. +++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.php
   @@ -58,30 +58,20 @@ class AuthenticationManager implements AuthenticationProviderInterface, Authenti
      /**
   ...
   +   * @var \Drupal\Core\Authentication\AuthenticationCollectorInterface
   ...
   +  protected $authCollector;
   

   Doc first line...

6. +++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.php
   @@ -58,30 +58,20 @@ class AuthenticationManager implements AuthenticationProviderInterface, Authenti
   +  public function __construct(AuthenticationCollectorInterface $auth_collector) {
   

   Constructor doc

7. +++ b/core/lib/Drupal/Core/Authentication/AuthenticationManagerInterface.php
   @@ -12,19 +12,4 @@
    interface AuthenticationManagerInterface {
    
   ...
    }
   

   We can remove AuthenticationManagerInterface now.

I did some code style changes requested by @almaudoh using patch attached in comment #13 by @martin107

Hi enzo,

There is a missing semicolon at the end of $authentication_collector = new AuthenticationCollector()

I hope this helps.

@@ -48,14 +50,16 @@ public function testDefaultFilter($applies, $has_route, $auth_option, $provider_
    * @covers ::applyFilter
    */
   public function testApplyFilterWithFilterprovider() {
-    $authentication_manager = new AuthenticationManager();
+    $authentication_collector = new AuthenticationCollector()
     $auth_provider = $this->getMock('Drupal\Tests\Core\Authentication\TestAuthenticationProviderInterface');
-    $authentication_manager->addProvider($auth_provider, 'filtered', 0);

Also look like the patch at #17 is missing a really big hunk - the AuthenticationCollector class

In this patch:
1. Addressed nits in #16
2. Removed some dead code in AuthenticationManager
3. Removed AuthenticationManagerInterface
4. Added unit test for AuthenticationCollector

#21 is a good improvement,

The tests cover what is needed, they are simple to understand and are complete so +1

I just picked out a few inconsequential nits while running the new tests through phpcs.

I would be really good to get this in, as it is holding up a console project command I use on a daily basis.
If I can work on a separate issue in exchange for a review here... just let me know :)

Updating the issue summary and adding beta evaluation.

I very much like the approach, feedback from #14, #15, #16 is included. This solution even narrows down the scope of AuthenticationManager to authentication - and nothing else. Information about the enabled providers can be retrieved from a separate service.

I'm really glad we did not simply reverted the visibility of the AuthenticationManager::getSortedProviders() method back to public. Thanks!

Two consecutive test runs, no changes in between. Probably there is too much randomness in the test? Maybe this happens if there are duplicate priorities?

$ ./vendor/bin/phpunit tests/Drupal/Tests/Core/Authentication/AuthenticationCollectorTest.php 
PHPUnit 4.6.4 by Sebastian Bergmann and contributors.

Configuration read from /home/lo/Documents/prj/drupal-8-qf/core/phpunit.xml.dist

F

Time: 232 ms, Memory: 5.75Mb

There was 1 failure:

1) Drupal\Tests\Core\Authentication\AuthenticationCollectorTest::testAuthenticationCollector
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     0 => Drupal\Tests\Core\Authentication\TestAuthenticationProvider Object (...)
     1 => Drupal\Tests\Core\Authentication\TestAuthenticationProvider Object (
-        'providerId' => 'qAC2F1SE'
+        'providerId' => 'xzDzkUFb'
     )
     2 => Drupal\Tests\Core\Authentication\TestAuthenticationProvider Object (
-        'providerId' => 'akSEwNYm'
+        'providerId' => 'qAC2F1SE'
     )
     3 => Drupal\Tests\Core\Authentication\TestAuthenticationProvider Object (
-        'providerId' => 'k36LmUwn'
+        'providerId' => 'akSEwNYm'
     )
     4 => Drupal\Tests\Core\Authentication\TestAuthenticationProvider Object (
-        'providerId' => 'umWj7N5U'
+        'providerId' => 'k36LmUwn'
     )
     5 => Drupal\Tests\Core\Authentication\TestAuthenticationProvider Object (
-        'providerId' => 'n42bFfVY'
+        'providerId' => 'umWj7N5U'
     )
     6 => Drupal\Tests\Core\Authentication\TestAuthenticationProvider Object (
-        'providerId' => 'hnx4DLMM'
+        'providerId' => 'n42bFfVY'
     )
     7 => Drupal\Tests\Core\Authentication\TestAuthenticationProvider Object (
-        'providerId' => 'dVJXfxtS'
+        'providerId' => 'hnx4DLMM'
     )
     8 => Drupal\Tests\Core\Authentication\TestAuthenticationProvider Object (
-        'providerId' => 'dImzwfwn'
+        'providerId' => 'dVJXfxtS'
     )
+    9 => Drupal\Tests\Core\Authentication\TestAuthenticationProvider Object (...)
 )

/home/lo/Documents/prj/drupal-8-qf/core/tests/Drupal/Tests/Core/Authentication/AuthenticationCollectorTest.php:44
                                     
FAILURES!                            
Tests: 1, Assertions: 1, Failures: 1.

$ ./vendor/bin/phpunit tests/Drupal/Tests/Core/Authentication/AuthenticationCollectorTest.php 
PHPUnit 4.6.4 by Sebastian Bergmann and contributors.

Configuration read from /home/lo/Documents/prj/drupal-8-qf/core/phpunit.xml.dist

.

Time: 211 ms, Memory: 5.50Mb

OK (1 test, 21 assertions)

Maybe this happens if there are duplicate priorities?

That's interesting considering that it's the same sort function krsort() being used for both the test and the actual code.

Still passes in my local system using both PHPUnit and run-tests.sh

Maybe this happens if there are duplicate priorities?

That's interesting considering that it's the same sort function krsort() being used for both the test and the actual code.

My bad. The way the test was written made no provision for duplicate priorities. Adjusted the test a little to cover this scenario.

In fact it wouldn't hurt if we'd test whether or not the authentication collector works properly with duplicated priorities. How about using a two-level array for the $providers in the test?

Edit: interdiff is against #22.

How about using a two-level array for the $providers in the test?

Great idea! Same as what we have in the class implementation itself. Here's the patch, also incorporating tests against duplicated priorities.

Thanks!

1. +++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.php
   @@ -58,30 +37,25 @@ class AuthenticationManager implements AuthenticationProviderInterface, Authenti
      /**
   -   * {@inheritdoc}
   +   * Creates a new authentication manager instance.
       */
   ...
   +  public function __construct(AuthenticationCollectorInterface $auth_collector) {
   

   We need to document the argument with an @param

2. +++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.php
   @@ -58,30 +37,25 @@ class AuthenticationManager implements AuthenticationProviderInterface, Authenti
   +    foreach ($this->authCollector->getSortedProviders() as $provider_id => $provider) {
   +      if ($provider instanceof AuthenticationProviderFilterInterface) {
   +        $this->filters[$provider_id] = $provider;
   +      }
   +      if ($provider instanceof AuthenticationProviderChallengeInterface) {
   +        $this->challengers[$provider_id] = $provider;
   +      }
   

   Looking at the code I don't get the point of storing the challengers and filters on the AuthenticationManager - in fact it now seems dangerous since if any code calls Authentication::addProvider() (for whatever reason) after the AuthenticationManager has been constructed the AuthenticationManager will behave unpredictably. If anything we should move this to AuthenticationCollector so that the list of challengers and filters can be maintained there.

1. Done
2. Not sure I agree with moving that to the auth collector. That is stuff that is pretty much internal to the auth manager. We can simplify things slight I think, not collect the filters and challengers and just check inline, when needed. Auth is not something that is going to be called lots of times (hopefully just once). So if anything, this will save a few checks. It also gets around the problem you outlined above (even though the likelyhood is very minimal).

Cool, yeah I agree optimizing for the case of multiple authentications is wrong.

Mhh, that's what the coverage is for. Missed that!

How about this? return the provider directly from getChallenger()?

+1 for returning objects directly. I'm curious whether this means we should better expand the test coverage for the AuthenticationManagerTest. Ideally it should have caused the problem, right?

So I have tightened up the documentation surrounding getChallenger() after #39

getChallenger() is private and only called by challengeException() -- all reasonable design decisions.

challengeException() is a preexisting segment of untested code ( I think ? ) My perspective is adding extra tests to confirm operation of the AuthenticationProviderChallengeInterface is a little out of scope.

I would like to move this "Contributed project blocker" along. So just let me know and I will open up and work on an issue to expanding test coverage.

Feel free to open up that issue, if we can open that and move this along, great. I think we have adequate coverage right now, especially with all the integration points these classes have. I don't see why it should block this right now.

I don't see why it should block this right now.

So does that mean RTBC then...

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.php
@@ -142,7 +97,7 @@ public function challengeException(Request $request, \Exception $previous) {
         return $provider_id;

@@ -150,21 +105,19 @@ protected function getProvider(Request $request) {
+        return $provider;

I understand that both getProvider and getChallenger are protected methods. Still there is a slight inconsistency if one returns an id while the other returns the object.

Yeah, I thought that too when doing the conversion. Would be good if they were consistent....

The trouble is the check to isGlobal() and defaultFilter() iirc. They always want an ID regardless of whether a provider is loaded. We could implement SplObjectStorage or something?

I would say it's just easier to go back the other way, just return IDs?

Yeah, that is better.

1. +++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.php
   @@ -97,7 +51,8 @@ public function applies(Request $request) {
        $provider_id = $this->getProvider($request);
   -    return $this->providers[$provider_id]->authenticate($request);
   +    $provider = $this->authCollector->getProvider($provider_id);
   +    return $provider->authenticate($request);
   

   I realize that this is a preexisting defect, but AuthenticationManager::getProvider() may return NULL. We should not attempt to authenticate in this case.

2. +++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.php
   @@ -126,8 +81,9 @@ public function appliesToRoutedRequest(Request $request, $authenticated) {
   +    if ($provider_id && ($provider = $this->authCollector->getProvider($provider_id))) {
   +      return $provider->challengeException($request, $previous);
        }
   

   Here, on the other hand, I suggest moving AuthenticationCollector::getProvider from the condition into the block. We can trust getChallenger to either return NULL or a valid provider id.

The trouble is the check to isGlobal() and defaultFilter() iirc. They always want an ID regardless of whether a provider is loaded.

Hit the same snag while working on that. +1 to #47.

Small docs nits:
1.

+
+  /**
+   * Add test providers to the AuthenticationCollector and test various methods.
+   *

Add => Adds, test => tests

2.

+class TestAuthenticationProvider implements AuthenticationProviderInterface {
+
+  /**
+   * The provider id.
+   *
+   * @var $provider_id
+   */
+  public $providerId;
+

s/@var $provider_id/@var/

The feedback got addressed in the meantime.

Committed bc3dad6 and pushed to 8.0.x. Thanks!