(core) fix bug where sharing doc with everyone@ as viewer made it unlisted for site viewers

Summary:
Shares of the same role (e.g. viewer) at different levels could interact for a resource (e.g. a doc) shared with everyone@, potentially blocking the listing of that resource. This diff removes the interaction.

The permission of a user on a resource is calculated by finding all acl rules that link that resource to a group to which the user belongs, or to a group that has a subgroup to which the user belongs, etc, and then bitwise-or-ing the permissions on the acl rules. A later wrinkle was to allow public sharing via special users. A still later wrinkle was to avoid listing resources if they were only shared with the special everyone@ user, while allowing access to them if user has their full link. That wrinkle had a bug, where if e.g. a doc were shared with everyone@ as a viewer, and the org the doc was in was shared with someone@ as a viewer, and the doc inherited the org permissions via a workspace, then that doc would end up not being listed.

The fix is straightforward enough, but needs different code for postgres and sqlite, and is a bit verbose because we unwrap subgroups to a few levels rather than doing recursion (which looks cleaner but was slower in benchmarks).

Test Plan: added test that fails without this fix

Reviewers: georgegevoian

Reviewed By: georgegevoian

Differential Revision: https://phab.getgrist.com/D3095
This commit is contained in:
Paul Fitzpatrick
2021-10-27 18:20:40 -04:00
parent e58df5df5b
commit 35e18cc0ad
2 changed files with 65 additions and 9 deletions

View File

@@ -42,6 +42,39 @@ export function bitOr(dbType: DatabaseType, column: string, bits: number): strin
}
}
/**
* Checks if a set of columns contains only the given ids (or null).
* Uses array containment operator on postgres (with array_remove to deal with nulls),
* and a clunkier syntax for sqlite.
*/
export function hasOnlyTheseIdsOrNull(dbType: DatabaseType, ids: number[], columns: string[]): string {
switch (dbType) {
case 'postgres':
return `array[${ids.join(',')}] @> array_remove(array[${columns.join(',')}],null)`;
case 'sqlite':
return columns.map(col => `coalesce(${col} in (${ids.join(',')}), true)`).join(' AND ');
default:
throw new Error(`hasOnlyTheseIdsOrNull not implemented for ${dbType}`);
}
}
/**
* Checks if at least one of a set of ids is present in a set of columns.
* There must be at least one id and one column.
* Uses the intersection operator on postgres, and a clunkier syntax for sqlite.
*/
export function hasAtLeastOneOfTheseIds(dbType: DatabaseType, ids: number[], columns: string[]): string {
switch (dbType) {
case 'postgres':
return `array[${ids.join(',')}] && array[${columns.join(',')}]`;
case 'sqlite':
return ids.map(id => `${id} in (${columns.join(',')})`).join(' OR ');
default:
throw new Error(`hasAtLeastOneOfTheseIds not implemented for ${dbType}`);
}
}
/**
* Convert a json value returned by the database into a javascript
* object. For postgres, the value is already unpacked, but for sqlite