Add isInRole to JS API as well

[rijk]

Something you frequently do in the application, is check if the current user is allowed to access something. As there currently is no shorthand for this in the Javascript API, you have to do something like:

if ( Roles.userIsInRole( Meteor.userId(), 'permission' ) )
    // ...

I noticed the UI helper offers a beautiful shortand syntax for this, using the currently logged in user. I think it would make a lot of sense to add this to the API as well, so that one can simply do:

if ( Roles.isInRole( 'permission' ) )
    // ...

or even better:

if ( Roles.has( 'permission' ) )
    // ...

[alanning]

That's a great idea!

The only caveat would be on the server-side where this.userId is only available in Meteor methods and publish functions.

Any ideas on how to handle that? I guess we could throw an error if this.userId is undefined and let people know that its only useable in publish and methods server-side.

[rijk]

Thanks! It's a good point I guess — but how would you handle this with the userIsInRole() function? I guess it's just not possible to use that in those places either? So I guess throwing an error in those cases would be ok (like Meteor does too if you call Meteor.userId()):

Error: Meteor.userId can only be invoked in method calls. Use this.userId in publish functions.

[alanning]

Currently handled by sneakily requiring the programmer to provide the user object (ie, let the programmer worry about it). :-) I think this is actually why I didn't include something like this before, it creates an interface that is inconsistent on the server-side.

But I think it makes sense to do it since it would be very useful and Meteor itself has the same inconsistency.

Yeah, my thought on the error is to throw it early so programmers can catch it in development.

Would you like to start work on this? A pull request would be much appreciated!

[rijk]

Sure I’d like to contribute, but the master branch doesn’t seem to be the latest version (Meteor said upgraded alanning:roles from version 1.2.13 to version 1.2.12 when I switched the package)?

[mitar]

As commented in #57, I made a package which allows one to use Meteor.userId() everywhere. Should we make this package depend on that package and then provide has function?

[SimonSimCity]

@mitar I would close this to avoid any 3rd party dependency. I'd take it rather as they've done it on the accounts-package by creating a new package which depends on this and adds the mentioned functionality.

Another approach, which would be easily readable is to use https://github.com/dburles/meteor-collection-helpers like following:

// Included on server and client
Meteor.users.helpers({
  has(permission, group) {
    return Roles.userIsInRole(this._id, permission, group);
  },
});

Which allows you to use the following code on the client:

Meteor.user().has('permission')

Maybe there's also a good way to get this into a meteor-method.

[rijk]

Sounds great @SimonSimCity !

[mitar]

@SimonSimCity Not sure if I understand what you are saying here. You are saying here that:

I would close this to avoid any 3rd party dependency.

But then you propose a 3rd party dependency:

Another approach, which would be easily readable is to use https://github.com/dburles/meteor-collection-helpers like following:

The main reason why dependency on this package might be necessary is to get Meteor.userId() to work for free everywhere. Then you can just call Roles.has( 'permission' ) anywhere and it just works, inside publish, inside methods, on client and server. I have this in my code everywhere and it is really great because you can make permission checks work the same everywhere, without having to have special cases for where the code is called from.

[rijk]

To be honest, I still don’t understand why Meteor.userId() doesn’t just work everywhere — it goes straight against the whole isomorphic philosophy of Meteor in my opinion.

But what I think @SimonSimCity is proposing is not to add a 3rd party dependency, but an additional package that adds this 'syntax sugar' to the user collection. Which is also not so easy now I think of it, because Meteor.user().has('permission') looks beautiful but will throw an error if not logged in (or logging in).

So yeah, maybe adding this to the Roles package (as Roles.has( 'permission' )) is still the preferred way to go.

[mitar]

So we could have a package which adds Roles.has or has something similar, but that package will have to depend on user-extra package to make it smooth to use.

So I do not care too much if this is part of this package or additional package.

Also, the dependency on user-extra can be a weak one. If you have it, then Roles.has works everywhere, otherwise it does not work everywhere.

[rijk]

Check. One thing I don’t like so much about the meteor-user-extra package, is this:

prevents direct modification of the user's profile from the client, which is otherwise allowed by Meteor

I use this functionality on multiple places in my app (updating user.profile from the client). It is strange to me that this package makes such an opinionated decision along with just enabling Meteor.userId() everywhere (which seems to be unrelated). If we add this package as a Roles dependency, installing the roles package will suddenly prevent people from updating user.profile from the client... That is unexpected, even if there is a way to override this deny rule (not sure if there is). Maybe meteor-user-extra can be split in two packages?

[mitar]

You are right. I do see that package as "best practices" package. And to me allowing any unchecked writes to a database is problematic. You should wrap it into a method and verify what is being updated in profile there. Otherwise a malicious user can simply write there anything, even if you on the client side do checks, they have to be done also on the server side.

[rijk]

You should wrap it into a method and verify what is being updated in profile there.

While I see your point, I think it is up to the developer to make this call. Especially since this is default Meteor behaviour, we shouldn’t force this change upon people just because they want to use the roles package in my opinion.

[mitar]

While I see your point, I think it is up to the developer to make this call.

A call to make their app insecure?

we shouldn’t force this change upon people just because they want to use the roles package in my opinion.

So this is why it would be a weak dependency.

[rijk]

A call to make their app insecure?

It is the same call the Meteor developers made. I think you are exaggerating. And the discussion is beside the point, anyway.

So this is why it would be a weak dependency.

But if you want Roles.has to work everywhere, you need to accept this opinionated side effect which has nothing to do with Roles.has. This I just don’t agree with, and is unnecessary in my opinion.

[mitar]

Yea, in fact it does not have to be that package anyway, to get it working everywhere. So we do not even have to weak-depend on it.

If you would like to extract that logic you like out into a separate package, and then make user-extra depend on that, I am open to that. But let's discuss that in user-extra package's repository.

[StorytellerCZ]

Looking at current situation I feel that most of the issues have been resolved in newer Meteor versions, so it should be safe to move forward with this. The basic transfer from UI helper to the API in general should not present any problems (hence why I marked this with "good first issue").
I suggest adding doing the absolute minimum in the next feature release and then we can add all the aliases and other features later on.
We should also bump the minimum Meteor version to 1.9 as bellow that it uses Node v8 which is no longer supported.