Objectify user_logout() and user_login_finalize()

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

nope. i dont think we should suggest calling controller methods directly. still the API call here is user_logout() which is procedural

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.

The last submitted patch, user-2012976-4.patch, failed testing.

if we are introducing a separate service for this i think we should generalize it more and include similar functions eg
user_login_finalize

maybe call it UserHandler? or AccountHandler

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

Ups.

The last submitted patch, user_handler-2012976-8.patch, failed testing.

Drupal 8.4.4 → was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. ( Drupal 8.5.0-alpha1 → is available for testing.)

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

Drupal 8.3.6 → was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. ( Drupal 8.4.0-alpha1 → is available for testing.)

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

Drupal 8.2.6 → was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. ( Drupal 8.3.0-alpha1 → is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should 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 → .

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

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

We have UserAuth now, its probably a nice place to put those there, too

Drupal 8.5.6 → was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. ( Drupal 8.6.0-rc1 → is available for testing.)

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

Which include file are we killing here?

Drupal 9.4.9 → was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Drupal 9.3.15 → was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Drupal 8 is end-of-life as of November 17, 2021 → . There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Drupal 8.8.7 → was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 → or Drupal 9.0.0 → for ongoing support.

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

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule → and the Allowed changes during the Drupal 8 and 9 release cycles → .

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 🐛 Remove try...catch from SessionHandler::write Fixed 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.

Prashant.c → made their first commit to this issue’s fork.

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

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 📌 Only load all modules when a hook gets invoked Needs review . 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 🐛 2.x-dev incompatible with social_auth Active .

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 🐛 2.x-dev incompatible with social_auth Active and allow us to be involved in more locations to provide enhanced security.
Having this as internal we could provide 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 ✨ Facilitate 2FA+MultiFactor compatibility (2FA/two-factor -> MFA/multi-factor) Active 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] 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.