Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

First Logins, folder generation, other user backends #21119

Open
blizzz opened this issue May 26, 2020 · 3 comments
Open

First Logins, folder generation, other user backends #21119

blizzz opened this issue May 26, 2020 · 3 comments

Comments

@blizzz
Copy link
Member

blizzz commented May 26, 2020

How to use GitHub

  • Please use the 👍 reaction to show that you are interested into the same feature.
  • Please don't comment if you have no relevant information to add. It's just extra noise for everyone subscribed to this issue.
  • Subscribe to receive notifications on status change and new comments.

Is your feature request related to a problem? Please describe.

User Alice logs in for the first time with user_saml. As expected their file system is prepared with the contents of the skeleton directory. What they are perhaps not aware about is, that user_saml is duplicating folder generation code, because it has to override the login mechanism.

User Bob logs in for the first time with user_ldap. Due to the password policy they have to change the initial password after first login. Having done that Bob is redirected to the files app - but is greeted with a web page of death, because the file system was not generated. Bob calls his sysadmin.

User Cecile does the same as Bob. But because their sysadmin has patched the code, Nextcloud works for them, alas the directory is not filled with the default content. Cecile cannot read the "first 7 things to do after your first login.pdf" from their organization as instructed.

Initialization of the user directory happens in \OC\User\Session::prepareUserLogin() and is called after the postLogin events are handled. user_saml is beyond that, the described user_ldap scenario forces a redirect to its password renew controller. There are other issues reported, where this issue occured even with the local backend.

User backends following the regular login chain do not know whether the user logs in for the first time or not, because the information is withhold and the new login date is already written to the database. This is also questionable, because as apps can cancel the login during postLogin event handling.

Describe the solution you'd like

  • The user folder initialization should be unbound from the login process. It shall be possible that this can happen at a later point, and/or be able to be triggered by another user backend.
  • Updating the user last login time could be a late part of the OC\Authentication\Login\Chain. It needs to be ensured that a delayed setting of this timestamp does not cause regressions.
  • The user folder initialization could be part of the OC\Authentication\Login\Chain (just before the login timestamp is set).
  • The base.php does it's own logic with handleLogin. The relevant parts might need to switch to using the OC\Authentication\Login\Chain.
  • The concrete user folder initialization method should still be part of the OC\User\Session, but become public and part of OCP\IUserSession, so it can be called out of context. Or, it shall be evaluated whether apps like user_saml should get a chance to trigger the relevant parts of the login chain handling tasks after the login was irrevocably successful.
  • a new [Login]RedirectException shall be an alternate way of cancelling the login process (currently it is LoginException) and redirect to a specified URL, instead of showing the error page. This avoids that apps have to use exit(0) to solve this case, after sending headers themselves. The LoginController ought to catch it.

Additional context

This is eventually driven by a one line patch:

diff --git a/apps/user_ldap/lib/User/User.php b/apps/user_ldap/lib/User/User.php
index 4ec7b27017..d3aa4d3695 100644
--- a/apps/user_ldap/lib/User/User.php
+++ b/apps/user_ldap/lib/User/User.php
@@ -752,6 +752,7 @@ class User {
            //handle pwdReset attribute
            if (!empty($pwdReset) && $pwdReset[0] === 'TRUE') { //user must change his password
                $this->config->setUserValue($uid, 'user_ldap', 'needsPasswordReset', 'true');
+               \OC::$server->getUserFolder($uid); // ensure user folder exists on first login
                header('Location: '.\OC::$server->getURLGenerator()->linkToRouteAbsolute(
                'user_ldap.renewPassword.showRenewPasswordForm', ['user' => $uid]));
                exit();

@rullzer @ChristophWurst this is my brain dump for today, and it well might not be the final state. I'll certainly sleep over it. Do you have thoughts about it?

@blizzz blizzz added enhancement 0. Needs triage Pending check for reproducibility or if it fits our roadmap feature: users and groups labels May 26, 2020
@blizzz blizzz added this to the Nextcloud 20 milestone May 26, 2020
@ChristophWurst
Copy link
Member

The user folder initialization should be unbound from the login process. It shall be possible that this can happen at a later point, and/or be able to be triggered by another user backend.

@rullzer and I had been discussing something like https://laravel.com/docs/7.x/queues for these cases. We could then make this initialization happen asynchronously.

@blizzz
Copy link
Member Author

blizzz commented Jun 3, 2020

The user folder initialization should be unbound from the login process. It shall be possible that this can happen at a later point, and/or be able to be triggered by another user backend.

@rullzer and I had been discussing something like https://laravel.com/docs/7.x/queues for these cases. We could then make this initialization happen asynchronously.

That's essentially background jobs on steroids. You'll have some priority queue to get the folder done asap, because otherwise the user will end up on an error page – unless we catch and inform the user that the dir is still being prepared. If SSE is disabled (that might even work in the cases, where we have the password - local backends or creation triggered from within nextcloud), the job could be done earlier, upon user creation. Here the exceptions are external backends that cannot list (thus announce) users in time, e.g. SAML and IMAP. You will be stuck with the same problem on very limited, webspace-type installations in theory. In practice those use cases most likely do not apply there. And you will have user directories before their first login, which might lead to complaints. When attaching a big external backend that can announce users, it could clog the queue.

=> Having it async can be a good approach, provided we do it in a reliable and quick manner (and not forgot the simple setups)

@MorrisJobke
Copy link
Member

Moving to 21

@rullzer rullzer modified the milestones: Nextcloud 21, Nextcloud 21.0.1 Mar 1, 2021
@szaimen szaimen added 1. to develop Accepted and waiting to be taken care of feature: filesystem and removed 0. Needs triage Pending check for reproducibility or if it fits our roadmap labels Aug 8, 2021
@blizzz blizzz modified the milestones: Nextcloud 21.0.5, Nextcloud 23 Sep 30, 2021
@blizzz blizzz modified the milestones: Nextcloud 23, Nextcloud 24 Nov 30, 2021
@blizzz blizzz modified the milestones: Nextcloud 24, Nextcloud 25 Apr 21, 2022
@blizzz blizzz modified the milestones: Nextcloud 25, Nextcloud 26 Oct 19, 2022
@blizzz blizzz modified the milestones: Nextcloud 26, Nextcloud 27 Mar 9, 2023
@skjnldsv skjnldsv removed this from the Nextcloud 27.0.2 milestone Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants