From 7455966b896b366fe6e6de5baf6f3b55f8a08bbe Mon Sep 17 00:00:00 2001 From: Michael Derynck Date: Thu, 20 Jun 2024 10:09:24 -0600 Subject: [PATCH] Add a simple phone number ban mechanism (#4557) # What this PR does Add a simple list for maintaining phone numbers to restrict from SMS, voice and verify. Works by removing the number as verified and block future verification attempts with that number rather than check every operation since all operations already check if a number is verified. ## Which issue(s) this PR closes ## Checklist - [x] Unit, integration, and e2e (if applicable) tests updated - [x] Documentation added (or `pr:no public docs` PR label added if not required) - [x] Added the relevant release notes label (see labels prefixed w/ `release:`). These labels dictate how your PR will show up in the autogenerated release notes. --- engine/apps/phone_notifications/exceptions.py | 4 ++ .../migrations/0002_bannedphonenumber.py | 22 ++++++ .../models/banned_phone_number.py | 61 +++++++++++++++++ .../apps/phone_notifications/phone_backend.py | 3 + .../tests/test_banned_phone_number.py | 41 ++++++++++++ .../test_phone_backend_phone_verification.py | 67 ++++++++++++++++++- 6 files changed, 197 insertions(+), 1 deletion(-) create mode 100644 engine/apps/phone_notifications/migrations/0002_bannedphonenumber.py create mode 100644 engine/apps/phone_notifications/models/banned_phone_number.py create mode 100644 engine/apps/phone_notifications/tests/test_banned_phone_number.py diff --git a/engine/apps/phone_notifications/exceptions.py b/engine/apps/phone_notifications/exceptions.py index a605ac81..c7c38f12 100644 --- a/engine/apps/phone_notifications/exceptions.py +++ b/engine/apps/phone_notifications/exceptions.py @@ -48,3 +48,7 @@ class CallsLimitExceeded(Exception): class SMSLimitExceeded(Exception): pass + + +class PhoneNumberBanned(Exception): + pass diff --git a/engine/apps/phone_notifications/migrations/0002_bannedphonenumber.py b/engine/apps/phone_notifications/migrations/0002_bannedphonenumber.py new file mode 100644 index 00000000..025dbaf2 --- /dev/null +++ b/engine/apps/phone_notifications/migrations/0002_bannedphonenumber.py @@ -0,0 +1,22 @@ +# Generated by Django 4.2.11 on 2024-06-19 21:03 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('phone_notifications', '0001_initial'), + ] + + operations = [ + migrations.CreateModel( + name='BannedPhoneNumber', + fields=[ + ('phone_number', models.CharField(max_length=20, primary_key=True, serialize=False)), + ('created_at', models.DateTimeField(auto_now=True)), + ('reason', models.TextField(default=None, null=True)), + ('users', models.JSONField(default=None, null=True)), + ], + ), + ] diff --git a/engine/apps/phone_notifications/models/banned_phone_number.py b/engine/apps/phone_notifications/models/banned_phone_number.py new file mode 100644 index 00000000..a6d6511c --- /dev/null +++ b/engine/apps/phone_notifications/models/banned_phone_number.py @@ -0,0 +1,61 @@ +import json +import logging +from dataclasses import asdict, dataclass +from typing import List + +from django.db import models + +from apps.phone_notifications.exceptions import PhoneNumberBanned + +logger = logging.getLogger(__name__) + + +@dataclass +class BannedPhoneUserEntry: + user_id: int + user_name: str + org_id: int + stack_slug: str + org_slug: str + + +class BannedPhoneNumber(models.Model): + phone_number = models.CharField(primary_key=True, max_length=20) + created_at = models.DateTimeField(auto_now=True) + reason = models.TextField(null=True, default=None) + users = models.JSONField(null=True, default=None) + + def get_user_entries(self) -> List[BannedPhoneUserEntry]: + return [BannedPhoneUserEntry(**data) for data in json.loads(self.users)] + + +def ban_phone_number(phone_number: str, reason: str): + from apps.user_management.models import User + + banned_phone_number = BannedPhoneNumber(phone_number=phone_number) + users = User.objects.filter(_verified_phone_number=phone_number) + # Record instances of phone number use + user_entries = [ + asdict( + BannedPhoneUserEntry( + user_id=user.id, + user_name=user.username, + org_id=user.organization.org_id, + stack_slug=user.organization.stack_slug, + org_slug=user.organization.org_slug, + ) + ) + for user in users + ] + users.update(_verified_phone_number=None) + banned_phone_number.users = json.dumps(user_entries) + banned_phone_number.reason = reason + banned_phone_number.save() + + logger.info(f"ban_phone_number={phone_number}, in use by users={len(user_entries)}, reason={reason}") + + +def check_banned_phone_number(phone_number: str): + banned_entry = BannedPhoneNumber.objects.filter(phone_number=phone_number).first() + if banned_entry: + raise PhoneNumberBanned diff --git a/engine/apps/phone_notifications/phone_backend.py b/engine/apps/phone_notifications/phone_backend.py index 26b64399..a8cf50d6 100644 --- a/engine/apps/phone_notifications/phone_backend.py +++ b/engine/apps/phone_notifications/phone_backend.py @@ -21,6 +21,7 @@ from .exceptions import ( SMSLimitExceeded, ) from .models import PhoneCallRecord, ProviderPhoneCall, ProviderSMS, SMSRecord +from .models.banned_phone_number import check_banned_phone_number from .phone_provider import PhoneProvider, get_phone_provider logger = logging.getLogger(__name__) @@ -316,6 +317,7 @@ class PhoneBackend: if self._validate_user_number(user): logger.info(f"PhoneBackend.send_verification_sms: number already verified for user {user.id}") raise NumberAlreadyVerified + check_banned_phone_number(user.unverified_phone_number) self.phone_provider.send_verification_sms(user.unverified_phone_number) def make_verification_call(self, user): @@ -327,6 +329,7 @@ class PhoneBackend: if self._validate_user_number(user): logger.info(f"PhoneBackend.make_verification_call: number already verified user_id={user.id}") raise NumberAlreadyVerified + check_banned_phone_number(user.unverified_phone_number) self.phone_provider.make_verification_call(user.unverified_phone_number) def verify_phone_number(self, user, code) -> bool: diff --git a/engine/apps/phone_notifications/tests/test_banned_phone_number.py b/engine/apps/phone_notifications/tests/test_banned_phone_number.py new file mode 100644 index 00000000..8363a790 --- /dev/null +++ b/engine/apps/phone_notifications/tests/test_banned_phone_number.py @@ -0,0 +1,41 @@ +import pytest + +from apps.phone_notifications.models.banned_phone_number import BannedPhoneNumber, ban_phone_number + + +@pytest.mark.django_db +def test_ban_phone_number(make_organization, make_user_for_organization): + organization = make_organization() + banned_phone_number = "+1234567890" + unbanned_phone_number = "+0987654321" + banned_user_1 = make_user_for_organization( + organization=organization, + _verified_phone_number=banned_phone_number, + unverified_phone_number=banned_phone_number, + ) + banned_user_2 = make_user_for_organization( + organization=organization, + _verified_phone_number=banned_phone_number, + unverified_phone_number=banned_phone_number, + ) + unbanned_user = make_user_for_organization( + organization=organization, + _verified_phone_number=unbanned_phone_number, + unverified_phone_number=unbanned_phone_number, + ) + reason = "usage too high" + ban_phone_number(banned_phone_number, reason) + banned_user_1.refresh_from_db() + assert banned_user_1._verified_phone_number is None + assert banned_user_1.unverified_phone_number == banned_phone_number + banned_user_2.refresh_from_db() + assert banned_user_2._verified_phone_number is None + assert banned_user_2.unverified_phone_number == banned_phone_number + unbanned_user.refresh_from_db() + assert unbanned_user._verified_phone_number == unbanned_phone_number + assert unbanned_user.unverified_phone_number == unbanned_phone_number + ban_phone_number_entry = BannedPhoneNumber.objects.get(pk=banned_phone_number) + assert ban_phone_number_entry is not None + assert ban_phone_number_entry.reason == reason + user_entries = ban_phone_number_entry.get_user_entries() + assert len(user_entries) == 2 diff --git a/engine/apps/phone_notifications/tests/test_phone_backend_phone_verification.py b/engine/apps/phone_notifications/tests/test_phone_backend_phone_verification.py index cfb8996c..a12380cb 100644 --- a/engine/apps/phone_notifications/tests/test_phone_backend_phone_verification.py +++ b/engine/apps/phone_notifications/tests/test_phone_backend_phone_verification.py @@ -2,7 +2,8 @@ from unittest import mock import pytest -from apps.phone_notifications.exceptions import NumberAlreadyVerified +from apps.phone_notifications.exceptions import NumberAlreadyVerified, PhoneNumberBanned +from apps.phone_notifications.models.banned_phone_number import ban_phone_number from apps.phone_notifications.phone_backend import PhoneBackend from apps.phone_notifications.tests.mock_phone_provider import MockPhoneProvider @@ -67,3 +68,67 @@ def test_make_verification_call_raises_when_number_verified( user.save_verified_phone_number("+1234567890") with pytest.raises(NumberAlreadyVerified): phone_backend.make_verification_call(user) + + +@pytest.mark.django_db +@mock.patch("apps.phone_notifications.phone_backend.PhoneBackend._validate_user_number", return_value=False) +@mock.patch("apps.phone_notifications.tests.mock_phone_provider.MockPhoneProvider.send_verification_sms") +def test_send_verification_sms_banned_number( + mock_send_verification_sms, mock_validate_user_number, make_organization_and_user +): + _, user = make_organization_and_user() + phone_backend = PhoneBackend() + + number_to_verify = "+1234567890" + user.unverified_phone_number = "+1234567890" + ban_phone_number(number_to_verify, "usage too high") + with pytest.raises(PhoneNumberBanned): + phone_backend.send_verification_sms(user) + + +@pytest.mark.django_db +@mock.patch("apps.phone_notifications.phone_backend.PhoneBackend._validate_user_number", return_value=False) +@mock.patch("apps.phone_notifications.tests.mock_phone_provider.MockPhoneProvider.send_verification_sms") +def test_send_verification_sms_unaffected_by_ban( + mock_send_verification_sms, mock_validate_user_number, make_organization_and_user +): + _, user = make_organization_and_user() + phone_backend = PhoneBackend() + + number_to_verify = "+1234567890" + user.unverified_phone_number = "+1234567890" + ban_phone_number("+0987654321", "usage too high") + phone_backend.send_verification_sms(user) + mock_send_verification_sms.assert_called_once_with(number_to_verify) + + +@pytest.mark.django_db +@mock.patch("apps.phone_notifications.phone_backend.PhoneBackend._validate_user_number", return_value=False) +@mock.patch("apps.phone_notifications.tests.mock_phone_provider.MockPhoneProvider.make_verification_call") +def test_make_verification_call_banned_number( + mock_make_verification_call, mock_validate_user_number, make_organization_and_user +): + _, user = make_organization_and_user() + phone_backend = PhoneBackend() + + number_to_verify = "+1234567890" + user.unverified_phone_number = "+1234567890" + ban_phone_number(number_to_verify, "usage too high") + with pytest.raises(PhoneNumberBanned): + phone_backend.make_verification_call(user) + + +@pytest.mark.django_db +@mock.patch("apps.phone_notifications.phone_backend.PhoneBackend._validate_user_number", return_value=False) +@mock.patch("apps.phone_notifications.tests.mock_phone_provider.MockPhoneProvider.make_verification_call") +def test_make_verification_call_unaffected_by_ban( + mock_make_verification_call, mock_validate_user_number, make_organization_and_user +): + _, user = make_organization_and_user() + phone_backend = PhoneBackend() + + number_to_verify = "+1234567890" + user.unverified_phone_number = "+1234567890" + ban_phone_number("+0987654321", "usage too high") + phone_backend.make_verification_call(user) + mock_make_verification_call.assert_called_once_with(number_to_verify)