reverse resolve the ip address in account lockout notifications

RESOLVED FIXED in Bugzilla 4.4

Status

()

Bugzilla
Email Notifications
--
enhancement
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: glob, Assigned: glob)

Tracking

unspecified
Bugzilla 4.4
Bug Flags:
approval +

Details

Attachments

(1 attachment, 2 obsolete attachments)

the lockout notifications currently look like:

> The IP address 8.8.8.8 failed too many login attempts (5) for
> the account someone@example.com.

it would be nice to have the rdns entry for the ip:

> The IP address 8.8.8.8 (google-public-dns-a.google.com) failed
> too many login attempts (5) for the account someone@example.com.
Created attachment 605670 [details] [diff] [review]
patch v1

simple patch.
note if there are proxies, multiple ip addresses may be provided.
Assignee: email-notifications → glob
Status: NEW → ASSIGNED
Attachment #605670 - Flags: review?(LpSolit)
Comment on attachment 605670 [details] [diff] [review]
patch v1

I *always* want to be able to see the IP. If there's a hostname, that's a nice bonus, but the IP is a must-have.
n/m, I can't read
Created attachment 605806 [details] [diff] [review]
patch v2

this revision replaces "IP address" with "address", so it now looks like:

> The address google-public-dns-a.google.com <8.8.8.8> failed too many
> login attempts (5) for the account someone@example.com.
Attachment #605670 - Attachment is obsolete: true
Attachment #605806 - Flags: review?(LpSolit)
Attachment #605670 - Flags: review?(LpSolit)

Comment 5

6 years ago
Comment on attachment 605806 [details] [diff] [review]
patch v2

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

>+            foreach my $ip (split(/, /, $attempts->[0]->{ip_addr})) {

id_addr should contain only one IP address, see bug 728639, so there is no need to split it anymore (especially because this will never happen once bug 728639 lands).
Attachment #605806 - Flags: review?(LpSolit) → review-

Comment 6

6 years ago
Let's wait for bug 728639 to land first.
Depends on: 728639
Target Milestone: --- → Bugzilla 4.4
Created attachment 616225 [details] [diff] [review]
patch v3
Attachment #605806 - Attachment is obsolete: true
Attachment #616225 - Flags: review?(LpSolit)

Comment 8

6 years ago
Comment on attachment 616225 [details] [diff] [review]
patch v3

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

>+            my $n = inet_aton($address);

Please add a comment about this line that inet_aton() is only able to resolve IPv4 addresses. To also resolve IPv6 addresses, we will have to use inet_pton(), but is only available since Perl 5.12.0, unfortunately.


>+                $address = gethostbyaddr($n, AF_INET) . " <$address>"

Why using brackets for the IP address? This looks really weird, and I don't remember having seen brackets being used for IP addresses elsewhere (they are used for email addresses). Parens would make more sense.


Note that depending on the network latency, it can take several seconds for this code to return the hostname. As this only affects the guy whose account is now locked, I think this is fine. So r=LpSolit with the comments above addressed.
Attachment #616225 - Flags: review?(LpSolit) → review+

Updated

6 years ago
Flags: approval+
Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Auth.pm
modified template/en/default/email/lockout.txt.tmpl
Committed revision 8223.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.