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

Allow admins to view user IP addresses in the admin panel users table and users page #1009

Draft
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

ghost
Copy link

@ghost ghost commented Aug 9, 2024

Introduces admin ability to view local users IP addresses as resolved from src/Service/IpResolver.php:

image

A user's IP address is updated on:

  • successful authentication
  • entry creation
  • entry edit
  • entry comment creation
  • entry comment edit
  • post creation
  • post edit
  • post comment creation
  • post comment edit

I also added local user email and IP to the user's page for admins:

image

Other things:
I noticed the GitHub authenticator was missing the IP being set in the user dto which will cause the registration limiter to be ineffective, so I added it in to be consistent with the other authentication mechanisms.

Closes: #833

@ghost ghost added the enhancement New feature or request label Aug 9, 2024
@ghost ghost added this to the v1.8.0 milestone Aug 9, 2024
@ghost ghost self-assigned this Aug 9, 2024
@ghost ghost marked this pull request as ready for review August 9, 2024 14:36
@ghost ghost requested a review from BentiGorlich August 9, 2024 14:36
@BentiGorlich
Copy link
Member

I thought that we fixed the trusted proxy problem a long time ago :O

@ghost
Copy link
Author

ghost commented Aug 9, 2024

I thought that we fixed the trusted proxy problem a long time ago :O

not sure how since it was never configured, lol

@ghost ghost changed the title Allow admins to view user IP addresses in the admin panel users table Allow admins to view user IP addresses in the admin panel users table and users page Aug 9, 2024
Copy link
Member

@BentiGorlich BentiGorlich left a comment

Choose a reason for hiding this comment

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

I have a few things that I would like to see changed:

  1. I would prefer if you'd move the trusted proxy stuff in its own PR
  2. Since we are already changing the db schema to have the IP-addresses of users we should create a new table for IP addresses and one that makes the references between users and IPs. That would have a few benefits:
    • accounts for multiple devices
    • The most important IP imo is the one used to register an account. At least that is the one I would be interested in for spam detection.
    • since this is personal information I think we should create a scheduled job to look for IP addresses that have been used over a year ago and deleting them
    • we can use the same table for IP-blocks in the future
    • easier to lookup all users with a specific IP

I think this would be a really good addition for admins to prevent spam right upon account creation

templates/admin/users.html.twig Outdated Show resolved Hide resolved
templates/admin/users.html.twig Outdated Show resolved Hide resolved
@ghost ghost marked this pull request as draft August 10, 2024 14:16
@melroy89
Copy link
Member

Should we follow-up this PR?

Copy link
Contributor

github-actions bot commented Nov 2, 2024

This PR is stale because it has been open 40 days with no activity.

@github-actions github-actions bot added the Stale Inactivity for too long label Nov 2, 2024
@melroy89
Copy link
Member

melroy89 commented Nov 8, 2024

Rebased..

@melroy89
Copy link
Member

melroy89 commented Nov 8, 2024

Fixed PHP cs fixer.

@github-actions github-actions bot removed the Stale Inactivity for too long label Nov 9, 2024
@BentiGorlich BentiGorlich modified the milestones: v1.8.0, v1.8.1 Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Add local user IP ban/visibility for instance admins
2 participants