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.

Sign up for GitHub

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

Jump to bottom

Save user's first login time and output it in occ user:info #37328

Closed
wants to merge 6 commits into from
Closed

Save user's first login time and output it in occ user:info #37328

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
badd79d
Add first login methods to IUser and User
akhil1508 Mar 21, 2023
64b870f
Add created to occ user:info
akhil1508 Mar 21, 2023
5d64fe0
Fix comment
akhil1508 Mar 21, 2023
bf411a4
Fix wrong type
akhil1508 Mar 22, 2023
450cba7
Fix wrong ATOM type
akhil1508 Mar 24, 2023
881e272
Fix CS
akhil1508 Mar 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion core/Command/User/Info.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,8 @@ protected function execute(InputInterface $input, OutputInterface $output): int
'storage' => $this->getStorageInfo($user),
'last_seen' => date(\DateTimeInterface::ATOM, $user->getLastLogin()), // ISO-8601
'user_directory' => $user->getHome(),
'backend' => $user->getBackendClassName()
'backend' => $user->getBackendClassName(),
'created' => date(\DateTimeInterface::ATOM, $user->getFirstLogin())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creation and first login are not the same thing.
Also, this should handle properly the case where firstlogin is 0.

];
$this->writeArrayInOutputFormat($input, $output, $data);
return 0;
Expand Down
8 changes: 8 additions & 0 deletions lib/private/User/LazyUser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,4 +145,12 @@ public function getQuota() {
public function setQuota($quota) {
$this->getUser()->setQuota($quota);
}

public function getFirstLogin() {
return $this->getUser()->getFirstLogin();
}

public function updateFirstLoginTimestamp(int $timestamp) {
return $this->getUser()->updateFirstLoginTimestamp($timestamp);
}
}
2 changes: 2 additions & 0 deletions lib/private/User/Session.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -560,6 +560,8 @@ protected function prepareUserLogin($firstTimeLogin, $refreshCsrfToken = true) {
// read only uses
}

$userObj = $this->getUser();
$userObj->updateFirstLoginTimestamp($userObj->getLastLogin());
// trigger any other initialization
\OC::$server->getEventDispatcher()->dispatch(IUser::class . '::firstLogin', new GenericEvent($this->getUser()));
Comment on lines +563 to 566
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would feel prettier if the call to updateFirstLoginTimestamp is instead in a Listener for the event firstLogin.
Sadly it seems there is no modern event for this yet, only the deprecated GenericEvent.
There is a UserCreatedEvent on the other hand.

}
Expand Down
25 changes: 25 additions & 0 deletions lib/private/User/User.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,9 @@ class User implements IUser {
/** @var int|null */
private $lastLogin;

/** @var int|null */
private $firstLogin;
Comment on lines +92 to +93
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/** @var int|null */
private $firstLogin;
private ?int $firstLogin;

Please strong type new code


/** @var \OCP\IConfig */
private $config;

Expand Down Expand Up @@ -243,6 +246,20 @@ public function getLastLogin() {
return (int) $this->lastLogin;
}

/**
* returns the timestamp of the user's first login or 0 if the user did never
* login
*
* @return int
*/

public function getFirstLogin() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be better to return false for cases when the user first logged in, but I have no strong feelings either way.

if ($this->firstLogin === null) {
$this->firstLogin = (int) $this->config->getUserValue($this->uid, 'login', 'firstLogin', 0);
}
return (int) $this->firstLogin;
}

/**
* updates the timestamp of the most recent login of this user
*/
Expand All @@ -260,6 +277,14 @@ public function updateLastLoginTimestamp() {
return $firstTimeLogin;
}

/**
* updates the timestamp of the first login of this user
*/
public function updateFirstLoginTimestamp(int $timestamp) {
$this->firstLogin = $timestamp;
$this->config->setUserValue($this->uid, 'login', 'firstLogin', (string)$this->firstLogin);
}
Comment on lines +283 to +286
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be some kind of check about whether a first login timestamp was already set for this user?
But not sure what could be done if it’s a case, maybe at least log an error.


/**
* Delete the user
*
Expand Down
15 changes: 15 additions & 0 deletions lib/public/IUser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,4 +270,19 @@ public function getQuota();
* @since 9.0.0
*/
public function setQuota($quota);

/**
* returns the timestamp of the user's first login or 0 if the user did never
* login
*
* @return int
* @since 27.0.0
*/
public function getFirstLogin();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public function getFirstLogin();
public function getFirstLogin(): int;


/**
* updates the timestamp of the first login of this user
* @since 27.0.0
*/
public function updateFirstLoginTimestamp(int $timestamp);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public function updateFirstLoginTimestamp(int $timestamp);
public function updateFirstLoginTimestamp(int $timestamp): void;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we only call this from core, does this need to be in the public interface?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well even in core most of the time the objects will be typed as IUser and not User, most likely this is the case here as well? Especially if it gets moved to a listener, most likely the event will give an IUser instance.

}
20 changes: 20 additions & 0 deletions tests/lib/User/UserTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -849,6 +849,26 @@ public function testGetLastLogin() {
$this->assertSame(42, $user->getLastLogin());
}

public function testGetFirstLogin() {
/**
* @var Backend | MockObject $backend
*/
$backend = $this->createMock(\Test\Util\User\Dummy::class);

$config = $this->createMock(IConfig::class);
$config->method('getUserValue')
->willReturnCallback(function ($uid, $app, $key, $default) {
if ($uid === 'foo' && $app === 'login' && $key === 'firstLogin') {
return 42;
} else {
return $default;
}
});

$user = new User('foo', $backend, $this->dispatcher, null, $config);
$this->assertSame(42, $user->getFirstLogin());
}

public function testSetEnabled() {
/**
* @var Backend | MockObject $backend
Expand Down