1192687 - add the ability for users to view and revoke existing sessions

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: General, defect, P1)

Description

[Dylan Hardison [:dylan] (he/him)]

- store successful logins in login_success (excluding Cookie and APIKey "logins" as those would be incredibly spammy)
- don't delete login_failures, instead set a cleared flag.
- add a Bugzilla::User->recent_logins() method to display the above information (currently limited to the most recent 100)
- display this information in the user_profile page for the current user

Comment 1

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1192687_1.patch

Hopefully this is something like what you had in mind.

Comment 2

[:glob ✱]

there's nothing security sensitive here, opening.

Comment 3

[:glob ✱]

Comment on attachment 8645526 [details] [diff] [review]
1192687_1.patch

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

- this should be part of the account info (preferences) page, not 'user profile' (the 'user profile' is for your public profile data, which this isn't)
  - add a new tab
  - given the large number of tabs it's time to move to a vertical list on the left rather than tabs on top
- keeping authentication data forever is going to cause issues with table size
  - add a mechanism to delete old entries from login_success and login_failure (older than 1 year), regardless of the 'cleared' bit
- we need to see current sessions, with the ability to revoke
- login_success entries are created for each API request.  this is unlikely to be useful and will result in a significant number of rows in the table

::: Bugzilla/User.pm
@@ +957,5 @@
> +         (SELECT login_time, ip_addr, NULL, 1 AS failure
> +          FROM login_failure
> +          WHERE user_id = ?)
> +          ORDER BY login_time DESC
> +          LIMIT 100",

the formatting of this sql is off

@@ +2292,4 @@
>          "SELECT login_time, ip_addr, user_id FROM login_failure
>            WHERE user_id = ? AND login_time > $time
>                  AND ip_addr = ?
> +                AND NOT cleared

to match the rest of bugzilla use 'AND cleared = 0'

::: extensions/UserProfile/template/en/default/pages/user_profile.html.tmpl
@@ +103,5 @@
> +[% MACRO login_class(class) BLOCK %]
> +  [% SWITCH class %]
> +    [% CASE "Bugzilla::Extension::GitHubAuth::Login" %]GitHub
> +    [% CASE "Bugzilla::Extension::Persona::Login" %]Persona
> +    [% CASE "Bugzilla::Auth::Login::CGI" %] Normal

"Normal" isn't informative here.  "Password" may be better.

@@ +144,5 @@
> +              [% login_class(login.login_class) FILTER html %]
> +            [% END %]
> +            login from [% login.ip_addr FILTER html %] at [% login.login_time FILTER time %]
> +          </li>
> +        [% END %]

this should be a table with columns instead of a list.

we should show the name along side the ip_addr.  this likely means resolving the address when we log rather than at display time

Comment 4

[:glob ✱]

oh, this information needs to be exposed via the REST api, for opsec tooling.

Comment 5

[Dylan Hardison [:dylan] (he/him)]

With the above review in mind, and our discussion -- I think the "overview" page I had worked on will be a good fit for this bug.

Changing the tab layout should be outside the scope of this bug. We can probably make another tab
fit, or merge one or more of (Account, API Keys, Secure Mail).

I believe a "Security" tab that showed the user all their recent login activity (and api key usage)
and allowed them to change their password (+ setup two-factor auth?) would be very compelling.

As a third option (similar to the other) is just provide much of this information on the existing "Account" tab.

Comment 6

[:glob ✱]

(In reply to Dylan William Hardison [:dylan] from comment #5)
> I believe a "Security" tab that showed the user all their recent login
> activity (and api key usage)
> and allowed them to change their password (+ setup two-factor auth?) would
> be very compelling.

a separate tab makes this information easily discoverable, which is important :)

i'd prefer to start with just the auditing stuff; we can look at moving password changing there when we have to touch that as part of other auth system changes (2fx/fxa/totp).

> Changing the tab layout should be outside the scope of this bug.

yeah, i agree with that :)  i've filed bug 1192854 which blocks this.

Comment 7

[:glob ✱]

as per our irc discussion, let's focus this bug on just allowing users to view and revoke their existing sessions.  i'll file another bug with the auditing requirements once we've fleshed those out.

Comment 8

[:glob ✱]

in terms of the information to display, i was thinking of:
- always log the ip address in the logincookies table (bug 1060970)
- foreach loginincookies row, show the last-used timestamp, the ip address, and what that ip address currently reverse-resolves to

we should extend this list later, but let's not let perfect get in the way of good :)

Comment 9

[Dylan Hardison [:dylan] (he/him)]

Alright. As it stands, logincookies primary key is the "cookie" field. This field really should stay secret, and I don't want to have to display it or have it exist in the "revoke sessions" page. Ideally, we'd have an "id" column, right?

Right. But "cookie" is the primary key. When I try adding an auto increment id field, with a UNIQUE index I get an error:
DBD::mysql::db do failed: Incorrect table definition; there can be only one auto column and it must be defined as a key [for Statement "ALTER TABLE logincookies ADD COLUMN id integer auto_increment NOT NULL"] at Bugzilla/DB.pm line 637.

Looking at this, I wonder if the best way forward is to:

1) drop the PRIMARY KEY constraint from "cookie"
2) add a unique constraint to "cookie"
3) add an auto increment id column as primary key.

Testing this in dev seems to be fine, except I have to craft the DDL myself and pass it to $dbh->do. Does this sound reasonable or should I explore another avenue?

Comment 10

[:glob ✱]

(In reply to Dylan William Hardison [:dylan] from comment #9)
> Testing this in dev seems to be fine, except I have to craft the DDL myself
> and pass it to $dbh->do. Does this sound reasonable or should I explore
> another avenue?

yup, that sounds reasonable.

Comment 11

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1192687_2.patch

This depends on the schema change in the blocking bug.

Comment 12

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1192687_3.patch

I always notice something after I upload the patch. Here's one without some useless imports.

Comment 13

[:glob ✱]

Comment on attachment 8651336 [details] [diff] [review]
1192687_3.patch

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

most of this looks good, except for the lack of ip-address tracking.

this patch a trailing whitespace

::: template/en/default/account/prefs/prefs.html.tmpl
@@ +99,4 @@
>        LAST;
>      END;
>    END;
> + %]

this line shouldn't be indented by one space

::: template/en/default/account/prefs/sessions.html.tmpl
@@ +20,5 @@
> +
> +<h3>Active Sessions</h3>
> +
> +[% IF too_many_sessions %]
> +  <p>You have too many active sessions. Display limited to most recent 20.</p>

this message should state the number of sessions.

@@ +33,5 @@
> +
> +  [% FOREACH session IN sessions %]
> +    <tr>
> +      <td>[% session.lastused FILTER time %]</td>
> +      <td>[% session.ipaddr OR "ANY" FILTER html %]</td>

showing "any" here doesn't make any sense.
as per comment 8 we need to always capture the ip address for display purposes.

@@ +48,5 @@
> +    <td colspan="2">All Sessions Except Current</td>
> +    <td>
> +      <input type="checkbox" name="session_logout_all">
> +    </td> 
> +  </tr>

i think this would work better as a button instead of a checkbox

Comment 14

[Dylan Hardison [:dylan] (he/him)]

(In reply to Byron Jones ‹:glob› from comment #13)

> showing "any" here doesn't make any sense.
> as per comment 8 we need to always capture the ip address for display
> purposes.

Sorry! I blanked on comment 8 when I got to that part.

> @@ +48,5 @@
> > +    <td colspan="2">All Sessions Except Current</td>
> > +    <td>
> > +      <input type="checkbox" name="session_logout_all">
> > +    </td> 
> > +  </tr>
> 
> i think this would work better as a button instead of a checkbox

I think so too. Within the preferences framework the easiest way of doing this is a little bit of js to update a hidden field and hit submit. I'll do that -- but it's ugly. :-(

Comment 15

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1192687_4.patch

Comment 16

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1192687_8.patch

Minus all the schema changes bits, and with a whitespace cleanup.

Comment 17

[:glob ✱]

Comment on attachment 8651846 [details] [diff] [review]
1192687_8.patch

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

r=glob with changes to fix on commit.

::: template/en/default/account/prefs/sessions.html.tmpl
@@ +7,5 @@
> +  #%]
> +
> +[%# INTERFACE:
> +  # sessions: array. Array of sessions this user has.
> +  # too_many_sessions: boolean. True if there are more than 20 sessions.

+ session_count

@@ +27,5 @@
> +
> +<p>
> +  You can logout all sessions except the current one using this button:
> +  <input type="submit" name="session_logout_all" value="Logout All">
> +</p>

this reads and looks a bit odd.

how about just a button with the value "Log out all other sessions".  i don't think explanatory text would be required then.

::: userprefs.cgi
@@ +47,5 @@
>  ###############################################################################
> +# Each panel has two functions - panel Foo has a DoFoo, to get the data
> +# necessary for displaying the panel, and a SaveFoo, to save the panel's
> +# contents from the form data (if appropriate).
> +# SaveFoo may be called before DoFoo.

to save dkl's sanity, please limit trailing whitespace removal to just the lines you're touching.

Comment 18

[Dylan Hardison [:dylan] (he/him)]

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   59f9641..d03b432  master -> master