Update rules

StanfordBDHG · Jan 16, 2025 · 87206b1 · 87206b1
1 parent 3b06e24
commit 87206b1
Show file tree

Hide file tree

Showing 2 changed files with 43 additions and 10 deletions.
diff --git a/firestore.rules b/firestore.rules
@@ -121,15 +121,18 @@ service cloud.firestore {
       }
 
       function isAllowedUpdateWithinOrganization() {
-        return resource.data.type in ['patient']
-          ? isOwnerOf(resource.data.organization)
-          : isOwnerOrClinicianOf(resource.data.organization) && resource.data.type in ['clinician'];
+        return valueExists(resource.data, 'type') ? 
+          (
+            resource.data.type in ['patient']
+            ? isOwnerOf(resource.data.organization)
+            : isOwnerOrClinicianOf(resource.data.organization) && resource.data.type in ['clinician']
+          ) : false;
       }
 
       allow read: if isAdmin()
-        || (resource == null && isAuthenticated())
         || (request.auth != null && request.auth.uid == userId)
-        || isOwnerOrClinicianOf(resource.data.organization);
+        || (resource == null && isAuthenticated())
+        || (resource != null && valueExists(resource.data, 'organization') && isOwnerOrClinicianOf(resource.data.organization));
 
       allow create: if isAdmin()
         || (isUser(userId) && !valueExists(request.resource.data, 'organization') && !valueExists(request.resource.data, 'type'));

diff --git a/functions/src/tests/rules/users.test.ts b/functions/src/tests/rules/users.test.ts
@@ -28,13 +28,15 @@ describe('firestore.rules: users/{userId}', () => {
   const patientId = 'mockPatient'
   const userId = 'mockUser'
   const unknownId = 'mockUnknown'
+  const disabledUserId = 'disabledMockUser'
 
   let testEnvironment: RulesTestEnvironment
   let adminFirestore: firebase.firestore.Firestore
   let ownerFirestore: firebase.firestore.Firestore
   let clinicianFirestore: firebase.firestore.Firestore
   let patientFirestore: firebase.firestore.Firestore
   let userFirestore: firebase.firestore.Firestore
+  let disabledUserFirestore: firebase.firestore.Firestore
 
   before(async () => {
     testEnvironment = await initializeTestEnvironment({
@@ -72,6 +74,14 @@ describe('firestore.rules: users/{userId}', () => {
       .firestore()
 
     userFirestore = testEnvironment.authenticatedContext(userId, {}).firestore()
+
+    disabledUserFirestore = testEnvironment
+      .authenticatedContext(disabledUserId, {
+        type: UserType.patient,
+        organization: organizationId,
+        disabled: true,
+      })
+      .firestore()
   })
 
   beforeEach(async () => {
@@ -91,6 +101,11 @@ describe('firestore.rules: users/{userId}', () => {
         .doc(`users/${patientId}`)
         .set({ type: UserType.patient, organization: organizationId })
       await firestore.doc(`users/${userId}`).set({})
+      await firestore.doc(`users/${disabledUserId}`).set({
+        type: UserType.patient,
+        organization: organizationId,
+        disabled: true,
+      })
     })
   })
 
@@ -99,45 +114,57 @@ 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())
+    await assertSucceeds(adminFirestore.doc(`users/${disabledUserId}`).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())
+    await assertSucceeds(ownerFirestore.doc(`users/${disabledUserId}`).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())
+    await assertSucceeds(
+      clinicianFirestore.doc(`users/${disabledUserId}`).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())
+    await assertFails(patientFirestore.doc(`users/${disabledUserId}`).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())
+    await assertFails(userFirestore.doc(`users/${disabledUserId}`).get())
+
+    await assertFails(disabledUserFirestore.doc(`users/${adminId}`).get())
+    await assertFails(disabledUserFirestore.doc(`users/${ownerId}`).get())
+    await assertFails(disabledUserFirestore.doc(`users/${clinicianId}`).get())
+    await assertFails(disabledUserFirestore.doc(`users/${patientId}`).get())
+    await assertFails(disabledUserFirestore.doc(`users/${userId}`).get())
+    await assertFails(disabledUserFirestore.doc(`users/${unknownId}`).get())
+    await assertSucceeds(
+      disabledUserFirestore.doc(`users/${disabledUserId}`).get(),
+    )
   })
 
   it('lists users', async () => {
@@ -211,6 +238,9 @@ describe('firestore.rules: users/{userId}', () => {
       await environment.firestore().doc(`users/${userId}`).delete()
     })
     await assertFails(userFirestore.doc(`users/${userId}`).set({}))
+    await assertFails(
+      disabledUserFirestore.doc(`users/${disabledUserId}`).set({}),
+    )
   })
 
   it('updates users/{userId} as admin', async () => {