Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 7 commits into
base: alpha
Choose a base branch
from

Conversation

mstniy
Copy link
Contributor

@mstniy mstniy commented Sep 6, 2024

Pull Request

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)

Copy link

Thanks for opening this pull request!

Copy link

codecov bot commented Sep 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.49%. Comparing base (435f0d1) to head (0cbeba6).
Report is 2 commits behind head on alpha.

Additional details and impacted files
@@           Coverage Diff           @@
##            alpha    #9299   +/-   ##
=======================================
  Coverage   93.49%   93.49%           
=======================================
  Files         186      186           
  Lines       14807    14810    +3     
=======================================
+ Hits        13844    13847    +3     
  Misses        963      963           

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

spec/CloudCode.spec.js Outdated Show resolved Hide resolved
spec/CloudCode.spec.js Outdated Show resolved Hide resolved
spec/CloudCode.spec.js Outdated Show resolved Hide resolved
@mstniy
Copy link
Contributor Author

mstniy commented Sep 10, 2024

Done @mtrezza

@mtrezza
Copy link
Member

mtrezza commented Sep 16, 2024

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
Copy link
Contributor Author

mstniy commented Sep 16, 2024

Done @mtrezza

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

@mtrezza
Copy link
Member

mtrezza commented Sep 16, 2024

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:

image

@mtrezza
Copy link
Member

mtrezza commented Sep 16, 2024

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
Copy link
Contributor Author

mstniy commented Sep 18, 2024

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.

@mtrezza
Copy link
Member

mtrezza commented Sep 25, 2024

Maybe adding this to the Parse.User object is not a good idea, as this property wouldn't exist on other Parse.User objects, which creates documentation issues, etc. How about just renaming it to getUserRoles() and leave it on the root of the req object?

I think this method needs clarification in the docs that this is cached. And what level of nested roles it returns. For example, for the following relations:

  • role A is member of role B
  • user A is member of role A

getUserRoles() would only return role A, not role A and role B, correct?

@mstniy
Copy link
Contributor Author

mstniy commented Sep 25, 2024

How about just renaming it to getUserRoles() and leave it on the root of the req object?

Done

@mstniy
Copy link
Contributor Author

mstniy commented Sep 25, 2024

getUserRoles() would only return role A, not role A and role B, correct?

It is recursive, as the Parse cache is, so it would return both. I added this case to the tests.

@mstniy mstniy requested a review from mtrezza September 25, 2024 15:05
* @example
* stripACLRolePrefix("role:myrole") // Returns "myrole"
*/
static stripACLRolePrefix(entry) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies if my earlier comment wasn't clear. I meant to add a method getUserRoleIds() to where getUserRoles() currently is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Let cloud code access user roles
2 participants