Provide test coverage to prove that an AuthenticationProvider can initiate a session

This isn't critical see https://drupal.org/core/issue-priority sounds like normal at most major.

@marcingy, I told @shivanshuag to file this as critical because it seems to me this a) needs to be fixed before release b) unfixable in contrib. Let me know.

yeah, like i said elsewhere our "pluggable" authentication is a lie:)

This should be probably be postponed on (or even dupe of) #2272189: Uncouple session handling from user module
In a perfect world we should be able to use session without having user module enabled

@shivanshuag, can you provide minimal steps to reproduce this issue? I don't get it.

Steps to reproduce:

1. Enable rest.module with basic authentication, visit a resource in your browser, log in via basic auth.
2. Now go back to the frontpage and try to log in.

Coming into the issue from https://drupal.org/node/2159937. Yep, exact same set of issues. Exact same (simple) way to replicate.

PS - thanks for creating a sandbox.

a) needs to be fixed before release b) unfixable in contrib.

Step 1) Uninstall basic_auth.module

Okay, then this needs a better issue summary and title, to explain why its still a problem with basic_auth.module uninstalled.

Yes! Much much clearer. Thank you so much @shivanshuag.

The patch in #2286971: Remove dependency of current_user on request and authentication manager removes AuthenticationEnhancer. Do you think that this is going in the right direction?

#17, I think that is a. the right direction and b. will certainly help with this stuff. As the current issue is that auth happens before we have the route information, which that fixes.

People seeking to speed up the issue (either this one or the other), please help reviewing #2265099: Cleanup SessionHttpsTest and fix redirect to non-existing URL after POST requests, #2288665: Remove _maintenance request attribute and replace it with a maintenance mode service and #2288911: Use route name instead of system path in user maintenance mode subscriber. Those patches need to be resolved before we can remove the AuthenticationEnhancer.

#2286971: Remove dependency of current_user on request and authentication manager will remove the troublesome AuthenticationEnhancer. The issues pointed to in the previous comment are either resolved or do not seem to block 2286971 anymore.

Does that mean that once #2286971: Remove dependency of current_user on request and authentication manager is fixed, this issue goes away?

Does that mean that once #2286971: Remove dependency of current_user on request and authentication manager is fixed, this issue goes away?

Yes, looks like, this issue would be like auto-fixed by #2286971: Remove dependency of current_user on request and authentication manager. @shivanshuag reopened this issue in #24 because of the inactivity which is not anymore. Let's postpone this until #2286971: Remove dependency of current_user on request and authentication manager gets in and close?

I guess what we should do at the end here is provide a test which does exactly what is written in the issue summary..

Set the session by something else than the cookie one.

Related: #2432911: Provide test coverage to prove that a third party authentication provider does not automatically start a session. Noticed that while manually testing #2286971: Remove dependency of current_user on request and authentication manager. Test case was authentication manager and cookie authentication completely removed from container and replaced entirely by basic auth.

.

Let's write some test.

Disclaimer: This will fail, because login using the frontpage login form + basic auth headeres together doesn't work.

I adapted the patch with the usecase to allow basic auth to be triggered on every page, by altering the authentication manager.
With that we at least have a pretty well defined set of behaviour, and we proove that generic use of basic_auth on contrib can work.

Does someone has some suggestion about more test coverage?
@shivanshuag It would be great if you could comment about the recent changes we made ...

+++ b/core/modules/system/src/Tests/Session/MultipleAuthenticationSessionTest.php
@@ -0,0 +1,90 @@
+    $filepath = $path . '/services.yml';
+    $data = Yaml::decode(file_get_contents($filepath));
+    $data['services']['authentication'] = [
+      'class' => 'Drupal\Core\Authentication\AuthenticationManager',

I was wondering in the original issue if we should make that argument a parameter, then we could use setContainerParameter() here... fine for now I guess.

The test makes sense, but I'm not quite sure it is really related to this issue.

What we could try is having two test routes, one saves something in the session and the other one displays it, and then access those pages with basic authentication. Or a test auth provider, that is similar to cookie and actually uses the session for authentication.

I really have no idea what happens then, a whole bunch of scenarios are possible?

* Not sure if saving the session will even work, and which user will be referenced...
* It's still the cookie auth provider that starts the session, so if basic_auth comes first, we likely won't ever try to actually load the session on the second request, which means that there will be no session data.
* Assuming that starting a session actually works, then I'm really wondering what happens if you start a session with basic auth and then no longer send the basic auth credentials on a normal page.. I wouldn't be surprised if the cookie authentication provider kicks in and authenticates you?

#2286971: Remove dependency of current_user on request and authentication manager probably helped a bit, but I really doubt that all problems might be solved now. #2432911: Provide test coverage to prove that a third party authentication provider does not automatically start a session is kind of about the problem that I just described, and if I'm not mistaken, then that might be a security issue?

What we could try is having two test routes, one saves something in the session and the other one displays it, and then access those pages with basic authentication. Or a test auth provider, that is similar to cookie and actually uses the session for authentication.

Well, yeah the problem is that the session is opened, but its not really clear how to access that session again, given that you use no cookie.

I think the usecase of a module like https://www.drupal.org/project/securesite is to be able to secure every page, but sort of allow the cookie form behind user/login, so you can actually log in, and then set the cookie and no longer have to worry about it.

Session: Yeah, no idea what will happen right now :) But I think the idea is that session and authentication is decoupled, so that you can start a session no matter what authentication mechanism you are using, which should then also make sure that the cookie is set (which I think happens automatically?)

So, session actually works right now, in fact, it works almost too well.

You just need to go to http://user:pass@d8/node/add/article

And you're logged in, something puts something in the session, you get the cookie set and on the next request, the cookie provider kicks in and authenticates you. Fun.

(Updating certain "Needs D8 critical triage" issues to a less misleading tag name.)

Why #36 includes #2468873-12: Test that the authentication provider doesn't leak authentication credentials from the previous request?

@jibran
The point is that both issues tries to test stuff with basic_auth, so we just add support for using basic_auth properly in drupalGet/drupalPost, which is the reason why both patches have similar content.

Can test from #25 of #2468873: Test that the authentication provider doesn't leak authentication credentials from the previous request be usefull somehow here ?

@Dom: There is a lot of overlap between this issue and #2468873: Test that the authentication provider doesn't leak authentication credentials from the previous request and we may want to focus effort on fixing one of them first (maybe during DDD sprint??)

Was advice by znerol and YesCT to close #2468873: Test that the authentication provider doesn't leak authentication credentials from the previous request as duplicate of another one. But indeed I'm ok to separate and to work at it as part of DDD.

+++ b/core/modules/system/src/Tests/Session/MultipleAuthenticationSessionTest.php
@@ -0,0 +1,90 @@
+    $user = $this->drupalCreateUser();
+    $this->drupalPostForm('<front>', ['name' => $user->getUsername(), 'pass' => $user->pass_raw], t('Log in'), [], [], NULL, NULL, [
+      CURLOPT_HTTPAUTH => CURLAUTH_BASIC,
+      CURLOPT_USERPWD => $user->getUsername() . ':' . $user->pass_raw,
+    ]);
+    // Basic auth is used for the login form, which results in a working
+    // authentication, so the access checking is denied.
+    $this->assertResponse(403);
+
+    // Basic auth doesn't open up a session, so the user is not logged in.
+    $this->drupalGet($user->urlInfo());
+    $this->assertResponse(403);
+
+    // Let's send some basic_auth authentication headers, but that particular
+    // route does not have basic_auth authentications.
+    $this->drupalGet($user->urlInfo(), [], [], [
+      CURLOPT_HTTPAUTH => CURLAUTH_BASIC,
+      CURLOPT_USERPWD => $user->getUsername() . ':' . $user->pass_raw,
+    ]);
+    $this->assertResponse(403);

This entire part is only testing the negative case: getting access denied when you are authenticated using basic authentication rather than cookie based authentication.

If the basic authentication failed then the test result would be identical.

Can we prove that the basic authentication succeeded before testing the 403's?

AuthenticationEnhancer is gone, the issue summary needs an update and the steps to reproduce need to be reevaluated. Because we still have issues with sessions not being properly isolated, the reevaluation should be done with #2228393-70: Decouple session from cookie based user authentication applied.

I already updated the part about AuthenticationEnhancer in the issue summary this morning.

I wonder if this is still critical if it only provides test coverage. Let's wait for #2228393: Decouple session from cookie based user authentication before continuing with this.

Agreed, this specific issue is at most a major task if it will just be adding test coverage for something that is already resolved elsewhere.

I still had a bit of work left in my branch for this issue but this is currently blocked by #2228393: Decouple session from cookie based user authentication. I have added a bit of documentation and a route that can be used to save session values.

There is a very big overlap between this issue and #2468873: Test that the authentication provider doesn't leak authentication credentials from the previous request. This test is based on code from that issue. Both are depending on #2228393: Decouple session from cookie based user authentication.

I will postpone this on #2468873: Test that the authentication provider doesn't leak authentication credentials from the previous request to avoid duplicate work.

We should also look if we can use $this->additionalCurlOptions instead of adding a new parameter to drupalGet() in order to set up basic authentication.

Going to reroll this already to incorporate changes done in #2468873: Test that the authentication provider doesn't leak authentication credentials from the previous request. This is still postponed on that issue.

Removed hacks on WebTestBase. Leveraged test methods and setup from #2468873: Test that the authentication provider doesn't leak authentication credentials from the previous request.

Reopening this now that #2468873: Test that the authentication provider doesn't leak authentication credentials from the previous request is in.

Let's kick this back off with a straight reroll. Not setting to 'Needs review' since it would be pointless, the code in the parent issue changed so much this will just throw fatal errors.

Changed the calls to $this->basicAuthGet() so they match the new signature from BasicAuthTestTrait. Launching test to see how it goes.

Next step is to put basicAuthPostForm() in the trait as well and let it accept the username and password as parameters. To make it universally useful it should have all parameters from drupalPostForm(). We can now also get rid of getBasicAuthHeaders().

+++ b/core/modules/system/src/Tests/Session/SessionAuthenticationTest.php
@@ -78,4 +80,103 @@ public function testSessionFromBasicAuthenticationDoesNotLeak() {
+    $filepath = $path . '/services.yml';
+    $data = Yaml::decode(file_get_contents($filepath));
+    $data['services']['authentication'] = [
+      'class' => 'Drupal\Core\Authentication\AuthenticationManager',
+      'arguments' => [['cookie' => TRUE, 'basic_auth' => TRUE]],
+      'tags' => [
+        [
+          'name' => 'service_collector',
+          'tag' => 'authentication_provider',
+          'call' => 'addProvider'
+        ]
+      ]
+    ];
+    file_put_contents($filepath, Yaml::encode($data));

We removed the constructor of AuthenticationManager in #2432585: Improve authentication manager service construction to support custom global service providers. Generally I think it should not be necessary to mess around with the service definition. It should be enough to simply set/get session properties via a basic_auth enabled route in the session test module.

+++ b/core/modules/system/src/Tests/Session/SessionAuthenticationTest.php
@@ -78,4 +80,103 @@ public function testSessionFromBasicAuthenticationDoesNotLeak() {
+  protected function basicAuthPostForm($path, $edit, $submit, array $options = array(), $form_html_id = NULL, $extra_post = NULL) {

Let's move that to the new trait.

Made a little bit of progress. Commented out the part that rewrites the service definitions so it doesn't throw a fatal error, that part still needs work.

Generally I think it should not be necessary to mess around with the service definition. It should be enough to simply set/get session properties via a basic_auth enabled route in the session test module.

OK so the idea is to stick with basic auth, and set a session variable in one request and then in a subsequent request retrieve the session variable again and assert that they are equal? That sounds like a good test indeed.

Forgot the patch.

Implemented the actual test.

I also kept part of the original test as testLoginWithBasicAuthCredentials() since it was testing some stuff that wasn't tested elsewhere.

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -934,9 +934,7 @@ protected function persistServices(ContainerInterface $container, array $persist
      /**
   -   * Force a container rebuild.
   -   *
   -   * @return \Symfony\Component\DependencyInjection\ContainerInterface
   +   * {@inheritdoc}
       */
      public function rebuildContainer() {
   
   +++ b/core/lib/Drupal/Core/DrupalKernelInterface.php
   @@ -50,6 +50,14 @@ public function discoverServiceProviders();
      /**
   +   * Force a container rebuild.
   +   *
   +   * @return \Symfony\Component\DependencyInjection\ContainerInterface
   +   *   The rebuilt container.
   +   */
   +  public function rebuildContainer();
   

   Now that the call to the rebuildContainer() method has been removed from the test, this change is out of scope. In addition it is arguable whether this method should be part of the official DrupalKernel interface in the first place. DrupalKernel::updateModules() seems preferalbe.

2. +++ b/core/modules/basic_auth/src/Tests/BasicAuthTestTrait.php
   @@ -28,10 +28,55 @@
   +   * This uses the same format as WebTestBase::drupalPostForm(), but uses basic
   +   * authentication.
   

   I do not quite understand this sentence. Remove it or improve it. I think that the @see reference is good enough.

3. +++ b/core/modules/system/src/Tests/Session/SessionAuthenticationTest.php
   @@ -78,4 +94,44 @@ public function testSessionFromBasicAuthenticationDoesNotLeak() {
   +    // Basic auth doesn't set a cookie, so if we do a subsequent request without
   +    // using basic authentication we are no longer logged in.
   +    $this->drupalGet($this->user->urlInfo());
   +    $this->assertResponse(403);
   +
   +    // Let's send some basic_auth authentication headers, but that particular
   +    // route does not have basic_auth authentications.
   +    $this->basicAuthGet($this->user->urlInfo(), $this->username, $this->password);
   +    $this->assertResponse(403);
   

   I feel that this is already covered by #2468873: Test that the authentication provider doesn't leak authentication credentials from the previous request.

   I looks like the the intention of this test is to test that it is not possible to use the login form if basic auth credentials are on the request. If that is the case, then let's limit the scope of this test to exactly that in order to reduce the risk of confusion here.

4. +++ b/core/modules/system/src/Tests/Session/SessionAuthenticationTest.php
   @@ -78,4 +94,44 @@ public function testSessionFromBasicAuthenticationDoesNotLeak() {
   +  /**
   +   * Tests if a session can be initiated through basic authentication.
   +   */
   +  public function testBasicAuthSession() {
   +    // Set a session value on a request through basic auth.
   +    $test_session_value = 'alpaca';
   +    $this->basicAuthGet('session-test/set-session/' . $test_session_value, $this->user->getUsername(), $this->user->pass_raw);
   +
   +    // Test that on a subsequent request the session value is still present.
   +    $result = $this->basicAuthGet('session-test/get-session', $this->username, $this->password);
   +    $data = json_decode($result, TRUE);
   +    $this->assertEqual(['test_value' => $test_session_value], $data['session']);
   +
   +    // Check that we are still logged in as the same user.
   +    $this->assertEqual($this->user->id(), $data['user']);
   +  }
   

   Nice. Add assertResponse() calls for improved robustness. Also perhaps check the return value for the first basicAuthGet().

5. +++ b/core/modules/system/src/Tests/Session/SessionAuthenticationTest.php
   @@ -60,7 +76,7 @@ public function testSessionFromBasicAuthenticationDoesNotLeak() {
   -    $this->basicAuthGet($protected_url, $this->user->getUsername(), $this->user->pass_raw);
   +    $this->basicAuthGet($protected_url, $this->username, $this->password);
   
   @@ -78,4 +94,44 @@ public function testSessionFromBasicAuthenticationDoesNotLeak() {
   +    $this->basicAuthPostForm('<front>', $edit, t('Log in'), $this->username, $this->password);
   ...
   +    $this->basicAuthGet('session-test/set-session/' . $test_session_value, $this->user->getUsername(), $this->user->pass_raw);
   

   Only use one way to pass credentials. In fact I like the simplicity of the old approach (without the username and password instance variables), but the shortness of the new also has some value. The important thing is to stick to one of them.

1. Done.
2. Done. Removed the offending line.
3. I agree, looking at it again 2 of the 3 asserts are indeed variations of tests covered by SessionAuthenticationTest::testSessionFromBasicAuthenticationDoesNotLeak(). Then there only remains the first assert which is actually testing that the user login form returns 403 if the user is already authenticated. This doesn't belong in this class since it doesn't have anything to do with testing sessions.

   So I ended up removing the whole thing to put this issue back in focus :)

4. Done. Also put back the initial returning of the session data and user ID.
5. Done. Reverted to the original to reduce the size of the patch.

Updated issue summary and added beta evaluation.

Thanks. The new test looks good!

Thanks for the quick review!

Committed 4d4635c and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.