From a0fc11c8d13af9084c72813f28cedbae4f29d3ca Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jaros=C5=82aw=20Sadzi=C5=84ski?=
 <jaroslaw.sadzinski@gmail.com>
Date: Tue, 5 Sep 2023 11:11:46 +0200
Subject: [PATCH] (core) Fixing memos in record dependent rules.

Summary:
Memos in row dependent rules were ignored if the action was denied unconditionally. It prevented creating different memos for different users.
Now ACL is forced to check each row, to find the correct memo to show when create/update/delete action is denied.

Test Plan: Existing and new.

Reviewers: paulfitz, dsagal

Reviewed By: paulfitz, dsagal

Subscribers: dsagal

Differential Revision: https://phab.getgrist.com/D4024
---
 app/server/lib/PermissionInfo.ts | 39 +++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/app/server/lib/PermissionInfo.ts b/app/server/lib/PermissionInfo.ts
index 897dc8d5..164db7ad 100644
--- a/app/server/lib/PermissionInfo.ts
+++ b/app/server/lib/PermissionInfo.ts
@@ -202,7 +202,8 @@ export class PermissionInfo extends RuleInfo<MixedPermissionSet, TablePermission
 
 /**
  * Evaluate a RuleSet on a given input (user and optionally record). If a record is needed but not
- * included, the result may include permission values like 'allowSome', 'denySome'.
+ * included, the result may include permission values like 'allowSome', 'denySome', or 'mixed' (for
+ * rules with memo).
  */
 function evaluateRule(ruleSet: RuleSet, input: AclMatchInput): PartialPermissionSet {
   let pset: PartialPermissionSet = emptyPermissionSet();
@@ -214,6 +215,42 @@ function evaluateRule(ruleSet: RuleSet, input: AclMatchInput): PartialPermission
     } catch (e) {
       if (e.code === 'NEED_ROW_DATA') {
         pset = mergePartialPermissions(pset, makePartialPermissions(rule.permissions));
+        if (rule.memo) {
+          // Quick reminder:
+          // - memos are only shown for denies, if user can't update/delete/create, they are not shown when user
+          //   can't read. Schema permissions are not row dependent.
+          // - memos can be extracted if ACL allows something, but they are not shown.
+          // - partial permissions are merged, so denySome + deny = deny, and allowSome + allow = allow.
+          // - but allow + denySome + deny = mixed, and allow + allowSome + deny = mixed.
+          // - mixed is a final state, it can't be combined with anything else and disables any optimizations (forces
+          //   row checks).
+          // - allowSome and denySome are not final states, they will be replaced by allow/deny/mixed.
+
+          // If rule has a memo, it will be shown if user is denied access to something, only if:
+          // - this rule denied this access
+          // - this rule would have allowed this access, but it didn't pass (e.g. row check failed, different user, etc)
+          //
+          // But there is one problem. If there is mix of deny and denySome (so some rules deny access based on the row,
+          // and some denies access unconditionally - or based on a user), the overall access is denied, and there is no
+          // reason to know exactly which row dependent rule denied it. So the access is denied at table/column level,
+          // without actually scanning the rows. This is a good optimization, but it means that we won't be able to show
+          // the memo, because we won't have the row data (rec in input) to test which rule (a row dependent rule)
+          // matched the data (see extractMemos below).
+
+          // To fix that, we need to convert denySome to mixed, which is a final state, and will force row checks, as
+          // the optimizer won't be able to tell if the access is denied or not, without actually scanning each row that
+          // is touched in the bundle. With that, we will be able to determine which rule denied the access, as the
+          // check will be performed for each row.
+
+          // We don't need to do that for allowSome, as this bit is converted only to "allow" or "mixed". When it is
+          // "allow", the memos won't be shown, and when it is "mixed", rows will be scanned anyway.
+
+          // We'll replace only denySome in create/update/delete bits. read doesn't show memo and schemaEdit is not row
+          // dependent.
+          const dataChangePerms: Array<keyof PermissionSet> = ['create', 'update', 'delete'];
+          const changesData = (perm: string) => dataChangePerms.includes(perm as keyof PermissionSet);
+          pset = mapValues(pset, (val, perm) => val === 'denySome' && changesData(perm) ? "mixed" : val);
+        }
       } else {
         // Unexpected error. Interpret rule pessimistically.
         // Anything it would explicitly allow, no longer allow through this rule.