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

<!--
*Note*: if you have more than one GitHub issue that this PR closes, be
sure to preface
each issue link with a [closing
keyword](https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests#linking-a-pull-request-to-an-issue).
This ensures that the issue(s) are auto-closed once the PR has been
merged.
-->

## 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.
This commit is contained in:
Michael Derynck 2024-06-20 10:09:24 -06:00 committed by GitHub
parent 21ba1aa9e7
commit 7455966b89
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
6 changed files with 197 additions and 1 deletions

View file

@ -48,3 +48,7 @@ class CallsLimitExceeded(Exception):
class SMSLimitExceeded(Exception):
pass
class PhoneNumberBanned(Exception):
pass

View file

@ -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)),
],
),
]

View file

@ -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

View file

@ -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:

View file

@ -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

View file

@ -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)