Problem/Motivation

A critical security issue was discovered by Dom. in which a basic authentication session that was initialized in a request would persist on subsequent requests, even if they were not using basic authentication any more. See the original report below for the steps that could replicate this issue.

The critical security bug has been fixed in the meanwhile thanks to #2228393: Decouple session from cookie based user authentication. But we still need test coverage to ensure that this issue does not resurface in the future.

A failing test has been posted in comment #60 which included the reverted patch from #2228393: Decouple session from cookie based user authentication to prove that the test correctly detects the security issue. The current test is green to prove that the problem is fixed in HEAD.

Proposed resolution

Test that if a user authenticates using basic authentication and then does a subsequent request _without_ basic authentication the user is no longer authenticated.

Remaining tasks

Review the patch.

User interface changes

None.

API changes

None.

Original report by Dom.

Say you have a drupal 8 website with an admin account and page not accessible by anonymous, for instance /admin/reports.

  • Drupal 8 HEAD
  • Standard profile install
  • Everything to out-of-the-box, thus using Bartik theme and Seven per admin pages.
  1. Activate HTTP Basic Authentication core module (or use you own module with an Authentication Provider)
  2. Disconnect from your site
  3. Visit forbidden page for anonymous using your browser, such as /admin/reports => you should have a 403 forbidden using Bartik theme.
  4. Perform a GET request as anonymous. You can use any poster for this. For my test, I made it with Postman for Chrome, allowing various authentication method. => You willl get "/admin/reports" and get the expected 403 error using Bartik theme
  5. Perform a GET request with basic auth registering as admin. => You willl get "/admin/reports", but as a 403 error using Seven theme, this is not expected for two reasons :
    - change of theme used
    - you provided authentication so you should be logged and see the page
  6. Refresh the page "/admin/reports" on your browser : you are now logged in. => This is wrong :
    - you should not be logged in the browser (although I'm not sure on this point).
  7. Do the GET to page "/admin/reports" again with your poster with the basic authentication : still 403. => but we saw using the browser that we are logged in !
  8. Do the GET to page "/admin/reports" again with your poster without authentication. => now you see the report page, although you did not provide authentication. This is wrong. The session from the browser should not affect what is done using the poster.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because we are adding missing test coverage.
Issue priority Major since this is part of a critical meta issue, and is providing test coverage for a critical security vulnerability.
Unfrozen changes Unfrozen because it only changes test coverage.
Prioritized changes The main goal of this issue is enhancing security by providing test coverage for an access restriction bypass vulnerability.
Disruption Not disruptive because it is adding test coverage for a problem that is already fixed in HEAD.
CommentFileSizeAuthor
#78 interdiff.txt2.13 KBpfrenssen
#78 2468873-78.patch6.51 KBpfrenssen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,890 pass(es). View
#65 2468873-65.patch6.51 KBpfrenssen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,802 pass(es). View
#64 interdiff.txt727 bytespfrenssen
#64 2468873-64.patch26.17 KBpfrenssen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#60 2468873-59-with-2228393-reverted-should-fail.patch26.19 KBpfrenssen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,652 pass(es), 2 fail(s), and 0 exception(s). View
#59 interdiff.txt2.1 KBpfrenssen
#59 2468873-59.patch6.53 KBpfrenssen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,661 pass(es). View
#58 interdiff.txt4.32 KBpfrenssen
#58 2468873-58.patch6.32 KBpfrenssen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#52 interdiff.txt1.1 KBpfrenssen
#52 2468873-52.patch5.8 KBpfrenssen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,409 pass(es). View
#49 interdiff.txt651 bytespfrenssen
#49 2468873-49.patch5.84 KBpfrenssen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,450 pass(es). View
#45 interdiff.txt4.47 KBpfrenssen
#45 2468873-45-with-fix-from-2228393.patch25.33 KBpfrenssen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,501 pass(es). View
#45 2468873-45.patch5.76 KBpfrenssen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,506 pass(es), 2 fail(s), and 0 exception(s). View
#37 interdiff.txt7.34 KBpfrenssen
#37 2468873-37.patch5.08 KBpfrenssen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,447 pass(es), 1 fail(s), and 0 exception(s). View
#29 web-test-base-basic-auth-credentials-leaking.png127.25 KBznerol
#25 authentication-leak.png31.67 KBDom.
#25 authentication-leak-2468873-25.patch3.12 KBDom.
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,432 pass(es), 1 fail(s), and 0 exception(s). View
#17 logoff.png26.89 KBDom.
#17 403-logoff-no_auth.png43.97 KBDom.
#17 403-logoff-basic_auth.png34.1 KBDom.
#17 403_login_no_auth.png39.96 KBDom.
#12 2468873-12.patch5.8 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,373 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

dawehner’s picture

Perform a GET request with basic auth registering as admin => you should get the node, but you get a 403 instead

Well, there seems to be a small misconception of what the module actually does.
Let's read hook_help():

The HTTP Basic Authentication module supplies an HTTP Basic authentication provider for web service requests.

This means, it doesn't enable basic auth for all routes, but really just for the ones using REST module. Technically it is an opt in feature for specific URLs,
rather what you expect for all of them. The 403 behaviour then is right, ... for the HTML route we don't have basic_auth as authentication method enabled.
If you do the same with hal_json accept headers and rest/hal_json enabled, you actually get back the data, as expected, in case you specify those headers.

What you then see is that the somehow a session is saved, started from core/lib/Drupal/Core/StackMiddleware/Session.php:65
After that you have the cookie and you continue on.

xjm’s picture

So I can reproduce steps 1-4, but not step 5. Does "Visit" mean in a browser (i.e. using cookie authentication?)

I agree that step 4 seems like not the expected behavior.

xjm’s picture

@dawehner, what I did is:

  1. Enable the basic_auth and rest modules
  2. Remove anonymous access for view published content.
  3. Log out.
  4. Make the following request with curl:
    curl --request GET --user myuser:mypassword --header 'Accept: application/hal+json' http://localhost:8888/d8git/node/1

I get the 403 in step 4 then. What am I missing?

dawehner’s picture

I can reproduce exactly what is happening.

I agree that step 4 seems like not the expected behavior.

I'm sorry, but it is the expected behaviour. basic_auth is just enabled for a specific subset of routes ... and if you haven't enabled it for HTML (you could do that via code)
its not enabled for that route, which means that you authenticate via a wrong authentication mechanism, which result in a 403.

But yeah, step 5 is IMHO wrong behaviour, \Drupal\Core\EventSubscriber\AuthenticationSubscriber::onKernelRequestFilterProvider
should reset the authenticated user, if authentication failed.

Berdir’s picture

Yeah, fun.

I found the same problem in #2283637: Provide test coverage to prove that an AuthenticationProvider can initiate a session, but I didn't notice that you get logged in even when access is denied, that's weird. Maybe if you already had a session before?

Note that except step 5, everything is working as expected. You are supposed to get an access denied, because those routes don't allow basic_auth. You can change the default providers by changing the service definition or hacking it into AuthenticationManager to test basic auth on normal pages.

#2432911: Provide test coverage to prove that a third party authentication provider does not automatically start a session is also related.

xjm’s picture

So let's update the summary with more detailed steps and explicit steps to reproduce this (including the exact profile to install, modules to enable, etc. and the client used for the requests and exactly how). @dawehner said he reproduced it using the postman Chrome extension to perform the request for step 4 within Chrome. Thanks!

dawehner’s picture

We have to reevaluate that part of the beta evaluation:

Critical because this might impact security

... you could have always logged in manually via the login form anyway.

Dom.’s picture

For n°5, yes, I mean visit in browser.

@dawehner regarding #1 : OK I understand what you explain regarding the expected behavior. It makes sense. However, considering I have made my own authentication provider (as per explained here : https://www.drupal.org/node/2031999)

My purpose is to be able to log a user from having a token in URL : url?HTTP_W3C_VALIDATOR_TOKEN=xxxxxxxxx
Thus I will be able to have w3c validate all my pages (even /admin... as I need to characterize #2467827: [META] W3C validation for Drupal Core but that another story).

So basically I want all pages being authorized by my authentication_provider, what is expected for me according to the description there.
Also I don't want step 5, which is I expect to still be NOT LOGGED in my browser (this is in response to #8 also)

dawehner’s picture

@Dom
So, if you would really want to do that you could do something similar as what is done in https://www.drupal.org/files/issues/2283637-36.patch
... You could write a ServiceProvider class, and set the argument of the authentication_manager explicitly.

  $data['services']['authentication'] = [
+      'class' => 'Drupal\Core\Authentication\AuthenticationManager',
+      'arguments' => [['cookie' => TRUE, 'basic_auth' => TRUE]],
+     

In your case you would remove the default cookie provider and just always use your authentication mechanism.

I'll work on some basic test coverage so we at least define what exactly should happen.

larowlan’s picture

Adding needs triage tag

dawehner’s picture

Status: Active » Needs review
FileSize
5.8 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,373 pass(es). View

Mh, I have a little bit of a trouble to reproduce exactly what I as able to see via the browser.

znerol’s picture

This happens because at step 4 the user is authenticated (via basic auth) and a session is started (because current user is not anonymous) and the user id of the current user is written into the uid attribute of the new session record.

This will be resolved when #2228393-64: Decouple session from cookie based user authentication.1 is implemented. The uid column of the session table should not be populated with the current user id, but with a uid attribute stored in the session. That attribute will only be altered upon login/logout. This will turn the uid column into some sort of secondary index into the session table.

almaudoh’s picture

xjm’s picture

Thanks @znerol and @almaudoh. Do you think we can mark this as a duplicate of #2228393: Decouple session from cookie based user authentication?

Berdir’s picture

Didn't check the other patch yet, but just storing the uid might not be enough, we possibly want to explicitly store the used provider?

Dom.’s picture

Issue summary: View changes
FileSize
39.96 KB
34.1 KB
43.97 KB
26.89 KB

@xjm for #7 : Updated the description for reproduction, adding more "weird things" and screenshots.
@dawehner for #8: Yes probably. I just assumed that requesting a page with a poster (not browser) would not create a session. Please see updated description for this.

According to theses steps, it looks like one of the given issues in comment is related indeed.
- Session should not be started using basic auth : #2432911: Provide test coverage to prove that a third party authentication provider does not automatically start a session
- I hope that giving a new authentication provider would let me authenticate that way to any pages of the site.

  authentication.token_auth:
    class: Drupal\w3c_validator\Authentication\Provider\W3CTokenAuth
    arguments: ['@w3c.token']
    tags:
      - { name: authentication_provider, priority: 200 }

*
I don't know yet if this is related : #2228393: Decouple session from cookie based user authentication

Next is :
- I will try the test given in this issue and see if this can be a validation test.
- I will try the patch given in link issue and see if it affects the behavior in anykind.

Dom.’s picture

Issue summary: View changes
YesCT’s picture

  1. +++ b/core/modules/system/src/Tests/Session/MultipleAuthenticationSessionTest.php
    @@ -0,0 +1,54 @@
    +    $this->drupalGet($node->urlInfo(), [], [], [
    +      CURLOPT_HTTPAUTH => CURLAUTH_BASIC,
    +      CURLOPT_USERPWD => $user->getUsername() . ':' . $user->pass_raw,
    +    ]);
    +
    

    should there be an assert true that the user is logged in?

    should there be an assert that the response included something that would have required permission/access cause they were authenticated ok?

  2. +++ b/core/modules/system/src/Tests/Session/MultipleAuthenticationSessionTest.php
    @@ -0,0 +1,54 @@
    +    $this->drupalGet('session-test/set/test', [], [], [
    

    what is session-test/set/test?

    it is part of the session_test module, but what would we learn differently than the one above?

Dom.’s picture

Issue summary: View changes

@YesCT:
1. I think there should be a test to validate you actually don't receive a 403 error
2. I think we could reproduce the scenario without the use of module session_test but targetting /admin/reports url for instance. I will work on that for a test proposition.

@almaudoh for #14:
I must confess I don't know specifically about #2432911: Provide test coverage to prove that a third party authentication provider does not automatically start a session but #2228393: Decouple session from cookie based user authentication does (see below)

@xjm for #15
#2228393: Decouple session from cookie based user authentication
Manually testing with reproduction scenario, I confirm that the patch #67 actually fixes the point 6 : after refresh of the browser, it does no log user in.

However, regarding other point :
- issue point 5 : Perform a GET request with basic auth registering as admin. => You will 403 error using Seven theme.
- issue point 7 : Do the GET to page "/admin/reports" again with your poster with the basic authentication : still 403.
- issue point 8: Do the GET to page "/admin/reports" again with your poster without authentication.
still exists. But because the session is not propagated to browser (at 6), then it gets better. But still, I expect basic auth not create a session since for me, from one request to another, you should still have to pass your credential. Can someone confirm this point ?

[edit: added point description in case issue summary change]

znerol’s picture

Re #16

Didn't check the other patch yet, but just storing the uid might not be enough, we possibly want to explicitly store the used provider?

I think storing uid is enough. It just means uid>0 = authenticated by cookie provider, otherwise authenticated by another provider or anonymous.

Dom.’s picture

I was going to upload a test-only patch but it #12 is already. I add made a patch reproducing the scenario with admin/reports, but it's wrong for two reasons:
- should include the fix in WebTestBase for curl options as per #12
- should not rely on /admin/reports page in case it changes URL
Thus I will do the same with a node created for the test and remove the "view published content" permission to end up with a self-contained test. Still removing the use of extra session_test module.

@znerol : hum... don't know what to say here, but I think this would replace patch from #2228393: Decouple session from cookie based user authentication and fix 6 ("after refresh of the browser, it does no log user in.") But still using poster the user get's answer 403 + a session is created (as per point 5)

almaudoh’s picture

#15 @xjm: #2228393: Decouple session from cookie based user authentication fixes some of the issues mentioned here, but I think we should at least do a test-only patch to confirm that that issue fixes things.

However, we may have to close #2432911: Provide test coverage to prove that a third party authentication provider does not automatically start a session as a duplicate and fix that issue here, since this issue already has a patch (albeit passing).

xjm’s picture

@almaudoh, the patch in this issue is only tests, which are not failing, so that is two problems. We need to do that the other way around. :) Writing a test that does fail is something we should add to the other issue.

Dom.’s picture

FileSize
3.12 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,432 pass(es), 1 fail(s), and 0 exception(s). View
31.67 KB

Here is a patch failing. On last test, anonymous user should still be unauthorized to access restricted page.

Also confirm that #70 here solves problem 5 on manual testing, but does not validates this test though, still trying to understand why.

Status: Needs review » Needs work

The last submitted patch, 25: authentication-leak-2468873-25.patch, failed testing.

Dom.’s picture

Status: Needs work » Closed (duplicate)

I can confirm patch #70 from #2228393-70: Decouple session from cookie based user authentication solves this (with manual testing) so closing as duplicate because this issue is newer.
Also copying failing test from #25 to that other issue.

xjm’s picture

Excellent work, thanks @Dom.!

znerol’s picture

Status: Closed (duplicate) » Needs work
FileSize
127.25 KB

Looked at what's going on with wireshark and noticed that the Authorization header is still present in the last GET request. This means that basic authentication credentials are leaking from the former request.

web test base leaks basic auth credentials

I reopen this issue such that we can fix the test independently of the ongoing work in the other one. We can merge them as soon as the test is working properly.

xjm’s picture

Thanks @znerol. Based on #29 I think whichever issue we end up with should be critical.

xjm’s picture

Title: Authentication provider does not behave as expected » Authentication provider leaks authentication credentials from the previous request
xjm’s picture

Can we add the test from #25 to #2228393: Decouple session from cookie based user authentication now (including with a test-only patch to expose the fail)? It looks like it does provide coverage for #29, correct?

znerol’s picture

Not yet. The problem is that the current test is still broken. The screenshot from #29 shows requests generated by the test, it does not expose the problem we are fixing in #2228393: Decouple session from cookie based user authentication.

Dom.’s picture

I do not succeed in fixing it right now. The problems comes from 937-944 I guess. Simpletest actually has a configuration by default at "Basic Auth" at "admin/config/development/testing/settings". That is at least the closest I am to understand why now.

// HTTP auth settings (<username>:<password>) for the simpletest browser
// when sending requests to the test site.
$this->httpAuthMethod = (int) $simpletest_config->get('httpauth.method');
$username = $simpletest_config->get('httpauth.username');
$password = $simpletest_config->get('httpauth.password');
if (!empty($username) && !empty($password)) {
  $this->httpAuthCredentials = $username . ':' . $password;
}
pfrenssen’s picture

Assigned: Unassigned » pfrenssen

I'll have a look.

pfrenssen’s picture

There is a very big overlap between this issue and #2283637: Provide test coverage to prove that an AuthenticationProvider can initiate a session.

  • Both are providing test coverage for issues solved in #2228393: Decouple session from cookie based user authentication.
  • Both are trying to bolt on basic authentication to the Curl requests in Simpletest, one by adding an additional parameter to WebTestBase::drupalGet(), and the other by providing its own request method.
  • Both are adding MultipleAuthenticationSessionTest and have borrowed code from eachother.

In order to avoid duplicate work and merge conflicts I will postpone #2283637: Provide test coverage to prove that an AuthenticationProvider can initiate a session on this issue since this is a critical issue.

Looking at how WebTestBase::curlExec() performs its requests it seems like we can use $this->additionalCurlOptions to provide our basic authentication credentials. If this works then this means that we can simplify the approach an don't need to hack drupalGet(), nor need to provide an alternative method to perform requests with basic authentication.

Removing tags about updating the issue summary and adding replication steps since I was able to replicate the issue perfectly by following the provided steps using the Postman extension for Chromium.

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
5.08 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,447 pass(es), 1 fail(s), and 0 exception(s). View
7.34 KB

I rewrote the test:

The test fails correctly on the last step, but the patch from #2228393: Decouple session from cookie based user authentication does not solve the problem. I'm not sure whether the bug is actually still present, or if curl_exec() still somehow reuses the basic authentication even after the option is unset. Does anyone know if you can inspect the request headers during a test run to see if basic authentication is used?

Status: Needs review » Needs work

The last submitted patch, 37: 2468873-37.patch, failed testing.

dawehner’s picture

+++ b/core/modules/system/src/Tests/Session/MultipleAuthenticationSessionTest.php
@@ -0,0 +1,100 @@
+    $this->drupalGet($url);
+    $this->assertResponse(401, 'A subsequent request to the same route without basic authentication is not authorized.');

It feels a little bit weak to just test the http response code but not care about the result body at all.

pfrenssen’s picture

Just sat with @znerol to discuss the next steps to take. He analyzed the HTTP requests and responses that are done during the test using tcpdump and wireshark. The requests are now all good, it works well to control the basic authentication with additionalCurlOptions. Tests were done with the patch from #2228393: Decouple session from cookie based user authentication.

  • The first response is fine. There is no cookie set. The HTML does contain some unexpected output: it shows an UnauthorizedHttpException instead of a human readable error message. This is out of scope for this issue.
  • In the second response a cookie is set. This should not happen if we were authenticated using basic authentication. This needs to be addressed in #2228393: Decouple session from cookie based user authentication.
  • The third response unexpectedly returns a 403 instead of a 401. This is probably because the cookie that was set during the second response is now present.

Next steps to take:

  1. Create a followup for the exception shown on a 401 page.
  2. Provide a route without any authentication that shows the contents of the current session and the UID so that we can really verify that there is no session and the user is not logged in.
  3. Fix the bug that sets the cookie in #2228393: Decouple session from cookie based user authentication.

@dawehner, do you think it is really useful to inspect the HTTP response body of a request that returns a 401? Do you want to see that the correct error message is visible?

Dom.’s picture

@pfrenssen, @dawehner regarding last point #40 :

do you think it is really useful to inspect the HTTP response body of a request that returns a 401

I would +1 this, because @zneroll can confirm we had seen error 401 (or 403 I'm not sure) replied while the page was actually visible because replied by cache.

Dom.’s picture

I was just working on #41 about patch #37 but then realized : the page tested is actually only authorized throught basic_auth.
Maybe stupid, but that seems just as one problem to me. Should not we test also what initial issue was with page accessible both with cookie and basic_auth, so that the session does leak from basic_auth to cookie ?

pfrenssen’s picture

Created followup for the UnauthorizedHttpException: #2472371: Exception shown on 401 Unauthorized.

znerol’s picture

@pfrenssen: How I do tcpdump:

sudo tcpdump -s0 -i lo -w /tmp/auth-leak.cap

Then you can open just open the .cap file in wireshark.

pfrenssen’s picture

Assigned: pfrenssen » Unassigned
Status: Needs work » Needs review
FileSize
5.76 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,506 pass(es), 2 fail(s), and 0 exception(s). View
25.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,501 pass(es). View
4.47 KB

After an epic struggle with trying to disable basic authentication in Curl, I finally tackled this thanks to the great help from @znerol. I've also attached a patch that includes the fix from #2228393: Decouple session from cookie based user authentication to see if it actually detects the bug correctly.

The last submitted patch, 45: 2468873-45.patch, failed testing.

znerol’s picture

@pfrenssen: Fantastic! Do you think we should merge the test into #2228393: Decouple session from cookie based user authentication or should we first add the use-case from #2283637: Provide test coverage to prove that an AuthenticationProvider can initiate a session?

pfrenssen’s picture

Status: Needs review » Postponed
Parent issue: » #2228393: Decouple session from cookie based user authentication

These 3 issues are already very interrelated, I would not make it even more confusing now by merging the tests. Everything hinges now on #2228393: Decouple session from cookie based user authentication, so we should focus on that first. Once that is in the two other issues can be picked up again.

Postponing on #2228393: Decouple session from cookie based user authentication.

pfrenssen’s picture

FileSize
5.84 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,450 pass(es). View
651 bytes

Forgot a line of documentation.

almaudoh’s picture

Status: Postponed » Needs review
pfrenssen’s picture

Title: Authentication provider leaks authentication credentials from the previous request » Test that the authentication provider doesn't leak authentication credentials from the previous request

Updating issue title to indicate that this is only providing test coverage.

pfrenssen’s picture

FileSize
5.8 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,409 pass(es). View
1.1 KB

Renamed the test, this was the original name of the test class of #2283637: Provide test coverage to prove that an AuthenticationProvider can initiate a session but this is not actually using multiple authentication methods.

xjm’s picture

Category: Bug report » Task
Priority: Critical » Major

Discussed with @pfrenssen. Now that the actual bug is fixed (and we have demonstration of the coverage in #45, we can downgrade this to a major task.

Thanks a ton @pfrenssen for all the work clarifying this!

pfrenssen’s picture

Juggling the issue links so that it's clear this is part of the session / authentication meta.

znerol’s picture

  1. +++ b/core/modules/system/src/Tests/Session/SessionAuthenticationTest.php
    @@ -0,0 +1,101 @@
    +    $this->user = $this->drupalCreateUser(array('administer site configuration'));
    

    Use [] syntax for arrays like throughout the rest of the class.

  2. +++ b/core/modules/system/src/Tests/Session/SessionAuthenticationTest.php
    @@ -0,0 +1,101 @@
    +   *
    +   * @see https://www.drupal.org/node/2468873
    

    We normally do not do that but instead rely on the fact that the commit message includes the issue number. We expect that people know how to use git blame and git log.

  3. +++ b/core/modules/system/src/Tests/Session/SessionAuthenticationTest.php
    @@ -0,0 +1,101 @@
    +  protected function basicAuthGet($path, array $options = array()) {
    

    This method might be useful in other tests as well. What about moving it to WebTestBase. Then pass username and password by parameters and add the rest of (optional) params for drupalGet. Might as well be a follow-up.

  4. +++ b/core/modules/system/src/Tests/Session/SessionAuthenticationTest.php
    @@ -0,0 +1,101 @@
    +      'Accept: */*',
    

    The Accept header should not be necessary here.

  5. +++ b/core/modules/system/tests/modules/session_test/session_test.routing.yml
    @@ -89,3 +89,21 @@ session_test.trace_handler:
    +session_test.get_session:
    +  path: '/session-test/get-session'
    

    In order to have parity with the other no-auth route used in the same test, why not add basic-auth to the route name / path.

znerol’s picture

What about moving it to WebTestBase. Then pass username and password by parameters and add the rest of (optional) params for drupalGet. Might as well be a follow-up.

Or perhaps move it into a trait in the basic_auth module, that might actually be a better idea.

pfrenssen’s picture

Assigned: Unassigned » pfrenssen
Status: Needs review » Needs work

Thanks for the excellent review! Will update the patch.

pfrenssen’s picture

Assigned: pfrenssen » Unassigned
Status: Needs work » Needs review
FileSize
6.32 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
4.32 KB

Addressed the remarks.

pfrenssen’s picture

FileSize
6.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,661 pass(es). View
2.1 KB

Was a bit too hasty, the trait method was still referencing the protected properties of the test class, made them into arguments as was suggested by @znerol. This should be much more useful.

pfrenssen’s picture

FileSize
26.19 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,652 pass(es), 2 fail(s), and 0 exception(s). View

This reverts #2228393: Decouple session from cookie based user authentication and includes the test from #59 as proof that the test works. This should fail.

Status: Needs review » Needs work

The last submitted patch, 60: 2468873-59-with-2228393-reverted-should-fail.patch, failed testing.

pfrenssen’s picture

Status: Needs work » Needs review

Back to needs review. The patch to review is the one from #59.

pfrenssen’s picture

pfrenssen’s picture

FileSize
26.17 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
727 bytes

Small cleanup.

pfrenssen’s picture

FileSize
6.51 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,802 pass(es). View

Woops! Still had the reverted patch from #2228393: Decouple session from cookie based user authentication in my branch. This should be clean & green.

The last submitted patch, 64: 2468873-64.patch, failed testing.

The last submitted patch, 58: 2468873-58.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 65: 2468873-65.patch, failed testing.

Status: Needs work » Needs review

pfrenssen queued 65: 2468873-65.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 65: 2468873-65.patch, failed testing.

Status: Needs work » Needs review

pfrenssen queued 65: 2468873-65.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 65: 2468873-65.patch, failed testing.

znerol’s picture

Status: Needs work » Needs review

znerol queued 65: 2468873-65.patch for re-testing.

znerol’s picture

+++ b/core/modules/basic_auth/src/Tests/BasicAuthTestTrait.php
@@ -0,0 +1,37 @@
+  protected function basicAuthGet($path, array $options = array(), $username, $password) {

Mandatory arguments cannot really go after optional ones. I.e. that renders the optional one mandatory also. I propose the following method signature: protected function basicAuthGet($path, $username, $password, array $options = [], array $headers = []);

The test case itself is spot-on, very well done.

pfrenssen’s picture

Thanks for linking that issue, I was hesitant to pick this back up because of these weird failures.

pfrenssen’s picture

Status: Needs review » Needs work

Setting to NW as per #75.

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
6.51 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,890 pass(es). View
2.13 KB

Fixed the ordering of the arguments as suggested in #75.

Status: Needs review » Needs work

The last submitted patch, 78: 2468873-78.patch, failed testing.

pfrenssen queued 78: 2468873-78.patch for re-testing.

pfrenssen’s picture

Status: Needs work » Needs review
znerol’s picture

The issue summary should point out that the bug was fixed in #2228393: Decouple session from cookie based user authentication and that this issue now just adds the necessary regression test. Also the beta-evaluation should be adapted accordingly.

pfrenssen’s picture

Updated issue summary and beta evaluation.

znerol’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 78: 2468873-78.patch, failed testing.

znerol queued 78: 2468873-78.patch for re-testing.

znerol’s picture

Status: Needs work » Reviewed & tested by the community
xjm’s picture

Waiting on a test run right now, but meanwhile, just a note regarding:

Use [] syntax for arrays like throughout the rest of the class.

I too prefer the [] syntax, and it's fine to use it in new code, but while #2135291: [Policy, no patch] PHP 5.4 short array syntax coding standards for Drupal 8 is unresolved, either is acceptable, and we should not push back on contributors using one vs. the other in new lines of code. (Also, as a reminder, patches should not change existing array() to [] as it makes them harder to review.)

Just wanted to clarify that; the patch in this issue is fine coding standards-wise.

  • xjm committed a4fac14 on 8.0.x
    Issue #2468873 by pfrenssen, Dom., dawehner, znerol, xjm: Test that the...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

So, the test looks great. I confirmed that it is providing the expected coverage as well. Thanks for the beta evaluation and clear summary.

[mandelbrot:d8git | Tue 08:30:43] $ git checkout 0fa529e
HEAD is now at 0fa529e... Issue #2228393 by almaudoh, andypost, pfrenssen, znerol, cpj, Dom.: Decouple session from cookie based user authentication
[mandelbrot:d8git | Tue 08:30:46] $ curl https://www.drupal.org/files/issues/2468873-78.patch | git apply --index -
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  6669  100  6669    0     0  30116      0 --:--:-- --:--:-- --:--:-- 47978
[mandelbrot:d8git | Tue 08:30:55] $ drupal-install-test 'Drupal\system\Tests\Session\SessionAuthenticationTest'
Do you really want to drop all tables in the database d8? (y/n): y
You are about to create a /Applications/MAMP/htdocs/d8git/sites/default/settings.php file and DROP all tables in your 'd8' database. Do you want to continue? (y/n): y
Starting Drupal installation. This takes a while. Consider using the [ok]
--notify global option.
Installation complete.  User name: admin  User password: 9727SjqZc3  [ok]
Congratulations, you installed Drupal!                               [status]
The following extensions will be enabled: simpletest
Do you really want to continue? (y/n): y
simpletest was enabled successfully.                                 [ok]
simpletest defines the following permissions: administer unit tests

Drupal test run
---------------

Tests to be run:
  - Drupal\system\Tests\Session\SessionAuthenticationTest

Test run started:
  Tuesday, April 28, 2015 - 15:31

Test summary
------------

Drupal\system\Tests\Session\SessionAuthenticationTest         16 passes                            4 messages

Test run duration: 10 sec

And then:

[mandelbrot:d8git | Tue 08:31:19] $ git checkout 0fa529e^
A	core/modules/basic_auth/src/Tests/BasicAuthTestTrait.php
A	core/modules/system/src/Tests/Session/SessionAuthenticationTest.php
M	core/modules/system/tests/modules/session_test/session_test.routing.yml
M	core/modules/system/tests/modules/session_test/src/Controller/SessionTestController.php
Previous HEAD position was 0fa529e... Issue #2228393 by almaudoh, andypost, pfrenssen, znerol, cpj, Dom.: Decouple session from cookie based user authentication
HEAD is now at cbc0316... Issue #2281989 by stefan.r: Add a fast and simple way to get module name from the module handler
[mandelbrot:d8git | Tue 08:32:27] $ drupal-install-test 'Drupal\system\Tests\Session\SessionAuthenticationTest'
Do you really want to drop all tables in the database d8? (y/n): y
You are about to create a /Applications/MAMP/htdocs/d8git/sites/default/settings.php file and DROP all tables in your 'd8' database. Do you want to continue? (y/n): y
Starting Drupal installation. This takes a while. Consider using the [ok]
--notify global option.
Installation complete.  User name: admin  User password: 2nKKuXbWbK  [ok]
Congratulations, you installed Drupal!                               [status]
The following extensions will be enabled: simpletest
Do you really want to continue? (y/n): y
simpletest was enabled successfully.                                 [ok]
simpletest defines the following permissions: administer unit tests

Drupal test run
---------------

Tests to be run:
  - Drupal\system\Tests\Session\SessionAuthenticationTest

Test run started:
  Tuesday, April 28, 2015 - 15:32

Test summary
------------

Drupal\system\Tests\Session\SessionAuthenticationTest         14 passes   2 fails                  4 messages

A note regarding the test failures earlier on the issue. When a patch fails and we retest it, we should document what the fail was on the issue. Sometimes it's testbot issues, but other times it's the test introducing a random failure. And the two APC patches were reverted, but we still saw more fails. I didn't see any of the usual suspects in this test though, so going to go ahead and commit it.

This issue only improves test coverage for what was a critical bug, and is a followup for a critical issue, so per https://www.drupal.org/core/beta-changes, this can be completed any time during the Drupal 8 beta phase. Committed and pushed to 8.0.x

znerol’s picture

Status: Fixed » Closed (fixed)

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