Allow nonexisting users to be queried & improve error logging (#175)

StanfordBDHG · Oct 31, 2024 · 2dcd268 · 2dcd268
1 parent b902840
commit 2dcd268
Show file tree

Hide file tree

Showing 3 changed files with 22 additions and 11 deletions.
diff --git a/firestore.rules b/firestore.rules
@@ -16,13 +16,15 @@ service cloud.firestore {
     }
 
     function isOwnerOf(organizationId) {
-      return isAuthenticated()
+      return organizationId != null
+        && isAuthenticated()
         && request.auth.token.type == 'owner'
         && isPartOf(organizationId);
     }
 
     function isOwnerOrClinicianOf(organizationId) {
-      return isAuthenticated()
+      return organizationId != null
+        && isAuthenticated()
         && request.auth.token.type in ['owner', 'clinician']
         && isPartOf(organizationId);
     }
@@ -115,6 +117,7 @@ service cloud.firestore {
       }
 
       allow read: if isAdmin()
+        || (resource == null && isAuthenticated())
         || (request.auth != null && request.auth.uid == userId)
         || isOwnerOrClinicianOf(resource.data.organization);
 

diff --git a/functions/src/functions/helpers.ts b/functions/src/functions/helpers.ts
@@ -23,25 +23,22 @@ export const serviceAccount = `cloudfunctionsserviceaccount@${process.env.GCLOUD
 export function validatedOnCall<Schema extends z.ZodTypeAny, Return>(
   name: string,
   schema: Schema,
-  handler: (request: CallableRequest<z.output<Schema>>) => Return,
+  handler: (request: CallableRequest<z.output<Schema>>) => Promise<Return>,
   options: CallableOptions = {
     invoker: 'public',
     serviceAccount: serviceAccount,
   },
-): CallableFunction<
-  z.input<Schema>,
-  Return extends Promise<unknown> ? Return : Promise<Return>
-> {
-  return onCall(options, (request) => {
+): CallableFunction<z.input<Schema>, Promise<Return>> {
+  return onCall(options, async (request) => {
     try {
       logger.debug(
-        `onCall(${name}) from user '${request.auth?.uid}' with ${JSON.stringify(request.data)}`,
+        `onCall(${name}) from user '${request.auth?.uid}' with '${JSON.stringify(request.data)}'`,
       )
       request.data = schema.parse(request.data) as z.output<Schema>
-      return handler(request)
+      return await handler(request)
     } catch (error) {
       logger.debug(
-        `onCall(${name}) from user '${request.auth?.uid}' failed with ${String(error)}.`,
+        `onCall(${name}) from user '${request.auth?.uid}' failed with '${String(error)}'.`,
       )
       if (error instanceof z.ZodError) {
         throw new https.HttpsError(

diff --git a/functions/src/tests/rules/users.test.ts b/functions/src/tests/rules/users.test.ts
@@ -27,6 +27,7 @@ describe('firestore.rules: users/{userId}', () => {
   const clinicianId = 'mockClinician'
   const patientId = 'mockPatient'
   const userId = 'mockUser'
+  const unknownId = 'mockUnknown'
 
   let testEnvironment: RulesTestEnvironment
   let adminFirestore: firebase.firestore.Firestore
@@ -98,35 +99,45 @@ describe('firestore.rules: users/{userId}', () => {
   })
 
   it('gets users/{userId}', async () => {
+    console.log('admin')
     await assertSucceeds(adminFirestore.doc(`users/${adminId}`).get())
     await assertSucceeds(adminFirestore.doc(`users/${ownerId}`).get())
     await assertSucceeds(adminFirestore.doc(`users/${clinicianId}`).get())
     await assertSucceeds(adminFirestore.doc(`users/${patientId}`).get())
     await assertSucceeds(adminFirestore.doc(`users/${userId}`).get())
+    await assertSucceeds(adminFirestore.doc(`users/${unknownId}`).get())
 
+    console.log('owner')
     await assertFails(ownerFirestore.doc(`users/${adminId}`).get())
     await assertSucceeds(ownerFirestore.doc(`users/${ownerId}`).get())
     await assertSucceeds(ownerFirestore.doc(`users/${clinicianId}`).get())
     await assertSucceeds(ownerFirestore.doc(`users/${patientId}`).get())
     await assertFails(ownerFirestore.doc(`users/${userId}`).get())
+    await assertSucceeds(ownerFirestore.doc(`users/${unknownId}`).get())
 
+    console.log('clinician')
     await assertFails(clinicianFirestore.doc(`users/${adminId}`).get())
     await assertSucceeds(clinicianFirestore.doc(`users/${ownerId}`).get())
     await assertSucceeds(clinicianFirestore.doc(`users/${clinicianId}`).get())
     await assertSucceeds(clinicianFirestore.doc(`users/${patientId}`).get())
     await assertFails(clinicianFirestore.doc(`users/${userId}`).get())
+    await assertSucceeds(clinicianFirestore.doc(`users/${unknownId}`).get())
 
+    console.log('patient')
     await assertFails(patientFirestore.doc(`users/${adminId}`).get())
     await assertFails(patientFirestore.doc(`users/${ownerId}`).get())
     await assertFails(patientFirestore.doc(`users/${clinicianId}`).get())
     await assertSucceeds(patientFirestore.doc(`users/${patientId}`).get())
     await assertFails(patientFirestore.doc(`users/${userId}`).get())
+    await assertSucceeds(patientFirestore.doc(`users/${unknownId}`).get())
 
+    console.log('user')
     await assertFails(userFirestore.doc(`users/${adminId}`).get())
     await assertFails(userFirestore.doc(`users/${ownerId}`).get())
     await assertFails(userFirestore.doc(`users/${clinicianId}`).get())
     await assertFails(userFirestore.doc(`users/${patientId}`).get())
     await assertSucceeds(userFirestore.doc(`users/${userId}`).get())
+    await assertFails(userFirestore.doc(`users/${unknownId}`).get())
   })
 
   it('lists users', async () => {