feat: Pass the set of roles the user has to triggers and cloud functions

[mstniy]

Pull Request

• Report security issues confidentially.
• Any contribution is under this license.
• Link this pull request to an issue.

Issue

Closes: #9298

Approach

Expose a field getRoles to cloud functions and triggers if the caller is a user. The field, if it exists, is an async function that resolves to a list of strings, which is the set of roles the user has.

The exposed function uses Parse's internal cache of user roles, if it is populated. Otherwise, it queries the set of roles and populates the cache before returning them.

Tasks

• Add tests
• Add changes to documentation (guides, repository pages, code comments)

[parse-github-assistant]

Thanks for opening this pull request!

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.48%. Comparing base (435f0d1) to head (7f7d5e5).

Additional details and impacted files

@@            Coverage Diff             @@
##            alpha    #9299      +/-   ##
==========================================
- Coverage   93.49%   93.48%   -0.02%     
==========================================
  Files         186      186              
  Lines       14807    14810       +3     
==========================================
+ Hits        13844    13845       +1     
- Misses        963      965       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mstniy]

Done @mtrezza

[mtrezza]

Please rebase the PR; you disabled repo owner permission to do so, so every time there is a merge in the base branch, you'd need to update yourself to make sure the CI still passes.

[mstniy]

Done @mtrezza

No idea about the repo owner permission. Didn't change anything myself.

[mtrezza]

r.substr('role:'.length) looks strange, how is the role obj ID retrieved in other parts of the code?

[mstniy]

Note that here we merely construct an async function, so the performance impact should be negligible.

I am not immediately aware of any other ways to get the set of roles.

The substr operation removes the role: prefix, as this is already a set of roles.

[mtrezza]

Yes, I have edited the comment above, the only question is the prefix. There's no need for a .length of fixed string. You can just set the number and add a code comment what this is doing. But how is that done in other pars of the code? Is there no "cleaner" way to get the role obj ID from the string?

[mstniy]

I find it more readable to explicitly include the string that the code is trying to remove as a prefix. Are you concerned about the performance implications of computing the length of a fixed string, or is your feedback stylistic?

The codebase never seems to strip the role: prefix. The uses of the getUserRoles() function that I could find either match it against the ACL, which similarly prefixes the roles, or if matching to an existing role name, it adds the prefix to the other side.

[mtrezza]

No idea about the repo owner permission. Didn't change anything myself.

There is a checkbox for this in your PR on the right side, but mind the note about secrets:

[mtrezza]

Are the roles already fetched somewhere from the cache during the auth process? If so, we may be able to use them and set them on the req obj instead of the async function.

The obj structure seems strange in general. The roles are related to the user and we already expose the user in the req obj. Would it make more sense to expose the roles as a property of the user object?

[mstniy]

Are the roles already fetched somewhere from the cache during the auth process? If so, we may be able to use them and set them on the req obj instead of the async function.

They seem to be fetched for trigger calls, there we can have synchronous access to the set of roles. For cloud function calls, they are not. But even for trigger calls, I don't think it is sound architecture to give synchronous access to the set of roles to triggers, as Parse might change under which conditions it fetches the set of roles in the future. Then, supplying an async function is more future-proof.

The obj structure seems strange in general. The roles are related to the user and we already expose the user in the req obj. Would it make more sense to expose the roles as a property of the user object?

We could do that, given that currently there is currently no helper to get the set of roles a user has. Note, however, that this patch reads the set of roles from the auth object, and the lifetime of that is limited to that of the request, by design. Caching roles any longer would involve tradeoffs around staleness of data.