Problem/Motivation

Despite past efforts to convert the session as well as the user authentication subsystems to OO code, the current status of both is still insufficient for release. This is likely due to both subsystems being worked on in separate efforts. However, for historical reasons sessions are hard-wired to user authentication and vice versa. This issue provides the big picture and a roadmap outlining the steps necessary to separate both subsystems and finalize the API for session and user authentication.

The most prominent sign of the current defects in both components is the presence of the global $user:

git grep -l -i 'global.*user'
Authentication/AuthenticationManager.php
Authentication/Provider/Cookie.php
Routing/Enhancer/AuthenticationEnhancer.php
Session/AccountInterface.php
Session/SessionHandler.php
Session/SessionManager.php
Session/SessionManagerInterface.php
Session/UserSession.php

Step 1: Remove the dependency of AccountProxy on AuthenticationManager

The AccountProxy currently triggers authentication automatically whenever one of the methods of the current_user service is accessed. As a result it has a dependency on the AuthenticationManager which is responsible for checking credentials on an incoming request and based on that setting the current user. This circular dependency is hidden because the AuthenticationManager acts on the global $user.

Thus in order to remove the global $user from AuthenticationManager, it is necessary to first remove automatic authentication from AccountProxy.

Issues which need to be resolved:

This also will fix:

User interface changes: None
API changes: Reverse dependency between AuthenticationManager and AccountProxy.
Expected impact on core and contrib code: Minor

Step 2: Register symfony session components in the DIC and inject the session service into the request object

Can and should be resolved in parallel to Step 1.

In Symfony based applications, the session is an object on the request. However, in Drupal services wishing to control the session currently do so by invoking methods on SessionManager. This is appropriate as a temporary solution. However, in order to maintain interoperability the final API should conform to what Symfony components expect.

NB: Whether the session will still be accessible from the DIC or only from the request in Symfony 3.0 is currently under discussion, (see [Symfony 3.0][RFC] The session must not be a service #10557).

User interface changes: None
API changes: Access and control the session by $request->getSession() (API Addition).
Expected impact on core and contrib code: Minor

Step 3: Remove user authentication related code from the session component (and remove global $user)

Based on the previous steps, user authentication can be separated completely from sessions by assigning the responsibility for loading and setting the current user to the authentication provider.

#2228393: Decouple session from cookie based user authentication

User interface changes: None
API changes: None
Expected impact on core and contrib code: Minor

Step 4: Audit session and user authentication components

Steps 1-3 block:

Comments

znerol’s picture

Issue summary: View changes
znerol’s picture

Issue summary: View changes
znerol’s picture

Issue summary: View changes
catch’s picture

Priority: Major » Critical

At least some of this is critical, would be great to figure out exactly what and divide up the issue summary.

catch’s picture

catch’s picture

Issue tags: +blocker
YesCT’s picture

Issue summary: View changes
xjm’s picture

xjm’s picture

Issue summary: View changes
effulgentsia’s picture

Issue tags: +authentication
almaudoh’s picture

Berdir’s picture

Note: I removed almost all remaining calls to global $user in #2328645: Remove remaining global $user. I believe some of the issues here will make parts of it prettier, and I kept one part (the interaction between SessionHandler and Cookie), but we could already remove all other global $user's, if that helps.

Berdir’s picture

What is left here? Do we really still need this issue?

#2286971: Remove dependency of current_user on request and authentication manager is RTBC, #2283637: Provide test coverage to prove that an AuthenticationProvider can initiate a session is critical on it's own and hopefully a test-only issue after the other one is in. Global user is gone (yay yay yay) and #2286591: [meta] Security audit the Authentication component is also critical on it's own as well.

Edit: OK, #2228393: Decouple session from cookie based user authentication is still left and #2345611: Load user entity in SessionHandler instead of using manual queries but I'm not sure if they really are part of the critical-ness of this meta.

Maybe we can demote this to major and keep it to keep track of the various related issues, but all the parts that are critical already are on their own.

znerol’s picture

There is still quite a bit of problematic interaction between the session subsystem and the current user.

cpj’s picture

I agree with @znerol #18. Now is not the time to demote this to major. Lets first wait and see how some of these related issues turn out and then evaluate what still needs to be done.

xjm’s picture

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

znerol’s picture

Step 1 and 2 are more or less completed. The goal is to be able to start the security audit after devdays2015.

The most sever problem we have right now is that the session subsystem still assumes that it is the only authority when it comes to user authentication. This leads to the following issues:

Both of those issues are resolved by #2228393: Decouple session from cookie based user authentication. A test is being worked on in the second of the above issues. The following issues are likely duplicates: #1277682: Move responsibility for global $user out of session, #2272189: Uncouple session handling from user module,

#2283637: Provide test coverage to prove that an AuthenticationProvider can initiate a session deals with the session not being usable when using authentication providers in addition with cookie auth. This is supposed to be a test-only issue after #2286971: Remove dependency of current_user on request and authentication manager landed.

#2437761: CSRF token seed and possibly other session data lost when set after a session regenerate contains a fix and a test. Should be ready soon.

To be continued...

znerol’s picture

Issue summary: View changes
almaudoh’s picture

#2432585: Improve authentication manager service construction to support custom global service providers would also be good DX for contrib to allow for easy modification of list of default authentication providers.

xjm’s picture

znerol’s picture

webchick’s picture

Tagging, since next steps here are unclear.

pfrenssen’s picture

My understanding was that we have to finish the 5 remaining child issues to bring this to conclusion.

dawehner’s picture

@pfrenssen are any of those particularly critical? There are some, like the moving of AccountInterface, I can't be critical as least for me.
Do you mind adding those which are left onto the issue summary directly?

webchick’s picture

Priority: Critical » Major

Agreed; remaining child issues are:

There's definitely still value in keeping this issue open as a tracking issue for the work required to finalize this API. However, thanks to several peoples' very hard work (yay!) this is no longer release-blocking to Drupal 8. Downgrading as a result.

Obviously, if release-blocking things come up as you're completing the above list, feel free to make additional critical issue for them.

pfrenssen’s picture

Yes indeed all the important stuff is done, now we just need to do the finishing touches.

znerol’s picture

Filed #2488116: Document the session subsystem, we probably should do the same for the authentication subsystem?

joelpittet’s picture

Category: Task » Plan

Added this child to remove the duplicate function of drupal_set_message() which likely still fails.

#2554261: Fix docs on test without injected drupal_set_message() AggregatorPluginSettingsBaseTest

Wim Leers’s picture

Per #2603046-23: Support anonymous users, another thing that is missing in the current APIs, is cacheability metadata.

We want to make sure that not all code needs to be aware of what mechanism is used to handle sessions. I.e. whether that uses a cookie, some kind of header, or something else still.

\Drupal\Core\Session\SessionConfigurationInterface actually is meant to encapsulate that. And it does. But it's unfortunately unaware of cache contexts, because that was overlooked. The default implementation, \Drupal\Core\Session\SessionConfiguration uses cookies.

So, what is missing is:

/**
  * @return string[]
  */
SessionConfigurationInterface::getCacheContexts();

And

SessionConfiguration::getCacheContexts() {
  return ['cookies:' . $this->getName()];
}
Wim Leers’s picture

Issue tags: +D8 cacheability

Because of #35.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

andypost’s picture

Version: 8.2.x-dev » 8.3.x-dev
dawehner’s picture

@Wim Leers
Issue or it simply never happens :)

Wim Leers’s picture

#40: good point!

Created #2826138: Authentication system lacks cacheability metadata as a child issue of this.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.