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.

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

Add isInRole to JS API as well #56

Open
rijk opened this issue Sep 19, 2014 · 17 comments · May be fixed by #390
Open

Add isInRole to JS API as well #56

rijk opened this issue Sep 19, 2014 · 17 comments · May be fixed by #390
Labels
enhancement good first issue This is an easy issue that beginners can tackle to get to know the code base. help wanted
Milestone

Comments

@rijk
Copy link

rijk commented Sep 19, 2014

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
Copy link
Contributor

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
Copy link
Author

rijk commented Sep 19, 2014

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
Copy link
Contributor

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
Copy link
Author

rijk commented Sep 22, 2014

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
Copy link
Member

mitar commented Jan 17, 2016

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
Copy link
Member

@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
Copy link
Author

rijk commented Oct 12, 2019

Sounds great @SimonSimCity !

@rijk rijk closed this as completed Oct 12, 2019
@mitar mitar reopened this Oct 12, 2019
@mitar
Copy link
Member

mitar commented Oct 12, 2019

@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
Copy link
Author

rijk commented Oct 12, 2019

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
Copy link
Member

mitar commented Oct 12, 2019

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
Copy link
Author

rijk commented Oct 12, 2019

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
Copy link
Member

mitar commented Oct 12, 2019

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
Copy link
Author

rijk commented Oct 12, 2019

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
Copy link
Member

mitar commented Oct 12, 2019

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
Copy link
Author

rijk commented Oct 12, 2019

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
Copy link
Member

mitar commented Oct 13, 2019

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 StorytellerCZ added the good first issue This is an easy issue that beginners can tackle to get to know the code base. label Oct 14, 2019
@StorytellerCZ StorytellerCZ pinned this issue Sep 13, 2020
@StorytellerCZ
Copy link
Member

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.

@StorytellerCZ StorytellerCZ added this to the Release 4.0 milestone Aug 5, 2021
StorytellerCZ added a commit that referenced this issue Feb 3, 2024
@StorytellerCZ StorytellerCZ linked a pull request Feb 3, 2024 that will close this issue
@StorytellerCZ StorytellerCZ linked a pull request Feb 3, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement good first issue This is an easy issue that beginners can tackle to get to know the code base. help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants