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 allow_with_resource #8

Merged
merged 13 commits into from
Dec 9, 2016
24 changes: 16 additions & 8 deletions lib/rolypoly/controller_role_dsl.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ def self.included(sub)
unless sub.method_defined? :current_user_roles
define_method(:current_user_roles) { [] }
end

unless sub.method_defined? :role_resource
define_method(:role_resource) { [] }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, you wanna change this to an empty hash.

end
sub.send :extend, ClassMethods
end

Expand All @@ -31,8 +35,8 @@ def failed_role_check!
def current_roles
return [] if rolypoly_gatekeepers.empty?
current_gatekeepers.reduce([]) { |array, gatekeeper|
if gatekeeper.role? current_user_roles
array += Array(gatekeeper.allowed_roles(current_user_roles, action_name))
if gatekeeper.role?(current_user_roles, role_resource)
array += Array(gatekeeper.allowed_roles(current_user_roles, action_name, role_resource))
end
array
}
Expand All @@ -52,7 +56,7 @@ def current_gatekeepers
def rolypoly_role_access?
rolypoly_gatekeepers.empty? ||
rolypoly_gatekeepers.any? { |gatekeeper|
gatekeeper.allow? current_roles, action_name
gatekeeper.allow?(current_roles, action_name, role_resource)
}
end
private :rolypoly_role_access?
Expand All @@ -64,15 +68,19 @@ def rolypoly_gatekeepers

module ClassMethods
def all_public
build_gatekeeper(nil, nil).all_public
build_gatekeeper(nil, nil, false).all_public
end

def restrict(*actions)
build_gatekeeper nil, actions
build_gatekeeper nil, actions, false
end

def allow(*roles)
build_gatekeeper roles, nil
build_gatekeeper roles, nil, false
end

def allow_with_resource(*roles)
build_gatekeeper roles, nil, true
end

def publicize(*actions)
Expand All @@ -90,8 +98,8 @@ def try_super(mname)
end
end

def build_gatekeeper(roles, actions)
RoleGatekeeper.new(roles, actions).tap { |gatekeeper|
def build_gatekeeper(roles, actions, require_resource)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could make this false by default and only pass true when you need to. So, def build_gatekeeper(roles, actions, require_resource = false) for definition and have the existing methods the same but call build_gatekeeper roles, nil, true in allow_with_resource method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I couldn't decide if I should default it or not. My thinking it make more sense to explicitly say it's false or true. But I'm not sure if that really helps, so defaulting seems good.

RoleGatekeeper.new(roles, actions, require_resource).tap { |gatekeeper|
rolypoly_gatekeepers << gatekeeper
}
end
Expand Down
25 changes: 18 additions & 7 deletions lib/rolypoly/role_gatekeeper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@
module Rolypoly
class RoleGatekeeper
attr_reader :roles
def initialize(roles, actions)
def initialize(roles, actions, require_resouce)
self.roles = Set.new Array(roles).map(&:to_s)
self.actions = Set.new Array(actions).map(&:to_s)
self.require_resouce = require_resouce
self.all_actions = false
self.public = false
end
Expand All @@ -31,22 +32,23 @@ def to_all
self.all_actions = true
end

def allow?(current_roles, action)
def allow?(current_roles, action, role_resource)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should name this differently if you intend to use the passed in param value in role?. It could get confusing very easily when you have the same name in initializer that sets the self attribute and the same name gets used in other parts of the class as local variable.

action?(action) &&
role?(current_roles)
role?(current_roles, role_resource)
end

def allowed_roles(current_roles, action)
def allowed_roles(current_roles, action, role_resource)
return [] if public? || !action?(action)
match_roles(current_roles)
match_roles(current_roles, role_resource)
end

def all_public
self.public = true
self.all_actions = true
end

def role?(check_roles)
def role?(check_roles, role_resource)
check_roles = filter_roles_by_resource(check_roles, role_resource) if require_resouce
check_roles = Set.new sanitize_role_input(check_roles)
public? || !(check_roles & roles).empty?
end
Expand All @@ -65,15 +67,24 @@ def public?
attr_accessor :actions
attr_accessor :all_actions
attr_accessor :public
attr_accessor :require_resouce

def match_roles(check_roles)
def match_roles(check_roles, role_resource)
check_roles = filter_roles_by_resource(check_roles, role_resource) if require_resouce
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you call filter_roles_by_resource only when require_resauce :trollface: is true, you should move the check in the method itself. You'll save yourself from repeated conditional and since require_resauce is already is an accessor, it's available across the class.

check_roles.reduce([]) { |array, role_object|
array << role_object if roles.include?(sanitize_role_object(role_object))
array
}
end
private :match_roles

def filter_roles_by_resource(check_roles, role_resource)
check_roles.select do |check_role|
check_role.respond_to?(:resource?) && check_role.resource?(role_resource)
end
end
private :filter_roles_by_resource

def sanitize_role_input(role_objects)
Array(role_objects).map { |r| sanitize_role_object(r) }
end
Expand Down
43 changes: 27 additions & 16 deletions spec/lib/rolypoly/role_gatekeeper_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,21 @@ module Rolypoly
describe RoleGatekeeper do
let(:roles) { %w[admin scorekeeper] }
let(:actions) { %w[index show] }
let(:role_resource) { [] }

subject { described_class.new roles, actions }
subject { described_class.new roles, actions, false }

shared_examples_for "allow should behave correctly" do
it "shouldn't auto-allow" do
expect(subject.allow?(nil, nil)).to be false
expect(subject.allow?(nil, nil, role_resource)).to be false
end

it "should allow scorekeepr access to index" do
expect(subject.allow?([:scorekeeper], "index")).to be true
expect(subject.allow?([:scorekeeper], "index", role_resource)).to be true
end

it "should not allow scorekeepr access to edit" do
expect(subject.allow?([:scorekeeper], "edit")).to be false
expect(subject.allow?([:scorekeeper], "edit", role_resource)).to be false
end

describe "all public" do
Expand All @@ -26,15 +27,15 @@ module Rolypoly
end

it "should allow whatever" do
expect(subject.allow?(nil, nil)).to be true
expect(subject.allow?(nil, nil, role_resource)).to be true
end

it "should allow scorekeepr access to index" do
expect(subject.allow?([:scorekeeper], "index")).to be true
expect(subject.allow?([:scorekeeper], "index", role_resource)).to be true
end

it "should allow scorekeepr access to edit" do
expect(subject.allow?([:scorekeeper], "edit")).to be true
expect(subject.allow?([:scorekeeper], "edit", role_resource)).to be true
end
end

Expand All @@ -44,17 +45,17 @@ module Rolypoly
end

it "shouldn't auto-allow" do
expect(subject.allow?(nil, nil)).to be false
expect(subject.allow?(nil, nil, role_resource)).to be false
end

it "should allow scorekeepr access to index" do
expect(subject.allow?([:janitor], "index")).to be true
expect(subject.allow?([:admin], "index")).to be true
expect(subject.allow?([:janitor], "index", role_resource)).to be true
expect(subject.allow?([:admin], "index", role_resource)).to be true
end

it "to should not allow scorekeepr access to edit" do
expect(subject.allow?([:scorekeeper], "edit")).to be false
expect(subject.allow?([:janitor], "edit")).to be false
expect(subject.allow?([:scorekeeper], "edit", role_resource)).to be false
expect(subject.allow?([:janitor], "edit", role_resource)).to be false
end
end

Expand All @@ -64,19 +65,19 @@ module Rolypoly
end

it "shouldn't auto-allow" do
expect(subject.allow?(nil, nil)).to be false
expect(subject.allow?(nil, nil, role_resource)).to be false
end

it "should allow scorekeepr access to index" do
expect(subject.allow?([:scorekeeper], "index")).to be true
expect(subject.allow?([:scorekeeper], "index", role_resource)).to be true
end

it "shouldn't allow janitor access to any" do
expect(subject.allow?([:janitor], "index")).to be false
expect(subject.allow?([:janitor], "index", role_resource)).to be false
end

it "should allow scorekeepr access to edit" do
expect(subject.allow?([:scorekeeper], "edit")).to be true
expect(subject.allow?([:scorekeeper], "edit", role_resource)).to be true
end
end
end
Expand All @@ -101,5 +102,15 @@ module Rolypoly

it_should_behave_like "allow should behave correctly"
end

describe "with role_resource defined" do
let(:role_resource) { [organization: 123] }

before do
subject.to(:admin, :scorekeeper)
end

it_should_behave_like "allow should behave correctly"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol wut!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right???

end
end
end