✨(backend) add 10-digit PIN codes on rooms for telephony
Enable users to join rooms via SIP telephony by: - Dialing the SIP trunk number - Entering the room's PIN followed by '#' The PIN code needs to be generated before the LiveKit room is created, allowing the owner to send invites to participants in advance. With 10-digit PINs (10^10 combinations) and a large number of rooms (e.g., 1M), collisions become statistically inevitable. A retry mechanism helps reduce the chance of repeated collisions but doesn't eliminate the overall risk. With 100K generated PINs, the probability of at least one collision exceeds 39%, due to the birthday paradox. To scale safely, we’ll later propose using multiple trunks. Each trunk will handle a separate PIN namespace, and the combination of trunk_id and PIN will ensure uniqueness. Room assignment will be evenly distributed across trunks to balance load and minimize collisions. Following XP principles, we’ll ship the simplest working version of this feature. The goal is to deliver value quickly without over-engineering. We’re not solving scaling challenges we don’t currently face. Our production load is around 10,000 rooms — well within safe limits for the initial implementation. Discussion points: - The `while` loop should be reviewed. Should we add rate limiting for failed attempts? - A systematic existence check before `INSERT` is more costly for a rare event and doesn't prevent race conditions, whereas retrying on integrity errors is more efficient overall. - Should we add logging or monitoring to track and analyze collisions? I tried to balance performance and simplicity while ensuring the robustness of the PIN generation process.
This commit is contained in:
committed by
aleb_the_flash
parent
d70dc41643
commit
3e93f5924c
@@ -54,3 +54,6 @@ ALLOW_UNREGISTERED_ROOMS=False
|
||||
|
||||
# Recording
|
||||
SCREEN_RECORDING_BASE_URL=http://localhost:3000/recordings
|
||||
|
||||
# Telephony
|
||||
ROOM_TELEPHONY_ENABLED=True
|
||||
|
||||
45
src/backend/core/migrations/0014_room_pin_code.py
Normal file
45
src/backend/core/migrations/0014_room_pin_code.py
Normal file
@@ -0,0 +1,45 @@
|
||||
# Generated by Django 5.1.9 on 2025-05-13 08:22
|
||||
|
||||
import secrets
|
||||
from django.db import migrations, models
|
||||
|
||||
|
||||
def generate_pin_for_rooms(apps, schema_editor):
|
||||
"""Generate unique 10-digit PIN codes for existing rooms.
|
||||
The PIN code is required for SIP telephony features.
|
||||
"""
|
||||
Room = apps.get_model('core', 'Room')
|
||||
rooms_without_pin_code = Room.objects.filter(pin_code__isnull=True)
|
||||
existing_pins = set(Room.objects.values_list('pin_code', flat=True))
|
||||
|
||||
length = 10
|
||||
|
||||
def generate_pin_code():
|
||||
while True:
|
||||
pin_code = str(secrets.randbelow(10 ** length)).zfill(length)
|
||||
if not pin_code in existing_pins:
|
||||
return pin_code
|
||||
|
||||
for room in rooms_without_pin_code:
|
||||
room.pin_code = generate_pin_code()
|
||||
room.save()
|
||||
|
||||
|
||||
|
||||
class Migration(migrations.Migration):
|
||||
|
||||
dependencies = [
|
||||
('core', '0013_alter_user_language'),
|
||||
]
|
||||
|
||||
operations = [
|
||||
migrations.AddField(
|
||||
model_name='room',
|
||||
name='pin_code',
|
||||
field=models.CharField(blank=True, help_text='Unique n-digit code that identifies this room in telephony mode.', max_length=None, null=True, unique=True, verbose_name='Room PIN code'),
|
||||
),
|
||||
migrations.RunPython(
|
||||
generate_pin_for_rooms,
|
||||
reverse_code=migrations.RunPython.noop
|
||||
),
|
||||
]
|
||||
@@ -2,6 +2,7 @@
|
||||
Declare and configure the models for the Meet core application
|
||||
"""
|
||||
|
||||
import secrets
|
||||
import uuid
|
||||
from datetime import datetime, timedelta
|
||||
from logging import getLogger
|
||||
@@ -382,6 +383,14 @@ class Room(Resource):
|
||||
verbose_name=_("Visio room configuration"),
|
||||
help_text=_("Values for Visio parameters to configure the room."),
|
||||
)
|
||||
pin_code = models.CharField(
|
||||
max_length=None,
|
||||
unique=True,
|
||||
blank=True,
|
||||
null=True,
|
||||
verbose_name=_("Room PIN code"),
|
||||
help_text=_("Unique n-digit code that identifies this room in telephony mode."),
|
||||
)
|
||||
|
||||
class Meta:
|
||||
db_table = "meet_room"
|
||||
@@ -392,6 +401,14 @@ class Room(Resource):
|
||||
def __str__(self):
|
||||
return capfirst(self.name)
|
||||
|
||||
def save(self, *args, **kwargs):
|
||||
"""Generate a unique n-digit pin code for new rooms."""
|
||||
if settings.ROOM_TELEPHONY_ENABLED and not self.pk and not self.pin_code:
|
||||
self.pin_code = self.generate_unique_pin_code(
|
||||
length=settings.ROOM_TELEPHONY_PIN_LENGTH
|
||||
)
|
||||
super().save(*args, **kwargs)
|
||||
|
||||
def clean_fields(self, exclude=None):
|
||||
"""
|
||||
Automatically generate the slug from the name and make sure it does not look like a UUID.
|
||||
@@ -406,6 +423,7 @@ class Room(Resource):
|
||||
pass
|
||||
else:
|
||||
raise ValidationError({"name": f'Room name "{self.name:s}" is reserved.'})
|
||||
|
||||
super().clean_fields(exclude=exclude)
|
||||
|
||||
@property
|
||||
@@ -413,6 +431,31 @@ class Room(Resource):
|
||||
"""Check if a room is public"""
|
||||
return self.access_level == RoomAccessLevel.PUBLIC
|
||||
|
||||
@staticmethod
|
||||
def generate_unique_pin_code(length):
|
||||
"""Generate a unique n-digit PIN code"""
|
||||
|
||||
if length < 4:
|
||||
raise ValueError(
|
||||
"PIN code length must be at least 4 digits for minimal security"
|
||||
)
|
||||
|
||||
max_value = 10**length
|
||||
|
||||
for _ in range(settings.ROOM_TELEPHONY_PIN_MAX_RETRIES):
|
||||
pin_code = str(secrets.randbelow(max_value)).zfill(length)
|
||||
if not Room.objects.filter(pin_code=pin_code).exists():
|
||||
return pin_code
|
||||
|
||||
# Log a warning as a temporary measure until backend observability is implemented.
|
||||
logger.warning(
|
||||
"Failed to generate unique PIN code of length %s after %s attempts",
|
||||
length,
|
||||
settings.ROOM_TELEPHONY_PIN_MAX_RETRIES,
|
||||
)
|
||||
|
||||
return None
|
||||
|
||||
|
||||
class BaseAccessManager(models.Manager):
|
||||
"""Base manager for handling resource access control."""
|
||||
|
||||
@@ -2,6 +2,12 @@
|
||||
Unit tests for the Room model
|
||||
"""
|
||||
|
||||
# pylint: disable=W0613
|
||||
|
||||
import secrets
|
||||
from logging import Logger
|
||||
from unittest import mock
|
||||
|
||||
from django.contrib.auth.models import AnonymousUser
|
||||
from django.core.exceptions import ValidationError
|
||||
|
||||
@@ -175,3 +181,169 @@ def test_models_rooms_is_public_property():
|
||||
# Test non-public room
|
||||
private_room = RoomFactory(access_level=RoomAccessLevel.RESTRICTED)
|
||||
assert private_room.is_public is False
|
||||
|
||||
|
||||
@mock.patch.object(Room, "generate_unique_pin_code")
|
||||
def test_telephony_disabled_skips_pin_generation(
|
||||
mock_generate_unique_pin_code, settings
|
||||
):
|
||||
"""Telephony disabled should not generate pin codes."""
|
||||
|
||||
settings.ROOM_TELEPHONY_ENABLED = False
|
||||
|
||||
room = RoomFactory()
|
||||
|
||||
mock_generate_unique_pin_code.assert_not_called()
|
||||
assert room.pin_code is None
|
||||
|
||||
|
||||
def test_default_and_custom_pin_length(settings):
|
||||
"""Pin codes should be created with correct configured length."""
|
||||
|
||||
settings.ROOM_TELEPHONY_ENABLED = True
|
||||
|
||||
room = RoomFactory()
|
||||
|
||||
# Assert default value is 10 for collision reasons
|
||||
assert len(room.pin_code) == 10
|
||||
|
||||
settings.ROOM_TELEPHONY_PIN_LENGTH = 5
|
||||
|
||||
room = RoomFactory()
|
||||
|
||||
# Assert custom size
|
||||
assert len(room.pin_code) == 5
|
||||
|
||||
|
||||
def test_room_updates_preserve_pin_code(settings):
|
||||
"""Room updates should preserve existing pin code."""
|
||||
|
||||
settings.ROOM_TELEPHONY_ENABLED = True
|
||||
|
||||
room = RoomFactory()
|
||||
|
||||
# Store the original pin code to compare after updates
|
||||
previous_pin_code = room.pin_code
|
||||
|
||||
# If this method is called, it would indicate the pin is being regenerated unnecessarily
|
||||
with mock.patch.object(
|
||||
Room, "generate_unique_pin_code"
|
||||
) as mock_generate_unique_pin_code:
|
||||
# Explicitly call save to persist the changes to the room
|
||||
room.slug = "aaa-aaaa-aaa"
|
||||
room.save()
|
||||
assert room.pin_code == previous_pin_code
|
||||
mock_generate_unique_pin_code.assert_not_called()
|
||||
|
||||
|
||||
@pytest.mark.parametrize("is_telephony_enabled", [True, False])
|
||||
def test_manual_pin_code_updates(is_telephony_enabled, settings):
|
||||
"""Manual pin code changes should persist regardless of telephony setting."""
|
||||
|
||||
settings.ROOM_TELEPHONY_ENABLED = is_telephony_enabled
|
||||
settings.ROOM_TELEPHONY_PIN_LENGTH = 5
|
||||
|
||||
room = RoomFactory()
|
||||
assert room.pin_code != "12345"
|
||||
|
||||
room.pin_code = "12345"
|
||||
room.save()
|
||||
|
||||
assert room.pin_code == "12345"
|
||||
|
||||
# No data validation when manual updates are made
|
||||
room.pin_code = "123"
|
||||
room.save()
|
||||
|
||||
assert room.pin_code == "123"
|
||||
|
||||
|
||||
def test_pin_code_uniqueness(settings):
|
||||
"""Duplicate pin codes should raise validation error."""
|
||||
|
||||
settings.ROOM_TELEPHONY_ENABLED = True
|
||||
settings.ROOM_TELEPHONY_PIN_LENGTH = 5
|
||||
|
||||
RoomFactory(pin_code="12345")
|
||||
|
||||
with pytest.raises(ValidationError) as excinfo:
|
||||
RoomFactory(pin_code="12345")
|
||||
|
||||
assert "Room with this Room PIN code already exists." in str(excinfo.value)
|
||||
|
||||
|
||||
@pytest.mark.parametrize("invalid_length", [0, 1, 2, 3])
|
||||
def test_pin_code_minimum_length(invalid_length, settings):
|
||||
"""Pin codes should enforce minimum length for security."""
|
||||
|
||||
settings.ROOM_TELEPHONY_ENABLED = True
|
||||
settings.ROOM_TELEPHONY_PIN_LENGTH = 4
|
||||
|
||||
# Assert no exception is raised with a valid length
|
||||
RoomFactory()
|
||||
|
||||
settings.ROOM_TELEPHONY_PIN_LENGTH = invalid_length
|
||||
|
||||
with pytest.raises(ValueError) as excinfo:
|
||||
RoomFactory()
|
||||
|
||||
assert "PIN code length must be at least 4 digits for minimal security" in str(
|
||||
excinfo.value
|
||||
)
|
||||
|
||||
|
||||
@mock.patch.object(Logger, "warning")
|
||||
@mock.patch.object(secrets, "randbelow", return_value=12345)
|
||||
def test_pin_generation_max_retries(mock_randbelow, mock_logger, settings):
|
||||
"""Pin generation should give up after max retries."""
|
||||
|
||||
settings.ROOM_TELEPHONY_ENABLED = True
|
||||
settings.ROOM_TELEPHONY_PIN_LENGTH = 5
|
||||
|
||||
RoomFactory(pin_code="12345")
|
||||
|
||||
# Assert default max retries is low, 3
|
||||
room1 = RoomFactory()
|
||||
assert mock_randbelow.call_count == 3
|
||||
assert room1.pin_code is None
|
||||
|
||||
mock_logger.assert_called_once_with(
|
||||
"Failed to generate unique PIN code of length %s after %s attempts", 5, 3
|
||||
)
|
||||
|
||||
mock_logger.reset_mock()
|
||||
mock_randbelow.reset_mock()
|
||||
settings.ROOM_TELEPHONY_PIN_MAX_RETRIES = 5
|
||||
|
||||
room2 = RoomFactory()
|
||||
assert mock_randbelow.call_count == 5
|
||||
assert room2.pin_code is None
|
||||
|
||||
mock_logger.assert_called_once_with(
|
||||
"Failed to generate unique PIN code of length %s after %s attempts", 5, 5
|
||||
)
|
||||
|
||||
|
||||
@mock.patch.object(secrets, "randbelow", return_value=12345)
|
||||
def test_pin_code_zero_padding(mock_randbelow, settings):
|
||||
"""Pin codes should be zero-padded to meet required length."""
|
||||
|
||||
settings.ROOM_TELEPHONY_ENABLED = True
|
||||
settings.ROOM_TELEPHONY_PIN_LENGTH = 10
|
||||
|
||||
room = RoomFactory()
|
||||
assert room.pin_code == "0000012345"
|
||||
|
||||
|
||||
@mock.patch.object(secrets, "randbelow", return_value=12345)
|
||||
def test_pin_generation_upper_bound(mock_randbelow, settings):
|
||||
"""Random number generator should use correct upper bound based on pin length."""
|
||||
|
||||
settings.ROOM_TELEPHONY_ENABLED = False
|
||||
|
||||
room = RoomFactory()
|
||||
|
||||
room.generate_unique_pin_code(length=5)
|
||||
|
||||
# Assert called with the right exclusive upper bound, 10^5
|
||||
mock_randbelow.assert_called_with(100000)
|
||||
|
||||
@@ -573,6 +573,23 @@ class Base(Configuration):
|
||||
environ_prefix=None,
|
||||
)
|
||||
|
||||
# SIP Telephony
|
||||
ROOM_TELEPHONY_ENABLED = values.BooleanValue(
|
||||
False,
|
||||
environ_name="ROOM_TELEPHONY_ENABLED",
|
||||
environ_prefix=None,
|
||||
)
|
||||
ROOM_TELEPHONY_PIN_LENGTH = values.PositiveIntegerValue(
|
||||
10,
|
||||
environ_name="ROOM_TELEPHONY_PIN_LENGTH",
|
||||
environ_prefix=None,
|
||||
)
|
||||
ROOM_TELEPHONY_PIN_MAX_RETRIES = values.PositiveIntegerValue(
|
||||
5,
|
||||
environ_name="ROOM_TELEPHONY_PIN_MAX_RETRIES",
|
||||
environ_prefix=None,
|
||||
)
|
||||
|
||||
# pylint: disable=invalid-name
|
||||
@property
|
||||
def ENVIRONMENT(self):
|
||||
|
||||
@@ -67,6 +67,7 @@ backend:
|
||||
SUMMARY_SERVICE_ENDPOINT: http://meet-summary:80/api/v1/tasks/
|
||||
SUMMARY_SERVICE_API_TOKEN: password
|
||||
SCREEN_RECORDING_BASE_URL: https://meet.127.0.0.1.nip.io/recordings
|
||||
ROOM_TELEPHONY_ENABLED: True
|
||||
SSL_CERT_FILE: /usr/local/lib/python3.12/site-packages/certifi/cacert.pem
|
||||
|
||||
|
||||
|
||||
@@ -94,6 +94,7 @@ backend:
|
||||
name: backend
|
||||
key: BREVO_API_KEY
|
||||
BREVO_API_CONTACT_LIST_IDS: 8
|
||||
ROOM_TELEPHONY_ENABLED: True
|
||||
SSL_CERT_FILE: /usr/local/lib/python3.12/site-packages/certifi/cacert.pem
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user