Record login timestamp per user. Required for new user managament. #8681

blizzz · 2014-05-23T08:57:57Z

This PR cleans up the tryRememberLogin method first, moves the core part to the user session and also adds tests for it. This is necessary, because we need to record the timestamp as well, when a magic cookie (remember checkbox active) is used. The \OC\User\User object gets method to update and return the login timestamp. Updating the timestamp is triggered by postLogin and postRememberedLogin (new!) events. I tested it for login via web interface, remember cookie and desktop client.

There is also a command line tool to get the last login of a specific user. Example:

$ ./occ user:lastseen master
master`s last login: 23.05.2014 08:48

This PR is required for new user management in #7151.

Please test/review @LukasReschke @icewind1991 @DeepDiver1975

karlitschek · 2014-05-23T09:06:36Z

👍

blizzz · 2014-05-23T09:11:05Z

@DeepDiver1975 IIRC you wanted to check this with Apache or another way of auth'ing?

DeepDiver1975 · 2014-05-23T09:16:44Z

lib/base.php

 			self::cleanupLoginTokens($_COOKIE['oc_username']);
-			// get stored tokens
-			$tokens = OC_Preferences::getKeys($_COOKIE['oc_username'], 'login_token');


where did that code goto? OC_User::loginWithCookie?

to lib/private/user/session.php line 188 (invoked by OC_User::loginWithCookie)

ghost · 2014-05-23T10:11:20Z

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4959/

ghost · 2014-05-23T10:33:54Z

💣 Test Failed. 💣
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4960/

blizzz · 2014-05-23T10:53:32Z

»Test Result (no failures)«

icewind1991 · 2014-05-23T14:18:38Z

Tested, code looks good 👍

blizzz · 2014-05-23T17:14:13Z

Oh, i found one minor thing. The hour was given in 12h format but without am/pm indicator. Now its 24h format. So last commit changed just the case of one letter :)

ghost · 2014-05-23T20:22:44Z

💣 Test Failed. 💣
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4986/

blizzz · 2014-05-23T20:35:54Z

Still no failures, why the script dos not return 0 is

16:22:43 npm ERR! Failed to parse json
16:22:43 npm ERR! Unexpected end of input
16:22:43 npm ERR! File: /var/lib/jenkins/.npm/karma-phantomjs-launcher/0.1.4/package/package.json

tests are fine

LukasReschke · 2014-05-24T08:16:43Z

Can we use this opportunity to rename the functions to something saner than magic cookie? - It's absolutely unusual named and hard to understand. (Remember me, Remember, whatever..)

LukasReschke · 2014-05-24T08:18:21Z

core/command/user/lastseen.php

+		$lastLogin = $user->getLastLogin();
+		if($lastLogin === 0) {
+			$output->writeln('User ' . $user->getUID() .
+				' has never logged in, yet.');


That statement is not entirely correct as this could also be triggered if an user has only not logged in since this feature has been added.

It may give the administrator a plausible misbelieve that the user has never logged in while this is not the case. Can we adjust this somehow?

"has never logged in, yet." → "has never logged in (since availability of ownCloud 7.0.0)."

OK?

LukasReschke · 2014-05-24T08:28:01Z

lib/private/user/user.php

+	/**
+	 * updates the timestamp of the most recent login of this user
+	 *
+	 * @return null


I think this returns void and not null. Therefore the @return statement can be dropped.

See http://www.phpdoc.org/docs/latest/for-users/phpdoc/tags/return.html:

It is RECOMMENDED when documenting to use this tag with every function and method. Exceptions to this recommendation are:

constructors, the @return tag MAY be omitted here, in which case @return self is implied.

functions and methods without a return value, the @return tag MAY be omitted here, in which case @return void is implied.

if no specific return is done (→ void) null will be returned, anyway, but I get the point and will drop it.

LukasReschke · 2014-05-24T08:30:42Z

I haven't properly tested this yet. Please don't merge.

ghost · 2014-05-24T09:33:36Z

💣 Test Failed. 💣
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4996/

blizzz · 2014-05-26T11:47:30Z

Can we use this opportunity to rename the functions to something saner than magic cookie? - It's absolutely unusual named and hard to understand. (Remember me, Remember, whatever..)

It would effect other files as well (e.g. core/lostpassword/controller.php, tests/autotest-clover-sqlite.xml), while I have nothing against changing the name I would prefer not to do it in this PR.

… in class descripttion. Also fixes documentation of postLogin hook

…logintimestamp

blizzz · 2014-05-26T12:00:50Z

Regarding the client: when launched the login timestamp will be updated, but as long as the client is running, even through days and suspending the laptop, the timestamp is not being updated.

How is the client keeping the authentication? @DeepDiver1975 @danimo

DeepDiver1975 · 2014-05-26T12:06:13Z

How is the client keeping the authentication? @DeepDiver1975 @danimo

Desktop clients use the same session mechanisms as the browser does.
But the mobile apps (as long as not authenticating via shibboleth) still use basic auth, which would trigger a login on every request

scrutinizer-notifier · 2014-05-26T12:17:24Z

The inspection completed: 6 new issues, 11 updated code elements

ghost · 2014-05-26T13:39:30Z

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/5022/

blizzz · 2014-05-26T13:46:59Z

@DeepDiver1975 right, makes sense, i set back the session life time to default. In that case it probably make sense to check the timestamp on session regenerate too, and update it when the old one is > 24hrs (resp. session lifetime) ago.

blizzz · 2014-05-28T10:47:03Z

@LukasReschke when you look into your crystal ball, can you see when you do proper testing?

LukasReschke · 2014-05-28T10:47:51Z

@LukasReschke when you look into your crystal ball, can you see when you do proper testing?

My crystal ball says tomorrow 18:00 CET - at latest ;-)

blizzz · 2014-05-28T10:58:47Z

k, cool, thx

LukasReschke · 2014-05-28T17:06:44Z

🔮 said to me that I should 👍 this.

Record login timestamp per user. Required for new user managament.

blizzz · 2014-05-28T17:24:09Z

thank you @:crystal_ball: and @LukasReschke

…perly done in #8681

blizzz added 7 commits May 21, 2014 18:03

clean up tryRememberLogin and save the timestamp of users last login

2c89962

unit tests for loginWithCookie()

c46fada

also test whether hooks work

bf1f5f2

test hooks also on login

60274d1

clean up here is superflous, will already be removed by user session

86880ac

add cli command to check a users last login

09bb8e0

save a whitespace

e9fa74b

DeepDiver1975 reviewed May 23, 2014
View reviewed changes

increase scrutinizer happyiness by removing minor/informational issues

2e85d5a

blizzz added this to the ownCloud 7 milestone May 23, 2014

print time in 24h format

7f7999c

LukasReschke reviewed May 24, 2014
View reviewed changes

Clarify comment

c03e7fc

LukasReschke reviewed May 24, 2014
View reviewed changes

blizzz added 2 commits May 26, 2014 13:53

add preRememberedLogin hook and document this and postRememberedLogin…

748a219

… in class descripttion. Also fixes documentation of postLogin hook

drop superflous statement in phpdoc

4a4ea67

Merge branch 'logintimestamp' of git://github.com/owncloud/core into …

3a21c3e

…logintimestamp

LukasReschke added a commit that referenced this pull request May 28, 2014

Merge pull request #8681 from owncloud/logintimestamp

ce9d5df

Record login timestamp per user. Required for new user managament.

LukasReschke merged commit ce9d5df into master May 28, 2014

LukasReschke deleted the logintimestamp branch May 28, 2014 17:06

blizzz added a commit that referenced this pull request Jun 2, 2014

undo lastLogin changes in base and user/user as this has now been pro…

b15a5a7

…perly done in #8681

lock bot locked as resolved and limited conversation to collaborators Aug 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Record login timestamp per user. Required for new user managament. #8681

Record login timestamp per user. Required for new user managament. #8681

blizzz commented May 23, 2014

karlitschek commented May 23, 2014

blizzz commented May 23, 2014

DeepDiver1975 May 23, 2014

blizzz May 23, 2014

ghost commented May 23, 2014

ghost commented May 23, 2014

blizzz commented May 23, 2014

icewind1991 commented May 23, 2014

blizzz commented May 23, 2014

ghost commented May 23, 2014

blizzz commented May 23, 2014

LukasReschke commented May 24, 2014

LukasReschke May 24, 2014

blizzz May 26, 2014

LukasReschke May 24, 2014

blizzz May 26, 2014

LukasReschke commented May 24, 2014

ghost commented May 24, 2014

blizzz commented May 26, 2014

blizzz commented May 26, 2014

DeepDiver1975 commented May 26, 2014

scrutinizer-notifier commented May 26, 2014

ghost commented May 26, 2014

blizzz commented May 26, 2014

blizzz commented May 28, 2014

LukasReschke commented May 28, 2014

blizzz commented May 28, 2014

LukasReschke commented May 28, 2014

blizzz commented May 28, 2014

Record login timestamp per user. Required for new user managament. #8681

Record login timestamp per user. Required for new user managament. #8681

Conversation

blizzz commented May 23, 2014

karlitschek commented May 23, 2014

blizzz commented May 23, 2014

DeepDiver1975 May 23, 2014

Choose a reason for hiding this comment

blizzz May 23, 2014

Choose a reason for hiding this comment

ghost commented May 23, 2014

ghost commented May 23, 2014

blizzz commented May 23, 2014

icewind1991 commented May 23, 2014

blizzz commented May 23, 2014

ghost commented May 23, 2014

blizzz commented May 23, 2014

LukasReschke commented May 24, 2014

LukasReschke May 24, 2014

Choose a reason for hiding this comment

blizzz May 26, 2014

Choose a reason for hiding this comment

LukasReschke May 24, 2014

Choose a reason for hiding this comment

blizzz May 26, 2014

Choose a reason for hiding this comment

LukasReschke commented May 24, 2014

ghost commented May 24, 2014

blizzz commented May 26, 2014

blizzz commented May 26, 2014

DeepDiver1975 commented May 26, 2014

scrutinizer-notifier commented May 26, 2014

ghost commented May 26, 2014

blizzz commented May 26, 2014

blizzz commented May 28, 2014

LukasReschke commented May 28, 2014

blizzz commented May 28, 2014

LukasReschke commented May 28, 2014

blizzz commented May 28, 2014