-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
[WIP] Fix Reaction.hasPermissions for anonymous users #3196
[WIP] Fix Reaction.hasPermissions for anonymous users #3196
Conversation
@zenweasel @impactmass I have found and written a fix for the problem reported in #2895. However, I want more info/history about why we wrote Similarly, I want to know if I can add some checks that handle when |
Going through this now. A quick note on why the |
Another thing to note is that we have two |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments
client/modules/core/main.js
Outdated
// permissions can be either a string or an array | ||
// we'll force it into an array and use that | ||
if (checkPermissions === undefined) { | ||
permissions = ["owner"]; | ||
} else if (typeof checkPermissions === "string") { | ||
permissions = [checkPermissions]; | ||
} else { | ||
/* TODO: Consult and add the code below if it is thought | ||
of as useful. | ||
if (!Array.isArray(checkPermissions)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it doesn't do any harm, I think we can drop this check. I say that because the final method called -Roles.userIsInRole- can handle both a string and an array
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Will do that.
client/modules/core/main.js
Outdated
if (!Array.isArray(checkPermissions)) { | ||
return false; | ||
} | ||
if (checkPermissions.length === 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same comment above should apply to this too
client/modules/core/main.js
Outdated
@@ -186,21 +186,33 @@ export default { | |||
// called out a userId is validated. | |||
// | |||
function roleCheck() { | |||
// TODO: There're some outdated comments in this function. Clean them out. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the outdated comments, you can make that change in this PR (i.e remove/edit them). That will start a discussion here about if it needs to change or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Roger that. I've done some, but left with a few.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But what should I do to this commented code? I wasn't the one who wrote it, so I don't know it still is/will be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at that piece of commented code, it's checking to see if a user has admin
role for shops then based on that, it decides hasPermissions access (i.e return true
or false
). So, if a user has admin
role, they get a pass.
That can lead us to the same situation we're in with the owner
role being concatenated in (i.e what you're fixing). I think that's why it was commented out. So, it's fine to remove it, since there are other ways to perform admin check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. Do you want me to do another review? or still working on other parts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you can perform another review. I've pushed changes to address the remaining part I was working on (see comments on main thread).
@impactmass Roger that. I'll investigate more about what are the effects of not auto-appending And about the server-side version of |
@impactmass I now understand why we were auto-appending |
@impactmass I tested the commit console.log("Reaction.hasPermission('owner'): ", Reaction.hasPermission("owner"));
console.log("Reaction.hasPermission('anonymous'): ", Reaction.hasPermission("anonymous"));
console.log("Reaction.hasPermission(): ", Reaction.hasPermission());
console.log("Reaction.hasPermission(''): ", Reaction.hasPermission(""));
console.log("Reaction.hasPermission([]): ", Reaction.hasPermission([]));
console.log("Reaction.hasPermission(['owner']): ", Reaction.hasPermission(["owner"]));
console.log("Reaction.hasPermission(['owner', 'random']): ", Reaction.hasPermission(["owner", "random"]));
console.log("Reaction.hasPermission(['random', 'owner']): ", Reaction.hasPermission(["random", "owner"])); Then I viewed the results in the browser console. NB: Don't forget to import |
client/modules/core/main.js
Outdated
// | ||
|
||
// This is declared as a function for the sake of reuse. Note that it is | ||
// only called after userId is validated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we're trying to document this function. In that case, use our jsdoc format; and since it's private function, it should use @private
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
client/modules/core/main.js
Outdated
function roleCheck() { | ||
// permissions can be either a string or an array | ||
// we'll force it into an array and use that | ||
// TODO: Should we also cater for checkPermissions === "" and checkPermissions === []? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't hurt, but it can also be deferred to final Roles method check just like this other comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted.
client/modules/core/main.js
Outdated
// when everything is ready | ||
// | ||
/* | ||
Checks for permissions. We'll skip performing some checks if the user is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can leave this part of the comment as is. It seems valid and clear (IMO)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I'll restore the former comment.
client/modules/core/main.js
Outdated
// this userId check happens because when logout | ||
// occurs it takes a few cycles for a new anonymous user | ||
// occurs. It takes a few cycles for a new anonymous user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you mean to use ,
here? seems like it should be a ,
instead of .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'll correct that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
client/modules/core/main.js
Outdated
if (checkPermissions === undefined) { | ||
permissions = ["owner"]; | ||
} else if (typeof checkPermissions === "string") { | ||
if (checkPermissions === "owner") { | ||
return Roles.userIsInRole(userId, "owner", group); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding a detailed comment about this on main thread.
Just to be sure we are both on the same page, the goal here is to remove hard coding of 'owner' inside this function. So that way when it's called to check for a specific role, it is clear what role it's checking for. When there's need to give access based on role x, and we want to give owner access as well, we then just write Those test cases you've written can also be fleshed out into a test file for the /server/api/core/core part (permissions.app-test.js maybe?) |
@impactmass Yeah, I agree that we should require calls to About writing tests based off of this comment, I'm on it. |
UPDATEAbout converting these logging statements to tests, we can't do that. "Why?", you say. Because I created a |
About the tests, note what I said (on the server):
|
Oh, I see. I missed that later part. I will work on writing tests for the server part. |
@foladipo What's the status on this? You said you were going to work on writing tests 10 days ago? Should I assign this to someone else to take over? |
@zenweasel Sorry for the delay. I've pushed the tests now. |
UPDATE: I have written tests for the server-side version of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding/updating the jsdocs.
I made some comments on the new changes.
This is close to being done. Need to sort out the comments, and fixing failing tests (after removing .only)
client/modules/core/main.js
Outdated
* @method roleCheck | ||
* @summary check whether or not a user is in a list of roles. | ||
* @private | ||
* @since 1.5.5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering that this rolecheck function has been in the code for longer than when you're writing the comment, I don't think you can say since 1.5.5
. It'll be better to go back and see when it was added originally, or leave out the @since
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also note that there is a jsdoc on the whole haspermission block. It states that it defaults the roles to "owner" if nothing is passed in. That is changing with this PR, so we should update the comment there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last comment also applies to server Reaction.hasPermission
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
roleCheck
was added in 0.15.0
. You can check the changes made to client/modules/core/main.js
in this commit to verify. You'll notice that that commit was added on 18 August, 2016
, and the next release after that, according to this release history, was on 23 August, 2016
. So, roleCheck
was added in 0.15.0
.
You can check if my findings above and let me know whether or not you think they are accurate.
client/modules/core/main.js
Outdated
* @summary verifies that Meteor.userId() returns a value, after which | ||
* it calls some functions, especially roleCheck. | ||
* @private | ||
* @since 1.5.5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I found out that validateUserId
was added in release 0.15.0
(commit where it was added, relevant commit history).
import { Factory } from "meteor/dburles:factory"; | ||
import Core from "./core"; | ||
|
||
describe.only("hasPermission", function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove describe.only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And all other it.only
s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tried removing the .only
s and, after running reaction test
, I saw some of the tests I wrote for this PR fail. What I am trying to figure out right now is why they fail solely when I remove the .only
s and run all the tests of this project. That's where I'm at right now. And if you have suggestions about the cause, do share them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These only
are for local debugging only and should never be committed/pushed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Roger that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@impactmass I have removed the .only
s and fixed the failing tests.
expect(hasPermissions).to.be.false; | ||
}); | ||
|
||
it.only("should return false for anonymous users when `permissions === [owner]`", function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test should be less about whether the user is anonymous, and more about whether they have the role/permission being checked for. You can make the test description clearer.
We expected it to return false in this case because anonymous users don't have the owner
role.
expect(hasPermissions).to.be.false; | ||
}); | ||
|
||
it.only("should return false for registered users when `permissions === [owner]`", function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment above (about test description) can be applied to this too
OK. I'm working on the comments now. |
@zenweasel @impactmass I am done with the last outstanding review. I have also added appropriate test instructions for this PR and removed the WIP tag. So, please review and share your thoughts. |
// It is necessary to explicity supply shopId to the Core.hasPermission call | ||
// below. If we don't, the said function defaults to using Core.getShopId | ||
// which, when this test is run, will give a shop ID that will be different | ||
// from the one used by Factory.create("anonymous"). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on this comment, do you think we should recommend that shopId be always passed to hasPermissions?
The value returned by getShopId() is based on the context it is called. I've seen this happen too when testing a cfs upload.
Also, considering that with v1.5.0+, there'll be more than one shop. It's important to be sure of the return value of getShopId().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zenweasel @spencern any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, being explicit with shopId
too wouldn't hurt. The question is: is it necessary? And what are the scenarios that exist right now or may happen that will make it necessary?
Consider this scenario: we have users X, Y and Z they are respectively the admins of shops X, Y and Z. If user X has shop X open, is there a button somewhere that s/he can use to delete a product in shop Z? Because then, even though s/he is logged into shop X, this app will need to check if s/he has the deleteProduct
permissions in a different shop, shop Z.
If scenarios similar to the above exist or can easily happen, then we definitely should require shopId
to be explicitly passed to hasPermission
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about this again, I think it's fine as we have it currently. With default values for shopId (getShopId) and user (Meteor.user). The edge cases seem rare and each can be handled.
(so let's take this comment thread as resolved).
// below. If we don't, the said function defaults to using Core.getShopId | ||
// which, when this test is run, will give a shop ID that will be different | ||
// from the one used by Factory.create("anonymous"). | ||
const shopId = Object.keys(user.roles)[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing [0] feels brittle because the value you're checking for could possibly be at index 1.
Is it possible to use another approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it's possible for that index to be 1. I based my decision to use index 0 off of the declaration of Factory.define("anonymous")
in this file. More specifically, if you check this line in that file, you will see that user.roles
can only have one key in it.
So, I do think this is the best approach. Or do you have another approach in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant that the fixture can change. A time will come when we make the fixtures multi-shop aware, then there will be more shopId keys (0,1 etc) there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it will be better to wait till that change is made then. Coz until then, we can't know the structure of the new fixtures and the index of the right shopId to use above.
@impactmass Is this ready to go now? |
I think there's a bit more planning to be done about how this will get merged in, because this might cause some side effects in places where hasPermission is in use.
This discussion should continue from the issue (and we can probably mark this as WIP?) |
@impactmass I appreciate that you're thinking about these things. This might need to be considered a breaking change in the sense that additional plugins or apps relying on |
@spencern @zenweasel @impactmass can we get a status update here? if this is not moving forward.. how about some closure? |
I am closing this with extreme prejudice. [WIP] Fix Reaction.hasPermissions for anonymous users Re-open if this is a real thing, or give up... |
Resolves #2895.
This PR removes the behaviour of
hasPermission
(on both the client and server) where it auto-appends theowner
permission to the list of permissions supplied to it. Before now, if anowner
user was logged in and the app ranhasPermission("anonymous")
, the call would returntrue
even thoughowner
users don't have theanonymous
permission. This PR fixes that by removing the auto-appending behaviour. Now, you have to explicitly list all the permissions you wanthasPermission
to check for whenever you call it.Test instructions
hasPermission
, I have written some unit tests for it inserver/api/core/hasPermission.app-test.js
. You can check out the tests and then run them withreaction test
to see whether they fail or not.hasPermission
, you can:render
method ofimports/plugins/core/ui/client/components/app/app.js
.localhost:PORT/
.hasPermission
and their results.