From e9bfa455d956a1a1f1658e54a30d0ef9c596517a Mon Sep 17 00:00:00 2001 From: "Petter H. Juliussen" Date: Thu, 2 Nov 2023 13:13:08 +0100 Subject: [PATCH] Simplify rules for reading api clients --- firestore.rules | 18 +------------- tests/firebase/firestore.test.js | 42 ++++++++++++-------------------- tests/firebase/utils.js | 4 ++- 3 files changed, 19 insertions(+), 45 deletions(-) diff --git a/firestore.rules b/firestore.rules index 92186b0c8..c9ac22e9c 100644 --- 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 index b3633f7fd..e4ffdaca0 100644 --- 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, 'user@example.com', async (db) => { const user = db.collection('users').doc('user2@example.com'); - 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, 'user@example.com', 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, 'user@example.com', 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, 'user2@example.com', 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, 'superadmin@example.com', 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, 'superadmin@example.com', 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 index de92018c9..d06d63c43 100644 --- 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; }