Skip to content

Commit

Permalink
Fix validation errors for import user numbers (#5362)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
tfidfwastaken authored Dec 21, 2023
1 parent b98529c commit 453b053
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 8 deletions.
14 changes: 10 additions & 4 deletions app/services/import_user.rb
Original file line number Diff line number Diff line change
@@ -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)
Expand All @@ -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|
Expand All @@ -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
18 changes: 14 additions & 4 deletions spec/services/patient_import/import_user_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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)
Expand All @@ -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
Expand All @@ -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

0 comments on commit 453b053

Please sign in to comment.