240437 - profiles table needs a "last seen" column

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, enhancement)

Description

[Gervase Markham [:gerv]]

It would be good to know when a particular account was last used. This would,
for example:

- Allow any site which wished to to auto-disable accounts not used for a period
- Allow admins to see when particular users last logged in (we found a use for
this recently on b.m.o)

Gerv

Comment 1

[Dave Miller [:justdave]]

We already store this information in the logincookies table, but it goes away
after 30 days...

Comment 2

[Gervase Markham [:gerv]]

Yeah; for this application we need something persistent. Do you think it should
replace or be in addition to the lastused value in logincookies?

Gerv

Comment 3

[Dave Miller [:justdave]]

in addition to would probably work better.  we need the age of each cookie
whether or not it's the last time the person logged in (a person can have
multiple active cookies if they log in from more than one computer)

Comment 4

[Gervase Markham [:gerv]]

We don't actually want to know when the user last logged in (people with static
IPs can stay logged in for months), we want to know when that account last made
a request for a page. So here's what we do:

- Add a datetime field to profiles, called "last_seen".
- Add a new method to the Bugzilla::Auth interface, called 
  update_last_seen($username).
- This method gets called when a login is successful (even if it's a Cookie.pm
  login, we still call it on Bugzilla::Auth)
- This method is implemented by DB.pm to update the profile field. It's a no-op  
  in LDAP.pm (at the moment; someone could implement it later, perhaps.)

CCing bbaetz for his views. Have I managed to avoid breaking the Auth
architecture? :-)

Gerv

Comment 5

[Bradley Baetz (:bbaetz)]

Sounds fine

Comment 6

[Gervase Markham [:gerv]]

I'm not sure what I was smoking in comment #4; I said we want to measure the last time a request was made, not the last time we logged in (which is right) but this means it's nothing to do with auth at all.

So yes, we need a new field last_seen on the profiles table, which is updated when the user makes a request.

However, we also want to avoid turning every read into a write (the "atime" problem). So we should probably store the value with a granularity of 1 day, and only update it if it's different to the current value.

Gerv

Comment 7

[Gervase Markham [:gerv]]

I'd still like to see this done, but having it assigned to me is misleading, as I'm not working on it.

Gerv

Comment 8

[:glob ✱]

Attachment: patch v1

this patch adds and updates the profiles.last_login_date column, but doesn't do anything with it yet.

Comment 9

[David Lawrence [:dkl]]

Comment on attachment 570956 [details] [diff] [review]
patch v1

Review of attachment 570956 [details] [diff] [review]:
-----------------------------------------------------------------

Comments below.

::: Bugzilla/Install/DB.pm
@@ +656,5 @@
>      # 2011-06-15 [e-mail] - Bug 658929
>      _migrate_disabledtext_boolean();
>  
> +    # 2011-11-01 [e-mail] - Bug 240437
> +    $dbh->bz_add_column('profiles', 'last_login_date', {TYPE => 'DATETIME'});

Should we also add a migrate function to update the profiles.last_login_date from the user's last cookie in logincookies? If yes, then there is the question as to what to do with the user's who do not have a cookie currently. Suppose we could leave those as NULL and anyone mining the data would understand that those users had not logged in since the change was implemented.

Otherwise if noone really cares for this information prior to the change being committed, we can leave last_login_date completely NULL and go from now.

::: Bugzilla/User.pm
@@ +286,4 @@
>      $_[0]->set('is_enabled', $_[1] ? 0 : 1);
>  }
>  
> +sub touch_last_login_date {

My preference would be that this should be called set_last_login_date() to be consistent with the other mutators but that is not completely accurate as we do not pass a value to the method and it also happens outside of the normal update(). "touch" is not always undertstandable to someone not coming from the Unix world. I would either 1) change it to "set" if you don't feel this causes confusion with the other setters since it doesn't wait for update() or 2) at a minimum, change it to either update or refresh to better describe what it does.

@@ +296,5 @@
> +        # pending changes
> +        my $dbh = Bugzilla->dbh;
> +        $dbh->do("UPDATE profiles SET last_login_date=? WHERE userid=?",
> +                 undef, $date, $_[0]->id);
> +    }

$_[0] is used enough in this method that I would just use $self everywhere instead. It is easier to read and it only adds one line. For methods with only one or two lines, $_[0] is acceptable.

Comment 10

[Gervase Markham [:gerv]]

glob: what about the issue I raise in para 3 of comment 6? Is it a problem or not?

Gerv

Comment 11

[:glob ✱]

(In reply to Gervase Markham [:gerv] from comment #10)
> glob: what about the issue I raise in para 3 of comment 6? Is it a problem
> or not?

i agree that it could be a problem, so the patch does exactly what you suggested; granularity of 1 day, and only update if required.

Comment 12

[Frédéric Buclin]

Comment on attachment 570956 [details] [diff] [review]
patch v1

>=== modified file 'Bugzilla/User.pm'

>+sub touch_last_login_date {

I don't really like this name. If its goal is to update the date, it should probably be set_* or update_*. We don't use touch_* anywhere else in our code for similar methods.


>+        $dbh->do("UPDATE profiles SET last_login_date=? WHERE userid=?",
>+                 undef, $date, $_[0]->id);

Why not last_login_date = CURRENT_TIMESTAMP instead of what you do? This would be much more efficient.


>+sub last_login_date { $_[0]->{last_login_date}; }

I guess you will have to format it before returning it.

Comment 13

[Frédéric Buclin]

Comment on attachment 570956 [details] [diff] [review]
patch v1

>=== modified file 'Bugzilla.pm'

>+    $class->user->touch_last_login_date() if $class->user->id;

If the user is already logged in, we won't trigger this code, and so the "1 day granularity" discussion can go away. A user doesn't relogin every time he accesses another page. So it's fine to use the real date and time instead of the date only. So my previous suggestion still stands.

Comment 14

[:glob ✱]

(In reply to Frédéric Buclin from comment #13)
> Comment on attachment 570956 [details] [diff] [review] [diff] [details] [review]
> patch v1
> 
> >=== modified file 'Bugzilla.pm'
> 
> >+    $class->user->touch_last_login_date() if $class->user->id;
> 
> If the user is already logged in, we won't trigger this code

this isn't correct; this code is triggered on each page load for me, even if i'm already logged in.

Comment 15

[Gervase Markham [:gerv]]

LpSolit's question raises an important point - is this a "last login date", in that it's the last time a user supplied their password and authenticated, or is it a "last seen date", as in the last time the user, logged in, loaded a page or took an action? I think it is, and should be, the latter - that's the useful information. So perhaps the field requires a change of name? (Yes, I know I filed the original bug, but I had a minor change of direction in comment 4.)

Comment 16

[:glob ✱]

(In reply to Gervase Markham [:gerv] from comment #15)
> LpSolit's question raises an important point - is this a "last login date",
> in that it's the last time a user supplied their password and authenticated,
> or is it a "last seen date", as in the last time the user, logged in, loaded
> a page or took an action? I think it is, and should be, the latter - that's
> the useful information. So perhaps the field requires a change of name?

ah, indeed, the name of the field is wrong.  last_seen is better (and what you suggested in comment 4 - sorry i missed that somehow).

Comment 17

[Frédéric Buclin]

(In reply to Byron Jones ‹:glob› from comment #14)
> > If the user is already logged in, we won't trigger this code
> 
> this isn't correct; this code is triggered on each page load for me, even if
> i'm already logged in.

sub login {
    my ($class, $type) = @_;

    return $class->user if $class->user->id;


If the user is already logged in, you won't go further and so you won't trigger your code.

Comment 18

[David Lawrence [:dkl]]

(In reply to Frédéric Buclin from comment #17)
> sub login {
>     my ($class, $type) = @_;
> 
>     return $class->user if $class->user->id;
> 
> If the user is already logged in, you won't go further and so you won't
> trigger your code.

That is only if Bugzilla->login is called more than once in a single page load. If it is a new page load, then the first call to Bugzilla->login will always call $class->user->touch_last_login_date(). $class->user is an empty User object until $class->set_user() is called.

dkl

Comment 19

[:glob ✱]

Attachment: patch v2

> Should we also add a migrate function to update the profiles.last_login_date
> from the user's last cookie in logincookies?

i don't think migration's required.

> last-login or last-seen?

last-seen :)

> rename sub touch_last_login_date {

now "update_last_seen_date".  to me set_* implies changing the field to a specified value, which isn't the case here.

> >+sub last_login_date { $_[0]->{last_login_date}; }
> I guess you will have to format it before returning it.

it's already in a format which matches other datetime fields.

Comment 20

[Gervase Markham [:gerv]]

How long do login cookies stay in logincookies? If it's a significant amount of time, then migration would be useful, as an approximate value for last_seen would help us to do analysis on the data earlier and easier. If it's not too complicated. 

Gerv

Comment 21

[Frédéric Buclin]

(In reply to Gervase Markham [:gerv] from comment #20)
> How long do login cookies stay in logincookies?

30 days, but it really doesn't worth the migration code at all.

Comment 22

[Frédéric Buclin]

Comment on attachment 571252 [details] [diff] [review]
patch v2

>=== modified file 'Bugzilla.pm'

>+    $class->user->update_last_seen_date() if $class->user->id;

To make your code more robust, the |if $class->user->id| check should be in update_last_seen_date() itself, in case the method is reused elsewhere, e.g. in an extension. It's much better/robust to let the method do the required checks itself that supposing that the caller will do it.



>=== modified file 'Bugzilla/User.pm'

>+sub update_last_seen_date {
>+    my ($year, $month, $day) = (localtime)[5, 4, 3];

Login cookies are stored using the local time of the DB server, not the one of the web server. In the case they are not located in the same timezone, we should be consistent and also use the DB server as reference here.


>+    my $date = sprintf("%4d-%02d-%02d 00:00:00", $year + 1900, $month + 1, $day);

Nit: ++$month


>+    if ($date ne $self->last_seen_date) {

This won't work based on the way you defined last_seen_date(). You cannot assume the format of the date stored in the DB to be of the form "%4d-%02d-%02d 00:00:00". You must use $dbh->sql_date_format() to correctly format the datetime coming from the DB, see e.g. Bugzilla::Bug::DB_COLUMNS to see how it's done with creation_ts and delta_ts.


>+sub last_seen_date { $_[0]->{last_seen_date} || ''; }

As an empty string is not a valid date, I think we should make this method return undef here if not yet defined.

Comment 23

[Gervase Markham [:gerv]]

glob: ping? Looks like this one is pretty close...

Gerv

Comment 24

[:glob ✱]

(In reply to Gervase Markham [:gerv] from comment #23)
> glob: ping? Looks like this one is pretty close...

thanks, i haven't forgotten about this, i've been sidetracked by other projects.

Comment 25

[:glob ✱]

Attachment: patch v3

Comment 26

[Frédéric Buclin]

Comment on attachment 582793 [details] [diff] [review]
patch v3

>=== modified file 'Bugzilla.pm'

>+    $class->user->update_last_seen_date();

While testing your patch, I realized that the last_seen attribute was updated also when impersonating another user. I don't think this should be the case, else this would give the wrong impression that the account is still active when it was only used by an admin for some reason. So IMO, we should only update this last_seen column when the user himself logs in.



>=== modified file 'Bugzilla/Install/DB.pm'

>+    # 2011-11-01 [e-mail] - Bug 240437
>+    $dbh->bz_add_column('profiles', 'last_seen_date', {TYPE => 'DATETIME'});

You forgot to include this new column in the DB schema (Bugzilla/DB/Schema.pm).
Also, why not last_seen instead of last_seen_date as column name?



>=== modified file 'Bugzilla/User.pm'

>+sub update_last_seen_date {

>+    my $date = $dbh->selectrow_array(
>+        'SELECT ' . $dbh->sql_date_format('NOW()', '%Y-%m-%d'));
>+    if (!$self->last_seen_date or $date ne $self->last_seen_date) {

Nit: please insert an empty line between my $date and if (). It's IMO easier to see where the IF block begins.


>+        $dbh->do("UPDATE profiles SET last_seen_date=? WHERE userid=?",

Add a whitespace around each =, as we do everywhere else.



>=== modified file 'template/en/default/admin/users/edit.html.tmpl'

>+    <th>Last Login:</th>

Why not also add it to the user list, to have this information easily? This information doesn't require any new SQL query, so we would get it for free.


Otherwise looks good.

Comment 27

[:glob ✱]

Attachment: patch v4

> While testing your patch, I realized that the last_seen attribute was
> updated also when impersonating another user.

nice catch.  this revision updates the last_seen_date for the sudoer, not the user they are impersonating.

> Also, why not last_seen instead of last_seen_date as column name?

because the column is a DATETIME type; i want to ensure it's clear that the time portion of the value is not valid.

> Why not also add it to the user list, to have this information easily? This
> information doesn't require any new SQL query, so we would get it for free.

while not totally free, i've added it to this revision.

Comment 28

[Frédéric Buclin]

Comment on attachment 590655 [details] [diff] [review]
patch v4

>=== modified file 'Bugzilla.pm'

>+    if (Bugzilla->sudoer) {
>+        Bugzilla->sudoer->update_last_seen_date();
>+    } else {
>+        $class->user->update_last_seen_date();
>+    }

For consistency with $class->user, write $class->sudoer.


Otherwise looks good and works fine. r=LpSolit

Comment 29

[Reed Loden [:reed]]

Can we also get a "last seen ip" column, or should that be a separate bug? logincookies is not sufficient for that purpose, as it does not store an IP unless you have it restricted to a netmask.

Comment 30

[Frédéric Buclin]

(In reply to Reed Loden [:reed] (very busy) from comment #29)
> Can we also get a "last seen ip" column, or should that be a separate bug?

This is unrelated to this bug.


> logincookies is not sufficient for that purpose, as it does not store an IP
> unless you have it restricted to a netmask.

Bugzilla stopped using netmasks for a long time, since 3.6, see bug 399073. So I don't see why you would need that.

Comment 31

[:glob ✱]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla.pm
modified editusers.cgi
modified Bugzilla/User.pm
modified Bugzilla/DB/Schema.pm
modified Bugzilla/Install/DB.pm
modified template/en/default/admin/users/edit.html.tmpl
modified template/en/default/admin/users/list.html.tmpl
Committed revision 8088.

Comment 32

[Max Kanat-Alexander]

Comment on attachment 590655 [details] [diff] [review]
patch v4

Review of attachment 590655 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry that I didn't comment on this until now, but:

::: Bugzilla/User.pm
@@ +75,5 @@
> +        'profiles.disabledtext',
> +        'profiles.disable_mail',
> +        'profiles.extern_id',
> +        'profiles.is_enabled',
> +        $dbh->sql_date_format('last_seen_date', '%Y-%m-%d') . ' AS last_seen_date',

WHY ARE YOU FORMATTING THINGS INSIDE OF OBJECTS? Please stop doing this. It makes it impossible to use this date in any other fashion! Formatting is for UIs, not for backends.

Comment 33

[Max Kanat-Alexander]

(In reply to Frédéric Buclin from comment #22)
> This won't work based on the way you defined last_seen_date(). You cannot
> assume the format of the date stored in the DB to be of the form
> "%4d-%02d-%02d 00:00:00". You must use $dbh->sql_date_format() to correctly
> format the datetime coming from the DB, see e.g. Bugzilla::Bug::DB_COLUMNS
> to see how it's done with creation_ts and delta_ts.

  Ahh, I see why you're doing this. If you're going to do this, you should have a date-formatting constant, and that constant should always format down to seconds.

  If we really did do this consistently across Bugzilla, we could support databases that have non-standard date formats, which could be nice.

Comment 34

[:glob ✱]

(In reply to Max Kanat-Alexander from comment #33)
>   Ahh, I see why you're doing this. If you're going to do this, you should
> have a date-formatting constant, and that constant should always format down
> to seconds.

filed as bug 720639

Comment 35

[Frédéric Buclin]

Added to relnotes for 4.4.