From 8bd8b0ac61eb96bde165bdfd36a1224b5f57e1b7 Mon Sep 17 00:00:00 2001 From: Paul Date: Wed, 25 Sep 2024 10:52:38 -0700 Subject: [PATCH] Make sure to correctly allow access to user collections (#144) # Make sure to correctly allow access to user collections ## :recycle: Current situation & Problem @arkadiuszbachorski noticed that clinicians and/or owners do not have the expected access to some user collections. This PR should address this and safeguard future changes to the Firestore rules with a regression test. ## :gear: Release Notes *Add a bullet point list summary of the feature and possible migration guides if this is a breaking change so this section can be added to the release notes.* *Include code snippets that provide examples of the feature implemented or links to the documentation if it appends or changes the public interface.* ## :books: Documentation *Please ensure that you properly document any additions in conformance to [Spezi Documentation Guide](https://github.com/StanfordSpezi/.github/blob/main/DOCUMENTATIONGUIDE.md).* *You can use this section to describe your solution, but we encourage contributors to document your reasoning and changes using in-line documentation.* ## :white_check_mark: Testing *Please ensure that the PR meets the testing requirements set by CodeCov and that new functionality is appropriately tested.* *This section describes important information about the tests and why some elements might not be testable.* ### Code of Conduct & Contributing Guidelines By submitting creating this pull request, you agree to follow our [Code of Conduct](https://github.com/StanfordBDHG/.github/blob/main/CODE_OF_CONDUCT.md) and [Contributing Guidelines](https://github.com/StanfordBDHG/.github/blob/main/CONTRIBUTING.md): - [x] I agree to follow the [Code of Conduct](https://github.com/StanfordBDHG/.github/blob/main/CODE_OF_CONDUCT.md) and [Contributing Guidelines](https://github.com/StanfordBDHG/.github/blob/main/CONTRIBUTING.md). --- firestore.rules | 10 +- .../src/tests/rules/userCollections.test.ts | 136 ++++++++++++++++++ 2 files changed, 142 insertions(+), 4 deletions(-) create mode 100644 functions/src/tests/rules/userCollections.test.ts diff --git a/firestore.rules b/firestore.rules index b6453fee..eb47495f 100644 --- a/firestore.rules +++ b/firestore.rules @@ -129,8 +129,10 @@ service cloud.firestore { match /users/{userId}/{collectionName}/{documentId} { function isOwnerOrClinicianOfSameOrganization() { - let user = getUser(userId); - return isOwnerOrClinicianOf(user.organization); + let userDoc = getUser(userId); + return (userDoc.data != null) + && ('organization' in userDoc.data) + && isOwnerOrClinicianOf(userDoc.data.organization); } function isPatientWritableCollectionName() { @@ -143,8 +145,8 @@ service cloud.firestore { || collectionName in ['allergyIntolerances', 'appointments', 'medicationRequests']; } - allow read: if isAdmin() - || isUser(userId) + allow read: if isAdmin() + || isUser(userId) || isOwnerOrClinicianOfSameOrganization(); allow write: if isAdmin() diff --git a/functions/src/tests/rules/userCollections.test.ts b/functions/src/tests/rules/userCollections.test.ts new file mode 100644 index 00000000..52e46858 --- /dev/null +++ b/functions/src/tests/rules/userCollections.test.ts @@ -0,0 +1,136 @@ +// +// This source file is part of the ENGAGE-HF project based on the Stanford Spezi Template Application project +// +// SPDX-FileCopyrightText: 2023 Stanford University +// +// SPDX-License-Identifier: MIT +// + +import fs from 'fs' +import { + assertFails, + assertSucceeds, + initializeTestEnvironment, + type RulesTestEnvironment, +} from '@firebase/rules-unit-testing' +import { UserType } from '@stanfordbdhg/engagehf-models' +import type firebase from 'firebase/compat/app' +import { describe, it } from 'mocha' + +describe('firestore.rules: users/{userId}/{collectionName}/{documentId}', () => { + const organizationId = 'stanford' + const otherOrganizationId = 'jhu' + + const adminId = 'mockAdmin' + const ownerId = 'mockOwner' + const clinicianId = 'mockClinician' + const patientId = 'mockPatient' + const userId = 'mockUser' + + 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 + + before(async () => { + testEnvironment = await initializeTestEnvironment({ + projectId: 'stanford-bdhg-engage-hf', + firestore: { + rules: fs.readFileSync('../firestore.rules', 'utf8'), + host: 'localhost', + port: 8080, + }, + }) + + adminFirestore = testEnvironment + .authenticatedContext(adminId, { type: UserType.admin }) + .firestore() + + ownerFirestore = testEnvironment + .authenticatedContext(ownerId, { + type: UserType.owner, + organization: organizationId, + }) + .firestore() + + clinicianFirestore = testEnvironment + .authenticatedContext(clinicianId, { + organization: organizationId, + type: UserType.clinician, + }) + .firestore() + + patientFirestore = testEnvironment + .authenticatedContext(patientId, { + organization: organizationId, + type: UserType.patient, + }) + .firestore() + + userFirestore = testEnvironment.authenticatedContext(userId, {}).firestore() + }) + + beforeEach(async () => { + await testEnvironment.clearFirestore() + await testEnvironment.withSecurityRulesDisabled(async (environment) => { + const firestore = environment.firestore() + await firestore.doc(`organizations/${organizationId}`).set({}) + await firestore.doc(`organizations/${otherOrganizationId}`).set({}) + await firestore.doc(`users/${adminId}`).set({ type: UserType.admin }) + await firestore + .doc(`users/${ownerId}`) + .set({ organization: organizationId }) + await firestore + .doc(`users/${clinicianId}`) + .set({ type: UserType.clinician, organization: organizationId }) + await firestore + .doc(`users/${patientId}`) + .set({ type: UserType.patient, organization: organizationId }) + await firestore.doc(`users/${userId}`).set({}) + }) + }) + + after(async () => { + await testEnvironment.cleanup() + }) + + it('gets users/{userId}/allergyIntolerances', async () => { + const adminPath = `users/${adminId}/allergyIntolerances` + const ownerPath = `users/${ownerId}/allergyIntolerances` + const clinicianPath = `users/${clinicianId}/allergyIntolerances` + const patientPath = `users/${patientId}/allergyIntolerances` + const userPath = `users/${userId}/allergyIntolerances` + + await assertSucceeds(adminFirestore.collection(adminPath).get()) + await assertSucceeds(adminFirestore.collection(ownerPath).get()) + await assertSucceeds(adminFirestore.collection(clinicianPath).get()) + await assertSucceeds(adminFirestore.collection(patientPath).get()) + await assertSucceeds(adminFirestore.collection(userPath).get()) + + await assertFails(ownerFirestore.collection(adminPath).get()) + await assertSucceeds(ownerFirestore.collection(ownerPath).get()) + await assertSucceeds(ownerFirestore.collection(clinicianPath).get()) + await assertSucceeds(ownerFirestore.collection(patientPath).get()) + await assertFails(ownerFirestore.collection(userPath).get()) + + await assertFails(clinicianFirestore.collection(adminPath).get()) + await assertSucceeds(clinicianFirestore.collection(ownerPath).get()) + await assertSucceeds(clinicianFirestore.collection(clinicianPath).get()) + await assertSucceeds(clinicianFirestore.collection(patientPath).get()) + await assertFails(clinicianFirestore.collection(userPath).get()) + + await assertFails(patientFirestore.collection(adminPath).get()) + await assertFails(patientFirestore.collection(ownerPath).get()) + await assertFails(patientFirestore.collection(clinicianPath).get()) + await assertSucceeds(patientFirestore.collection(patientPath).get()) + await assertFails(patientFirestore.collection(userPath).get()) + + await assertFails(userFirestore.collection(adminPath).get()) + await assertFails(userFirestore.collection(ownerPath).get()) + await assertFails(userFirestore.collection(clinicianPath).get()) + await assertFails(userFirestore.collection(patientPath).get()) + await assertFails(userFirestore.collection(userPath).get()) + }) +})