Fix global role bug #58
Conversation
|
+1 on specs. Also, @maeve do you mind taking a look at this? |
There was a problem hiding this comment.
I'm still waiting on Jira access, so I'm not sure of the exact bug we're trying to fix here. In the meantime, I did review the code, added a couple suggestions for tweaks, and pointed out what might potentially cause problems.
It definitely needs specs. Right now the existing tests are passing because admin? returns a superset of admin?(true), but we should have tests around both cases for each method that's been added or changed.
| end | ||
| end | ||
|
|
||
| def admin?(global_scope=false) |
There was a problem hiding this comment.
I'm concerned about the side effects of changing the default behavior for a set of methods that are already used across multiple applications. It could be the right thing to do but it may have unforeseen consequences somewhere. Has there been an audit of the places where the admin?, editor? and viewer? methods are already being used?
There was a problem hiding this comment.
for those who are calling admin? editor?``viewer?without params it will have the same efect, check if there is any role with that name. Just those that call them with the params global_scope knows that they are traing to search the role but at global scope. In this case we add a new filter that is resource_type: nil
In other part of the code I found this
@current_user.roles.global.exists? where is global scope defined ?
| @@ -92,7 +102,7 @@ def has_global_role? | |||
| end | |||
|
|
|||
| def global_role? | |||
| super_admin? || admin? || editor? || viewer? | |||
| super_admin? || admin?(true) || editor?(true) || viewer?(true) | |||
There was a problem hiding this comment.
As long as we're changing the implementation of this method, we can do this in one trip to the DB.
| super_admin? || admin?(true) || editor?(true) || viewer?(true) | |
| user.roles.global.exists? |
| def check_for_role(role_name,global_scope=false) | ||
| if user.present? | ||
| query = user.roles.where(name: role_name) | ||
| query = query.where(resource_type: nil) if global_scope |
There was a problem hiding this comment.
The rolify gem provides a number of convenient scopes on the roles relation.
| query = query.where(resource_type: nil) if global_scope | |
| query = query.global if global_scope |
| user.present? && user.has_role?(:admin) | ||
| def check_for_role(role_name,global_scope=false) | ||
| if user.present? | ||
| query = user.roles.where(name: role_name) |
There was a problem hiding this comment.
If the global_scope argument is false, this query will return all roles with that role_name, including global roles. Is that the intention?
Fix the code that checks for global roles
On this comment I explain what it the error that we have when we ask for global roles to a user
https://teamg5.atlassian.net/browse/FD-287?focusedCommentId=10492&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel