sandstorm-io / meteor-accounts-sandstorm

Sandstorm.io login integration for Meteor applications.
Other
20 stars 18 forks source link

Doesn't seem to work in allow/deny rules #19

Open ndarilek opened 8 years ago

ndarilek commented 8 years ago

Normally I use methods for RPC, but I'm using CollectionFS which uses the insert/update/remove methods directly. I tried something like:

Books.allow
  insert: ->
    console.log(@connection)
    if @connection?.sandstormUser()?.permissions.indexOf("modify") != -1
      true
    else
      false
  update: ->
    if @connection?.sandstormUser()?.permissions.indexOf("modify") != -1
      true
    else
      false
  remove: ->
    if @connection?.sandstormUser()?.permissions.indexOf("modify") != -1
      true
    else
      false

but unfortunately this.connection doesn't appear to exist in this context.

kentonv commented 8 years ago

Hmm, I suspect you can do DDP._CurrentInvocation.get().connection, but that is of course using a Meteor private API. Maybe we should add a Meteor.sandstormUser() server-side that calls this?

FWIW I highly recommend defining explicit methods to do mutations rather than allow/deny rules. With methods it's much easier to reason about whether the call fits the expected pattern, whether the parameter types are correct, etc. But we shouldn't be imposing that style in meteor-accounts-sandstorm.

ndarilek commented 8 years ago

I like Meteor.sandstormUser() everywhere, it makes defining universal helpers easier. Or I suppose you could do AccountsSandstorm.user() if you'd rather not inject another method on Meteor. No strong feelings either way, though the latter is at least in keeping with Meteor's use of Accounts, Mongo, etc. for things that aren't core APIs.

And yes, definitely agreed on the methods. Methods are normally how I do things too, but unfortunately CollectionFS is opinionated and doesn't really document how to disagree with its opinions. :) File uploads are initiated by collection.insert on the client, which of course subjects the call to allow/deny rules.

Thanks for the workaround, I'll give it a shot.

simonv3 commented 8 years ago

Just wanted to pipe up that I was running into this as well and left scratching my head a bit until I came here. I'll look at the workaround.

(This is for annotate, so I'm also working with CollectionFS, though I saw @ndarilek comment somewhere that it's now depricated, sooooo)

ndarilek commented 8 years ago

I think this issue may be invalid but I haven't dug into it.

A few days ago I was getting undefined at some stage in running the workaround. I didn't investigate too far--it was planned for after I'd implemented some other features. I was using coffeescript's ? operator so didn't figure out what stage of the chain was nulling out.

But I don't think allow/deny rules have access to the DDP connection, or at least, are run within a context that has access. I think they're passed the userId and do their calculations that way, meaning they wouldn't work with anonymous users. They're kind of a dirty corner of the framework I barely touch, so I don't know for sure. I usually use methods.

kentonv commented 8 years ago

If DDP._CurrentInvocation.get() doesn't work (and there's no other way to get at the connection) then I think we're out of luck. I don't know of anywhere else where we can stash the permissions. You'll need to rely on the user being logged in I guess?

simonv3 commented 8 years ago

DDP._CurrentInvocation.get() seemed to work fine for me in permissions, but somehow adding this package broke file uploading on CollectionFS in general. (linked to above). I'll have to investigate more later.