pulsejet
User Last Login Not Getting Set
When users login from an OIDC provider via this plugin, their last login timestamp is not getting set in the user's preference table in the database. This causes the Impersonate module to fail with the message "The user cannot be impersonated because it has not logged in."
Looking at the code, it seems like the following line is an approximate cause:
https://github.com/pulsejet/nextcloud-oidc-login/blob/925d4680165be58fe460fa667981a7b7412f5ffa/lib/Controller/LoginController.php#L154
$this->userSession->completeLogin($user, [
'loginName' => $user->getUID(),
'password' => $userPassword,
'token' => empty($userPassword) ? $token : null, // <----------------------
], false);
Looking at the User/Session module in the main Nextcloud server codebase, it seems like having this token be set (as userpassword would not generally be set by this module) causes the server to bypass the updateLastLoginTimestamp() function during the user login process, which is on line:
https://github.com/nextcloud/server/blob/18e32104ce4a5750f7316aadf9363cc780bdf750/lib/private/User/Session.php#L398
$isToken = isset($loginDetails['token']) && $loginDetails['token'] instanceof IToken;
if ($isToken) {
$this->setToken($loginDetails['token']->getId());
$this->lockdownManager->setToken($loginDetails['token']);
$firstTimeLogin = false;
} else {
$this->setToken(null);
$firstTimeLogin = $user->updateLastLoginTimestamp(); // <-----------
}
In our environment, we are seeing that manually adding a call to $user->updateLastLoginTimestamp() after the completeLogin() function in LoginController.php fixes this issue - but all of our users are logging in via Okta OIDC, these are not robotic logins. Is there any case where you would not want to update the updateLastLoginTimestamp from an OIDC login through this module?
$this->userSession->completeLogin($user, [
'loginName' => $user->getUID(),
'password' => $userPassword,
'token' => empty($userPassword) ? $token : null,
], false);
$user->updateLastLoginTimestamp(); // <-----------
If there aren't any objections, I can put together a PR to resolve this issue for additional review.
Thank your for your report. I do not see why not calling updateLastLoginTimestamp on OIDC login. Please open a PR :heart:
Accidentally PR'ed it into my own fork first, sorry. This should be ready for your review whenever you are! :)
@justledbetter your issue/fix is related to the one #144 I recently opened regarding the handling of first time logins and last login timestamps. Unfortunately your changes are only partly fixing things, at least when it's about the linked issue I am confronted with. The point is that this part you already mentioned within the Nextcloud internal Session handling
$isToken = isset($loginDetails['token']) && $loginDetails['token'] instanceof IToken;
if ($isToken) {
$this->setToken($loginDetails['token']->getId());
$this->lockdownManager->setToken($loginDetails['token']);
$firstTimeLogin = false;
} else {
$this->setToken(null);
$firstTimeLogin = $user->updateLastLoginTimestamp(); // <-----------
}
also sets the local variable $firstTimeLogin. Now as your report shows this variable will always be false which in fact was the reason for your fix in the first place (and the fact that $user->updateLastLoginTimestamp()never was called).
However, this variable in turn is being used a few lines later within $this->prepareUserLogin($firstTimeLogin, $regenerateSessionId); leading to unexpected results if it's not handled correctly one of which is the fact that for instance default skeleton files are not being copied during a user's first login.
Now the question is why this part
$this->userSession->completeLogin($user, [
'loginName' => $user->getUID(),
'password' => $userPassword,
'token' => empty($userPassword) ? $token : null, // <----------------------
], false);
is passing a token almost every time (except for a completely new user which is created from scratch during login) as this is the reason for the mentioned behaviour/issue(s). Unfortunately I don't know the internals of this project but maybe we can sort things out in a way that solves both, your and my issue? Thx a lot!
@justledbetter your issue/fix is related to the one #144 I recently opened regarding the handling of first time logins and last login timestamps. Unfortunately your changes are only partly fixing things, at least when it's about the linked issue I am confronted with. The point is that this part you already mentioned within the Nextcloud internal Session handling
$isToken = isset($loginDetails['token']) && $loginDetails['token'] instanceof IToken;
if ($isToken) {
$this->setToken($loginDetails['token']->getId());
$this->lockdownManager->setToken($loginDetails['token']);
$firstTimeLogin = false;
} else {
$this->setToken(null);
$firstTimeLogin = $user->updateLastLoginTimestamp(); // <-----------
}
also sets the local variable $firstTimeLogin. Now as your report shows this variable will always be false which in fact was the reason for your fix in the first place (and the fact that $user->updateLastLoginTimestamp()never was called).
However, this variable in turn is being used a few lines later within $this->prepareUserLogin($firstTimeLogin, $regenerateSessionId); leading to unexpected results if it's not handled correctly one of which is the fact that for instance default skeleton files are not being copied during a user's first login.
Now the question is why this part
$this->userSession->completeLogin($user, [
'loginName' => $user->getUID(),
'password' => $userPassword,
'token' => empty($userPassword) ? $token : null, // <----------------------
], false);
is passing a token almost every time (except for a completely new user which is created from scratch during login) as this is the reason for the mentioned behaviour/issue(s). Unfortunately I don't know the internals of this project but maybe we can sort things out in a way that solves both, your and my issue? Thx a lot!