Deprecate user_logout() and user_login_finalize() and replace with a service

It's already on the UserController, so I think we're done here ?

We need a service or method on a service that explicitly logs out an active user. It should be called FROM a controller, but not be on a controller. Controllers should not have meaningful business logic in them.

I really wonder whether it is worth to make an interface here and I can't get some sleep.

Mark will come up with a better name anyway if needed.

Ups.

Which include file are we killing here?

Yeah, I agree. This is just methods on user.module

The issue summary must be updated too. It is not completely clear what should be done and why.

It looks nice but naming confusing me, as just landed #2536052: Remove try...catch from SessionHandler::write with another session handler

Maybe user module should just decorate \Drupal\Core\Session\SessionHandler?
it's the real storage service which is mussing in related issue

All threads appear to be resolved and tests all green.

This was tagged for CR updates so moving to NW for that.

Good job everyone!

Updated CR

Updated CR reads well.

Thanks @kim.pepper!

Added few comments to the MR. Tagging for framework manager review to get a review on the new API.

Thanks for the reviews. Addressed all feedback.

With 10.2 approaching going to remark this for committers/framework managers to take a look.

#32 was never really addressed - the name UserSessionHandler implies something that extends \SessionHandlerInterface like \Drupal\Core\Session\SessionHandler but this is not that.

Also by introducing \Drupal\user\UserSessionHandlerInterface and the service we are creating a new swappable API which feels like something we don't actually want to do. Are there use-cases?

We also need to update the deprecations for 10.3.

Removed UserSessionHandlerInterface and updated deprecation notices to 10.3.0.

Not sure what a better name is? Maybe UserLoginHandler?

Back to NR for feedback on that suggestion.

OTOH there's SessionManagerInterface which can be extended with finalizeLogin() and finalizeLogout() methods

OTOH there's SessionManagerInterface which can be extended with finalizeLogin() and finalizeLogout() methods

I'd prefer if SessionManagerInterface will go away in the long run. All of the remaining code in SessionManager is supposed to be extracted to session handler decorators or even to upstream.

How about UserSessionFinalizer with finalizeLogin() and finalizeLogout() ?

extracted to session handler decorators

it sounds good idea, needs to check which services can get decorators to react of login/logout

Looks great.
Couple of places where we're missing docs (can't use @inheritdoc if there is no parent interface).

#45 sounds like a good idea.

This feels like a case where we would want to make the service final to make it a bit harder to swap out.

Looks to have failing tests.

Appears all feedback has been addressed.

Since this has gone through several rounds of reviews I'm going to remark it vs the new #needs-review-queue-initiative strategy of waiting a week.

I think #51 is premature. #45 suggests a name for the class and there's general agreement but that's not what is the MR.

Also the service class is not final as suggested by #49

Renamed to UserSessionFinalizer and made the service class final.

Getting this error in the test failure which I think is unrelated:

1) Drupal\Tests\standard\FunctionalJavascript\StandardJavascriptTest::testBigPipe
TypeError: trim(): Argument #1 ($string) must be of type string, bool given

Looks like it was a random fail.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Rebased on 11.x

Thank you! It looks ready!

btw as the new service is public it makes it an extension point, literally hooks user_(ligin|logout) can be implemented as the service decorators?

Appears feedback has been addressed.

@andypost let me know if I'm wrong but sounds like in #57 you're asking a general question correct?

The issue summary and change record no longer match the MR.

Updated CR and IS. As I just updated the service and method names, and there are no code changes, I think it's safe to re-RTBC.

I'm triaging RTBC issues. I read the IS and the comments. I didn't find any unanswered questions.

I read the MR and made some comments. One of which is questioning the name of the service so I am setting to NR to settle that.

I chatted with @kim.pepper who wasn't not opposed to changing UserSessionFinalizer to UserSessionFinalize. Therefore, I have made a commit for that. And also updated a comment.

I did not update the change record. Happy to do that if folks agree to the name change.

I think Finalize makes sense too. I updated the CR and issue summary to reflect that.

There are some things I'm not sure about with this one but not sure I can articulate all of them. Left some review comments on the MR for now.

@catch asked for further opinions on this. My first thought was that really we are just moving code around; procedural code is now in a service that isn't meant to be swapped (because it's declared final), so what do we gain here?

Also, a new service with seven very different dependencies is a bit of a code smell. Should login and logout be events instead? Then each subsystem can declare event subscribers and react as they see fit, and each part would be replaceable individually if someone wanted to switch something out?

Should login and logout be events instead? Then each subsystem can declare event subscribers and react as they see fit, and each part would be replaceable individually if someone wanted to switch something out?

I was thinking the same.

> @catch asked for further opinions on this. My first thought was that really we are just moving code around; procedural code is now in a service that isn't meant to be swapped (because it's declared final), so what do we gain here?

See #1905334: Only load all modules when a hook gets invoked. What we gain is that we move one of the few remaining .module functions called from controllers and other OOP code, allowing us to load .module files only when a hook is invoked.

> Should login and logout be events instead? Then each subsystem can declare event subscribers and react as they see fit, and each part would be replaceable individually if someone wanted to switch something out?

We already have login/logout hooks. This is the API responsible for calling these hooks. I think we shouldn't have events *and* hooks for the same thing, so is the proposal to replace the existing hooks with an event and the sole API for finalizing login/logout would be to invoke the respective event?

That does sound sensible, we do have some existing examples for that, for example around file name sanitization. But that would also make this a considerably larger issue as we'd need to convert existing hook_user_login/logout implementations, deprecate the hook and also think about the API design of these hooks. One of the major use cases for hook_user_login() is to control where the user is redirected to afterwards and that's only possible with workarounds atm which tend to break other implementations of that hook.

FWIW, basically everything the service does is a user module thing, I don't think we could really split that up much apart from separating login and logout.

> Given we log separately here, do we need the logging to be done in the finalize service itself? I think we could take that out of the service and callers are responsible for logging if they want to.

I can see an argument for both options. Ensuring that each login is properly logged seems important from a security perspective, so it seems sensible to do it in the API. But separating would avoid duplicate logs and logs could be more specific.

So this is something that should still move forward?

I think we shouldn't have events *and* hooks for the same thing, so is the proposal to replace the existing hooks with an event and the sole API for finalizing login/logout would be to invoke the respective event?

I think what @longwave meant here was that the methods added here would be a wrapper around the even dispatcher, and the implementation would move into the event - e.g. the logging could then be its own event listener which a site could remove if they wanted to. I haven't thought through the impact of this myself yet.

Speaking as a voice for contrib that interfaces with authentication.

My reading is that user_login_finalize() is currently public API and that it is intended to be called whenever a module wishes to indicate a user has logged-in and user_logout() is intended to be called whenever we wish to logout a user no matter how they were logged into the system.

Both of these global functions currently perform double duty, where they trigger actions for contrib to act upon and they setup/tear down User module internal information (setting the user in the session).

I recently mistakenly thought user_login_finalize() was only used for session based auth and that all others would provide their own authentication provider event listener, however in TFA we recently encountered a bug (due to our increasing security) that forced me to realize the user_login_finalize() method is called by modules that perform external authentication, ref #3426189: 2.x-dev incompatible with other modules calling user_login_finalize().

This raises the question what is the future vision for this new service? Is it intended to only be used by the user module and the rest of us should invoking the hooks ourselves or is it intended to still be called by 3rd party contrib to 'log in' a user?

If its intended to be used by the user module only than I would suggest the deprecation should be changed to indicate these methods have been deprecated with no direct replacment, the user should instead call the necessary hooks/events themselves. The service may not even be necessary if the actions can be done in-line.

If its intended to be used by contrib than I believe we need to either put this on an interface or at a minimal make the class non-final to allow it to be mocked in unit tests. If the class stays final with no interface a helper trait (that provisions it in a dummy mode) for unit tests would be helpful to avoid boilerplate code in contrib. Addtionaly it would likely be helpful if the User module specific items (setting up a session with the UID set) are moved outside of the service and instead done where elsewhere.

From a TFA maintainer standpoint:
Having this as a service that could be decorated would allow a long term cleaner fix to #3426189: 2.x-dev incompatible with other modules calling user_login_finalize() and allow us to be involved in more locations to provide enhanced security.
Having this as internal we could likely provide our own authentication provider and cleanup some of our code. It would probably make intercepting the login form more complicated however I doubt it would be a blocker.

In either case I do agree that user_login_finalize() and user_logout() should be be removed from procedural code.

Tagging as related to #2901795: Facilitate 2FA+MultiFactor compatibility (2FA/two-factor -> MFA/multi-factor) as user_login_finalize() and replacements impact the MFA ecosystem.

Glad to see there's an issue for this. I was going to open an issue if there wasn't one already.

Only moving to NW for the open threads from @catch on the MR.

#144538: User logout is vulnerable to CSRF was committed, which added a new UserLogoutConfirm form calling user_logout().

The MR will need to be rebased and then the form will need to be updated to use the new service.

Also wondering why login and logout are on the same service - they have quite different dependencies, e.g. loggout out doesn't need the entity storage.

I think I would prefer to see two distinct services.

Anybody up for a reroll of this one? I might have some ideas on how to continue here. I do not intend to touch the MR just yet, maybe I can keep my role as a reviewer a little bit longer here.

I had a go at rebasing on 11.x which was what it was originally back in March 2024.

Changes are first done in the main branch; the merge request should be for the main branch, not the 11.x one.

Moving the code 1:1 is fine by me, but @catch raises some valid points regarding dependencies. We should split up the code in 2 services so we don't load dependencies on login or logout that we don't need.

I don't think we need to wrap the logger dependency in a closure per se, but rather the whole service wherever it's injected. Then we can keep the logging for now to keep this a 1:1 conversion and decide whether we want to make callers responsible at a later time.

These functions are reminders of the time when the user module was the only source of user accounts, the login form was the only way to login into a Drupal site and TFA was only used by a small circle of bankers and top secret government agencies.

The world has changed since then, and Drupal core needs to adapt. Not in this MR, but let's prepare for it.

The responsibility of the service introduced in this issue is to safely change the privilege level of a user agent after a credential check. In most cases the credential check is a login form, sometimes a password reset. The web installer just grants the current user agent access to user 1. Outside core it could also be social login via OIDC or some other kind of SSO maybe combined with TFA, etc.

Regardless of how the credential check was performed, the important thing is to renew the session id and tell the authentication provider that a user account is associated with this session.

Subsequent requests are checked by the Cookie authentication provider until the user agent is logged out.

I'd like this issue to prepare for two things in the future, both have minimal influence for this MR:

• I wish there was some metadata available along with the session about the login mechanism. This is handy to restrict unsafe authentication to some limited tasks. Or to provide step-up authentication for administrative actions.
• I wish that other authentication providers than the cookie provider could be associated with a certain session.

Both of those points could be achieved with the following function (rough sectch):

public function login(AccountInterface $account, string $loginMechanism, $loginData = NULL): void {
  $this->accountProxy->setAccount($account);

  // Regenerate the session ID to prevent against session fixation attacks.
  $session = $this->requestStack->getSession();
  $session->migrate();

  // Loop through session aware authentication providers. The first one which
  // claims this login mechanism associates the user with the session. The 
  // Cookie provider would just do this:
  //
  //  $session->set('uid', $user->id());
  //  $session->set('check_logged_in', TRUE);
  //
  // An OIDC redirect controller will probably first provision the user if
  // necessary, then call login($account, 'oidc.redirect', $idpResponse) to
  // login the user. The OIDC authentication provider can look at the login
  // mechanism and associate the id token, refresh token and access token with
  // the session.
  $this->associateAuthenticationProvider($account, $session, $loginMechanism, $loginData);

  // @todo: Call a hook or fire an event here. The user module will use this
  // opportunity to update the last login timestamp.

  // Finally log the event.
  $this->logger->info('Session opened for %name via %mechanism.', [
    '%name' => $account->getAccountName(),
    '%mechanism' => $loginMechanism,
  ]);
}

For the first iteration, I just like to suggest to add the $loginMechanism parameter. It is useful on its own even if it only appears in the log message for now. The $loginData parameter could be added later on - its optional anyway.

Also I'd suggest to place all of the code into one class in the \Drupal\Core\Session namespace. Accepting the smell for now that updating the timestamp on the user entity isn't the responsibility of core.

For the class name I suggest UserAgentSession. That would result in the following methods:

• \Drupal\Core\Session\UserAgentSession::login($account, $mechanism)
• \Drupal\Core\Session\UserAgentSession::logout()

For the mechanism parameter there should be consistent naming pattern. I'd suggest to namespace it using what is known as the provider from plugins.

• In the installer:
  
  \Drupal::service(UserAgentSession::class)->login($account, 'core.install');
• In user RegisterForm:
  $this->userAgentSession->login($account, 'user.register_form')
• In UserAuthenticationController

$this->userAgentSession->login($account, 'user.http_login')
• In UserController

$this->userAgentSession->login($account, 'user.password_reset')
• In UserLoginForm

$this->userAgentSession->login($account, 'user.login_form')

I agree that this requires quite some dependencies at this stage. The following dependencies will go away over time:

• SessionManager as part of #3570824: Deprecate SessionManager::destroy() and rely on PHP lazy sessions
• EntityTypeManager when the timestamp update code is extracted

Also I'd like to suggest to pass in the Request instead of injecting the request stack. A $request object is readily available at every call site.

Finally I'd like to make it really clear that I'm not proposing to rework everything here, but just to move the procedural code to a future proof location and with one ($loginMechanism) or two if you like ($request) additional parameter(s).

Per question in slack, I'm +1 on currently keeping these things in user.module. Long term is long term. This is a very tightly scoped issue and shouldn't be moving things around too much

So, the plan is to ignore #82 for the moment but address the MR discussions. I'll stick around for reviews.

I switched the target branch of the MR to main. Please start with a rebase.

Rebased on main. Haven't made any other changes yet.

This needs a rebase and versions updated.

Rebased and updated versions in the deprecation messages.

Leaving at NW as there are a lot of other comments that haven't been addressed yet.

Got the tests passing at least :-D

As per the feedback, I split out the Login and Logout finalizers into their own services.

Bumping to needs review as I think we should re-address the comment about using service closures.

Split out the tests too.

Updated the IS and CR

Didn't read the whole issue or MR yet, just jumping in to ask whether it was discussed if we need the interfaces or not. More and more, we're not adding interfaces for new services when we don't see a use case where someone would want to exchange it with a different implementation. See the recent locale refactoring issues for example, or the field purger.

No, it wasn't discussed, I just added them. I wasn't aware we were not adding interfaces anymore. 🤔

Removed the interfaces.

Thanks for reviewing.

Outstanding feedback with open questions/comments:

• The existing comment and link to a closed Symfony issue https://github.com/symfony/symfony/issues/12375 Does this need to be addressed in this issue?
• Open question about whether injected loggers need to be closures
• Open discussion on whether the logger should be kept as is, or moved to calling code.

These all seem fairly trivial to me, with a high chance of bike-shedding. My preferences would be to get this 13 year old issue in and punt anything like where we should be doing logging to follow ups.

Re #99

The existing comment and link to a closed Symfony issue https://github.com/symfony/symfony/issues/12375 Does this need to be addressed in this issue?

This is addressed in #3570824: Deprecate SessionManager::destroy() and rely on PHP lazy sessions.