From 19792c77161f004a10d99e12ea75a8d18764879f Mon Sep 17 00:00:00 2001 From: Marian Beermann Date: Fri, 16 Sep 2016 13:19:02 +0200 Subject: [PATCH 1/8] Add normalize_phone_number and related utilities --- index_service/utils.py | 26 ++++++++++++++++++++ qabel_index/settings.py | 2 +- requirements.txt | 5 ++-- tests/utils.py | 54 ++++++++++++++++++++++++++++++++++++++--- 4 files changed, 80 insertions(+), 7 deletions(-) diff --git a/index_service/utils.py b/index_service/utils.py index 4fdc421..918dd19 100644 --- a/index_service/utils.py +++ b/index_service/utils.py @@ -1,9 +1,35 @@ import random +from django.utils import translation + +import phonenumbers + def short_id(length): CHARSET = 'CDEHKMPRSTUWXY2458' get_character = CHARSET.__getitem__ random_numbers = (random.randrange(len(CHARSET)) for i in range(length)) return ''.join(map(get_character, random_numbers)) + + +def normalize_phone_number(phone_number, fallback_cc): + """ + Return (str) *phone_number* (str) normalized to ITU-T E.164. + + Apply fallback_CC (str/None) country code, if necessary. + """ + try: + phone_number = phonenumbers.parse(phone_number, region=fallback_cc) + except phonenumbers.NumberParseException as exc: + raise ValueError('Unable to parse phone number %r: %s' % (phone_number, exc)) from exc + return phonenumbers.format_number(phone_number, phonenumbers.PhoneNumberFormat.E164) + + +def get_current_cc(language=None): + language = language or translation.get_language() + return language.split('-')[-1].upper() + + +def normalize_phone_number_localised(phone_number): + return normalize_phone_number(phone_number, get_current_cc()) diff --git a/qabel_index/settings.py b/qabel_index/settings.py index 8defecd..4e24dc2 100644 --- a/qabel_index/settings.py +++ b/qabel_index/settings.py @@ -105,7 +105,7 @@ # Internationalization # https://docs.djangoproject.com/en/1.8/topics/i18n/ -LANGUAGE_CODE = 'en-us' +LANGUAGE_CODE = 'de-de' TIME_ZONE = 'UTC' diff --git a/requirements.txt b/requirements.txt index abc7a2f..b3aaf3f 100644 --- a/requirements.txt +++ b/requirements.txt @@ -7,8 +7,6 @@ django-mail-templated==2.6.2 django-sendsms==0.2.3 twilio==5.4.0 flake8==2.5.4 -mccabe==0.4.0 -pep8==1.7.0 psycopg2==2.6.1 py==1.4.31 pyflakes==1.0.0 @@ -25,4 +23,5 @@ pprintpp==0.2.3 # We grab one bag of crypto from pynacl, which gives us the Curve25519 primitives pynacl==1.0.1 # We grab another bag of crypto from cryptography, which gives us AES-GCM and others -cryptography==1.4 \ No newline at end of file +cryptography==1.4 +phonenumbers==7.6.0 \ No newline at end of file diff --git a/tests/utils.py b/tests/utils.py index 1ebabb3..7d99bfd 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -1,9 +1,57 @@ -from index_service import utils +from django.utils import translation + +import pytest + +from index_service.utils import short_id, normalize_phone_number, get_current_cc def test_short_id(): - id = utils.short_id(5) + id = short_id(5) assert len(id) == 5 - other_id = utils.short_id(5) + other_id = short_id(5) assert other_id != id # failure probability: 18**-5 ~ 529 ppb + + +class NormalizePhoneNumberTest: + def test_no_fallback_failing(self): + with pytest.raises(ValueError): + normalize_phone_number('1234 / 5678', None) # No CC, no fallback + + def test_no_fallback(self): + number = normalize_phone_number('+49 1234-5678', None) + assert number == '+4912345678' + + @pytest.mark.parametrize('input, fallback_cc, output', ( + ('0010 1234', 'DE', '+101234'), + ('0010 1234', 'AZ', '+101234'), + ('001110 1234', 'AU', '+101234'), + )) + def test_outgoing_international(self, input, fallback_cc, output): + assert normalize_phone_number(input, fallback_cc) == output + + def test_outgoing_international_no_fallback(self): + with pytest.raises(ValueError): + normalize_phone_number('0010 1234', None) + + def test_fallback_differs(self): + assert normalize_phone_number('+10 1234', 'DE') == '+101234' # DE = +49 + + def test_fallback(self): + assert normalize_phone_number('1234', 'DE') == '+491234' + + def test_fallback_capitalization(self): + with pytest.raises(ValueError): + assert normalize_phone_number('1234', 'de') == '+491234' + + +class GetCurrentCcTest: + def test_from_default(self): + assert get_current_cc() == 'DE' + + def test_explicit(self): + assert get_current_cc('en-us') == 'US' + + def test_activated(self): + with translation.override('en-au'): + assert get_current_cc() == 'AU' From a8cc435cc60cc466e22cda14590022fe6e1241d8 Mon Sep 17 00:00:00 2001 From: Marian Beermann Date: Fri, 16 Sep 2016 13:19:05 +0200 Subject: [PATCH 2/8] Scrub phone numbers --- index_service/serializers.py | 23 ++++++++++++++++++- index_service/views.py | 6 ++++- qabel_index/settings.py | 12 ++++++++++ tests/api.py | 43 ++++++++++++++++++++++++++++++++++++ tests/serializers.py | 33 +++++++++++++++++++++++++++ 5 files changed, 115 insertions(+), 2 deletions(-) diff --git a/index_service/serializers.py b/index_service/serializers.py index 69e36d9..a4f94eb 100644 --- a/index_service/serializers.py +++ b/index_service/serializers.py @@ -1,11 +1,24 @@ from rest_framework import serializers -from django.utils import timezone from rest_framework.exceptions import ValidationError from rest_framework.validators import UniqueTogetherValidator from index_service.logic import UpdateRequest, UpdateItem from index_service.models import Identity, Entry from index_service.crypto import decode_key +from index_service.utils import normalize_phone_number_localised + + +FIELD_SCRUBBERS = { + 'phone': normalize_phone_number_localised +} + + +def scrub_field(field, value): + scrubber = FIELD_SCRUBBERS.get(field) + if scrubber: + return scrubber(value) + else: + return value class IdentitySerializer(serializers.ModelSerializer): @@ -45,6 +58,14 @@ class UpdateItemSerializer(serializers.Serializer): field = serializers.ChoiceField(Entry.FIELDS) value = serializers.CharField() + def validate(self, data): + field = data['field'] + try: + data['value'] = scrub_field(field, data['value']) + except ValueError as exc: + raise serializers.ValidationError('Scrubber for %r failed: %s' % (field, exc)) from exc + return data + def create(self, validated_data): return UpdateItem(action=validated_data['action'], field=validated_data['field'], diff --git a/index_service/views.py b/index_service/views.py index 89df0c9..f678bcc 100644 --- a/index_service/views.py +++ b/index_service/views.py @@ -9,7 +9,7 @@ from .crypto import NoiseBoxParser, KeyPair, encode_key from .models import Identity, Entry, PendingUpdateRequest, PendingVerification -from .serializers import IdentitySerializer, UpdateRequestSerializer +from .serializers import IdentitySerializer, UpdateRequestSerializer, scrub_field from .verification import execute_if_complete @@ -67,6 +67,10 @@ def search(request, format=None): if not data or set(data.keys()) > Entry.FIELDS: return error('No or unknown fields specified: ' + ', '.join(data.keys())) for field, value in data.items(): + try: + value = scrub_field(field, value) + except ValueError as exc: + return error('Failed to parse field %r: %s' % (field, exc)) identities = identities.filter(entry__field=field, entry__value=value) return Response({'identities': IdentitySerializer(identities, many=True).data}) diff --git a/qabel_index/settings.py b/qabel_index/settings.py index 4e24dc2..4eab98f 100644 --- a/qabel_index/settings.py +++ b/qabel_index/settings.py @@ -53,6 +53,7 @@ 'django.contrib.messages.middleware.MessageMiddleware', 'django.middleware.clickjacking.XFrameOptionsMiddleware', 'django.middleware.security.SecurityMiddleware', + 'django.middleware.locale.LocaleMiddleware', ) ROOT_URLCONF = 'qabel_index.urls' @@ -105,7 +106,18 @@ # Internationalization # https://docs.djangoproject.com/en/1.8/topics/i18n/ + +from django.utils.translation import ugettext_lazy as _ + LANGUAGE_CODE = 'de-de' +LANGUAGES = ( + ('de', _('German')), + # The Django docs are wrong. If you want a sublang, you absolutely need to specifiy it in LANGUAGES, + # and the default does not include it. Yikes. + # Note that this cannot be used to ocntrol which country codes we allow for phone number registration, + # since this only affects scrubbing of phone numbers passed into the system *without* a country code. + ('en-us', _('US English')), +) TIME_ZONE = 'UTC' diff --git a/tests/api.py b/tests/api.py index d5da415..e7f172d 100644 --- a/tests/api.py +++ b/tests/api.py @@ -76,6 +76,49 @@ def test_create(self, api_client, mocker, simple_identity): response = api_client.put(self.path, request, content_type='application/json') assert response.status_code == status.HTTP_204_NO_CONTENT + @pytest.mark.parametrize('accept_language', ( + 'de-de', # an enabled language, also the default + 'ko-kr', # best korea + None, # no header set + )) + @pytest.mark.parametrize('phone_number, search_number', ( + ('+661234', '+661234'), + ('1234', '+491234'), + )) + def test_create_phone_normalization(self, api_client, mocker, simple_identity, phone_number, accept_language, search_number): + self._test_create_phone(api_client, mocker, simple_identity, phone_number, accept_language, search_number) + + @pytest.mark.parametrize('phone_number, accept_language, search_number', ( + ('555', 'en-us', '+1555'), + )) + def test_create_phone(self, api_client, mocker, simple_identity, phone_number, accept_language, search_number): + self._test_create_phone(api_client, mocker, simple_identity, phone_number, accept_language, search_number) + + def _test_create_phone(self, api_client, mocker, simple_identity, phone_number, accept_language, search_number): + request = json.dumps({ + 'identity': simple_identity, + 'items': [ + { + 'action': 'create', + 'field': 'phone', + 'value': phone_number, + } + ] + }) + # Short-cut verification to execution + mocker.patch.object(UpdateRequest, 'start_verification', lambda self, *_: self.execute()) + kwargs = {} + if accept_language: + kwargs['HTTP_ACCEPT_LANGUAGE'] = accept_language + response = api_client.put(self.path, request, content_type='application/json', **kwargs) + assert response.status_code == status.HTTP_204_NO_CONTENT, response.json() + response = api_client.get(SearchTest.path, {'phone': search_number}) + assert response.status_code == status.HTTP_200_OK, response.json() + result = response.data['identities'] + assert len(result) == 1 + assert result[0]['alias'] == 'public alias' + assert result[0]['drop_url'] == 'http://example.com' + @pytest.fixture def delete_prerequisite(self, api_client, email_entry): # Maybe use pytest-bdd here? diff --git a/tests/serializers.py b/tests/serializers.py index bb44f08..9ccc2db 100644 --- a/tests/serializers.py +++ b/tests/serializers.py @@ -1,4 +1,6 @@ +from django.utils import translation + import pytest from index_service.serializers import IdentitySerializer, UpdateRequestSerializer, UpdateItemSerializer @@ -67,6 +69,37 @@ def test_simple_item(): assert item.value == 'asdf@example.com' +def test_phone_item(): + serializer = UpdateItemSerializer(data=make_update_item('phone', '1234')) + assert serializer.is_valid(), serializer.errors + item = serializer.save() + assert item.field == 'phone' + assert item.value == '+491234' + + +def test_phone_item_international(): + serializer = UpdateItemSerializer(data=make_update_item('phone', '+631234')) + assert serializer.is_valid(), serializer.errors + item = serializer.save() + assert item.field == 'phone' + assert item.value == '+631234' + + +@pytest.mark.parametrize('locale, input, output', ( + ('en-us', '555-1234', '+15551234'), + # Yes, the US actually has "1" as their country calling code. + ('ky-kg', '555-1234', '+9965551234'), + # Kyrgyzstan on the other hand has 996. Priorities, nuclear super power etc. +)) +def test_phone_item_international_request(locale, input, output): + with translation.override(locale): + serializer = UpdateItemSerializer(data=make_update_item('phone', input)) + assert serializer.is_valid(), serializer.errors + item = serializer.save() + assert item.field == 'phone' + assert item.value == output + + @pytest.mark.parametrize('invalid', [ {}, { From 2938da1559fc0b8937f307697555709fd16bf5f3 Mon Sep 17 00:00:00 2001 From: Marian Beermann Date: Fri, 16 Sep 2016 13:19:06 +0200 Subject: [PATCH 3/8] SMS_ALLOWED_COUNTRIES --- defaults.yaml | 6 ++++++ index_service/serializers.py | 8 +++++++- index_service/utils.py | 12 ++++++++---- qabel_index/settings.py | 6 +++++- tests/serializers.py | 13 +++++++++++++ 5 files changed, 39 insertions(+), 6 deletions(-) diff --git a/defaults.yaml b/defaults.yaml index eb85e01..235fdaa 100644 --- a/defaults.yaml +++ b/defaults.yaml @@ -50,6 +50,12 @@ qabel: # This setting doesn't matter for the dummy backends. SENDSMS_DEFAULT_FROM_PHONE: '+15005550006' + # Configure countries allowed for phone number registration. Check the applicable charges before + # adding a country code. + # https://en.wikipedia.org/wiki/List_of_country_calling_codes + SMS_ALLOWED_COUNTRIES: + - 49 + uwsgi: processes: 2 http-socket: :9698 diff --git a/index_service/serializers.py b/index_service/serializers.py index a4f94eb..1332b65 100644 --- a/index_service/serializers.py +++ b/index_service/serializers.py @@ -1,3 +1,5 @@ +from django.conf import settings + from rest_framework import serializers from rest_framework.exceptions import ValidationError from rest_framework.validators import UniqueTogetherValidator @@ -5,7 +7,7 @@ from index_service.logic import UpdateRequest, UpdateItem from index_service.models import Identity, Entry from index_service.crypto import decode_key -from index_service.utils import normalize_phone_number_localised +from index_service.utils import normalize_phone_number_localised, parse_phone_number, get_current_cc FIELD_SCRUBBERS = { @@ -64,6 +66,10 @@ def validate(self, data): data['value'] = scrub_field(field, data['value']) except ValueError as exc: raise serializers.ValidationError('Scrubber for %r failed: %s' % (field, exc)) from exc + if field == 'phone': + country_code = parse_phone_number(data['value'], get_current_cc()).country_code + if country_code not in settings.SMS_ALLOWED_COUNTRIES: + raise serializers.ValidationError('This country code (+%d) is not available at this time.' % country_code) return data def create(self, validated_data): diff --git a/index_service/utils.py b/index_service/utils.py index 918dd19..cc2f3bd 100644 --- a/index_service/utils.py +++ b/index_service/utils.py @@ -13,16 +13,20 @@ def short_id(length): return ''.join(map(get_character, random_numbers)) +def parse_phone_number(phone_number, fallback_cc): + try: + return phonenumbers.parse(phone_number, region=fallback_cc) + except phonenumbers.NumberParseException as exc: + raise ValueError('Unable to parse phone number %r: %s' % (phone_number, exc)) from exc + + def normalize_phone_number(phone_number, fallback_cc): """ Return (str) *phone_number* (str) normalized to ITU-T E.164. Apply fallback_CC (str/None) country code, if necessary. """ - try: - phone_number = phonenumbers.parse(phone_number, region=fallback_cc) - except phonenumbers.NumberParseException as exc: - raise ValueError('Unable to parse phone number %r: %s' % (phone_number, exc)) from exc + phone_number = parse_phone_number(phone_number, fallback_cc) return phonenumbers.format_number(phone_number, phonenumbers.PhoneNumberFormat.E164) diff --git a/qabel_index/settings.py b/qabel_index/settings.py index 4eab98f..0734098 100644 --- a/qabel_index/settings.py +++ b/qabel_index/settings.py @@ -114,7 +114,7 @@ ('de', _('German')), # The Django docs are wrong. If you want a sublang, you absolutely need to specifiy it in LANGUAGES, # and the default does not include it. Yikes. - # Note that this cannot be used to ocntrol which country codes we allow for phone number registration, + # Note that this cannot be used to control which country codes we allow for phone number registration, # since this only affects scrubbing of phone numbers passed into the system *without* a country code. ('en-us', _('US English')), ) @@ -143,5 +143,9 @@ SENDSMS_DEFAULT_FROM_PHONE = '+15005550006' +SMS_ALLOWED_COUNTRIES = ( + 49, 1, 63, 66, 996 +) + # Enable shallow verification, i.e. do not confirm via verification mails or SMSes. FACET_SHALLOW_VERIFICATION = False diff --git a/tests/serializers.py b/tests/serializers.py index 9ccc2db..21bf4b8 100644 --- a/tests/serializers.py +++ b/tests/serializers.py @@ -100,6 +100,19 @@ def test_phone_item_international_request(locale, input, output): assert item.value == output +def test_phone_item_international_not_allowed(): + serializer = UpdateItemSerializer(data=make_update_item('phone', '+641234')) + assert not serializer.is_valid(), serializer.errors + assert 'is not available at this time' in str(serializer.errors) + + +def test_phone_item_international_request_not_allowed(): + with translation.override('ht_HT'): + serializer = UpdateItemSerializer(data=make_update_item('phone', '1234')) + assert not serializer.is_valid(), serializer.errors + assert 'is not available at this time' in str(serializer.errors) + + @pytest.mark.parametrize('invalid', [ {}, { From 0087141d63daab5f152059b4362a1cf7a85ce634 Mon Sep 17 00:00:00 2001 From: Marian Beermann Date: Fri, 16 Sep 2016 13:19:06 +0200 Subject: [PATCH 4/8] Refactor some API tests --- tests/api.py | 55 +++++++++++++++++++++++++--------------------------- 1 file changed, 26 insertions(+), 29 deletions(-) diff --git a/tests/api.py b/tests/api.py index e7f172d..97bcd1f 100644 --- a/tests/api.py +++ b/tests/api.py @@ -60,22 +60,33 @@ def test_match_is_exact(self, api_client, email_entry): class UpdateTest: path = '/api/v0/update/' - def test_create(self, api_client, mocker, simple_identity): + def _update_request_with_no_verification(self, api_client, mocker, simple_identity, items, **kwargs): request = json.dumps({ 'identity': simple_identity, - 'items': [ - { - 'action': 'create', - 'field': 'email', - 'value': 'onlypeople_who_knew_this_address_already_can_find_the_entry@example.com', - } - ] + 'items': items }) # Short-cut verification to execution mocker.patch.object(UpdateRequest, 'start_verification', lambda self, *_: self.execute()) - response = api_client.put(self.path, request, content_type='application/json') + response = api_client.put(self.path, request, content_type='application/json', **kwargs) assert response.status_code == status.HTTP_204_NO_CONTENT + def _search(self, api_client, what): + response = api_client.get(SearchTest.path, what) + assert response.status_code == status.HTTP_200_OK, response.json() + result = response.data['identities'] + assert len(result) == 1 + assert result[0]['alias'] == 'public alias' + assert result[0]['drop_url'] == 'http://example.com' + + def test_create(self, api_client, mocker, simple_identity): + email = 'onlypeople_who_knew_this_address_already_can_find_the_entry@example.com' + self._update_request_with_no_verification(api_client, mocker, simple_identity, [{ + 'action': 'create', + 'field': 'email', + 'value': email, + }]) + self._search(api_client, {'email': email}) + @pytest.mark.parametrize('accept_language', ( 'de-de', # an enabled language, also the default 'ko-kr', # best korea @@ -95,29 +106,15 @@ def test_create_phone(self, api_client, mocker, simple_identity, phone_number, a self._test_create_phone(api_client, mocker, simple_identity, phone_number, accept_language, search_number) def _test_create_phone(self, api_client, mocker, simple_identity, phone_number, accept_language, search_number): - request = json.dumps({ - 'identity': simple_identity, - 'items': [ - { - 'action': 'create', - 'field': 'phone', - 'value': phone_number, - } - ] - }) - # Short-cut verification to execution - mocker.patch.object(UpdateRequest, 'start_verification', lambda self, *_: self.execute()) kwargs = {} if accept_language: kwargs['HTTP_ACCEPT_LANGUAGE'] = accept_language - response = api_client.put(self.path, request, content_type='application/json', **kwargs) - assert response.status_code == status.HTTP_204_NO_CONTENT, response.json() - response = api_client.get(SearchTest.path, {'phone': search_number}) - assert response.status_code == status.HTTP_200_OK, response.json() - result = response.data['identities'] - assert len(result) == 1 - assert result[0]['alias'] == 'public alias' - assert result[0]['drop_url'] == 'http://example.com' + self._update_request_with_no_verification(api_client, mocker, simple_identity, [{ + 'action': 'create', + 'field': 'phone', + 'value': phone_number, + }], **kwargs) + self._search(api_client, {'phone': search_number}) @pytest.fixture def delete_prerequisite(self, api_client, email_entry): From 2ed717104a31952d6af69efe38a8cca616203c2b Mon Sep 17 00:00:00 2001 From: Marian Beermann Date: Fri, 16 Sep 2016 13:19:06 +0200 Subject: [PATCH 5/8] Add prometheus stats --- index_service/models.py | 10 ++++++---- qabel_index/settings.py | 5 +++++ qabel_index/urls.py | 3 +++ requirements.txt | 3 ++- tests/api.py | 6 ++++++ 5 files changed, 22 insertions(+), 5 deletions(-) diff --git a/index_service/models.py b/index_service/models.py index c4b4e0e..a2b48c4 100644 --- a/index_service/models.py +++ b/index_service/models.py @@ -5,10 +5,12 @@ from django.db import models from django.utils import timezone +from django_prometheus.models import ExportModelOperationsMixin + from .utils import short_id -class Identity(models.Model): +class Identity(ExportModelOperationsMixin('Identity'), models.Model): """ An identity, composed of the public key, drop URL and alias. @@ -33,7 +35,7 @@ def __repr__(self): return u'alias: {0} public_key: {1}'.format(self.alias, repr(self.public_key)) -class Entry(models.Model): +class Entry(ExportModelOperationsMixin('Entry'), models.Model): """ An Entry connects a piece of private data (email, phone, ...) to an identity. @@ -57,7 +59,7 @@ class Meta: index_together = ('field', 'value') -class PendingUpdateRequest(models.Model): +class PendingUpdateRequest(ExportModelOperationsMixin('PendingUpdateRequest'), models.Model): """ Pending update request: When additional user-asynchronous authorization is required a request has to be stored in the database (and all verifications have to complete) before it can be executed. @@ -94,7 +96,7 @@ def is_expired(self): return False -class PendingVerification(models.Model): +class PendingVerification(ExportModelOperationsMixin('PendingVerification'), models.Model): """ A pending verification, e.g. a confirmation mail or SMS that has not been acted upon yet. """ diff --git a/qabel_index/settings.py b/qabel_index/settings.py index 0734098..ffd72eb 100644 --- a/qabel_index/settings.py +++ b/qabel_index/settings.py @@ -38,6 +38,7 @@ 'django.contrib.sessions', 'django.contrib.messages', 'django.contrib.staticfiles', + 'django_prometheus', 'mail_templated', 'rest_framework', 'sendsms', @@ -45,6 +46,7 @@ ) MIDDLEWARE_CLASSES = ( + 'django_prometheus.middleware.PrometheusBeforeMiddleware', 'django.contrib.sessions.middleware.SessionMiddleware', 'django.middleware.common.CommonMiddleware', 'django.middleware.csrf.CsrfViewMiddleware', @@ -54,6 +56,7 @@ 'django.middleware.clickjacking.XFrameOptionsMiddleware', 'django.middleware.security.SecurityMiddleware', 'django.middleware.locale.LocaleMiddleware', + 'django_prometheus.middleware.PrometheusAfterMiddleware', ) ROOT_URLCONF = 'qabel_index.urls' @@ -136,6 +139,8 @@ # Application configuration +PROMETHEUS_EXPORT_MIGRATIONS = False + # Pending update requests expire after this time interval PENDING_REQUEST_MAX_AGE = datetime.timedelta(days=3) diff --git a/qabel_index/urls.py b/qabel_index/urls.py index 44db46f..ecc1db0 100644 --- a/qabel_index/urls.py +++ b/qabel_index/urls.py @@ -1,6 +1,8 @@ from django.conf.urls import include, url from django.contrib import admin +import django_prometheus.urls + from index_service import views from index_service import verification @@ -21,4 +23,5 @@ url(r'^admin/', include(admin.site.urls)), url(r'^api/v0/', include(rest_urls)), url(r'^verify/', include(verification_urls)), + url('', include(django_prometheus.urls)), ] diff --git a/requirements.txt b/requirements.txt index b3aaf3f..242234b 100644 --- a/requirements.txt +++ b/requirements.txt @@ -24,4 +24,5 @@ pprintpp==0.2.3 pynacl==1.0.1 # We grab another bag of crypto from cryptography, which gives us AES-GCM and others cryptography==1.4 -phonenumbers==7.6.0 \ No newline at end of file +phonenumbers==7.6.0 +git+https://github.com/enkore/django-prometheus@34191053cc8beadd51f0b277ff28a329633de261 diff --git a/tests/api.py b/tests/api.py index 97bcd1f..b054325 100644 --- a/tests/api.py +++ b/tests/api.py @@ -213,3 +213,9 @@ def test_encrypted_failure(self, api_client, settings): encrypted_json = bytes.fromhex('cc0330af7d17d21a58f3c277897b1290405960') response = api_client.put(self.path, encrypted_json, content_type='application/vnd.qabel.noisebox+json') assert response.status_code == status.HTTP_400_BAD_REQUEST + + +def test_prometheus_metrics(api_client): + response = api_client.get('/metrics') + assert response.status_code == 200 + assert b'django_http_requests_latency_seconds' in response.content \ No newline at end of file From e4026d093fa569d6d596db8a2e76e17eb8fd158f Mon Sep 17 00:00:00 2001 From: Marian Beermann Date: Fri, 16 Sep 2016 13:19:08 +0200 Subject: [PATCH 6/8] Add optional authorization for all APIs --- defaults.yaml | 7 ++++ index_service/utils.py | 90 +++++++++++++++++++++++++++++++++++++++++ index_service/views.py | 6 +++ qabel_index/settings.py | 4 ++ requirements.txt | 1 + tests/api.py | 37 ++++++++++++++++- tests/utils.py | 49 +++++++++++++++++++++- 7 files changed, 191 insertions(+), 3 deletions(-) diff --git a/defaults.yaml b/defaults.yaml index 235fdaa..cce67b8 100644 --- a/defaults.yaml +++ b/defaults.yaml @@ -5,6 +5,13 @@ qabel: ALLOWED_HOSTS: - '*' + # Any and all operations on the index server can be made to require authorization. + # The old behaviour is to not require it. + REQUIRE_AUTHORIZATION: False + # If REQUIRE_AUTHORIZATION is set, then the APISECRET and URL must also be set. + # ACCOUNTING_APISECRET: '1234' + # ACCOUNTING_URL: 'https://accs04.shard5.hypercloud.in.4d.example.net/' + # SECURITY WARNING: keep the secret key used in production secret! SECRET_KEY: '=tmcici-p92_^_jih9ud11#+wb7*i21firlrtcqh$p+d7o*49@' diff --git a/index_service/utils.py b/index_service/utils.py index cc2f3bd..7e8f0bc 100644 --- a/index_service/utils.py +++ b/index_service/utils.py @@ -1,8 +1,14 @@ +import functools +import logging import random +from django.conf import settings +from django.http import JsonResponse from django.utils import translation +import requests + import phonenumbers @@ -37,3 +43,87 @@ def get_current_cc(language=None): def normalize_phone_number_localised(phone_number): return normalize_phone_number(phone_number, get_current_cc()) + + +logger = logging.getLogger('index_service.utils.authorization') + + +def check_authorization(request): + if not settings.REQUIRE_AUTHORIZATION: + reason = 'No authorization required, none checked.' + logger.info('Request is authorized: %s', reason) + return True, reason + auth_header = request.META.get('HTTP_AUTHORIZATION') + if not auth_header: + reason = 'No authorization supplied.' + logger.warning('Request is unauthorized: %s', reason) + return False, reason + acked, reason = AccountingAuthorization().check(auth_header) + if not acked: + logger.warning('Request is unauthorized: %s', reason) + return False, reason + logger.info('Request is authorized: %s', reason) + return True, reason + + +class AccountingAuthorization: + def endpoint_url(self): + return settings.ACCOUNTING_URL + '/api/v0/internal/user/' + + def headers(self): + return { + 'APISECRET': settings.ACCOUNTING_APISECRET, + } + + def check_response(self, response): + code = response.status_code + if code == 404: + return False, 'User not found.' + if code != 200: + logger.warning('Failed accounting request was: status=%d, response=\n%s', code, response.content) + try: + reason = response.json()['error'] + except: + reason = 'Unknown.' + return False, reason + + try: + json = response.json() + except ValueError: + logger.exception('Invalid JSON in reponse from accounting server: %s', response.content) + return False, 'Invalid response.' + + try: + active = json['active'] + user_id = json['user_id'] + except KeyError: + logger.exception('Unable to parse accounting server response: %s', json) + return False, 'Invalid response.' + + logger.info('Acknowledged token of user ID %s (active=%s)', user_id, active) + if not active: + return False, 'Account is disabled.' + return True, '' + + def check(self, authorization, session=requests): + """Check supplied authorization, return (authorized, public-reason).""" + json = { + 'auth': authorization, + } + try: + response = session.post(self.endpoint_url(), headers=self.headers(), json=json) + except requests.RequestException: + logger.exception('Accounting server request failed:') + return False, 'Accounting server unreachable.' + return self.check_response(response) + + +def authorized_api(view): + """View decorator that enforces authorization, if enabled.""" + @functools.wraps(view) + def wrapper(request, format=None): + authorized, reason = check_authorization(request) + if not authorized: + return JsonResponse({'error': reason}, status=403) + return view(request, format) + return wrapper diff --git a/index_service/views.py b/index_service/views.py index f678bcc..7e4b5f3 100644 --- a/index_service/views.py +++ b/index_service/views.py @@ -1,6 +1,7 @@ from django.conf import settings from django.db import transaction +from django.utils.decorators import method_decorator from rest_framework.decorators import api_view from rest_framework.parsers import JSONParser from rest_framework.response import Response @@ -11,6 +12,7 @@ from .models import Identity, Entry, PendingUpdateRequest, PendingVerification from .serializers import IdentitySerializer, UpdateRequestSerializer, scrub_field from .verification import execute_if_complete +from .utils import authorized_api """ @@ -28,6 +30,7 @@ def error(description): @api_view(('GET',)) +@authorized_api def api_root(request, format=None): """ Return mapping of API names to API endpoint paths. @@ -48,6 +51,7 @@ def root_index(*apis): @api_view(('GET',)) +@authorized_api def key(request, format=None): """ Return the ephemeral server public key. @@ -58,6 +62,7 @@ def key(request, format=None): @api_view(('GET',)) +@authorized_api def search(request, format=None): """ Search for identities registered for private data. @@ -75,6 +80,7 @@ def search(request, format=None): return Response({'identities': IdentitySerializer(identities, many=True).data}) +@method_decorator(authorized_api, 'dispatch') class UpdateView(APIView): """ Atomically create or delete entries in the user directory. diff --git a/qabel_index/settings.py b/qabel_index/settings.py index ffd72eb..668a2d0 100644 --- a/qabel_index/settings.py +++ b/qabel_index/settings.py @@ -141,6 +141,10 @@ PROMETHEUS_EXPORT_MIGRATIONS = False +REQUIRE_AUTHORIZATION = False +ACCOUNTING_APISECRET = '1234' +ACCOUNTING_URL = 'http://localhost:1234' + # Pending update requests expire after this time interval PENDING_REQUEST_MAX_AGE = datetime.timedelta(days=3) diff --git a/requirements.txt b/requirements.txt index 242234b..c3a084b 100644 --- a/requirements.txt +++ b/requirements.txt @@ -26,3 +26,4 @@ pynacl==1.0.1 cryptography==1.4 phonenumbers==7.6.0 git+https://github.com/enkore/django-prometheus@34191053cc8beadd51f0b277ff28a329633de261 +requests==2.11.1 diff --git a/tests/api.py b/tests/api.py index b054325..1997c04 100644 --- a/tests/api.py +++ b/tests/api.py @@ -10,6 +10,7 @@ from index_service.crypto import decode_key from index_service.models import Entry from index_service.logic import UpdateRequest +from index_service.utils import AccountingAuthorization class RootTest: @@ -19,8 +20,10 @@ def test_root(self, api_client): class KeyTest: + path = '/api/v0/key/' + def test_get_key(self, api_client): - response = api_client.get('/api/v0/key/') + response = api_client.get(self.path) assert response.status_code == status.HTTP_200_OK decode_key(response.data['public_key']) # The public key is ephemeral (generated when the server starts); can't really check much else. @@ -218,4 +221,34 @@ def test_encrypted_failure(self, api_client, settings): def test_prometheus_metrics(api_client): response = api_client.get('/metrics') assert response.status_code == 200 - assert b'django_http_requests_latency_seconds' in response.content \ No newline at end of file + assert b'django_http_requests_latency_seconds' in response.content + + +class AuthorizationTest: + APIS = ( + KeyTest.path, + SearchTest.path, + UpdateTest.path, + ) + + @pytest.fixture(autouse=True) + def require_authorization(self, settings): + settings.REQUIRE_AUTHORIZATION = True + + @pytest.mark.parametrize('api', APIS) + def test_no_header(self, api_client, api): + response = api_client.get(api) + assert response.status_code == 403 + assert response.json()['error'] == 'No authorization supplied.' + + @pytest.mark.parametrize('api', APIS) + def test_with_invalid_header(self, api_client, api): + response = api_client.get(api, HTTP_AUTHORIZATION='Token 567') + assert response.status_code == 403 + assert response.json()['error'] == 'Accounting server unreachable.' + + @pytest.mark.parametrize('api', APIS) + def test_valid(self, mocker, api_client, api): + mocker.patch.object(AccountingAuthorization, 'check', lambda self, authorization: (authorization.startswith('Token'), 'All is well')) + response = api_client.get(api, HTTP_AUTHORIZATION='Token 567') + assert response.status_code != 403 # It'll usually be no valid request, but it should be authorized. diff --git a/tests/utils.py b/tests/utils.py index 7d99bfd..578333c 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -1,9 +1,11 @@ +from unittest.mock import MagicMock + from django.utils import translation import pytest -from index_service.utils import short_id, normalize_phone_number, get_current_cc +from index_service.utils import short_id, normalize_phone_number, get_current_cc, AccountingAuthorization def test_short_id(): @@ -55,3 +57,48 @@ def test_explicit(self): def test_activated(self): with translation.override('en-au'): assert get_current_cc() == 'AU' + + +class AccountingAuthorizationTest: + def test_headers(self, settings): + aa = AccountingAuthorization() + settings.ACCOUNTING_APISECRET = 'foo und bar' + headers = aa.headers() + assert len(headers) == 1 + assert headers['APISECRET'] == 'foo und bar' + + def test_endpoint_url(self, settings): + aa = AccountingAuthorization() + settings.ACCOUNTING_URL = 'gopher://accounting.plan9/foo' + assert aa.endpoint_url() == 'gopher://accounting.plan9/foo/api/v0/internal/user/' + + def test_check(self): + aa = AccountingAuthorization() + session = MagicMock() + token = 'Token 1234' + ok, reason = aa.check(token, session) + assert not ok + json = { + 'auth': token, + } + session.post.assert_called_once_with(aa.endpoint_url(), headers=aa.headers(), json=json) + + @pytest.mark.parametrize('status_code, json, ok, reason', ( + (200, {'user_id': 5, 'active': True}, True, None), + (200, {'user_id': 5, 'active': False}, False, 'Account is disabled.'), + (200, {'active': False}, False, 'Invalid response.'), + (200, {'user_id': 5}, False, 'Invalid response.'), + (200, {}, False, 'Invalid response.'), + (404, {}, False, 'User not found.'), + (400, {'user_id': 5, 'active': True}, False, 'Unknown.'), # wrong status code but proper JSON shall fail + (400, {'error': 'the foo did not bar'}, False, 'the foo did not bar'), + )) + def test_check_response(self, status_code, json, ok, reason): + aa = AccountingAuthorization() + response = MagicMock() + response.status_code = status_code + response.json.return_value = json + result, result_reason = aa.check_response(response) + assert result is ok + if reason: + assert result_reason == reason From d10d090379e044f41ef0e21bec6dd13d2a0752b5 Mon Sep 17 00:00:00 2001 From: Marian Beermann Date: Fri, 16 Sep 2016 13:21:26 +0200 Subject: [PATCH 7/8] Fix requirements --- requirements.txt | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/requirements.txt b/requirements.txt index c3a084b..da3482e 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,6 +1,5 @@ Babel==2.3.4 Django==1.9.6 -django-phonenumber-field==1.1.0 django-rest-auth==0.7.0 djangorestframework==3.3.3 django-mail-templated==2.6.2 @@ -25,5 +24,5 @@ pynacl==1.0.1 # We grab another bag of crypto from cryptography, which gives us AES-GCM and others cryptography==1.4 phonenumbers==7.6.0 -git+https://github.com/enkore/django-prometheus@34191053cc8beadd51f0b277ff28a329633de261 +git+https://github.com/enkore/django-prometheus@1a400d4 requests==2.11.1 From e26dc9ea39ba01b208d261a953ee9b322293293f Mon Sep 17 00:00:00 2001 From: Marian Beermann Date: Fri, 16 Sep 2016 13:27:11 +0200 Subject: [PATCH 8/8] clarify wording on REQUIRE_AUTHORIZATION --- defaults.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/defaults.yaml b/defaults.yaml index cce67b8..ee7c478 100644 --- a/defaults.yaml +++ b/defaults.yaml @@ -5,7 +5,7 @@ qabel: ALLOWED_HOSTS: - '*' - # Any and all operations on the index server can be made to require authorization. + # All REST operations on the index server can be made to require authorization. # The old behaviour is to not require it. REQUIRE_AUTHORIZATION: False # If REQUIRE_AUTHORIZATION is set, then the APISECRET and URL must also be set.