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.

Sign up for GitHub

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

Jump to bottom

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

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

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
17999b7
Cloud code and triggers: Pass the set of roles the user has
mstniy Sep 6, 2024
4171665
Update spec/CloudCode.spec.js
mstniy Sep 10, 2024
c63e4d7
Update spec/CloudCode.spec.js
mstniy Sep 10, 2024
b661fc9
Use arrow functions
mstniy Sep 10, 2024
7f7d5e5
Merge branch 'alpha' into cloud_getroles_mr
mstniy Sep 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 78 additions & 0 deletions spec/CloudCode.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1163,6 +1163,53 @@ describe('Cloud Code', () => {
);
});

it('test save triggers get user roles', async () => {
let beforeSaveFlag = false;
let afterSaveFlag = false;
Parse.Cloud.beforeSave('SaveTriggerUserRoles', async req => {
expect(await req.getRoles()).toEqual(['TestRole']);
beforeSaveFlag = true;
});

Parse.Cloud.afterSave('SaveTriggerUserRoles', async req => {
expect(await req.getRoles()).toEqual(['TestRole']);
afterSaveFlag = true;
});

const user = new Parse.User();
user.set('password', 'asdf');
user.set('email', 'asdf@example.com');
user.set('username', 'zxcv');
await user.signUp();
const role = new Parse.Role('TestRole', new Parse.ACL({ '*': { read: true, write: true } }));
role.getUsers().add(user);
await role.save();

const obj = new Parse.Object('SaveTriggerUserRoles');
await obj.save();
expect(beforeSaveFlag).toBeTrue();
expect(afterSaveFlag).toBeTrue();
});

it('should not have user roles for anonymous calls', async () => {
let beforeSaveFlag = false;
let afterSaveFlag = false;
Parse.Cloud.beforeSave('SaveTriggerUserRoles', async req => {
expect(req.getRoles).toBeUndefined();
beforeSaveFlag = true;
});

Parse.Cloud.afterSave('SaveTriggerUserRoles', async req => {
expect(req.getRoles).toBeUndefined();
afterSaveFlag = true;
});

const obj = new Parse.Object('SaveTriggerUserRoles');
await obj.save();
expect(beforeSaveFlag).toBeTrue();
expect(afterSaveFlag).toBeTrue();
});

it('beforeSave change propagates through the save response', done => {
Parse.Cloud.beforeSave('ChangingObject', function (request) {
request.object.set('foo', 'baz');
Expand Down Expand Up @@ -2014,6 +2061,37 @@ describe('cloud functions', () => {

Parse.Cloud.run('myFunction', {}).then(() => done());
});

it('should have user roles', async () => {
let flag = false;
Parse.Cloud.define('myFunction', async req => {
expect(await req.getRoles()).toEqual(['TestRole']);
flag = true;
});

const user = new Parse.User();
user.set('password', 'asdf');
user.set('email', 'asdf@example.com');
user.set('username', 'zxcv');
await user.signUp();
const role = new Parse.Role('TestRole', new Parse.ACL({ '*': { read: true, write: true } }));
role.getUsers().add(user);
await role.save();

await Parse.Cloud.run('myFunction', { sessionToken: user.getSessionToken() });
expect(flag).toBeTrue();
});

it('should not have user roles for anonymous calls', async () => {
let flag = false;
Parse.Cloud.define('myFunction', async req => {
expect(req.getRoles).toBeUndefined();
flag = true;
});

await Parse.Cloud.run('myFunction');
expect(flag).toBeTrue();
});
});

describe('beforeSave hooks', () => {
Expand Down
6 changes: 6 additions & 0 deletions src/Routers/FunctionsRouter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,12 @@ export class FunctionsRouter extends PromiseRouter {
params: params,
master: req.auth && req.auth.isMaster,
user: req.auth && req.auth.user,
getRoles:
req.auth && req.auth.user
? async () => {
return (await req.auth.getUserRoles()).map(r => r.substr('role:'.length));
}
: undefined,
Comment on lines +134 to +139
Copy link
Member

@mtrezza mtrezza Sep 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@mstniy mstniy Sep 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@mtrezza mtrezza Sep 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@mstniy mstniy Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

installationId: req.info.installationId,
log: req.config.loggerController,
headers: req.config.headers,
Expand Down
3 changes: 3 additions & 0 deletions src/triggers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,9 @@ export function getRequestObject(
}
if (auth.user) {
request['user'] = auth.user;
request['getRoles'] = async () => {
return (await auth.getUserRoles()).map(r => r.substr('role:'.length));
};
}
if (auth.installationId) {
request['installationId'] = auth.installationId;
Expand Down
Loading