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.

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

Show last active it on admin users page #1245

Merged
merged 1 commit into from
Dec 8, 2020

Conversation

janost
Copy link
Contributor

@janost janost commented Nov 30, 2020

Feature request from #246.

This change implements saving last login date and time for users to the database and adds this information to the admin panel's user list. I also added the user creation date to the admin page.

@jjlin
Copy link
Contributor

jjlin commented Nov 30, 2020

I don't think it's necessary to add a new last_login column. The device updated_at timestamp is already updated on each login, so you could just get that instead, e.g.

SELECT d.updated_at
FROM devices d INNER JOIN users u ON d.user_uuid = u.uuid
WHERE u.email = 'user@example.com'
ORDER BY d.updated_at DESC LIMIT 1

@BlackDex
Copy link
Collaborator

BlackDex commented Nov 30, 2020

I was thinking about this my self a few weeks back. And i think there is an other way without modifyy the database.

I think you can use the device table for this. It has an updated at field. With that you can show when last login and with which device.

(Missed the comment from @jjlin)

@janost
Copy link
Contributor Author

janost commented Nov 30, 2020

Thanks for the review @jjlin, @BlackDex!

If I understand the API correctly, what you're proposing is not functionally equivalent to this implementation.
The current last_login field only gets updated if a user logs in with username and password.
The updated_at field in the device model gets updated with a new timestamp even if the application is just locked, for example the user uses biometrics to unlock the Android app, since it requests a refresh token.

I'm fine with either implementation, we just need to agree on the term "Last login".

@jjlin
Copy link
Contributor

jjlin commented Nov 30, 2020

As an admin, I think I would want to know when a user was "last active" or "last online". The updated_at field is updated whenever the user logs in fresh, or if they're already logged in, whenever the cached token is expiring "soon" (in the next hour, AFAIK) and needs refreshing. The token refresh can happen even if the user simply opens the app, but doesn't actually unlock it. I think updated_at is a reasonable first approximation for last activity; ideally, a user's last sync, last cipher written, etc. could be factored in as well, but these aren't tracked currently.

IMO at least, last login (the current implementation) is not very useful since users can log in once via the browser extension or mobile app and then stay logged in indefinitely. So you could have a very active user whose "last login" is months or even years ago.

@BlackDex
Copy link
Collaborator

I my self was thinking about a more detailed overview when for example clicking on a user, and show all active devices. Also with the ability to deauthorize per device. But for the main user overview i agree with @jjlin .

@janost
Copy link
Contributor Author

janost commented Nov 30, 2020

@jjlin @BlackDex You're right, I'm convinced. 👍
I've changed the implementation, please check it out.

@janost
Copy link
Contributor Author

janost commented Dec 1, 2020

@jjlin @BlackDex I changed the implementation according to your feedback.

src/db/models/user.rs Outdated Show resolved Hide resolved
@BlackDex BlackDex changed the title Save last login timestamp for users and show it on admin page Show last active it on admin users page Dec 2, 2020
@BlackDex
Copy link
Collaborator

BlackDex commented Dec 2, 2020

Looking at it visually it's take up a lot of space. Also very much on a mobile (small screen) device.
I'm thinking about how to show this a bit nicer.

@BlackDex
Copy link
Collaborator

BlackDex commented Dec 2, 2020

I have something like this as the first row of the user:

                    <tr>
                        <td colspan="5">
                            <span>Created at: {{created_at}}</span> | <span>Last active: {{last_active}}</span>
                        </td>
                    </tr>

With the following at every <td> in the next <tr>:

 class="border-top-0"

Which then looks like this:
image

I'm also not quite sure about this approach.

@jjlin
Copy link
Contributor

jjlin commented Dec 2, 2020

It might be useful to sort on these values sometimes, so maybe they could each go under a separate column, formatted like

2020-01-01
12:34:56

I can't imagine how mobile devices would ever display this much data nicely, so that may be a lost cause anyway.

@BlackDex
Copy link
Collaborator

BlackDex commented Dec 2, 2020

@jjlin The table is responsive, so it kinda looks ok, you have to scroll left/right to see all.
I do have a lot of orgs with this test user, but it's of course possible.

Maybe we need to create some kind of overlay when there are more then x orgs a user is a member of.
That would create some space. And sorting on date-time is maybe already active. I have to check that JS-Library.

@BlackDex
Copy link
Collaborator

BlackDex commented Dec 2, 2020

What about using popover for the orgs when there are more then x?
https://thednp.github.io/bootstrap.native/#popoverExamples

@BlackDex
Copy link
Collaborator

BlackDex commented Dec 2, 2020

or, maybe just a collapse. That would be better for the searchbox i think.

@BlackDex
Copy link
Collaborator

BlackDex commented Dec 2, 2020

I think that Datatables can sort dates by default especially when using the ISO Format.

@BlackDex
Copy link
Collaborator

BlackDex commented Dec 3, 2020

This example i found looks ok for the orgs: https://jsfiddle.net/g4q5vp2f/1/
WIth that in place, we can just shrink that column, and add two new ones for last active and created

@janost
Copy link
Contributor Author

janost commented Dec 3, 2020

@BlackDex What is your opinion on merging this and working on the UI changes in a separate issue?
I feel like it is out of scope of this feature implementation and it might need further discussion.

Copy link
Collaborator

@BlackDex BlackDex left a comment

Choose a reason for hiding this comment

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

I think that is a good idea.
I'm also comfortable to merge this, since it's not something which could break other items.

I have only one remark left see my suggestion.

src/db/models/user.rs Outdated Show resolved Hide resolved
@BlackDex
Copy link
Collaborator

BlackDex commented Dec 3, 2020

Also, would be nice if you can merge these commit's all into one :).

@janost
Copy link
Contributor Author

janost commented Dec 3, 2020

@BlackDex Done! 👍

@dani-garcia dani-garcia merged commit db710bb into dani-garcia:master Dec 8, 2020
@dani-garcia
Copy link
Owner

Nice, thanks!

@janost janost deleted the user-last-login branch December 8, 2020 22:16
@BlackDex BlackDex mentioned this pull request Dec 10, 2020
60 tasks
@omueller
Copy link

omueller commented Jan 7, 2021

Many thanks for this new very useful feature ! A way to sort the date by "last active" would also be great, and/or a way to export all data as CSV (then we can sort over the terminal or in a spreadsheet program).

@BlackDex
Copy link
Collaborator

BlackDex commented Jan 7, 2021

I'm planning to do a bit of a restyle of that specific interface, that should make sorting by date possible.
Don't know when it will be done.

@omueller
Copy link

omueller commented Jan 7, 2021

Great, thanks for the update @BlackDex!

thelittlefireman pushed a commit to thelittlefireman/bitwarden_rs that referenced this pull request Mar 19, 2021
Show last active it on admin users page
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants