Fix upload image error for merchant store owner

[impactmass]

Resolves #2888

Change Introduced:

• Ensure that userId is passed into getShopId(). 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 primaryShop (hence why it has been working for the primary shop owner).

What else might be affected by this change:

• The above change was made for the Security's defined method for ifHasRoleForActiveShop and ifFileBelongsToShop (see diff). This is place on a few other collections (part from Media), I tested around the app to confirm no side effects.
  And based on the way getShopId is setup, it first tries to use it's internal activeUserId first before falling to the passed in one. So if the userId changed here isn't available, it's previous setup should still be intact.
  Please raise concerns if any.

How To Test:

• Create a merchant store (either by logging in as guest and using "Become A Seller button" or via Dashboard > Shop > Marketplace in primary shop).
• While logged into the merchant store as it's owner, create product and add an image to that product.
• Confirm the upload works and image is uploaded successfully.

[impactmass]

@spencern reminder on this

[spencern]

Thanks for the reminder @impactmass - pulling now.

[spencern]

This works, but I have one security concern that's detailed in a comment. Because this is directly related to a security, we need to investigate further.

[spencern]

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. 👍

[spencern]

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.

[impactmass]

Thanks. I'll get back to this shortly asap

[impactmass]

I added passing the userId into ifShopIdMatches and ifShopIdMatchesThisId (i.e the two other methods using Reaction.getShopId in the file. I tested all around creating product + image upload, editing shop detail, and editing account detail.

I also left comments everywhere getshoopId is called in the file, for easy reference in the future when we need to use getshopid in there.

This can be tested again.

[spencern]

Looks good. Checking the same shop context in each method ensures we won't end up checking different shops in the same chain and permitting users to act on documents they don't have permissions on.