Per today's SA, http://drupal.org/node/1890222, Cookie-based authentication for REST POST requests is insecure, particularly if you're running "certain browser plugins". (Need we guess which?) That can be worked around with a token, and we'll have to add support for that to Drupal 8 as well.

However, cookie-based authentication is not an appropriate approach for REST in the first place. It's not really "RESTful" because it's stateful, and it's really designed for browsers, not for web services. Instead, there are two more appropriate methods for doing user authentication for REST:

1) Http Basic Auth: Baked into the HTTP Spec, very simple.
2) OAuth: A big complex mess with less standardization than you might think.

OAuth is not going to make it into core for Drupal 8, and that's fine. It can live in contrib. However, we really should try to get Http Auth in so that we can do "proper" authenticated REST requests out of the box. It's very likely that we can steal code from http://drupal.org/project/services_basic_auth to save time.

The tricky part is that we don't have an authentication API right now. We have a session-login API (aka, a cookie-based one) that sets a global. This may necessitate cleaning that up so that we have an actual API to work with.

Also no doubt related: #1858196: [meta] Leverage Symfony Session components

I think this definitely qualifies as a feature, but because of the security implications I am marking it major. Also it probably should not go in REST module directly, but that seems the closest component for the time being.

Files: 
CommentFileSizeAuthor
#156 1890878.eighth-bootstrap-phase.patch407 bytesrszrama
PASSED: [[SimpleTest]]: [MySQL] 56,424 pass(es).
[ View ]
#153 drupal-1890878-153.patch41.68 KBcorvus_ch
PASSED: [[SimpleTest]]: [MySQL] 56,252 pass(es).
[ View ]
#149 1890878-149.patch41.65 KBfubhy
PASSED: [[SimpleTest]]: [MySQL] 58,018 pass(es).
[ View ]
#149 interdiff.txt6.11 KBfubhy
#144 1890878-144.patch41.47 KBfubhy
PASSED: [[SimpleTest]]: [MySQL] 57,966 pass(es).
[ View ]
#144 interdiff.txt12.32 KBfubhy
#136 drupal-1890878-136.patch40.67 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 58,571 pass(es).
[ View ]
#132 drupal-1890878-132.patch40.54 KBcorvus_ch
PASSED: [[SimpleTest]]: [MySQL] 58,151 pass(es).
[ View ]
#132 drupal-1890878-132.interdiff.txt5.79 KBcorvus_ch
#127 1890878-authentication.patch40.67 KBCrell
PASSED: [[SimpleTest]]: [MySQL] 58,272 pass(es).
[ View ]
#127 interdiff.txt7.5 KBCrell
#125 drupal-1890878-125.patch40.54 KBcorvus_ch
PASSED: [[SimpleTest]]: [MySQL] 58,847 pass(es).
[ View ]
#125 drupal-1890878-125.interdiff.txt3.74 KBcorvus_ch
#123 drupal-1890878-123.patch40.5 KBcorvus_ch
FAILED: [[SimpleTest]]: [MySQL] 58,019 pass(es), 23 fail(s), and 17 exception(s).
[ View ]
#123 drupal-1890878-123.interdiff.txt3.7 KBcorvus_ch
#121 drupal-1890878-121.patch38.85 KBcorvus_ch
PASSED: [[SimpleTest]]: [MySQL] 58,016 pass(es).
[ View ]
#121 drupal-1890878-121.interdiff.txt6.64 KBcorvus_ch
#118 drupal-1890878-118.patch38.33 KBcorvus_ch
PASSED: [[SimpleTest]]: [MySQL] 58,130 pass(es).
[ View ]
#118 drupal-1890878-118.interdiff.patch3.7 KBcorvus_ch
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1890878-118.interdiff.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#110 drupal-1890878-110.patch38.25 KBcorvus_ch
PASSED: [[SimpleTest]]: [MySQL] 57,984 pass(es).
[ View ]
#110 drupal-1890878-110.interdiff.txt6.05 KBcorvus_ch
#108 drupal-1890878-108.patch36.76 KBcorvus_ch
FAILED: [[SimpleTest]]: [MySQL] 58,017 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#108 drupal-1890878-108.interdiff.txt4.57 KBcorvus_ch
#106 drupal-1890878-106.patch36.7 KBcorvus_ch
FAILED: [[SimpleTest]]: [MySQL] 58,087 pass(es), 9 fail(s), and 0 exception(s).
[ View ]
#106 drupal-1890878-106.interdiff.txt1.61 KBcorvus_ch
#101 1890878-authentication.patch37.04 KBCrell
FAILED: [[SimpleTest]]: [MySQL] 58,295 pass(es), 65 fail(s), and 352 exception(s).
[ View ]
#101 interdiff.txt26.34 KBCrell
#97 core-1890878-authentication-providers-97.patch34.71 KBcorvus_ch
FAILED: [[SimpleTest]]: [MySQL] 58,264 pass(es), 49 fail(s), and 200 exception(s).
[ View ]
#97 core-1890878-authentication-providers-97.interdiff.txt19.84 KBcorvus_ch
#94 core-1890878-authentication-providers-94.patch32.75 KBcorvus_ch
FAILED: [[SimpleTest]]: [MySQL] 58,120 pass(es), 1 fail(s), and 198 exception(s).
[ View ]
#94 core-1890878-authentication-providers-94.interdiff.txt6.59 KBcorvus_ch
#92 core-1890878-authentication-providers-91.patch30.61 KBygerasimov
FAILED: [[SimpleTest]]: [MySQL] 56,869 pass(es), 0 fail(s), and 194 exception(s).
[ View ]
#92 core-1890878-authentication-providers-91.interdiff.txt7.95 KBygerasimov
#90 core-1890878-authentication-providers-80.patch28.58 KBcorvus_ch
FAILED: [[SimpleTest]]: [MySQL] 57,843 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#90 core-1890878-authentication-providers-80.interdiff.txt2.92 KBcorvus_ch
#88 core-1890878-authentication-providers-88.patch27.74 KBcorvus_ch
PASSED: [[SimpleTest]]: [MySQL] 57,827 pass(es).
[ View ]
#88 core-1890878-authentication-providers-88.interdiff.txt14.48 KBcorvus_ch
#81 core-1890878-authentication-providers-81.patch26.15 KBcorvus_ch
PASSED: [[SimpleTest]]: [MySQL] 57,874 pass(es).
[ View ]
#81 core-1890878-authentication-providers-81.interdiff.txt778 bytescorvus_ch
#78 core-1890878-authentication-providers-78.patch25.92 KBcorvus_ch
FAILED: [[SimpleTest]]: [MySQL] 57,965 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
#78 core-1890878-authentication-providers-78.interdiff.txt9.2 KBcorvus_ch
#75 core-1890878-authentication-providers-75.patch25.24 KBcorvus_ch
PASSED: [[SimpleTest]]: [MySQL] 58,083 pass(es).
[ View ]
#75 core-1890878-authentication-providers-75.interdiff.txt6.83 KBcorvus_ch
#71 core-1890878-authentication-providers-71.patch24.43 KBcorvus_ch
FAILED: [[SimpleTest]]: [MySQL] 57,751 pass(es), 1 fail(s), and 60 exception(s).
[ View ]
#71 core-1890878-authentication-providers-71.interdiff.txt7.21 KBcorvus_ch
#70 core-1890878-authentication-providers-70.patch22.5 KBygerasimov
FAILED: [[SimpleTest]]: [MySQL] 57,745 pass(es), 6 fail(s), and 5 exception(s).
[ View ]
#66 core-1890878-authentication-providers-66.patch22.41 KBcorvus_ch
FAILED: [[SimpleTest]]: [MySQL] 57,626 pass(es), 10 fail(s), and 49 exception(s).
[ View ]
#66 core-1890878-authentication-providers-66.interdiff.txt1008 bytescorvus_ch
#64 core-1890878-authentication-providers-64.patch22.3 KBygerasimov
FAILED: [[SimpleTest]]: [MySQL] 57,314 pass(es), 68 fail(s), and 49 exception(s).
[ View ]
#62 core-1890878-authentication-providers-62.patch21.83 KBygerasimov
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#57 core-1890878-authentication-providers-57.patch21.7 KBygerasimov
FAILED: [[SimpleTest]]: [MySQL] 53,450 pass(es), 118 fail(s), and 542 exception(s).
[ View ]
#55 core-1890878-authentication-providers-54.patch20.63 KBygerasimov
FAILED: [[SimpleTest]]: [MySQL] 54,481 pass(es), 147 fail(s), and 856 exception(s).
[ View ]
#53 core-1890878-authentication-providers-53.patch20.6 KBygerasimov
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-1890878-authentication-providers-53.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#51 core-1890878-authentication-providers-51.patch20.05 KBygerasimov
FAILED: [[SimpleTest]]: [MySQL] 54,212 pass(es), 706 fail(s), and 899 exception(s).
[ View ]
#43 core-1890878-authentication-providers-43.patch20.26 KBygerasimov
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#38 core-1890878-authentication-providers-38.patch16.75 KBygerasimov
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#36 core-1890878-authentication-providers-36.patch16.74 KBygerasimov
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-1890878-authentication-providers-36.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#29 core-1890878-authentication-providers.patch8.4 KBygerasimov
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#25 rest-authentication-1890878-25.patch3.13 KBberenddeboer
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch rest-authentication-1890878-25.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#12 rest-authentication-1890878-12.patch3.44 KBberenddeboer
FAILED: [[SimpleTest]]: [MySQL] 50,445 pass(es), 39 fail(s), and 0 exception(s).
[ View ]

Comments

moshe weitzman’s picture

restws also has code we can borrow here.

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

moshe weitzman’s picture

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

Title:Add support for HTTP Basic Auth» Add support for HTTP Auth (Digest or Basic TBD)
berenddeboer’s picture

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

So that REST part is handled.

berenddeboer’s picture

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

berenddeboer’s picture

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

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

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

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

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

berenddeboer’s picture

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

berenddeboer’s picture

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

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

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

Crell on IRC:

So catch a 403, if the request is one that's HTTP_Digest-able (by some definition) set a 401 response.
Then we have a request listener that checks for an auth attempt, and if it finds one it logs the user in. And that happens pre-routing.

berenddeboer’s picture

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

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

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

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

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

berenddeboer’s picture

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

berenddeboer’s picture

Status:Active» Needs review
StatusFileSize
new3.44 KB
FAILED: [[SimpleTest]]: [MySQL] 50,445 pass(es), 39 fail(s), and 0 exception(s).
[ View ]

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

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

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

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

All suggestions most appreciated!

berenddeboer’s picture

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

berenddeboer’s picture

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

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

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

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

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

Or from the command-line:

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

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

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

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

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

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

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

Status:Needs review» Needs work

The last submitted patch, rest-authentication-1890878-12.patch, failed testing.

Crell’s picture

Does digest mandate md5? Because md5() in Drupal core is verbotten at this point since it runs afoul of government security standards. (Basically the US Govt requires any use of something as insecure as md5 to be justified before they will install software that uses it; Drupal's answer has been to just kill use of md5 entirely.)

+++ b/core/lib/Drupal/Core/EventSubscriber/HttpBasicAuthenticationSubscriber.php
@@ -0,0 +1,54 @@
+      $username = $request->server->get ('PHP_AUTH_USER');
+      $password = $request->server->get ('PHP_AUTH_PW');

No space between get and (.

+++ b/core/lib/Drupal/Core/ExceptionController.php
@@ -120,7 +120,14 @@ public function on403Html(FlattenException $exception, Request $request) {
-      $response = new Response($content, 403);
+      global $user;
+      if (!$user->uid) {
+        $realm = config('system.site')->get('name');
+        header('WWW-Authenticate: Basic realm="'. $realm . '"');
+        $response = new Response($content, 401);
+      }
+      else

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

kim.pepper’s picture

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

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

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

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

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

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

berenddeboer’s picture

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

berenddeboer’s picture

Crell> Does digest mandate md5?

See previous reply.

Crell> No space between get and (.

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

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

Working on that.

berenddeboer’s picture

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

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

Crell’s picture

The route object should be on the request at $request->attributes->get(RouteObjectInterface::ROUTE_OBJECT);

At least I think that's the right spot. It's something like that. :-)

scor’s picture

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

Edit: apparently Facebook uses HMAC (updated link).

moshe weitzman’s picture

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

I have not looked into HMAC.

berenddeboer’s picture

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

berenddeboer’s picture

StatusFileSize
new3.13 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch rest-authentication-1890878-25.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

berenddeboer’s picture

This always returns NULL in the exception handler.

ygerasimov’s picture

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

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

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

Crell’s picture

If I understand you correctly, that's effectively how the Access Check objects work already. They don't use Plugins API, but they don't need to.

ygerasimov’s picture

StatusFileSize
new8.4 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

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

moshe weitzman’s picture

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

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

Thanks for working on this.

klausi’s picture

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

atchijov’s picture

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

bshaffer’s picture

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

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

Crell’s picture

Status:Needs work» Needs review

We're working on switching page caching as we have it now over to proper HTTP header based caching. We absolutely should be caching RESTful GET requests, just like we do HTML requests.

That said, let's not worry about caching here for now. There's 8 other issues dealing with that already. :-)

Moshe: Yes, the idea is that we can then easily add an HttpAuth-based auth provider next to the cookie one, and/or whatever else anyone feels like adding.

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.php
@@ -0,0 +1,40 @@
+  public function addProvider(AuthenticationProviderInterface $provider) {

Needs docblock.

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.php
@@ -0,0 +1,40 @@
+  public function authenticate(Request $request) {

Needs {@inheritdoc}

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.php
@@ -0,0 +1,40 @@
+    $auth_providers = $route->getAttribute('auth');
+    if (empty($auth_providers)) {
+      $auth_providers = array('cookie');
+    }

This can be collapsed to $auth_providers = $route->getAttribute('auth') ?: array('cookie');

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationManagerInterface.php
@@ -0,0 +1,9 @@
+
+interface AuthenticationManagerInterface {
+  public function authenticate(Request $request);

This interface is not needed. It's redundant with the provider interface. The Manager class should use the exact same interface as the providers, which makes swapping things out much easier.

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationProviderInterface.php
@@ -0,0 +1,9 @@
+interface AuthenticationProviderInterface {
+  public function authenticate(Request $request);

Needs docblocks.

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterAuthenticationPass.php
@@ -0,0 +1,28 @@
+    $matcher = $container->getDefinition('authentication');
+    foreach ($container->findTaggedServiceIds('authentication_provider') as $id => $attributes) {
+      $matcher->addMethodCall('addProvider', array(new Reference($id)));

We need a priority marker for these services. See breadcrumbs and path processors for easy to copy examples.

+++ b/core/lib/Drupal/Core/EventSubscriber/AuthenticationSubscriber.php
@@ -0,0 +1,43 @@
+    $authentication_manager = \Drupal::service('authentication');

This should be injected via the container. Drupal:: should never be called from within a class.

Status:Needs review» Needs work

The last submitted patch, core-1890878-authentication-providers.patch, failed testing.

ygerasimov’s picture

Status:Needs work» Needs review
StatusFileSize
new16.74 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-1890878-authentication-providers-36.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

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

Status:Needs review» Needs work

The last submitted patch, core-1890878-authentication-providers-36.patch, failed testing.

ygerasimov’s picture

Status:Needs work» Needs review
StatusFileSize
new16.75 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Reroll of the patch.

juampy’s picture

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

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

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

Status:Needs review» Needs work
Issue tags:-security, -WSCCI

The last submitted patch, core-1890878-authentication-providers-38.patch, failed testing.

ygerasimov’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+security, +WSCCI

The last submitted patch, core-1890878-authentication-providers-38.patch, failed testing.

ygerasimov’s picture

StatusFileSize
new20.26 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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

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

ygerasimov’s picture

Status:Needs work» Needs review
Crell’s picture

I think most of my comments in #34 are still outstanding.

+++ b/core/lib/Drupal/Core/EventSubscriber/AuthenticationSubscriber.php
@@ -0,0 +1,62 @@
+class AuthenticationSubscriber extends ContainerAware implements EventSubscriberInterface {

Don't make this class ContainerAware. Just inject the authentication service via the constructor and be done with it. Also, don't use settings() in here. That's also an injectable service.

Damien Tournoud’s picture

Status:Needs review» Needs work
<?php
+    $username = $request->server->get('PHP_AUTH_USER');
+   
$password = $request->server->get('PHP_AUTH_PW');
?>

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

Damien Tournoud’s picture

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

Crell’s picture

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

Damien Tournoud’s picture

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

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

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

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

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

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

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

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

Damien Tournoud’s picture

Title:Add support for HTTP Auth (Digest or Basic TBD)» Add support for HTTP Basic Auth

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

ygerasimov’s picture

Status:Needs work» Needs review
StatusFileSize
new20.05 KB
FAILED: [[SimpleTest]]: [MySQL] 54,212 pass(es), 706 fail(s), and 899 exception(s).
[ View ]

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

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

kim.pepper’s picture

Status:Needs review» Needs work

Hi ygerasimov,

Looking good.

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

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

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

Needs newline at end of file.

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

Needs newline at end of file.

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

Extra blank lines should be removed.

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

Needs docs.

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

Needs docs.

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

Needs docs.

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

Needs newline at end of file.

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

Needs docs.

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

Needs docs.

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

Needs newline at end of file.

ygerasimov’s picture

Status:Needs work» Needs review
StatusFileSize
new20.6 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-1890878-authentication-providers-53.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Kim, thank you very much for the review.

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

Status:Needs review» Needs work

The last submitted patch, core-1890878-authentication-providers-53.patch, failed testing.

ygerasimov’s picture

Status:Needs work» Needs review
StatusFileSize
new20.63 KB
FAILED: [[SimpleTest]]: [MySQL] 54,481 pass(es), 147 fail(s), and 856 exception(s).
[ View ]

Reroll of the patch.

Status:Needs review» Needs work

The last submitted patch, core-1890878-authentication-providers-54.patch, failed testing.

ygerasimov’s picture

Status:Needs work» Needs review
StatusFileSize
new21.7 KB
FAILED: [[SimpleTest]]: [MySQL] 53,450 pass(es), 118 fail(s), and 542 exception(s).
[ View ]

Rerolling the patch. Checking tests.

Status:Needs review» Needs work

The last submitted patch, core-1890878-authentication-providers-57.patch, failed testing.

berenddeboer’s picture

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

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

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

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

Damien Tournoud’s picture

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

Crell’s picture

HTTP Basic (over SSL) in core, OAuth in contrib. That's the plan, as I understand it.

ygerasimov’s picture

Status:Needs work» Needs review
StatusFileSize
new21.83 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Patch reroll. Trying to fix tests.

Status:Needs review» Needs work

The last submitted patch, core-1890878-authentication-providers-62.patch, failed testing.

ygerasimov’s picture

Status:Needs work» Needs review
StatusFileSize
new22.3 KB
FAILED: [[SimpleTest]]: [MySQL] 57,314 pass(es), 68 fail(s), and 49 exception(s).
[ View ]

Another reroll. Add initializing of session during install.

Status:Needs review» Needs work

The last submitted patch, core-1890878-authentication-providers-64.patch, failed testing.

corvus_ch’s picture

StatusFileSize
new1008 bytes
new22.41 KB
FAILED: [[SimpleTest]]: [MySQL] 57,626 pass(es), 10 fail(s), and 49 exception(s).
[ View ]

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

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

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

corvus_ch’s picture

Status:Needs work» Needs review

Triggering testbot.

Status:Needs review» Needs work

The last submitted patch, core-1890878-authentication-providers-66.patch, failed testing.

Crell’s picture

Unfortunately #1858196: [meta] Leverage Symfony Session components keeps running into problems with simpletest. We've been trying on and off for months. We can't wait for it.

ygerasimov’s picture

Status:Needs work» Needs review
StatusFileSize
new22.5 KB
FAILED: [[SimpleTest]]: [MySQL] 57,745 pass(es), 6 fail(s), and 5 exception(s).
[ View ]

We are getting closer with simpletest! Another round.

corvus_ch’s picture

StatusFileSize
new7.21 KB
new24.43 KB
FAILED: [[SimpleTest]]: [MySQL] 57,751 pass(es), 1 fail(s), and 60 exception(s).
[ View ]

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

corvus_ch’s picture

Oh, sorry. Did not see the previous patch.

corvus_ch’s picture

Assigned:Unassigned» corvus_ch

Claim that for now.

Status:Needs review» Needs work

The last submitted patch, core-1890878-authentication-providers-71.patch, failed testing.

corvus_ch’s picture

Assigned:corvus_ch» Unassigned
Status:Needs work» Needs review
StatusFileSize
new6.83 KB
new25.24 KB
PASSED: [[SimpleTest]]: [MySQL] 58,083 pass(es).
[ View ]

That one should give it the rest ;)

Crell’s picture

This is looking great, guys! Comments below, most of them are nitpicky.

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.php
@@ -0,0 +1,123 @@
+    // Prevent the fallback provider to not be executed before any other
+    // provider.

Bad grammar. I suggest "Ensure the fallback provider always fires last."

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.php
@@ -0,0 +1,123 @@
+    $providers = $this->providers;
+    unset($providers[$this->fallbackProvider]);

That said, I think we could do better. Just unset the fallback provider... Then add it again at the end of the array. That will eliminate the second redundant code block below.

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

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

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.php
@@ -0,0 +1,123 @@
+    // Save id of provider to request so it is checked in
+    // AuthenticationProviderAccessCheck.

Grammar: "Save the ID of the triggered provider to the request so that it can be accessed in AuthenticationProviderAccessCheck."

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationProviderInterface.php
@@ -0,0 +1,41 @@
+   * This method called late on KernelEvents::RESPONSE event.

Technically not relevant here, since one COULD call cleanup at any time. The comment is a tight coupling. :-)

+++ b/core/lib/Drupal/Core/Authentication/Provider/HttpBasic.php
@@ -0,0 +1,37 @@
+        return user_load($uid);

I'm pretty sure we can inject the user entity manager and do a proper injected load here now. No need to call user_load() directly. (I don't know if we can inject user_authenticate yet...)

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterAuthenticationPass.php
@@ -0,0 +1,31 @@
+  public function process(ContainerBuilder $container) {
+    if (!$container->hasDefinition('authentication')) {
+      return;
+    }
+    $matcher = $container->getDefinition('authentication');
+    foreach ($container->findTaggedServiceIds('authentication_provider') as $id => $attributes) {
+      $matcher->addMethodCall('addProvider', array($id, new Reference($id)));
+    }

Do we need a priority marker here? We have it almost everywhere else that we have this pattern.

+++ b/core/modules/system/lib/Drupal/system/Tests/Authentication/HttpBasicTest.php
@@ -0,0 +1,75 @@
+    $this->assertText($account->name, 'Account name should be displayed.');

I think our standard pattern here is "Account name is displayed". Vis, describe the success condition as if it happened.

+++ b/core/modules/system/lib/Drupal/system/Tests/Authentication/HttpBasicTest.php
@@ -0,0 +1,75 @@
+  /**
+   * We do not use drupalGet because we need to set curl settings for basic authentication.
+   *
+   * @param string $username
+   * @param string $password
+   *
+   * @return string
+   *   Curl output.
+   */

First line is longer than 80 chars. The first line should be what this method does, not why we use it instead of something else. What's currently the first line should be a longdesc paragraph after the first lien.

Missing $path param.

The main thing that is missing that I see is how to force authentication with HTTP Basic. I am no expert on it, but my understanding is that in case of a Basic Auth failure an HTTP 401 should be returned so the browser knows to prompt the user for credentials (or the non-browser client application knows that it sent bad credentials): http://httpstatus.es/401

How are we going to trigger that?

corvus_ch’s picture

Assigned:Unassigned» corvus_ch
Status:Needs review» Needs work
corvus_ch’s picture

Status:Needs work» Needs review
StatusFileSize
new9.2 KB
new25.92 KB
FAILED: [[SimpleTest]]: [MySQL] 57,965 pass(es), 8 fail(s), and 0 exception(s).
[ View ]

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

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

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

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

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

Berdir’s picture

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

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

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

Status:Needs review» Needs work

The last submitted patch, core-1890878-authentication-providers-78.patch, failed testing.

corvus_ch’s picture

Status:Needs work» Needs review
StatusFileSize
new778 bytes
new26.15 KB
PASSED: [[SimpleTest]]: [MySQL] 57,874 pass(es).
[ View ]

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

corvus_ch’s picture

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

That was what I meant.

klausi’s picture

Status:Needs review» Needs work

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

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

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

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

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

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

2 white spaces before "Priority".

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

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

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

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

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

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

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

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

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

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

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

You should also assert the HTTP response status codes here.

Crell’s picture

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.php
@@ -52,9 +52,38 @@ public function __construct($fallback_provider = 'authentication.cookie') {
+    uksort($this->providers, array($this, 'compareProviderPriority'));

This seems weird. See the PathProcessors for the boilerplate code we're using in a half-dozen places now. We don't need to sort until we actually use it. Sorting every time a provider is added is needlessly slow.

The cleanup() method on providers is something we added because cookie needs it; I don't think it's that ugly, really. I don't know what other methods would need it, but it seemed better to make it an easy no-op than to try and do something weird and hacky for cookie. (Especially as cookie is what we'll be using 98% of the time anyway.)

klausi’s picture

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

Crell’s picture

Berdir: Actually no, I meant sending a 401 rather than 403. :-) Because if you're using basic auth, AFAIK "you can't access this, what's your password?" is the standard logic. We should try to support that.

Crell’s picture

OK, we discussed this a bit in IRC. Sounds like we're still missing a piece here. Here's the suggested way forward:

* We need to differentiate 401 (Who are you?) from 403 (Oh, you, I hate you). Specifically, that should be done here:

http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/lib/D...

That needs to be tweaked to throw a 403 exception if the user is authenticated, and a 401 exception if the user is not authenticated. (Right now we're just throwing the 403; I'm not sure if Symfony already has an exception for 401. Probably?)

* We then need an exception listener (which could maybe live in that same subscriber?) that only catches the 401 exception and prompts the user for credentials. To do so, it calls a method on each provider that is relevant to that route (just as we're already doing for auth), and lets the provider return an appropriate response. In the case of http basic, that would be a 401 response with the WWW-Authenticate header. For cookie... it probably wouldn't do anything, at least for right now. :-)

* If it's a 403, we already handle that based on mime type so ignore it for now.

So that should be just one more method on the Providers that gets called in the case of "user unknown" and returns an appropriate 401 challenge-response. Not sure what to call it right now, but that's the basic idea.

corvus_ch’s picture

Status:Needs work» Needs review
StatusFileSize
new14.48 KB
new27.74 KB
PASSED: [[SimpleTest]]: [MySQL] 57,827 pass(es).
[ View ]

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

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

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

ygerasimov’s picture

Status:Needs review» Needs work

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

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

corvus_ch’s picture

Status:Needs work» Needs review
StatusFileSize
new2.92 KB
new28.58 KB
FAILED: [[SimpleTest]]: [MySQL] 57,843 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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

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

ygerasimov’s picture

Status:Needs review» Needs work

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

I will work on this patch tomorrow.

ygerasimov’s picture

Assigned:corvus_ch» Unassigned
Status:Needs work» Needs review
StatusFileSize
new7.95 KB
new30.61 KB
FAILED: [[SimpleTest]]: [MySQL] 56,869 pass(es), 0 fail(s), and 194 exception(s).
[ View ]

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

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

Status:Needs review» Needs work

The last submitted patch, core-1890878-authentication-providers-91.patch, failed testing.

corvus_ch’s picture

Status:Needs work» Needs review
StatusFileSize
new6.59 KB
new32.75 KB
FAILED: [[SimpleTest]]: [MySQL] 58,120 pass(es), 1 fail(s), and 198 exception(s).
[ View ]

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

Besides that I am stuck here too.

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

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

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

Status:Needs review» Needs work

The last submitted patch, core-1890878-authentication-providers-94.patch, failed testing.

Berdir’s picture

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

Should be lowercase false/null I think.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Missing namespace.

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

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

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

Missing \

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

Reads strange...

"Cleans up"?

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

Missing namespace.

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

\, Same for all other files.

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

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

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

namespace.

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

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

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

Does.

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

Should be lowercase false/null I think.

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

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

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

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

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

corvus_ch’s picture

StatusFileSize
new19.84 KB
new34.71 KB
FAILED: [[SimpleTest]]: [MySQL] 58,264 pass(es), 49 fail(s), and 200 exception(s).
[ View ]

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

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

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

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

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

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

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

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

That was obvios :p

corvus_ch’s picture

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

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

Status:Needs review» Needs work

The last submitted patch, core-1890878-authentication-providers-97.patch, failed testing.

Crell’s picture

Assigned:Unassigned» Crell

Working on this. A lot of the fails are due to legacy menu items, which don't have a route! (duh) I should have a fix soon but need sleep first.

Crell’s picture

Title:Add support for HTTP Basic Auth» Add modular authentication system, including Http Basic; deprecate global $user
Priority:Major» Critical
StatusFileSize
new26.34 KB
new37.04 KB
FAILED: [[SimpleTest]]: [MySQL] 58,295 pass(es), 65 fail(s), and 352 exception(s).
[ View ]

OK, this took a bit longer than I intended but I think it's worth the wait. :-)

I fixed the legacy route problem and cleaned up a lot of other code. However, that very easily lent itself to some additional cleanup. Most notably, rather than saving the authenticated user to just global $user, we also add it to the request as a "session" attribute. Why "session"? Because it's not a user object. It's a UserSession object. Any similarity between that and a user entity is and always has been coincidental. (Yes, calling the global $user has always been a bug.) That means...

... This patch now deprecates global $user And there was so much rejoicing...

I also moved the "were you authenticated by the RIGHT authenticator" check to its own route enhancer, since it's going to fire all the time anyway and what it actually does is not deny access but change the active user.

The upshot of all this is that we now have true separation between Authentication (Who are you?) and Authorization (Do I let you see this?) I have not renamed the "Access" system to "Authorization" to avoid scope creep, but really that's what it is. And both are pluggable.

This also means that Http Digest or OAuth should be way easier to implement from contrib now, too.

Let's see what the bot thinks. Big bucks no wammy!

Crell’s picture

Category:feature» task

Note: I'm bumping this to critical because allowing alternate authentication mechanisms is a major factor for any sort of decent web service support; also, it's deprecating a global which is a factor for all of the caching stuff going on.

Crell’s picture

Status:Needs work» Needs review

And I am setting it to needs review, because I'm an idiot and forgot do so in either my earlier comments. *sigh*.

Status:Needs review» Needs work

The last submitted patch, 1890878-authentication.patch, failed testing.

corvus_ch’s picture

Assigned:Crell» corvus_ch

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

corvus_ch’s picture

Status:Needs work» Needs review
StatusFileSize
new1.61 KB
new36.7 KB
FAILED: [[SimpleTest]]: [MySQL] 58,087 pass(es), 9 fail(s), and 0 exception(s).
[ View ]

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

Also took care about the warnings.

Status:Needs review» Needs work

The last submitted patch, drupal-1890878-106.patch, failed testing.

corvus_ch’s picture

Assigned:corvus_ch» Unassigned
Status:Needs work» Needs review
StatusFileSize
new4.57 KB
new36.76 KB
FAILED: [[SimpleTest]]: [MySQL] 58,017 pass(es), 6 fail(s), and 0 exception(s).
[ View ]

This should fixes the Basic Auth test.

The interdiff is against #101.

Status:Needs review» Needs work

The last submitted patch, drupal-1890878-108.patch, failed testing.

corvus_ch’s picture

Status:Needs work» Needs review
StatusFileSize
new6.05 KB
new38.25 KB
PASSED: [[SimpleTest]]: [MySQL] 57,984 pass(es).
[ View ]

This should also fix the unit tests.

Interdiff still against #101.

Status:Needs review» Needs work
Issue tags:-security, -WSCCI

The last submitted patch, drupal-1890878-110.patch, failed testing.

corvus_ch’s picture

Status:Needs work» Needs review
Issue tags:+security, +WSCCI

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

Crell’s picture

Thanks, corvus!

+++ b/core/lib/Drupal/Core/Routing/Enhancer/AuthenticationEnhancer.php
@@ -28,7 +28,7 @@ class AuthenticationEnhancer implements RouteEnhancerInterface {
-      $route = $request->attributes->get(RouteObjectInterface::ROUTE_OBJECT);
+      $route = $defaults['_route_object'];

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

Aside from that, I think we're close to done here. Youri, Moshe, anything you want to add and/or RTBC? :-)

ygerasimov’s picture

Status:Needs review» Needs work

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

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

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

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

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

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

One semicolon should be removed.

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

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

corvus_ch’s picture

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

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

corvus_ch’s picture

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

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

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

Two questions here from my side:

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

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

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

Berdir’s picture

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

corvus_ch’s picture

Assigned:Unassigned» corvus_ch
Status:Needs work» Needs review
StatusFileSize
new3.7 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1890878-118.interdiff.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new38.33 KB
PASSED: [[SimpleTest]]: [MySQL] 58,130 pass(es).
[ View ]

Readded provider iteration on error handling.

Status:Needs review» Needs work

The last submitted patch, drupal-1890878-118.interdiff.patch, failed testing.

ygerasimov’s picture

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

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

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

corvus_ch’s picture

Status:Needs work» Needs review
StatusFileSize
new6.64 KB
new38.85 KB
PASSED: [[SimpleTest]]: [MySQL] 58,016 pass(es).
[ View ]

Let's give this a try.

ygerasimov’s picture

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

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

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

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

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

corvus_ch’s picture

Status:Needs work» Needs review
StatusFileSize
new3.7 KB
new40.5 KB
FAILED: [[SimpleTest]]: [MySQL] 58,019 pass(es), 23 fail(s), and 17 exception(s).
[ View ]

Now for legacy stuff only cookie authentication is allowed.

Status:Needs review» Needs work

The last submitted patch, drupal-1890878-123.patch, failed testing.

corvus_ch’s picture

Status:Needs work» Needs review
StatusFileSize
new3.74 KB
new40.54 KB
PASSED: [[SimpleTest]]: [MySQL] 58,847 pass(es).
[ View ]

My bad, wrong condition. Added additional check.

effulgentsia’s picture

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

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

Should we decrement the higher constants accordingly?

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

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

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

s/Add/Adds a/

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

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

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

Code doesn't "feel".

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

Code doesn't "feel".

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

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

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

Could use a more informative short sentence.

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

Handles an exception.

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

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

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

Should we use the ROUTE_OBJECT constant here instead?

Crell’s picture

StatusFileSize
new7.5 KB
new40.67 KB
PASSED: [[SimpleTest]]: [MySQL] 58,272 pass(es).
[ View ]

Thanks, Alex! Consider your nits picked. And no, I don't believe there's anything left to do here unless Yuriy or Corvus have anything more to add.

ygerasimov’s picture

Status:Needs review» Reviewed & tested by the community
Berdir’s picture

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

Not completely happy with this being called session.

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

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

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

corvus_ch’s picture

Not completely happy with this being called session.

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

[…]

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

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

ParisLiakos’s picture

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

corvus_ch’s picture

StatusFileSize
new5.79 KB
new40.54 KB
PASSED: [[SimpleTest]]: [MySQL] 58,151 pass(es).
[ View ]

Session is now renamed to account.

The naming of this was not introduced by this patch.

Well I really thought it was already there. :)

Status:Reviewed & tested by the community» Needs work
Issue tags:-security, -WSCCI

The last submitted patch, drupal-1890878-132.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
Issue tags:+security, +WSCCI

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

corvus_ch’s picture

Status:Needs review» Reviewed & tested by the community

Back to RTBC then.

effulgentsia’s picture

StatusFileSize
new40.67 KB
PASSED: [[SimpleTest]]: [MySQL] 58,571 pass(es).
[ View ]

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

Crell’s picture

I can live with "account", but remember that it is *not* a user object! It's a UserSession object, which is NOT a full User object. We've often used "account" to mean "a User object that is not global $user". Let's just hope that's not even more confusing. :-)

But, I'm fine with the RTBC here.

Status:Reviewed & tested by the community» Needs work
Issue tags:-security, -WSCCI

The last submitted patch, drupal-1890878-136.patch, failed testing.

corvus_ch’s picture

Status:Needs work» Needs review
Issue tags:+security, +WSCCI

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

Crell’s picture

Status:Needs review» Reviewed & tested by the community

Silly HEAD.

pounard’s picture

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

moshe weitzman’s picture

Identity works for me.

juampy’s picture

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

fubhy’s picture

StatusFileSize
new12.32 KB
new41.47 KB
PASSED: [[SimpleTest]]: [MySQL] 57,966 pass(es).
[ View ]

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

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

Fixes some other minor issues too.

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

fubhy’s picture

Status:Reviewed & tested by the community» Needs review
corvus_ch’s picture

Assigned:corvus_ch» Unassigned
Status:Needs review» Reviewed & tested by the community

This certainly has some improvements. Thanks.

Crell’s picture

I completely disagree with #144. Services should not be ContainerAware unless absolutely necessary. For Access we do that because we know that there will be potentially dozens of access checkers on a given site, so lazy loading them is worthwhile. That's not the case here. I cannot envision a site that will have more than 2 providers active, frankly. 3 at the outside. Certainly not dozens. And even then... we iterate through all of them anyway so there would not possibly be a performance benefit. As such, ContainerAware is unnecessary and superfluous.

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

fubhy’s picture

Status:Reviewed & tested by the community» Needs work

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

Okay, agreed...

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

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

Will post a re-roll without ContainerAware asap.

fubhy’s picture

Status:Needs work» Needs review
StatusFileSize
new6.11 KB
new41.65 KB
PASSED: [[SimpleTest]]: [MySQL] 58,018 pass(es).
[ View ]

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

Crell’s picture

Status:Needs review» Reviewed & tested by the community

OK, that's a good improvement. Thanks, fubhy!

Dries’s picture

Status:Reviewed & tested by the community» Needs work

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

corvus_ch’s picture

Assigned:Unassigned» corvus_ch

On it.

corvus_ch’s picture

Assigned:corvus_ch» Unassigned
Status:Needs work» Needs review
StatusFileSize
new41.68 KB
PASSED: [[SimpleTest]]: [MySQL] 56,252 pass(es).
[ View ]

Rerolled to lates HEAD.

jibran’s picture

Status:Needs review» Reviewed & tested by the community

Once again.

alexpott’s picture

Title:Add modular authentication system, including Http Basic; deprecate global $user» Change notice: Add modular authentication system, including Http Basic; deprecate global $user
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

Committed aa9c2cc and pushed to 8.x. Thanks!

rszrama’s picture

StatusFileSize
new407 bytes
PASSED: [[SimpleTest]]: [MySQL] 56,424 pass(es).
[ View ]

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

Crell’s picture

Status:Active» Reviewed & tested by the community

I don't know, but it's here so let's just fix it. :-)

Dries’s picture

Status:Reviewed & tested by the community» Fixed

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

larowlan’s picture

Status:Fixed» Active

Back to active - needs change notice

ygerasimov’s picture

Status:Active» Needs review

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

corvus_ch’s picture

I changed paragraph on the authentication sequence from

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

to

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

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

corvus_ch’s picture

corvus_ch’s picture

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

ygerasimov’s picture

Title:Change notice: Add modular authentication system, including Http Basic; deprecate global $user» Change notice: Deprecate global $user
Status:Needs review» Active

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

Crell’s picture

Title:Change notice: Deprecate global $user» Add modular authentication system, including Http Basic; deprecate global $user
Status:Active» Fixed
Issue tags:-Needs change record

Added: https://drupal.org/node/2032447
Updated: https://drupal.org/node/2017231

This feels so good. :-)

Xano’s picture

jibran’s picture

Issue tags:-Needs change record

Untagging.

Automatically closed -- issue fixed for 2 weeks with no activity.