chore(server): Prepare access interfaces for bulk permission checks

[adamantike]

This change adds the AccessCore.getAllowedIds method, to evaluate permissions in bulk, along with some other getAllowedIds* private methods.

The added methods still calculate permissions by id, and are not optimized to reduce the amount of queries and execution time, which will be implemented in separate pull requests.

Services that were evaluating permissions in a loop have been refactored to make use of the bulk approach.

[jrasm91]

What I liked about how this worked before was how clean and intuitive the interfaces were for checking access. It is a bit more complicated now.

What do you think about adding a new method checkAccess that returns back a map of IDs mapped to whether or not they have access.

Then the change everywhere else is removing the lookup from inside the loop, moving it to check access at the top, and replacing the line in the loop with access[id] instead.

[adamantike]

I think the approaches we are considering are very similar, if I improve the current getAllowedIds function name to checkAccess, as you suggest.

I considered returning a map, but found a few inconveniences that would make the solution less performant and more complex for some scenarios:

• The current logic for hasPermission requires returning a boolean depending on whether the user has access to all provided ids. By returning a map, we would need to iterate over all its values, looking for any false. With a set, we only validate its size is equal to the amount of received ids.
• Same scenario for hasAny, which would also involve iterating over the map values, and only returning early when true is found.
• I found some checks where the same id is evaluated for multiple permissions, and if any of them is valid, the check is successful. Having a map for each of those checks would require, in the worst scenario, numAssets * numPermissions lookups, as we would need access1[id] || access2[id] || ... for each of them. I'm thinking about running a single set union in those scenarios, and reduce the complexity to a single lookup per asset.

[jrasm91]

Maybe I am just being picky, but it seems like you have added a lot of boilerplate code to work with sets, specifically on the calling side. I just want the code to be clean, concise, and easy to read, and I feel like these changes, right now at least, make it less simple.

Maybe a few small changes would help improve things:

• Rename hasPermission to checkAccess
• Update checkAccess to accept a list instead of a set
• requirePermission throws an error if the checkAccess length mismatches`
• Remove the hasAny implementation and swap it out with two separate calls to checkAccess (it is only used in one place)

What do you think?

[adamantike]

Please, I really appreciate the feedback!

it seems like you have added a lot of boilerplate code to work with sets, specifically on the calling side.

I'm not very familiar with TypeScript, but ideally this would be an Iterable<string> to handle any structure that can be iterated over. I didn't go the Protocol-based way, as I didn't find a clean approach to get that, while excluding string from being considered an iterable too.

I have changed the signature to Set<string> | string[] to support both at the moment, as I think the small addition to support both is worth it in case we want to chain calls to checkAccess.

Maybe a few small changes would help improve things

Great suggestions! I have implemented all of them, so now there's only checkAccess and requirePermission as the public permissions API.

[jrasm91]

This change is pretty small, we can merge it as is, or because it is pretty small we can look at porting some calls to bulk here as well.

[jrasm91]

This looks good to me.

[jrasm91]

This works, it might be a bit cleaner if these were two separate sets, like canRemove or canShare and then it was used below like const hasAccess = canRemove.has(id) || canShare.has(id), but overall this looks good too.

[adamantike]

Do you find this more readable, while we still maintain a single set lookup per id?

const canRemove = await this.access.checkAccess(authUser, Permission.ALBUM_REMOVE_ASSET, existingAssetIds);
const canShare = await this.access.checkAccess(authUser, Permission.ASSET_SHARE, existingAssetIds);
const allowedAssetIds = new Set([...canRemove, ...canShare]);

[jrasm91]

That looks better to me, yes. Thanks