feat: use geoip info for display (#13745)

* feat: use geoip info for display

Basic implementation of pulling the GeoIP info off `Event.additional`

Needs test cases, as well as refining the display order of values.

Signed-off-by: Mike Fiedler <[email protected]>

* make translations

Signed-off-by: Mike Fiedler <[email protected]>

---------

Signed-off-by: Mike Fiedler <[email protected]>

Author: miketheman

tests/unit/events/test_models.py

@@ -0,0 +1,39 @@
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import pytest
+
+from warehouse.events.models import GeoIPInfo
+
+
+class TestGeoIPInfo:
+    @pytest.mark.parametrize(
+        "test_input, expected",
+        [
+            ({}, ""),
+            (
+                {"city": "new york", "country_code": "us", "region": "ny"},
+                "New York, NY, US",
+            ),
+            (
+                {"city": "new york", "country_code": "us"},
+                "New York, US",
+            ),
+            ({"city": "new york", "region": "ny"}, "New York, NY"),
+            ({"region": "ny", "country_code": "us"}, "NY, US"),
+            ({"country_name": "United States"}, "United States"),
+        ],
+    )
+    def test_display_output(self, test_input, expected):
+        """Create a GeoIPInfo object and test the display method."""
+        dataklazz = GeoIPInfo(**test_input)
+        assert dataklazz.display() == expected

tests/unit/organizations/test_models.py

@@ -183,14 +183,14 @@ def test_acl(self, db_session):
             key=lambda x: x[1],
         )
 
-    def test_record_event(self, db_request):
+    def test_record_event_with_geoip(self, db_request):
         """
         Test to cover condition when record_event is called with geoip_info as
         part of the inbound request.
         Possibly could be removed once more comprehensive tests are in place,
         but nothing explicitly covers `HasEvents.record_event`
         """
-        db_request.ip_address.geoip_info = {"country_code": "US"}
+        db_request.ip_address.geoip_info = {"country_name": "United States"}
 
         organization = DBOrganizationFactory.create()
 
@@ -201,6 +201,49 @@ def test_record_event(self, db_request):
             additional={},
         )
 
+        event = organization.events[0]
+
+        assert event.additional == {
+            "organization_name": organization.name,
+            "geoip_info": {"country_name": "United States"},
+        }
+        assert event.location_info == "United States"
+
+    def test_location_info_without_geoip(self, db_request):
+        organization = DBOrganizationFactory.create()
+        organization.record_event(
+            tag="",
+            ip_address=db_request.ip_address.ip_address,
+            request=db_request,
+            additional={},
+        )
+
+        event = organization.events[0]
+
+        assert event.additional == {
+            "organization_name": organization.name,
+        }
+        assert event.location_info == db_request.ip_address
+
+    def test_location_info_with_partial(self, db_request):
+        db_request.ip_address.geoip_info = {"country_code3": "USA"}
+
+        organization = DBOrganizationFactory.create()
+        organization.record_event(
+            tag="",
+            ip_address=db_request.ip_address.ip_address,
+            request=db_request,
+            additional={},
+        )
+
+        event = organization.events[0]
+
+        assert event.additional == {
+            "organization_name": organization.name,
+            "geoip_info": {"country_code3": "USA"},
+        }
+        assert event.location_info == db_request.ip_address
+
 
 class TestTeamFactory:
     def test_traversal_finds(self, db_request):

warehouse/admin/templates/admin/projects/detail.html

@@ -321,6 +321,7 @@ <h3 class="card-title">Project activity</h3>
           <th scope="col">Time</th>
           <th scope="col">IP address</th>
           <th scope="col">Hashed IP address</th>
+          <th scope="col">Location Info</th>
           <th scope="col">Additional information</th>
         </tr>
       </thead>
@@ -331,6 +332,7 @@ <h3 class="card-title">Project activity</h3>
           <td>{{ event.time }}</td>
           <td>{{ event.ip_address }}</td>
           <td>{{ event.ip_address.hashed_ip_address }}</td>
+          <td>{{ event.location_info }}</td>
           <td>{{ event.additional }}</td>
         </tr>
         {% endfor %}

warehouse/admin/templates/admin/users/detail.html

@@ -458,6 +458,7 @@ <h3 class="card-title">Account activity</h3>
                 <th scope="col">Time</th>
                 <th scope="col">IP address</th>
                 <th scope="col">Hashed IP address</th>
+                <th scope="col">Location info</th>
                 <th scope="col">Additional information</th>
               </tr>
             </thead>
@@ -468,6 +469,7 @@ <h3 class="card-title">Account activity</h3>
                 <td>{{ event.time }}</td>
                 <td>{{ event.ip_address }}</td>
                 <td>{{ event.ip_address.hashed_ip_address }}</td>
+                <td>{{ event.location_info }}</td>
                 <td>{{ event.additional }}</td>
               </tr>
               {% endfor %}

warehouse/events/models.py

@@ -13,6 +13,8 @@
 
 import typing
 
+from dataclasses import dataclass
+
 from sqlalchemy import Column, DateTime, ForeignKey, Index, String, orm, sql
 from sqlalchemy.dialects.postgresql import JSONB, UUID
 from sqlalchemy.exc import NoResultFound
@@ -27,6 +29,49 @@
     from pyramid.request import Request
 
 
+@dataclass
+class GeoIPInfo:
+    """
+    Optional values passed in from upstream CDN
+    https://github.com/pypi/infra/blob/3e57592ba91205cb5d10c588d529767b101753cd/terraform/warehouse/vcl/main.vcl#L181-L191
+    """
+
+    city: str | None = None
+    continent_code: str | None = None
+    country_code3: str | None = None
+    country_code: str | None = None
+    country_name: str | None = None
+    region: str | None = None
+
+    @property
+    def _city(self) -> str:
+        return self.city.title() if self.city else ""
+
+    @property
+    def _region(self) -> str:
+        return self.region.upper() if self.region else ""
+
+    @property
+    def _country_code(self) -> str:
+        return self.country_code.upper() if self.country_code else ""
+
+    def display(self) -> str:
+        """
+        Construct a reasonable location, depending on optional values
+        """
+        if self.city and self.region and self.country_code:
+            return f"{self._city}, {self._region}, {self._country_code}"
+        elif self.city and self.region:
+            return f"{self._city}, {self._region}"
+        elif self.city and self.country_code:
+            return f"{self._city}, {self._country_code}"
+        elif self.region:
+            return f"{self._region}, {self._country_code}"
+        elif self.country_name:
+            return self.country_name
+        return ""
+
+
 class Event(AbstractConcreteBase):
     tag = Column(String, nullable=False)
     time = Column(DateTime, nullable=False, server_default=sql.func.now())
@@ -99,6 +144,23 @@ def ip_address(cls, value):  # noqa: N805
             session.add(_ip_address)
         cls.ip_address_obj = _ip_address
 
+    @property
+    def location_info(cls) -> str:  # noqa: N805
+        """
+        Determine "best" location info to display.
+
+        Dig into `.additional` for `geoip_info` and return that if it exists.
+        It was stored at the time opf the event, and may change in the related
+        `IpAddress` object over time.
+        Otherwise, return the `ip_address_obj` and let its repr decide.
+        """
+        if cls.additional is not None and "geoip_info" in cls.additional:
+            g = GeoIPInfo(**cls.additional["geoip_info"])
+            if g.display():
+                return g.display()
+
+        return cls.ip_address_obj
+
     def __init_subclass__(cls, /, parent_class, **kwargs):
         cls._parent_class = parent_class
         return cls

warehouse/locale/messages.pot

@@ -3356,13 +3356,8 @@ msgstr ""
 
 #: warehouse/templates/manage/account.html:685
 #: warehouse/templates/manage/account.html:696
-#: warehouse/templates/manage/organization/history.html:203
 #: warehouse/templates/manage/organization/history.html:215
-#: warehouse/templates/manage/project/history.html:290
-#: warehouse/templates/manage/project/history.html:302
-#: warehouse/templates/manage/team/history.html:110
-#: warehouse/templates/manage/team/history.html:122
-msgid "IP address"
+msgid "Location Info"
 msgstr ""
 
 #: warehouse/templates/manage/account.html:704
@@ -4666,6 +4661,14 @@ msgstr ""
 msgid "Time"
 msgstr ""
 
+#: warehouse/templates/manage/organization/history.html:203
+#: warehouse/templates/manage/project/history.html:290
+#: warehouse/templates/manage/project/history.html:302
+#: warehouse/templates/manage/team/history.html:110
+#: warehouse/templates/manage/team/history.html:122
+msgid "Location info"
+msgstr ""
+
 #: warehouse/templates/manage/organization/manage_organization_base.html:20
 #, python-format
 msgid "Manage '%(organization_name)s'"

warehouse/templates/manage/account.html

@@ -682,7 +682,7 @@ <h2>{% trans %}Security history{% endtrans %}</h2>
       <thead>
         <th scope="col">{% trans %}Event{% endtrans %}</th>
         <th scope="col">{% trans %}Date / time{% endtrans %}</th>
-        <th scope="col">{% trans %}IP address{% endtrans %}</th>
+        <th scope="col">{% trans %}Location Info{% endtrans %}</th>
       </thead>
       <tbody>
         {% for event in recent_events %}
@@ -693,8 +693,8 @@ <h2>{% trans %}Security history{% endtrans %}</h2>
             {{ humanize(event.time, time="true") }}
           </td>
           <td>
-            <span class="table__mobile-label">{% trans %}IP address{% endtrans %}</span>
-            {{ "Redacted" if event.additional.redact_ip else event.ip_address }}
+            <span class="table__mobile-label">{% trans %}Location Info{% endtrans %}</span>
+            {{ "Redacted" if event.additional.redact_ip else event.location_info }}
           </td>
         </tr>
         {% endfor %}

warehouse/templates/manage/organization/history.html

@@ -200,7 +200,7 @@ <h2>{% trans %}Security history{% endtrans %}</h2>
       <tr>
         <th scope="col">{% trans %}Event{% endtrans %}</th>
         <th scope="col">{% trans %}Time{% endtrans %}</th>
-        <th scope="col">{% trans %}IP address{% endtrans %}</th>
+        <th scope="col">{% trans %}Location info{% endtrans %}</th>
       </tr>
     </thead>
     <tbody>
@@ -212,8 +212,8 @@ <h2>{% trans %}Security history{% endtrans %}</h2>
           {{ humanize(event.time, time="true") }}
         </td>
         <td>
-          <span class="table__mobile-label">{% trans %}IP address{% endtrans %}</span>
-          {{ event.ip_address }}
+          <span class="table__mobile-label">{% trans %}Location Info{% endtrans %}</span>
+          {{ event.location_info }}
         </td>
       </tr>
       {% endfor %}

warehouse/templates/manage/project/history.html

@@ -287,7 +287,7 @@ <h2>{% trans %}Security history{% endtrans %}</h2>
       <tr>
         <th scope="col">{% trans %}Event{% endtrans %}</th>
         <th scope="col">{% trans %}Time{% endtrans %}</th>
-        <th scope="col">{% trans %}IP address{% endtrans %}</th>
+        <th scope="col">{% trans %}Location info{% endtrans %}</th>
       </tr>
     </thead>
     <tbody>
@@ -299,8 +299,8 @@ <h2>{% trans %}Security history{% endtrans %}</h2>
           {{ humanize(event.time, time="true") }}
         </td>
         <td>
-          <span class="table__mobile-label">{% trans %}IP address{% endtrans %}</span>
-          {{ event.ip_address }}
+          <span class="table__mobile-label">{% trans %}Location info{% endtrans %}</span>
+          {{ event.location_info }}
         </td>
       </tr>
       {% endfor %}

warehouse/templates/manage/team/history.html

@@ -107,7 +107,7 @@ <h2>{% trans %}Security history{% endtrans %}</h2>
       <tr>
         <th scope="col">{% trans %}Event{% endtrans %}</th>
         <th scope="col">{% trans %}Time{% endtrans %}</th>
-        <th scope="col">{% trans %}IP address{% endtrans %}</th>
+        <th scope="col">{% trans %}Location info{% endtrans %}</th>
       </tr>
     </thead>
     <tbody>
@@ -119,8 +119,8 @@ <h2>{% trans %}Security history{% endtrans %}</h2>
           {{ humanize(event.time, time="true") }}
         </td>
         <td>
-          <span class="table__mobile-label">{% trans %}IP address{% endtrans %}</span>
-          {{ event.ip_address }}
+          <span class="table__mobile-label">{% trans %}Location info{% endtrans %}</span>
+          {{ event.location_info }}
         </td>
       </tr>
       {% endfor %}