From d3d85ade8c20f1fbf850c056bf48ba50c88cd4e6 Mon Sep 17 00:00:00 2001 From: Andrew Williamson Date: Wed, 24 Aug 2022 15:18:28 +0100 Subject: [PATCH] replace geoip lookup with HTTP_X_COUNTRY_CODE request header (#19603) --- Dockerfile | 3 -- Dockerfile.deploy | 3 -- requirements/prod.txt | 6 ---- src/olympia/abuse/models.py | 19 ------------ src/olympia/abuse/serializers.py | 4 +-- src/olympia/abuse/tests/test_models.py | 21 +------------ src/olympia/abuse/tests/test_views.py | 43 +++++++++++++++++++------- src/olympia/lib/settings_base.py | 2 -- 8 files changed, 33 insertions(+), 68 deletions(-) diff --git a/Dockerfile b/Dockerfile index c360ead400..be85d8c7db 100644 --- a/Dockerfile +++ b/Dockerfile @@ -57,9 +57,6 @@ RUN touch /addons-server-docker-container \ librsvg2-bin \ # Use pngcrush to optimize the PNGs uploaded by developers pngcrush \ - # Use libmaxmind for speedy geoip lookups - libmaxminddb0 \ - libmaxminddb-dev \ && rm -rf /var/lib/apt/lists/* # IMPORTANT: When editing one of these lists below, make sure to also update diff --git a/Dockerfile.deploy b/Dockerfile.deploy index f2dacfb467..e5ea95bd37 100644 --- a/Dockerfile.deploy +++ b/Dockerfile.deploy @@ -51,9 +51,6 @@ RUN touch /addons-server-docker-container \ librsvg2-bin \ # Use pngcrush to optimize the PNGs uploaded by developers pngcrush \ - # Use libmaxmind for speedy geoip lookups - libmaxminddb0 \ - libmaxminddb-dev \ && rm -rf /var/lib/apt/lists/* # IMPORTANT: When editing one of these lists below, make sure to also update diff --git a/requirements/prod.txt b/requirements/prod.txt index 2de9e79aac..1032533e60 100644 --- a/requirements/prod.txt +++ b/requirements/prod.txt @@ -51,9 +51,6 @@ MarkupSafe==2.1.1 \ --hash=sha256:efc1913fd2ca4f334418481c7e595c00aad186563bbc1ec76067848c7ca0a933 \ --hash=sha256:f121a1420d4e173a5d96e47e9a0c0dcff965afdf1626d28de1460815f7c4ee7a \ --hash=sha256:fc7b548b17d238737688817ab67deebb30e8073c95749d55538ed473130ec0c7 -# maxminddb is required by geoip2 -maxminddb==2.2.0 \ - --hash=sha256:e37707ec4fab115804670e0fb7aedb4b57075a8b6f80052bdc648d3c005184e5 mysqlclient==2.1.1 \ --hash=sha256:0d1cd3a5a4d28c222fa199002810e8146cffd821410b67851af4cc80aeccd97c \ --hash=sha256:828757e419fb11dd6c5ed2576ec92c3efaa93a0f7c39e263586d1ee779c3d782 \ @@ -347,9 +344,6 @@ feedparser==6.0.10 \ --hash=sha256:79c257d526d13b944e965f6095700587f27388e50ea16fd245babe4dfae7024f filtercascade==0.4.1 \ --hash=sha256:1db4c2f1f628e3e399a6f3437738bdb304eb11521bf37684b56addaa5576a89f -geoip2==4.6.0 \ - --hash=sha256:745e2d7a665072690056f9566774727da89216c7858793f89804f6945f969714 \ - --hash=sha256:f0e80bce80b06bb38bd08bf4877d5a84e354e932095e6ccfb4d27bb598fa4f83 html5lib==1.1 \ --hash=sha256:0d78f8fde1c230e99fe37986a60526d7049ed4bf8a9fadbad5f00e22e58e041d \ --hash=sha256:b2e5b40261e20f354d198eae92afc10d750afb487ed5e50f9c4eaf07c184146f diff --git a/src/olympia/abuse/models.py b/src/olympia/abuse/models.py index 90ebc0e6b4..096ce9e571 100644 --- a/src/olympia/abuse/models.py +++ b/src/olympia/abuse/models.py @@ -1,10 +1,6 @@ -from django import forms -from django.contrib.gis.geoip2 import GeoIP2, GeoIP2Exception -from django.core.validators import validate_ipv46_address from django.db import models from extended_choices import Choices -from geoip2.errors import GeoIP2Error from olympia import amo from olympia.amo.models import BaseQuerySet, ManagerBase, ModelBase @@ -271,21 +267,6 @@ class AbuseReport(ModelBase): # soft-deleted. return self.update(state=self.STATES.DELETED) - @classmethod - def lookup_country_code_from_ip(cls, ip): - try: - # Early check to avoid initializing GeoIP2 on invalid addresses - if not ip: - raise forms.ValidationError('No IP') - validate_ipv46_address(ip) - geoip = GeoIP2() - value = geoip.country_code(ip) - # Annoyingly, we have to catch both django's GeoIP2Exception (setup - # issue) and geoip2's GeoIP2Error (lookup issue) - except (forms.ValidationError, GeoIP2Exception, GeoIP2Error): - value = '' - return value - @property def type(self): if self.guid: diff --git a/src/olympia/abuse/serializers.py b/src/olympia/abuse/serializers.py index 53f49eeb1e..dbf9271286 100644 --- a/src/olympia/abuse/serializers.py +++ b/src/olympia/abuse/serializers.py @@ -29,9 +29,7 @@ class BaseAbuseReportSerializer(serializers.ModelSerializer): def to_internal_value(self, data): output = super().to_internal_value(data) request = self.context['request'] - output['country_code'] = AbuseReport.lookup_country_code_from_ip( - request.META.get('REMOTE_ADDR') - ) + output['country_code'] = request.META.get('HTTP_X_COUNTRY_CODE', '') if request.user.is_authenticated: output['reporter'] = request.user return output diff --git a/src/olympia/abuse/tests/test_models.py b/src/olympia/abuse/tests/test_models.py index d2c6c93db9..45dbc0929e 100644 --- a/src/olympia/abuse/tests/test_models.py +++ b/src/olympia/abuse/tests/test_models.py @@ -1,6 +1,4 @@ -from unittest import mock - -from olympia.abuse.models import AbuseReport, GeoIP2Error, GeoIP2Exception +from olympia.abuse.models import AbuseReport from olympia.amo.tests import addon_factory, TestCase, user_factory @@ -187,23 +185,6 @@ class TestAbuse(TestCase): report = AbuseReport.objects.create(user=user_factory()) assert report.type == 'User' - @mock.patch('olympia.abuse.models.GeoIP2') - def test_lookup_country_code_from_ip(self, GeoIP2_mock): - GeoIP2_mock.return_value.country_code.return_value = 'ZZ' - assert AbuseReport.lookup_country_code_from_ip('') == '' - assert AbuseReport.lookup_country_code_from_ip('notanip') == '' - assert GeoIP2_mock.return_value.country_code.call_count == 0 - - GeoIP2_mock.return_value.country_code.return_value = 'ZZ' - assert AbuseReport.lookup_country_code_from_ip('127.0.0.1') == 'ZZ' - assert AbuseReport.lookup_country_code_from_ip('::1') == 'ZZ' - - GeoIP2_mock.return_value.country_code.side_effect = GeoIP2Exception - assert AbuseReport.lookup_country_code_from_ip('127.0.0.1') == '' - - GeoIP2_mock.return_value.country_code.side_effect = GeoIP2Error - assert AbuseReport.lookup_country_code_from_ip('127.0.0.1') == '' - def test_save_soft_deleted(self): report = AbuseReport.objects.create(guid='@foo') report.delete() diff --git a/src/olympia/abuse/tests/test_views.py b/src/olympia/abuse/tests/test_views.py index e200a79017..472b9b3e1d 100644 --- a/src/olympia/abuse/tests/test_views.py +++ b/src/olympia/abuse/tests/test_views.py @@ -1,8 +1,6 @@ import json from datetime import datetime -from unittest import mock - from olympia import amo from olympia.abuse.models import AbuseReport from olympia.amo.tests import ( @@ -20,17 +18,12 @@ class AddonAbuseViewSetTestBase: def setUp(self): self.url = reverse_ns('abusereportaddon-list') - geoip_patcher = mock.patch('olympia.abuse.models.GeoIP2') - self.GeoIP2_mock = geoip_patcher.start() - self.GeoIP2_mock.return_value.country_code.return_value = 'ZZ' - self.addCleanup(geoip_patcher.stop) def check_reporter(self, report): raise NotImplementedError def check_report(self, report, text): assert str(report) == text - assert report.country_code == 'ZZ' self.check_reporter(report) def test_report_addon_by_id(self): @@ -393,6 +386,22 @@ class AddonAbuseViewSetTestBase: assert report.addon_install_method == (AbuseReport.ADDON_INSTALL_METHODS.OTHER) assert report.addon_install_source == (AbuseReport.ADDON_INSTALL_SOURCES.OTHER) + def test_report_country_code(self): + addon = addon_factory(status=amo.STATUS_NULL) + response = self.client.post( + self.url, + data={'addon': str(addon.guid), 'message': 'abuse!'}, + REMOTE_ADDR='123.45.67.89', + HTTP_X_FORWARDED_FOR=f'123.45.67.89, {get_random_ip()}', + HTTP_X_COUNTRY_CODE='YY', + ) + assert response.status_code == 201 + + assert AbuseReport.objects.filter(guid=addon.guid).exists() + report = AbuseReport.objects.get(guid=addon.guid) + self.check_report(report, 'Abuse Report for Addon %s' % addon.guid) + assert report.country_code == 'YY' + class TestAddonAbuseViewSetLoggedOut(AddonAbuseViewSetTestBase, TestCase): def check_reporter(self, report): @@ -438,17 +447,12 @@ class UserAbuseViewSetTestBase: def setUp(self): self.url = reverse_ns('abusereportuser-list') - geoip_patcher = mock.patch('olympia.abuse.models.GeoIP2') - self.GeoIP2_mock = geoip_patcher.start() - self.GeoIP2_mock.return_value.country_code.return_value = 'ZZ' - self.addCleanup(geoip_patcher.stop) def check_reporter(self, report): raise NotImplementedError def check_report(self, report, text): assert str(report) == text - assert report.country_code == 'ZZ' self.check_reporter(report) def test_report_user_id(self): @@ -517,6 +521,21 @@ class UserAbuseViewSetTestBase: ) assert response.status_code == 429 + def test_report_country_code(self): + user = user_factory() + response = self.client.post( + self.url, + data={'user': str(user.id), 'message': 'abuse!'}, + REMOTE_ADDR='123.45.67.89', + HTTP_X_COUNTRY_CODE='YY', + ) + assert response.status_code == 201 + + assert AbuseReport.objects.filter(user_id=user.id).exists() + report = AbuseReport.objects.get(user_id=user.id) + self.check_report(report, 'Abuse Report for User %s' % user) + assert report.country_code == 'YY' + class TestUserAbuseViewSetLoggedOut(UserAbuseViewSetTestBase, TestCase): def check_reporter(self, report): diff --git a/src/olympia/lib/settings_base.py b/src/olympia/lib/settings_base.py index e7a75df074..7cc8293f98 100644 --- a/src/olympia/lib/settings_base.py +++ b/src/olympia/lib/settings_base.py @@ -1450,8 +1450,6 @@ MOZILLA_NEWLETTER_URL = env( 'MOZILLA_NEWSLETTER_URL', default='https://www.mozilla.org/en-US/newsletter/' ) -GEOIP_PATH = '/usr/local/share/GeoIP/GeoLite2-Country.mmdb' - EXTENSION_WORKSHOP_URL = env( 'EXTENSION_WORKSHOP_URL', default='https://extensionworkshop-dev.allizom.org' )