From 67e7111bce03524757afb020b7534aa31ac6fcae Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Simen=20Heggest=C3=B8yl?=
 <simen.heggestoyl@origo.oslo.kommune.no>
Date: Tue, 29 Aug 2023 10:43:55 +0200
Subject: [PATCH] Fix create/edit/delete rights for org admins

Fix several bugs in the `isAdminOfParent` Firestore rule check to make
organization admins able to create/edit/delete items within their
organizations (again?).
---
 CHANGELOG.md    |  4 ++++
 firestore.rules | 25 +++++++++++++++++--------
 2 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index f19cccb68..5a86b619e 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,6 +4,10 @@ All notable changes to this project will be documented in this file. The format
 
 ## [UNRELEASED]
 
+### Fixed
+
+- Fixed create/edit/delete rights for organization admins.
+
 ## [3.9.0] 2023-09-01
 
 ### Added
diff --git a/firestore.rules b/firestore.rules
index 24b3ab06b..efaf087c1 100644
--- a/firestore.rules
+++ b/firestore.rules
@@ -39,19 +39,28 @@ service cloud.firestore {
       return isSignedIn() && (isAdminOfOrg || isAdminFromOrgOfProdOrDep);
     }
 
-    // Same as isMemberOfParent but to check if you are an admin instead of the organization the parent is a part of
+    /**
+     * Return true if the current user is an admin of the organization that
+     * `document` belongs to.
+     *
+     * The document can belong to an organization either by a transitive link
+     * (`document` → `parent` → `organization`) or directly
+     * (`document` → `parent`).
+     */
     function isAdminOfParent(document, type) {
       let userDoc = getUserDoc();
       let userIsAdmin = isAdmin();
+      let item = getAfter(/databases/$(database)/documents/$(type)/$(document));
+      let parentDoc = getAfter(item.data.parent);
 
-      let parentDoc = getAfter(get(/databases/$(database)/documents/$(type)/$(document)).data.parent);
-
-      // Check if the parentDoc the user tries to access has an organization - this means that is is a product or department
+      // Check whether the parent of the document the user tries to access has
+      // an organization – this means that it is a product or department.
       let hasOrgDocumentInParent = 'organization' in parentDoc.data;
-      let isAdminFromOrgOfProdOrDep = userIsAdmin && hasOrgDocumentInParent && getAfter(parentDoc.data.organization).data.id in userDoc.data.admin;
+      let isAdminFromOrgOfProdOrDep = userIsAdmin && hasOrgDocumentInParent && getAfter(parentDoc.data.organization).id in userDoc.data.admin;
 
-      // Check if the user has access to the parentDoc which is an organization (given that the first check is false)
-      let isAdminOfParent = userIsAdmin && !isAdminFromOrgOfProdOrDep && parentDoc.data.id in userDoc.data.admin;
+      // Check whether the user has access to the parent, which is an
+      // organization (given that the first check is false).
+      let isAdminOfParent = userIsAdmin && !isAdminFromOrgOfProdOrDep && parentDoc.id in userDoc.data.admin;
 
       return isSignedIn() && (isAdminFromOrgOfProdOrDep || isAdminOfParent);
     }
@@ -158,7 +167,7 @@ service cloud.firestore {
       allow read: if isSignedIn();
       allow create: if isSuperAdmin() || isMemberOfParent(document, 'kpis') || isAdminOfParent(document, 'kpis');
       allow update: if isSuperAdmin() || isMemberOfParent(document, 'kpis') || isAdminOfParent(document, 'kpis');
-      allow delete: if isSuperAdmin() || isMemberOfParent(document, 'kpis') || isAdminOfParent(document, 'kpis');
+      allow delete: if isSuperAdmin();
     }
 
     match /kpis/{document}/progress/{progress} {