From 453b05388fbae9b2906f01a2d279a412bde69d3d Mon Sep 17 00:00:00 2001
From: Atharva Raykar <24277692+tfidfwastaken@users.noreply.github.com>
Date: Thu, 21 Dec 2023 16:40:47 +0530
Subject: [PATCH] Fix validation errors for import user numbers (#5362)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Switching to an import user per organization introduced a bug where
creating a new Import User causes a bug because the phone number fails
the uniqueness validations. We no longer have a globally constant phone
number—we instead generate a random string as our fake phone number that
is unique to each organization and guaranteed to not collide again.

Explanations for some choices:
* We convert the UUID of the org into a url-safe base64 representation
because:
    * It is simple and reversible
* It is only 22 bytes, which is shorter than the 36-byte UUID
representation (this helps with displaying and copying)
* There's guaranteed to be no collisions since organizations are
universally unique
* We do not include a data migration to migrate the existing
"0000000001" numbers because the phone number of a bot user is a filler
value with no intrinsic purpose.
---
 app/services/import_user.rb                    | 14 ++++++++++----
 .../patient_import/import_user_spec.rb         | 18 ++++++++++++++----
 2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/app/services/import_user.rb b/app/services/import_user.rb
index 4b7aac2fc7..c1c6fdcaee 100644
--- a/app/services/import_user.rb
+++ b/app/services/import_user.rb
@@ -1,13 +1,11 @@
 class ImportUser
-  IMPORT_USER_PHONE_NUMBER = "0000000001"
-
   def self.find_or_create(org_id:)
     find(org_id) || create(org_id)
   end
 
   def self.find(org_id)
     PhoneNumberAuthentication.joins(:user)
-      .find_by(phone_number: IMPORT_USER_PHONE_NUMBER, user: {organization_id: org_id})&.user
+      .find_by(phone_number: bot_phone_number(org_id), user: {organization_id: org_id})&.user
   end
 
   def self.create(org_id)
@@ -24,7 +22,7 @@ def self.create(org_id)
     )
 
     phone_number_authentication = PhoneNumberAuthentication.new(
-      phone_number: IMPORT_USER_PHONE_NUMBER,
+      phone_number: bot_phone_number(org_id),
       password: generate_pin,
       registration_facility_id: facility.id
     ).tap do |pna|
@@ -43,4 +41,12 @@ def self.create(org_id)
   def self.generate_pin
     "#{rand(10)}#{rand(10)}#{rand(10)}#{rand(10)}"
   end
+
+  def self.bot_phone_number(org_id)
+    # Losslessly compresses an organisation UUID value into a url-safe base64 representation.
+    # This is a bit more compact than stuffing a UUID string as a phone number directly. Other
+    # forms of hashing may lead to information loss, doing it this way keeps the collision
+    # probability the same as the organization UUID itself.
+    [[org_id.remove("-")].pack("H*")].pack("m0").tr("+/", "-_")[..21]
+  end
 end
diff --git a/spec/services/patient_import/import_user_spec.rb b/spec/services/patient_import/import_user_spec.rb
index 883bb5a1be..0d5ac4e3e7 100644
--- a/spec/services/patient_import/import_user_spec.rb
+++ b/spec/services/patient_import/import_user_spec.rb
@@ -4,7 +4,7 @@
   describe ".find" do
     it "finds existing import user by phone number" do
       org_id = build_stubbed(:organization).id
-      import_user = create(:user, phone_number: ImportUser::IMPORT_USER_PHONE_NUMBER, organization_id: org_id)
+      import_user = create(:user, phone_number: ImportUser.bot_phone_number(org_id), organization_id: org_id)
 
       expect(ImportUser.find(org_id)).to eq(import_user)
     end
@@ -15,7 +15,7 @@
 
     it "returns nil if it does not exist in the given organization" do
       org_id = build_stubbed(:organization).id
-      _user = create(:user, phone_number: ImportUser::IMPORT_USER_PHONE_NUMBER, organization_id: org_id)
+      _user = create(:user, phone_number: ImportUser.bot_phone_number(org_id), organization_id: org_id)
 
       expect(ImportUser.find("bar")).to be_nil
     end
@@ -24,7 +24,7 @@
   describe ".find_or_create" do
     it "finds existing import user by phone number" do
       org_id = build_stubbed(:organization).id
-      import_user = create(:user, phone_number: ImportUser::IMPORT_USER_PHONE_NUMBER, organization_id: org_id)
+      import_user = create(:user, phone_number: ImportUser.bot_phone_number(org_id), organization_id: org_id)
 
       expect { ImportUser.find_or_create(org_id: org_id) }.not_to change { User.count }
       expect(ImportUser.find_or_create(org_id: org_id)).to eq(import_user)
@@ -33,8 +33,10 @@
     it "creates a new user if not found" do
       facility = create(:facility)
       import_user = ImportUser.find_or_create(org_id: facility.organization_id)
+      import_user_phone_number = ImportUser.bot_phone_number(facility.organization_id)
+
       expect(import_user).to be_persisted
-      expect(import_user.phone_number).to eq(ImportUser::IMPORT_USER_PHONE_NUMBER)
+      expect(import_user.phone_number).to eq(import_user_phone_number)
     end
 
     it "ensures the new user cannot sync data" do
@@ -45,4 +47,12 @@
       expect(import_user).to be_sync_approval_status_denied
     end
   end
+
+  describe "#bot_phone_number" do
+    it "always gives the same output for an org ID" do
+      some_org_id = SecureRandom.uuid
+
+      expect(5.times.map { ImportUser.bot_phone_number(some_org_id) }.uniq.length).to eq(1)
+    end
+  end
 end