Skip to content

Allow users to reuse unverified phone numbers used by banned users

Eugie Limpin requested to merge el-ban-on-verified-phone-number-reuse into master

What does this MR do and why?

Resolves https://gitlab.com/gitlab-org/modelops/anti-abuse/team-tasks/-/issues/530+

Problem

Previously we were banning new users automatically when they use a phone number that has already been used by a banned user even if the banned user didn't validate the phone number (i.e. banned user triggered verification code delivery to the phone number but didn't enter the code in GitLab to prove the phone number was theirs). This resulted to needing to delete phone number validation records associated with banned users just in case they didn't actually own the phone numbers they used.

Solution

This MR updates the logic of the method (Users::PhoneNumberValidation.related_to_banned_user?(international_dial_code, phone_number) that checks if a phone number was previously used by a banned user. Now related_to_banned_user? only returns true when there is a Users::PhoneNumberValidation record that meets all of the following conditions:

  1. record belongs to a banned user
  2. (NEW) record validated_at is not nil - meaning the banned user actually proved the phone number was theirs
  3. record international_dial_code is equal to given international_dial_code
  4. record phone_number is equal to given phone_number

With this change we will only be auto-banning new users that we know were already banned before.

Database changes

Users::PhoneNumberValidation.related_to_banned_user? class method

Before

SELECT
    1 AS one
FROM
    "user_phone_number_validations"
    INNER JOIN "banned_users" ON "banned_users"."user_id" = "user_phone_number_validations"."user_id"
WHERE
    "user_phone_number_validations"."international_dial_code" = 380
    AND "user_phone_number_validations"."phone_number" = '689818097'
LIMIT 1

Explain:

https://console.postgres.ai/shared/cdc96f59-0747-405e-a1af-c72198026b29

Limit  (cost=0.71..6.75 rows=1 width=4) (actual time=0.057..0.059 rows=1 loops=1)
   Buffers: shared hit=10
   I/O Timings: read=0.000 write=0.000
   ->  Nested Loop  (cost=0.71..6.75 rows=1 width=4) (actual time=0.056..0.057 rows=1 loops=1)
         Buffers: shared hit=10
         I/O Timings: read=0.000 write=0.000
         ->  Index Scan using index_user_phone_validations_on_dial_code_phone_number on public.user_phone_number_validations  (cost=0.42..3.44 rows=1 width=8) (actual time=0.046..0.046 rows=1 loops=1)
               Index Cond: ((user_phone_number_validations.international_dial_code = 380) AND (user_phone_number_validations.phone_number = '689818097'::text))
               Buffers: shared hit=7
               I/O Timings: read=0.000 write=0.000
         ->  Index Only Scan using banned_users_pkey on public.banned_users  (cost=0.29..3.31 rows=1 width=8) (actual time=0.007..0.007 rows=1 loops=1)
               Index Cond: (banned_users.user_id = user_phone_number_validations.user_id)
               Heap Fetches: 0
               Buffers: shared hit=3
               I/O Timings: read=0.000 write=0.000

After

SELECT
    1 AS one
FROM
    "user_phone_number_validations"
    INNER JOIN "banned_users" ON "banned_users"."user_id" = "user_phone_number_validations"."user_id"
WHERE
    "user_phone_number_validations"."international_dial_code" = 380
    AND "user_phone_number_validations"."phone_number" = '689818097'
    AND "user_phone_number_validations"."validated_at" IS NOT NULL
LIMIT 1

Explain:

https://console.postgres.ai/shared/a16b1a95-3e00-4006-8c6a-7a510c446c59

 Limit  (cost=0.71..6.75 rows=1 width=4) (actual time=15.211..15.214 rows=1 loops=1)
   Buffers: shared hit=5 read=5
   I/O Timings: read=15.067 write=0.000
   ->  Nested Loop  (cost=0.71..6.75 rows=1 width=4) (actual time=15.209..15.211 rows=1 loops=1)
         Buffers: shared hit=5 read=5
         I/O Timings: read=15.067 write=0.000
         ->  Index Scan using index_user_phone_validations_on_dial_code_phone_number on public.user_phone_number_validations  (cost=0.42..3.44 rows=1 width=8) (actual time=12.704..12.704 rows=1 loops=1)
               Index Cond: ((user_phone_number_validations.international_dial_code = 380) AND (user_phone_number_validations.phone_number = '689818097'::text))
               Filter: (user_phone_number_validations.validated_at IS NOT NULL)
               Rows Removed by Filter: 0
               Buffers: shared hit=3 read=4
               I/O Timings: read=12.609 write=0.000
         ->  Index Only Scan using banned_users_pkey on public.banned_users  (cost=0.29..3.31 rows=1 width=8) (actual time=2.494..2.495 rows=1 loops=1)
               Index Cond: (banned_users.user_id = user_phone_number_validations.user_id)
               Heap Fetches: 0
               Buffers: shared hit=2 read=1
               I/O Timings: read=2.458 write=0.000

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other merge requests.

Before After

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Eugie Limpin

Merge request reports