These are chat archives for reactioncommerce/reaction

13th
Dec 2015
newsiberian
@newsiberian
Dec 13 2015 04:41 UTC
@mikemurray, hello. I'm looking at addressBook methods and I curious why you make such api for accounts/addressBookAdd, accounts/addressBookUpdate, accounts/addressBookRemove where these methods receive accounId from outside? Maybe you were planned to use these methods somewhere else, maybe in dashboard? If not, then I'll want to change this api if you don't mind, to get userId string from within this methods.
Mike Murray
@mikemurray
Dec 13 2015 06:52 UTC
@newsiberian I think my reasoning was maybe letting admins have the ability to edit user account info. Like they could make accounts on behalf of others and manage them from their own admin account.
Mike Murray
@mikemurray
Dec 13 2015 07:00 UTC
I think what we do in other places is, if the passed in userId is null (in this case accountId), then we use Meteor.userId() to default it to something. Though, this all depends on if you want the method to fail if the supplied userId is null, or some how magically work and potentially do something unexpected from passed in properties.
newsiberian
@newsiberian
Dec 13 2015 07:08 UTC
Ok, if I understand you correctly, we could do accountId param optional and not pass it with checkout situation, but it could be used in dashboard to help manage accounts. Then, inside methods we could do something accountId = accountId || Meteor.userId(); Is that correct?

Though, this all depends on if you want the method to fail if the supplied userId is null, or some how magically work and potentially do something unexpected from passed in properties.

Now I get it... we could broke admin profile by this, right?

newsiberian
@newsiberian
Dec 13 2015 07:58 UTC
Ok, if addressBook will be reusable component, then the best way will be send to it accountId like a props. It could be done from checkout, or from dashboard. So it will be easier to understand the process flow.
So we could leave this without many changes for now.
Mike Murray
@mikemurray
Dec 13 2015 15:45 UTC
@newsiberian :thumbsup: The idea of the addressBook was to be a reusable component, and you hit the mark with passing around the props.
newsiberian
@newsiberian
Dec 13 2015 15:53 UTC
@mikemurray Currently I've just tried to reproduce accounts/addressBookAdd call with overriding guest id to admin id and it work. Maybe something wrong with publications in that case?
newsiberian
@newsiberian
Dec 13 2015 16:00 UTC
I think this case not about publication. Think about it. We just send a parameter to server method without any checks. And it is internally work out with this parameter, am I wrong with that? We not asked any collections here, right?
Mike Murray
@mikemurray
Dec 13 2015 16:22 UTC

@newsiberian If I'm a general user, and try to use some one else's userId to modify something, I'll be blocked. The publication says "no, I did not publish that data for you, you cannot read or modify it".

While we may not be directly checking props in that server method, the publication at a lower level is managing the security of the data.

Aaron Judd
@aaronjudd
Dec 13 2015 17:40 UTC
but remember, the publication is for server side filtering of the data sent to the client side. A server method bypasses publications and goes direct to the collection (for insert,etc). That’s where ongoworks:security and security.js comes into play.
Mike Murray
@mikemurray
Dec 13 2015 18:10 UTC
@aaronjudd so then we do need to inforce security in all server methods, and not rely on publications to do that.
Aaron Judd
@aaronjudd
Dec 13 2015 18:15 UTC
yes, publications control what can be seen, but not what is inserted/updated/etc that’s where the security rules, and permissions checks come into play. Accounts / address book shouldn’t update unless it’s the user’s own account, so this would be appropriate:
/*
 * Users may update their own account
 */
Accounts.permit(["insert", "update"]).ifHasRole({
  role: ["anonymous", "guest"],
  group: ReactionCore.getShopId()
}).ifShopIdMatchesThisId().ifUserIdMatches().apply();

while

/**
 * admin security
 * Permissive security for users with the "admin" role
 */

Security.permit(["insert", "update", "remove"]).collections([Accounts, Products, Tags,
  Translations, Discounts, Taxes, Shipping, Orders, Packages, Layouts, Templates
]).ifHasRole({
  role: "admin",
  group: ReactionCore.getShopId()
}).ifShopIdMatches().exceptProps(["shopId"]).apply();

would give the admin rights to do whatever

Mike Murray
@mikemurray
Dec 13 2015 18:18 UTC
got it