Problem/Motivation

Before #2228393: Decouple session from cookie based user authentication went in, it was not possible to use the session if the request was authenticated by a another authentication provider (e.g. basic_auth). This was due to the SessionManager starting a new session with a newly generated session id on every request.

Proposed resolution

Add test coverage which proves that a session value set in one request can be accessed in a subsequent request when authenticated by the basic_auth provider.

Remaining tasks

Review & Commit.

User interface changes

None

API changes

None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task
Issue priority Major because it affects the new session and authentication subsystems
Unfrozen changes Unfrozen because it only adds tests.

Original report

There is no way to set a session for an authenticated user by an AuthenticationProvider provided by another module.

Suppose a module provides an AuthenticationProvider which has higher priority than the Cookie AuthenticationProvider. This AuthenticationProvider will be used to authenticate all the requests but will not be able to set a session. The session set by this AuthenticationProvider will be reset by the AuthenticationEnhancer class.

AuthenticationManager::getProvider() checks for permitted AuthenticationProvider, which is by default the AuthenticationProvider with lowest priority i.e. Cookie (infact default auth provider can never be anything other than cookie - more on this later). You can also set permitted AuthenticationProvider by '_auth' key in routing.yml. For any authentication which is not done by a permitted authentication provider, the session is reset to anonymous by this class. The '_auth' key in the route can be set only for some routes. What if I want to use this authentication provider for all the routes?

If any AuthenticationProvider has a higher priority than Cookie, it won't be the default - the default authentication provider is the one with lowest priority. A module's AuthenticationProvider has to have a priority higher than Cookie (because the cookie authentication provider applies on all the requests and an authentication provider with a lower priority will never be used). So, the default authentication provider can never be anything other than Cookie.

This problem can be seen when the basic_auth module is enabled. If a request has basic_auth credentials in its header and the basic_auth module is enabled, you cannot login using the login form in the front page. If basic_auth is enabled and the request has basic auth credentials, the BasicAuth AuthenticationProvider will be used to authenticate the user since it has higher priority than Cookie, and session will not be set.

Proposed resolution
Allow all AuthenticationProviders to set the session when the '_auth' key is not present.

Comments

shivanshuag’s picture

Component: routing system » request processing system
shivanshuag’s picture

Priority: Normal » Critical
marcingy’s picture

Priority: Critical » Normal

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

chx’s picture

@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.

marcingy’s picture

Priority: Normal » Critical
ParisLiakos’s picture

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

juampynr’s picture

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

Berdir’s picture

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.

shivanshuag’s picture

@juampy To reproduce this, enable this sandbox module I just created - https://drupal.org/sandbox/shivanshuag/2286581 . This will ask for basic auth credentials. Then after you have given some credentials, try logging in through user login form. The session for user will not be set.

The real problem is that there is no way to bypass this problem if a request has basic_auth credentials.

BTMash’s picture

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.

tim.plunkett’s picture

Component: request processing system » basic_auth.module
Priority: Critical » Major

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

Step 1) Uninstall basic_auth.module

shivanshuag’s picture

I think this should stay in request processing system because this is not just the problem with basic_auth module.

A part of the problem is that there is no way for a module to provide its own authentication provider for all the urls and still be able to set a session because the session set by it will be reset by AuthenticationEnhancer class.

tim.plunkett’s picture

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

shivanshuag’s picture

Title: No way to set a session when request has basic auth credentials in its header and basic_auth module is enabled » Session for an authenticated user can only be set by Cookie AuthenticationProvider
Issue summary: View changes

@tim.plunkett is this better?

tim.plunkett’s picture

Priority: Major » Critical
Issue tags: -Needs issue summary update

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

shivanshuag’s picture

Component: basic_auth.module » request processing system

@tim.plunkett I think you forgot to change the component? or was it intentional?

znerol’s picture

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?

damiankloip’s picture

#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.

shivanshuag’s picture

@zenrol, Your patch should fix this issue as it removes the AuthenticationEnhancer class and allows all authentication providers if _auth key is missing from the route.

shivanshuag’s picture

Issue summary: View changes
shivanshuag’s picture

Issue summary: View changes
shivanshuag’s picture

Status: Active » Closed (duplicate)

Dpulicate of #2286971: Remove dependency of current_user on request and authentication manager.

The patch in the issue removes the AuthenticationEnhancer class and allows all the authentication providers if _auth key is not present in the route.

shivanshuag’s picture

shivanshuag’s picture

Status: Closed (duplicate) » Active

I am re-opening this issue because of two reasons - 1) The issue which it is duplicate of has been inactive for some time and this is a critical issue 2) Now that I think of it, this issue should be independent of the other one. The other issue simply fixes this because of some modifications not necessary in it.

znerol’s picture

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.

znerol’s picture

#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.

webchick’s picture

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

webchick’s picture

effulgentsia’s picture

Issue tags: +authentication
vijaycs85’s picture

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?

dawehner’s picture

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.

znerol’s picture

YesCT’s picture

Issue summary: View changes
Issue tags: +Needs tests

.

dawehner’s picture

Status: Active » Needs review
FileSize
7.16 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,600 pass(es), 1 fail(s), and 0 exception(s). View

Let's write some test.

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

Status: Needs review » Needs work

The last submitted patch, 34: 2283637-34.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
10.1 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,057 pass(es). View
4.04 KB

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 ...

Berdir’s picture

+++ 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?

dawehner’s picture

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.

Berdir’s picture

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.

xjm’s picture

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

jibran’s picture

dawehner’s picture

@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.

Dom.’s picture

pfrenssen’s picture

Issue summary: View changes
pfrenssen’s picture

Assigned: Unassigned » pfrenssen
xjm’s picture

Issue tags: +D8 Accelerate Dev Days
almaudoh’s picture

@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??)

Dom.’s picture

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.

pfrenssen’s picture

+++ 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?

znerol’s picture

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.

pfrenssen’s picture

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.

xjm’s picture

Category: Bug report » Task
Priority: Critical » Major
Issue tags: -D8 critical triage deferred

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.

pfrenssen’s picture

Title: Session for an authenticated user can only be set by Cookie AuthenticationProvider » Provide test coverage to prove that an AuthenticationProvider can initiate a session
pfrenssen’s picture

Assigned: pfrenssen » Unassigned
FileSize
11.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,476 pass(es). View
3.69 KB

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.

pfrenssen’s picture

Status: Needs review » Postponed

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.

pfrenssen’s picture

Assigned: Unassigned » pfrenssen

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.

pfrenssen’s picture

FileSize
8.82 KB
9.89 KB
pfrenssen’s picture

pfrenssen’s picture

FileSize
8.27 KB

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.

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
8.42 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,179 pass(es), 1 fail(s), and 0 exception(s). View
1.58 KB

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().

Status: Needs review » Needs work

The last submitted patch, 60: 2283637-60.patch, failed testing.

znerol’s picture

+++ 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.

almaudoh queued 60: 2283637-60.patch for re-testing.

The last submitted patch, 60: 2283637-60.patch, failed testing.

pfrenssen’s picture

Status: Needs work » Needs review

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.

pfrenssen’s picture

FileSize
9.77 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,295 pass(es). View
5.9 KB

Forgot the patch.

pfrenssen’s picture

Issue tags: -Needs tests
FileSize
10.05 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,305 pass(es). View
6.18 KB

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.

znerol’s picture

Status: Needs review » Needs work
  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.

pfrenssen’s picture

FileSize
7.2 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,373 pass(es). View
6.07 KB
  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.
pfrenssen’s picture

Assigned: pfrenssen » Unassigned
Status: Needs work » Needs review
znerol’s picture

Issue summary: View changes

Updated issue summary and added beta evaluation.

znerol’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update

Thanks. The new test looks good!

pfrenssen’s picture

Thanks for the quick review!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 4d4635c on 8.0.x
    Issue #2283637 by pfrenssen, dawehner, shivanshuag, znerol: Provide test...

Status: Fixed » Closed (fixed)

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