Simplify rules for reading api clients

oslokommune · Nov 2, 2023 · e9bfa45 · e9bfa45
1 parent 5b863c7
commit e9bfa45
Show file tree

Hide file tree

Showing 3 changed files with 19 additions and 45 deletions.
diff --git a/firestore.rules b/firestore.rules
@@ -217,23 +217,7 @@ service cloud.firestore {
     }
 
     match /apiClients/{document} {
-      function isAdminOfQueriedParent() {
-        let userDoc = getUserDoc();
-        let userIsAdmin = isAdmin();
-        let parentDoc = getAfter(resource.data.parent);
-        let hasOrgDocumentInParent = 'organization' in parentDoc.data;
-        let isAdminFromOrgOfProdOrDep = userIsAdmin && hasOrgDocumentInParent && getAfter(parentDoc.data.organization).id in userDoc.data.admin;
-        let isAdminOfParent = userIsAdmin && !isAdminFromOrgOfProdOrDep && parentDoc.data.id in userDoc.data.admin;
-        return isSignedIn() && (isAdminFromOrgOfProdOrDep || isAdminOfParent);
-      }
-      function isMemberOfQueriedParent() {
-        let userRef = /databases/$(database)/documents/users/$(request.auth.token.email);
-        let parentDoc = getAfter(resource.data.parent);
-        let userIsMemberOfParent = userRef in parentDoc.data.team;
-        return isSignedIn() && userIsMemberOfParent;
-      }
-
-      allow read: if isSuperAdmin() || isMemberOfQueriedParent() || isAdminOfQueriedParent();
+      allow read: if isSignedIn();
       allow write: if isSuperAdmin() || isMemberOfParent(document, 'apiClients') || isAdminOfParent(document, 'apiClients');
 
       match /secrets/{key} {

diff --git a/tests/firebase/firestore.test.js b/tests/firebase/firestore.test.js
@@ -48,7 +48,8 @@ describe('Test Firestore rules', () => {
       await apiClients
         .doc('organization-api-client')
         .collection('secrets')
-        .add({ secret: 'bar' });
+        .doc('organization-api-client-secret')
+        .set({ secret: 'bar' });
     });
   });
 
@@ -62,8 +63,7 @@ describe('Test Firestore rules', () => {
   test('users can read user other profiles', async () => {
     await withAuthenticatedUser(testEnv, '[email protected]', async (db) => {
       const user = db.collection('users').doc('[email protected]');
-      const result = await user.get();
-      expectGetSucceeds(result);
+      const result = await expectGetSucceeds(user.get());
       expect(result.data().name).toBe('User 2');
     });
   });
@@ -120,29 +120,20 @@ describe('Test Firestore rules', () => {
       const clients = db.collection('apiClients');
       await expectPermissionDenied(clients.get());
     });
-  });
 
-  test('users cannot read any random api client', async () => {
-    await withAuthenticatedUser(testEnv, '[email protected]', async (db) => {
-      const clients = db.collection('apiClients');
-      await expectPermissionDenied(clients.get());
+    await withUnauthenticatedUser(testEnv, async (db) => {
+      const client = db.collection('apiClients').doc('organization-api-client');
+      await expectPermissionDenied(client.get());
     });
   });
 
-  test('members can read own api clients', async () => {
+  test('signed in users can read api clients', async () => {
     await withAuthenticatedUser(testEnv, '[email protected]', async (db) => {
       const orgRef = db.collection('organizations').doc('organization');
       const clients = db.collection('apiClients').where('parent', '==', orgRef);
-      const result = await clients.get();
-      expectGetSucceeds(result);
+      const result = await expectGetSucceeds(clients.get());
       expect(result).toHaveProperty('size', 1);
     });
-
-    await withAuthenticatedUser(testEnv, '[email protected]', async (db) => {
-      const orgRef = db.collection('organizations').doc('organization');
-      const clients = db.collection('apiClients').where('parent', '==', orgRef);
-      await expectPermissionDenied(clients.get());
-    });
   });
 
   test('members can write own api clients and secrets', async () => {
@@ -161,15 +152,6 @@ describe('Test Firestore rules', () => {
     });
   });
 
-  test('super admin can read any api client', async () => {
-    await withAuthenticatedUser(testEnv, '[email protected]', async (db) => {
-      const clients = db.collection('apiClients');
-      const result = await clients.get();
-      expectGetSucceeds(result);
-      expect(result).toHaveProperty('size', 1);
-    });
-  });
-
   test('super admin can write any api client', async () => {
     await withAuthenticatedUser(testEnv, '[email protected]', async (db) => {
       const clients = db.collection('apiClients');
@@ -182,9 +164,15 @@ describe('Test Firestore rules', () => {
 
   test('no one can read api client secrets', async () => {
     async function readApiClientSecrets(db) {
-      const apiClientRef = db.collection('apiClients').doc('organization-api-client');
+      const apiClients = db.collection('apiClients');
+      const apiClientRef = apiClients.doc('organization-api-client');
       const apiClientSecrets = apiClientRef.collection('secrets');
       await expectPermissionDenied(apiClientSecrets.get());
+
+      const apiClientSecretRef = apiClientRef
+        .collection('secrets')
+        .doc('organization-api-client-secret');
+      await expectPermissionDenied(apiClientSecretRef.get());
     }
 
     await withUnauthenticatedUser(testEnv, readApiClientSecrets);

diff --git a/tests/firebase/utils.js b/tests/firebase/utils.js
@@ -21,5 +21,7 @@ export async function expectUpdateSucceeds(promise) {
 }
 
 export async function expectGetSucceeds(promise) {
-  expect(assertSucceeds(promise)).not.toBeUndefined();
+  const successResult = await assertSucceeds(promise);
+  expect(successResult).not.toBeUndefined();
+  return successResult;
 }