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

Fix upload image error for merchant store owner #2918

Merged
merged 12 commits into from
Sep 28, 2017
Merged

Fix upload image error for merchant store owner #2918

Changes from 10 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
bb27df0
Fix security.defineMethod's incorrect call of getShopId
impactmass Sep 21, 2017
0738ca6
Merge branch 'marketplace' into seun-fix-image-upload
impactmass Sep 21, 2017
badb44a
Merge branch 'marketplace' into seun-fix-image-upload
impactmass Sep 25, 2017
aee33e4
Merge branch 'marketplace' into seun-fix-image-upload
spencern Sep 27, 2017
728e7ba
Merge branch 'marketplace' into seun-fix-image-upload
spencern Sep 27, 2017
60f782c
Merge branch 'marketplace' into seun-fix-image-upload
spencern Sep 27, 2017
a2983ce
Merge branch 'marketplace' into seun-fix-image-upload
spencern Sep 28, 2017
1c9f221
Pass userId to ifShopIdMatchesThisId and ifShopIdMatches; add comments
impactmass Sep 28, 2017
6ab000b
Merge branch 'marketplace' into seun-fix-image-upload
impactmass Sep 28, 2017
c5cc4bc
Merge branch 'marketplace' into seun-fix-image-upload
spencern Sep 28, 2017
5045b38
Merge branch 'marketplace' into seun-fix-image-upload
spencern Sep 28, 2017
38ffb9b
Merge branch 'marketplace' into seun-fix-image-upload
spencern Sep 28, 2017
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
16 changes: 12 additions & 4 deletions server/security/collections.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,9 @@ export default function () {
allow(type, arg, userId) {
if (!arg) throw new Error("ifHasRole security rule method requires an argument");
if (arg.role) {
return Roles.userIsInRole(userId, arg.role, Reaction.getShopId());
// Note: userId is passed to getShopId to ensure that it returns the correct shop based on the User Preference
// if not passed, getShopId can default to primaryShopId if Meteor.userId is not available in the context the code is run
return Roles.userIsInRole(userId, arg.role, Reaction.getShopId(userId));
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a great catch - I thought that getShopId was finding the user preferred shop, but it seems that there was a case with CFS uploads where the correct shopId was not getting found. Presumably because Meteor.userId() was not available.

Thanks for adding the comments too. 👍

Copy link
Contributor

@spencern spencern Sep 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Adding a separate security review here because this code directly affects allow/deny methods:

This update from using the default getShopId to passing in the userId changes the shop that the passed in role (arg.role) is being checked for. This results in the user's active shop being checked against in cases where that shop is different than the default shop.

Any place where we're allowing db operations from the client has potential to be a security hole, so it's imperative that we're checking both that a user has the correct role - which this is doing for the active shop, and that the document being updated belongs to the shop that we are checking the role of the user for. As long as those two things are in alignment, we shouldn't see any security vulnerabilities such as permitting a user with a role for shopA to affect a document that belongs to shopB.

In this PR, we're changing ifFileBelongsToShop as well as ifHasRoleForActiveShop, but we're not changing ifShopIdMatches or ifShopIdMatchesThisId

My only concern here would be that somehow we end up checking the user roles against shopA and checking that a document belongs to a different shop, shopB which would permit a user to perform an operation against a file they don't own.

I believe that we should update ifShopIdMatches and ifShopIdMatchesThisId to have the userId explicitly passed as well, even though they are likely getting the correct shopId anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I'll get back to this shortly asap

}
return Roles.userIsInRole(userId, arg);
}
Expand All @@ -57,22 +59,28 @@ export default function () {
Security.defineMethod("ifShopIdMatches", {
fetch: [],
deny: function (type, arg, userId, doc) {
return doc.shopId !== Reaction.getShopId();
// Note: userId is passed to getShopId to ensure that it returns the correct shop based on the User Preference
// if not passed, getShopId can default to primaryShopId if Meteor.userId is not available in the context the code is run
return doc.shopId !== Reaction.getShopId(userId);
}
});
// this rule is for the Shops collection
// use ifShopIdMatches for match on this._id
Security.defineMethod("ifShopIdMatchesThisId", {
fetch: [],
deny: function (type, arg, userId, doc) {
return doc._id !== Reaction.getShopId();
// Note: userId is passed to getShopId to ensure that it returns the correct shop based on the User Preference
// if not passed, getShopId can default to primaryShopId if Meteor.userId is not available in the context the code is run
return doc._id !== Reaction.getShopId(userId);
}
});

Security.defineMethod("ifFileBelongsToShop", {
fetch: [],
deny: function (type, arg, userId, doc) {
return doc.metadata.shopId !== Reaction.getShopId();
// Note: userId is passed to getShopId to ensure that it returns the correct shop based on the User Preference
// if not passed, getShopId can default to primaryShopId if Meteor.userId is not available in the context the code is run
return doc.metadata.shopId !== Reaction.getShopId(userId);
}
});

Expand Down