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

[#2691] Track IP addresses associated with User signins #6822

Merged
merged 5 commits into from
May 17, 2022
Merged

[#2691] Track IP addresses associated with User signins #6822

merged 5 commits into from
May 17, 2022

Conversation

garethrees
Copy link
Member

@garethrees garethrees commented Feb 25, 2022 •
edited
Loading

If a retention period is configured, create a record associated with the
user of the signin and the IP address used to do so.

A rake/cron task is provided to purge records older than the retention
period.

A version of #2691

View/search all recorded sign ins:

Screenshot 2022-02-25 at 21 36 47

View a given user's sign ins. Check how many others have signed in with the IP:

Screenshot 2022-02-25 at 21 36 27

Click the IP address to go through to the main listing filtered by that IP (or country, added after screenshot):

Screenshot 2022-02-25 at 21 36 38

Filter by partial IP:

Screenshot 2022-02-25 at 21 55 32

Filter by country:

Screenshot 2022-02-25 at 21 55 23

@mysociety-pusher mysociety-pusher force-pushed the 2691-user-ip-tracking branch 2 times, most recently from a315a9d to e5ac312 Compare February 25, 2022 22:03
@garethrees garethrees requested a review from gbp February 28, 2022 09:11
@garethrees garethrees marked this pull request as ready for review February 28, 2022 09:12
gbp
gbp requested changes Mar 23, 2022
View reviewed changes
Copy link
Member

@gbp gbp left a comment

Choose a reason for hiding this comment

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

This is good, I think there is still some work to do tho.

Does this work with both IPv4 and IPv6 addresses. I think we also need to consider what we do when a user is closed and anonymised.

db/migrate/20220225094330_create_user_sign_ins.rb Outdated Show resolved Hide resolved
app/views/admin/users/_sign_in_table.html.erb Outdated Show resolved Hide resolved
app/views/layouts/admin/users.html.erb Outdated Show resolved Hide resolved
app/views/admin_user/show.html.erb Show resolved Hide resolved
config/crontab-example Show resolved Hide resolved
app/models/user.rb Outdated Show resolved Hide resolved
app/controllers/admin/users/sign_ins_controller.rb Outdated Show resolved Hide resolved
app/models/user/sign_in.rb Show resolved Hide resolved
db/migrate/20220225214524_add_country_to_user_sign_ins.rb Show resolved Hide resolved
app/views/admin/users/_sign_in_table.html.erb Outdated Show resolved Hide resolved
@gbp gbp assigned garethrees and unassigned gbp Mar 23, 2022
@gbp
Copy link
Member

gbp commented Mar 28, 2022

How about making this more generic and recording IP on sign ins and sign ups?

Could then link the "rate limited signup from" email to the index action with IP already filtered.

@garethrees garethrees mentioned this pull request Apr 6, 2022
Closed
@garethrees
Copy link
Member Author

Does this work with both IPv4 and IPv6 addresses.

I've now used postgres inet type and added a basic test, so should do, though I don't know much about IPv6 tbh.

I think we also need to consider what we do when a user is closed and anonymised.

Good point. I think the regular purge would have been fine, but also added on-demand purging via User#close_and_anonymise in 97c6d71.

How about making this more generic and recording IP on sign ins and sign ups? Could then link the "rate limited signup from" email to the index action with IP already filtered.

I did think about that, but this is the more immediate problem. To consider in relation to #6931.

@garethrees
Copy link
Member Author

garethrees commented Apr 28, 2022 •
edited
Loading

Couple of things left to do, but in order to do so I'll want to squash these commits:

  • Move the addition of default_scope to the previous (first) commit
  • Add "none yet" conditional to the sign in table
  • Squash eab805a and 73e939d into a single commit (~ "Render user sign ins in the admin interface")
  • Handle USER_SIGN_IN_ACTIVITY_RETENTION_DAYS == 0. Instead of hiding, I'll use a notice saying "sign in tracking is not configured; see general.yml", or something to that effect.

@garethrees garethrees requested a review from gbp April 28, 2022 11:29
@garethrees garethrees assigned gbp and unassigned garethrees Apr 28, 2022
@garethrees
Copy link
Member Author

@gbp a quick pair of eyes over the fixups would be great before I squash them down to complete the remaining tasks.

@gbp
Copy link
Member

gbp commented Apr 28, 2022

@gbp a quick pair of eyes over the fixups would be great before I squash them down to complete the remaining tasks.

Fixups look good me, feel free to squash and rebase

@gbp gbp assigned garethrees and unassigned gbp Apr 28, 2022
@garethrees
Record IP addresses associated with User signins
280e753 
If a retention period is configured, create a record associated with the
user of the signin and the IP address used to do so.

Uses Postgres' inet so that we can handle both IPv4 and IPv6 addresses.
Fake ipv6 taken from Faker example [1].

A rake/cron task is provided to purge records older than the retention
period.

A version of #2691

[1] https://github.com/faker-ruby/faker/blob/a00b4c6388c69fdafb7e12cbacdb6d8a9927fba5/lib/faker/default/internet.rb#L423
@garethrees
Add .alert-disabled boostrap extension
9b68e8c 
A grey alert box [1].

[1] https://getbootstrap.com/2.2.0/components.html#alerts
@garethrees
Show user signins in the admin interface
cfb5f7f 
Renders a given user's signins on their user admin page.

Adds `Admin::Users::SignInsController#index` to list all recorded
signins.
@garethrees
Show count of other users signing in with an IP
fd3ed51 
Helps to identify possible misusers.
@garethrees
Record User::SignIn#country
abc113c 
Record the geolocation country code of the IP at the time of sign in so
that it can be used for misuse detection and filtering.
@garethrees
Copy link
Member Author

Disabled with no signins

Screenshot 2022-04-28 at 14 19 12

Screenshot 2022-04-28 at 14 38 04

Disabled but signins exist

Screenshot 2022-04-28 at 14 19 22

Screenshot 2022-04-28 at 14 37 27

Enabled with no signins

Screenshot 2022-04-28 at 14 22 19

Enabled with signins

Screenshot 2022-04-28 at 14 21 42

@garethrees garethrees requested review from gbp and removed request for gbp April 28, 2022 14:02
@garethrees garethrees assigned gbp and unassigned garethrees Apr 28, 2022
@garethrees
Copy link
Member Author

Think this is good to go now 🥳

gbp
gbp approved these changes May 10, 2022
View reviewed changes
Copy link
Member

@gbp gbp left a comment

Choose a reason for hiding this comment

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

LGTM

@gbp gbp assigned garethrees and unassigned gbp May 10, 2022
@mysociety-pusher mysociety-pusher merged commit 10140bc into develop May 17, 2022
@mysociety-pusher mysociety-pusher deleted the 2691-user-ip-tracking branch May 17, 2022 10:50
@garethrees garethrees mentioned this pull request May 17, 2022
Open
This was referenced May 24, 2022
Closed
Open
@RichardTaylor RichardTaylor mentioned this pull request Jul 27, 2022
Closed
@garethrees
Copy link
Member Author

Linking to #7592.

@garethrees garethrees mentioned this pull request Mar 17, 2023
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gbp gbp gbp approved these changes

Assignees

@garethrees garethrees

Labels
awaiting-deploy
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@garethrees @gbp @mysociety-pusher