Skip to content

Commit

Permalink
Better handling for permission policies on limited admins.
Browse files Browse the repository at this point in the history
If a limited admin user didn't belong to any groups with the specified
permission, then the policy scope could return all results, rather than
limiting the results to no results. Luckily, each place these
policy scopes were used were also followed by the controller
individually checking the policy/permission of individual results, so
the unpermitted results ended up getting stripped. This scenario is also
tested for in the "assert_default_admin_permissions" tests that apply to
our existing API controllers. So while this doesn't change existing
behavior, making sure the query returns nothing from the database to
begin with seems like a better approach (most of the other policies were
already doing this).
  • Loading branch information
GUI committed Mar 5, 2018
1 parent a296512 commit 313036f
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 3 deletions.
6 changes: 5 additions & 1 deletion src/api-umbrella/web-app/app/policies/admin_group_policy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,11 @@ def resolve
api_scope_ids << api_scope.id
end

scope.in(:api_scope_ids => api_scope_ids)
if(api_scope_ids.any?)
scope.in(:api_scope_ids => api_scope_ids)
else
scope.none
end
end
end
end
Expand Down
7 changes: 6 additions & 1 deletion src/api-umbrella/web-app/app/policies/admin_policy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,12 @@ def resolve
end

group_ids = AdminGroup.in(:api_scope_ids => api_scope_ids).map { |g| g.id }
scope.in(:group_ids => group_ids)

if(group_ids.any?)
scope.in(:group_ids => group_ids)
else
scope.none
end
end
end
end
Expand Down
6 changes: 5 additions & 1 deletion src/api-umbrella/web-app/app/policies/api_policy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,11 @@ def resolve(permission = "backend_manage")
}
end

scope.or(query_scopes)
if(query_scopes.any?)
scope.or(query_scopes)
else
scope.none
end
end
end
end
Expand Down

0 comments on commit 313036f

Please sign in to comment.