Closed Bug 729001 Opened 10 years ago Closed 10 years ago

reverse resolve the ip address in account lockout notifications

Categories

(Bugzilla :: Email Notifications, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 4.4

People

(Reporter: glob, Assigned: glob)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch patch v1 (obsolete) — Splinter Review
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
Attached patch patch v2 (obsolete) — Splinter Review
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 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-
Let's wait for bug 728639 to land first.
Depends on: CVE-2012-0465
Target Milestone: --- → Bugzilla 4.4
Attached patch patch v3Splinter Review
Attachment #605806 - Attachment is obsolete: true
Attachment #616225 - Flags: review?(LpSolit)
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+
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
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.