diff --git a/api/views/phones.py b/api/views/phones.py index 3526c10d48..be65dee5ee 100644 --- a/api/views/phones.py +++ b/api/views/phones.py @@ -57,10 +57,6 @@ RelayNumber, area_code_numbers, get_last_text_sender, - get_pending_unverified_realphone_records, - get_valid_realphone_verification_record, - get_verified_realphone_record, - get_verified_realphone_records, location_numbers, send_welcome_message, suggested_numbers, @@ -163,10 +159,15 @@ def create(self, request): # number and verification code and mark it verified. verification_code = serializer.validated_data.get("verification_code") if verification_code: - valid_record = get_valid_realphone_verification_record( - request.user, serializer.validated_data["number"], verification_code - ) - if not valid_record: + try: + valid_record = ( + RealPhone.recent_objects.get_for_user_number_and_verification_code( + request.user, + serializer.validated_data["number"], + verification_code, + ) + ) + except RealPhone.DoesNotExist: incr_if_enabled("phones_RealPhoneViewSet.create.invalid_verification") raise exceptions.ValidationError( "Could not find that verification_code for user and number." @@ -190,16 +191,16 @@ def create(self, request): # to prevent sending verification codes to verified numbers, # check if the number is already a verified number. - is_verified = get_verified_realphone_record(serializer.validated_data["number"]) - if is_verified: + if RealPhone.verified_objects.exists_for_number( + serializer.validated_data["number"] + ): raise ConflictError("A verified record already exists for this number.") # to prevent abusive sending of verification messages, - # check if there is an un-expired verification code for the user - pending_unverified_records = get_pending_unverified_realphone_records( + # check if there is an un-expired verification code for number + if RealPhone.pending_objects.exists_for_number( serializer.validated_data["number"] - ) - if pending_unverified_records: + ): raise ConflictError( "An unverified record already exists for this number.", ) @@ -437,10 +438,11 @@ def search(self, request): [apn]: https://www.twilio.com/docs/phone-numbers/api/availablephonenumberlocal-resource#read-multiple-availablephonenumberlocal-resources """ # noqa: E501 # ignore long line for URL incr_if_enabled("phones_RelayNumberViewSet.search") - real_phone = get_verified_realphone_records(request.user).first() - if real_phone: - country_code = real_phone.country_code - else: + try: + country_code = RealPhone.verified_objects.country_code_for_user( + request.user + ) + except RealPhone.DoesNotExist: country_code = DEFAULT_REGION location = request.query_params.get("location") if location is not None: @@ -1166,7 +1168,7 @@ def outbound_call(request): {"detail": "Requires outbound_phone waffle flag."}, status=403 ) try: - real_phone = RealPhone.objects.get(user=request.user, verified=True) + real_phone = RealPhone.verified_objects.get_for_user(user=request.user) except RealPhone.DoesNotExist: return response.Response( {"detail": "Requires a verified real phone and phone mask."}, status=400 @@ -1355,7 +1357,7 @@ def _get_phone_objects(inbound_to): # Get RelayNumber and RealPhone try: relay_number = RelayNumber.objects.get(number=inbound_to) - real_phone = RealPhone.objects.get(user=relay_number.user, verified=True) + real_phone = RealPhone.verified_objects.get_for_user(relay_number.user) except ObjectDoesNotExist: raise exceptions.ValidationError("Could not find relay number.") diff --git a/phones/management/commands/delete_phone_data.py b/phones/management/commands/delete_phone_data.py index 2aa1b1d0ee..92dcc9a201 100644 --- a/phones/management/commands/delete_phone_data.py +++ b/phones/management/commands/delete_phone_data.py @@ -60,7 +60,7 @@ class _PhoneData: """Helper class to hold phone data for a user.""" fxa: SocialAccount - real_phone: RealPhone | None = None + real_phones: list[RealPhone] | None = None relay_phone: RelayNumber | None = None inbound_contact_count: int = 0 @@ -69,9 +69,8 @@ def from_fxa(cls, fxa_id: str) -> "_PhoneData": """Initialize from an FxA ID.""" fxa = SocialAccount.objects.get(provider="fxa", uid=fxa_id) - try: - real_phone = RealPhone.objects.get(user=fxa.user) - except RealPhone.DoesNotExist: + real_phones = RealPhone.objects.filter(user=fxa.user) + if not real_phones.exists(): return cls(fxa=fxa) try: @@ -80,11 +79,11 @@ def from_fxa(cls, fxa_id: str) -> "_PhoneData": relay_number=relay_phone ).count() except RelayNumber.DoesNotExist: - return cls(fxa=fxa, real_phone=real_phone) + return cls(fxa=fxa, real_phones=list(real_phones)) return cls( fxa=fxa, - real_phone=real_phone, + real_phones=list(real_phones), relay_phone=relay_phone, inbound_contact_count=inbound_contact_count, ) @@ -92,13 +91,13 @@ def from_fxa(cls, fxa_id: str) -> "_PhoneData": @property def has_data(self) -> bool: """Return True if the user has phone data to reset.""" - return self.real_phone is not None + return self.real_phones is not None and len(self.real_phones) > 0 @property - def real_number(self) -> str | None: + def real_numbers(self) -> list[str] | None: """Get user's real phone number, if it exists.""" - if self.real_phone: - return self.real_phone.number + if self.real_phones: + return [real_phone.number for real_phone in self.real_phones] return None @property @@ -114,8 +113,13 @@ def bullet_report(self) -> str: f"* FxA ID: {self.fxa.uid}\n" f"* User ID: {self.fxa.user_id}\n" f"* Email: {self.fxa.user.email}\n" - f"* Real Phone: {self.real_number or ''}\n" - f"* Relay Phone: {self.relay_number or ''}\n" + f"* Real Phone: " + + ( + "\n* Real Phone: ".join(number for number in self.real_numbers) + if self.real_numbers + else "" + ) + + f"\n* Relay Phone: {self.relay_number or ''}\n" f"* Inbound Contacts: {self.inbound_contact_count}\n" ) @@ -125,5 +129,5 @@ def reset(self) -> None: if self.inbound_contact_count: InboundContact.objects.filter(relay_number=self.relay_phone).delete() self.relay_phone.delete() - if self.real_phone: - self.real_phone.delete() + for real_phone in self.real_phones or []: + real_phone.delete() diff --git a/phones/models.py b/phones/models.py index 6be6aa17af..e78a1ffdd4 100644 --- a/phones/models.py +++ b/phones/models.py @@ -46,48 +46,6 @@ def verification_sent_date_default(): return datetime.now(UTC) -def get_expired_unverified_realphone_records(number): - return RealPhone.objects.filter( - number=number, - verified=False, - verification_sent_date__lt=( - datetime.now(UTC) - - timedelta(0, 60 * settings.MAX_MINUTES_TO_VERIFY_REAL_PHONE) - ), - ) - - -def get_pending_unverified_realphone_records(number): - return RealPhone.objects.filter( - number=number, - verified=False, - verification_sent_date__gt=( - datetime.now(UTC) - - timedelta(0, 60 * settings.MAX_MINUTES_TO_VERIFY_REAL_PHONE) - ), - ) - - -def get_verified_realphone_records(user): - return RealPhone.objects.filter(user=user, verified=True) - - -def get_verified_realphone_record(number): - return RealPhone.objects.filter(number=number, verified=True).first() - - -def get_valid_realphone_verification_record(user, number, verification_code): - return RealPhone.objects.filter( - user=user, - number=number, - verification_code=verification_code, - verification_sent_date__gt=( - datetime.now(UTC) - - timedelta(0, 60 * settings.MAX_MINUTES_TO_VERIFY_REAL_PHONE) - ), - ).first() - - def get_last_text_sender(relay_number: RelayNumber) -> InboundContact | None: """ Get the last text sender. @@ -127,6 +85,73 @@ def iq_fmt(e164_number: str) -> str: return "1" + str(phonenumbers.parse(e164_number, "E164").national_number) +class VerifiedRealPhoneManager(models.Manager["RealPhone"]): + """Return verified RealPhone records.""" + + def get_queryset(self) -> models.query.QuerySet[RealPhone]: + return super().get_queryset().filter(verified=True) + + def get_for_user(self, user: User) -> RealPhone: + """Get the one verified RealPhone for the user, or raise DoesNotExist.""" + return self.get(user=user) + + def exists_for_number(self, number: str) -> bool: + """Return True if a verified RealPhone exists for this number.""" + return self.filter(number=number).exists() + + def country_code_for_user(self, user: User) -> str: + """Return the RealPhone country code for this user.""" + return self.values_list("country_code", flat=True).get(user=user) + + +class ExpiredRealPhoneManager(models.Manager["RealPhone"]): + """Return RealPhone records where the sent verification is no longer valid.""" + + def get_queryset(self) -> models.query.QuerySet[RealPhone]: + return ( + super() + .get_queryset() + .filter( + verified=False, + verification_sent_date__lt=RealPhone.verification_expiration(), + ) + ) + + def delete_for_number(self, number: str) -> tuple[int, dict[str, int]]: + return self.filter(number=number).delete() + + +class RecentRealPhoneManager(models.Manager["RealPhone"]): + """Return RealPhone records where the sent verification is still valid.""" + + def get_queryset(self) -> models.query.QuerySet[RealPhone]: + return ( + super() + .get_queryset() + .filter( + verified=False, + verification_sent_date__gte=RealPhone.verification_expiration(), + ) + ) + + def get_for_user_number_and_verification_code( + self, user: User, number: str, verification_code: str + ) -> RealPhone: + """Get the RealPhone with this user, number, and recently sent code, or raise""" + return self.get(user=user, number=number, verification_code=verification_code) + + +class PendingRealPhoneManager(RecentRealPhoneManager): + """Return unverified RealPhone records where verification is still valid.""" + + def get_queryset(self) -> models.query.QuerySet[RealPhone]: + return super().get_queryset().filter(verified=False) + + def exists_for_number(self, number: str) -> bool: + """Return True if a verified RealPhone exists for this number.""" + return self.filter(number=number).exists() + + class RealPhone(models.Model): user = models.ForeignKey(User, on_delete=models.CASCADE) number = models.CharField(max_length=15) @@ -140,6 +165,12 @@ class RealPhone(models.Model): verified_date = models.DateTimeField(blank=True, null=True) country_code = models.CharField(max_length=2, default=DEFAULT_REGION) + objects = models.Manager() + verified_objects = VerifiedRealPhoneManager() + expired_objects = ExpiredRealPhoneManager() + recent_objects = RecentRealPhoneManager() + pending_objects = PendingRealPhoneManager() + class Meta: constraints = [ models.UniqueConstraint( @@ -149,29 +180,31 @@ class Meta: ) ] + @classmethod + def verification_expiration(self) -> datetime: + return datetime.now(UTC) - timedelta( + 0, 60 * settings.MAX_MINUTES_TO_VERIFY_REAL_PHONE + ) + def save(self, *args, **kwargs): # delete any expired unverified RealPhone records for this number # note: it doesn't matter which user is trying to create a new # RealPhone record - any expired unverified record for the number # should be deleted - expired_verification_records = get_expired_unverified_realphone_records( - self.number - ) - expired_verification_records.delete() + RealPhone.expired_objects.delete_for_number(self.number) # We are not ready to support multiple real phone numbers per user, # so raise an exception if this save() would create a second # RealPhone record for the user - user_verified_number_records = get_verified_realphone_records(self.user) - for verified_number in user_verified_number_records: - if ( + try: + verified_number = RealPhone.verified_objects.get_for_user(self.user) + if not ( verified_number.number == self.number and verified_number.verification_code == self.verification_code ): - # User is verifying the same number twice - return super().save(*args, **kwargs) - else: raise BadRequest("User already has a verified number.") + except RealPhone.DoesNotExist: + pass # call super save to save into the DB # See also: realphone_post_save receiver below @@ -254,8 +287,9 @@ def storing_phone_log(self) -> bool: return bool(self.user.profile.store_phone_log) def save(self, *args, **kwargs): - realphone = get_verified_realphone_records(self.user).first() - if not realphone: + try: + realphone = RealPhone.verified_objects.get(user=self.user) + except RealPhone.DoesNotExist: raise ValidationError("User does not have a verified real phone.") # if this number exists for this user, this is an update call @@ -391,7 +425,7 @@ def relaynumber_post_save(sender, instance, created, **kwargs): def send_welcome_message(user, relay_number): - real_phone = RealPhone.objects.get(user=user) + real_phone = RealPhone.verified_objects.get(user=user) if not settings.SITE_ORIGIN: raise ValueError( "settings.SITE_ORIGIN must contain a value when calling " @@ -440,8 +474,9 @@ class Meta: def suggested_numbers(user): - real_phone = get_verified_realphone_records(user).first() - if real_phone is None: + try: + real_phone = RealPhone.verified_objects.get_for_user(user) + except RealPhone.DoesNotExist: raise BadRequest( "available_numbers: This user hasn't verified a RealPhone yet." ) diff --git a/phones/tests/mgmt_delete_phone_data_tests.py b/phones/tests/mgmt_delete_phone_data_tests.py index f42e23811b..a61d95e727 100644 --- a/phones/tests/mgmt_delete_phone_data_tests.py +++ b/phones/tests/mgmt_delete_phone_data_tests.py @@ -159,6 +159,55 @@ def test_no_relay_phone(phone_user: User) -> None: assert not RealPhone.objects.filter(user=phone_user).exists() +@pytest.mark.django_db +def test_two_real_phones() -> None: + # Add user + now = datetime.now(tz=UTC) + phone_user = make_phone_test_user() + phone_user.profile.date_subscribed = now - timedelta(days=15) + phone_user.profile.save() + + # Add unconfirmed real phone + baker.make( + RealPhone, + user=phone_user, + number="+12005550122", + verified=False, + ) + + # Add confirmed real phone + baker.make( + RealPhone, + user=phone_user, + number="+12005550123", + verification_sent_date=now - timedelta(days=14), + verified=True, + ) + + stdout = StringIO() + assert (fxa := phone_user.profile.fxa) + call_command(THE_COMMAND, fxa.uid, "--force", stdout=stdout) + + expected_stdout = f"""\ +Found a matching user: + +* FxA ID: {phone_user.profile.fxa.uid} +* User ID: {phone_user.id} +* Email: phone_user@example.com +* Real Phone: +12005550122 +* Real Phone: +12005550123 +* Relay Phone: +* Inbound Contacts: 0 + +Deleted user's phone data. +""" + assert stdout.getvalue() == expected_stdout + + phone_user.refresh_from_db() + assert phone_user.profile.has_phone + assert not RealPhone.objects.filter(user=phone_user).exists() + + def test_no_real_phone(phone_user: User) -> None: """Nothing is done if a user doesn't have a real phone setup.""" RelayNumber.objects.filter(user=phone_user).delete() diff --git a/phones/tests/models_tests.py b/phones/tests/models_tests.py index 39293c49f5..81bc627947 100644 --- a/phones/tests/models_tests.py +++ b/phones/tests/models_tests.py @@ -25,9 +25,7 @@ RealPhone, RelayNumber, area_code_numbers, - get_expired_unverified_realphone_records, get_last_text_sender, - get_valid_realphone_verification_record, iq_fmt, location_numbers, suggested_numbers, @@ -114,21 +112,21 @@ def django_cache(): cache.clear() -def test_get_valid_realphone_verification_record_returns_object(phone_user): +def test_realphone_pending_objects_includes_new(phone_user): number = "+12223334444" real_phone = RealPhone.objects.create( user=phone_user, number=number, verification_sent_date=datetime.now(UTC), ) - record = get_valid_realphone_verification_record( + assert RealPhone.pending_objects.exists_for_number(number) + recent_phone = RealPhone.recent_objects.get_for_user_number_and_verification_code( phone_user, number, real_phone.verification_code ) - assert record.user == phone_user - assert record.number == number + assert recent_phone.id == real_phone.id -def test_get_valid_realphone_verification_record_returns_none(phone_user): +def test_realphone_pending_objects_excludes_old(phone_user): number = "+12223334444" real_phone = RealPhone.objects.create( user=phone_user, @@ -138,10 +136,11 @@ def test_get_valid_realphone_verification_record_returns_none(phone_user): - timedelta(0, 60 * settings.MAX_MINUTES_TO_VERIFY_REAL_PHONE + 1) ), ) - record = get_valid_realphone_verification_record( - phone_user, number, real_phone.verification_code - ) - assert record is None + assert not RealPhone.pending_objects.exists_for_number(number) + with pytest.raises(RealPhone.DoesNotExist): + RealPhone.recent_objects.get_for_user_number_and_verification_code( + phone_user, number, real_phone.verification_code + ) def test_create_realphone_creates_twilio_message(phone_user, mock_twilio_client): @@ -205,13 +204,13 @@ def test_create_realphone_deletes_expired_unverified_records( - timedelta(0, 60 * settings.MAX_MINUTES_TO_VERIFY_REAL_PHONE + 1) ), ) - expired_verification_records = get_expired_unverified_realphone_records(number) + expired_verification_records = RealPhone.expired_objects.filter(number=number) assert len(expired_verification_records) >= 1 mock_twilio_client.messages.create.assert_called_once() # now try to create the new record RealPhone.objects.create(user=baker.make(User), number=number) - expired_verification_records = get_expired_unverified_realphone_records(number) + expired_verification_records = RealPhone.expired_objects.filter(number=number) assert len(expired_verification_records) == 0 mock_twilio_client.messages.create.assert_called() @@ -351,6 +350,39 @@ def test_create_relaynumber_creates_twilio_incoming_number_and_sends_welcome( assert relay_number_obj.vcard_lookup_key in call_kwargs["media_url"][0] +def test_create_relaynumber_with_two_real_numbers( + phone_user, mock_twilio_client, settings, twilio_number_sid +): + """A user with a second unverified RealPhone is OK.""" + RealPhone.objects.create(user=phone_user, number="+12223334444", verified=False) + phone2 = RealPhone.objects.create( + user=phone_user, number="+12223335555", verified=False + ) + phone2.mark_verified() + mock_twilio_client.reset_mock() + + relay_number = "+19998887777" + relay_number_obj = RelayNumber.objects.create(user=phone_user, number=relay_number) + + mock_twilio_client.incoming_phone_numbers.create.assert_called_once_with( + phone_number=relay_number, + sms_application_sid=settings.TWILIO_SMS_APPLICATION_SID, + voice_application_sid=settings.TWILIO_SMS_APPLICATION_SID, + ) + mock_services = mock_twilio_client.messaging.v1.services + mock_services.assert_called_once_with(settings.TWILIO_MESSAGING_SERVICE_SID[0]) + mock_services.return_value.phone_numbers.create.assert_called_once_with( + phone_number_sid=twilio_number_sid + ) + + mock_messages_create = mock_twilio_client.messages.create + mock_messages_create.assert_called_once() + call_kwargs = mock_messages_create.call_args.kwargs + assert "Welcome" in call_kwargs["body"] + assert call_kwargs["to"] == phone2.number + assert relay_number_obj.vcard_lookup_key in call_kwargs["media_url"][0] + + def test_create_relaynumber_already_registered_with_service( phone_user, real_phone_us, mock_twilio_client, caplog, settings, twilio_number_sid ): @@ -623,7 +655,7 @@ def test_relaynumber_remaining_minutes_returns_properly_formats_remaining_second relay_number_obj.save() assert relay_number_obj.remaining_minutes == 8 - # If more call time is spent than alotted (negative remaining_seconds), + # If more call time is spent than allotted (negative remaining_seconds), # the remaining_minutes property should return zero relay_number_obj.remaining_seconds = -522 relay_number_obj.save() diff --git a/privaterelay/models.py b/privaterelay/models.py index 39764f3bc7..e3e4ddbcfc 100644 --- a/privaterelay/models.py +++ b/privaterelay/models.py @@ -378,7 +378,7 @@ def date_phone_registered(self) -> datetime | None: from phones.models import RealPhone, RelayNumber try: - real_phone = RealPhone.objects.get(user=self.user) + real_phone = RealPhone.verified_objects.get_for_user(self.user) relay_number = RelayNumber.objects.get(user=self.user) except RealPhone.DoesNotExist: return None diff --git a/privaterelay/tests/model_tests.py b/privaterelay/tests/model_tests.py index e97640fa82..c552dcfac2 100644 --- a/privaterelay/tests/model_tests.py +++ b/privaterelay/tests/model_tests.py @@ -263,6 +263,20 @@ def test_real_phone_no_relay_number_returns_verified_date(self) -> None: ) assert self.profile.date_phone_registered == datetime_now + def test_two_real_phones_returns_verified_date(self) -> None: + self.upgrade_to_phone() + datetime_now = datetime.now(UTC) + RealPhone.objects.create( + user=self.profile.user, number="+12223335555", verified=False + ) + RealPhone.objects.create( + user=self.profile.user, + number="+12223334444", + verified=True, + verified_date=datetime_now, + ) + assert self.profile.date_phone_registered == datetime_now + def test_real_phone_and_relay_number_w_created_at_returns_created_at_date( self, ) -> None: