Skip to content

Feature: Define RoleManager as interface#37

Merged
hsluoyz merged 1 commit intoapache:masterfrom
kenjones-cisco:feature/role-manager-interface
Aug 30, 2017
Merged

Feature: Define RoleManager as interface#37
hsluoyz merged 1 commit intoapache:masterfrom
kenjones-cisco:feature/role-manager-interface

Conversation

@kenjones-cisco
Copy link
Contributor

Define RoleManager as interface, and rename existing RoleManager struct
to defaultRoleManager to provide a default implementation and backwards
compatability.

Defines RoleManagerConstructor to allow specifying a function for
creating a new RoleManager.

Adds SetRoleManagerConstructor method to Model to allow setting the
function to call for a new instance of RoleManager.
Adds implementation of RoleManagerConstructor to Model that will create
an instance of the defaultRoleManager just like before.

Updates Assertion to reference RoleManager interface instead of an
instance. Updates Assertions.buildRoleLinks() to use newRoleManagerFunc
that holds the RoleManagerConstructor.

All test are passing with the only change being *RoleManager to
RoleManager.

Resolves #36

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 91.543% when pulling 8514f98 on kenjones-cisco:feature/role-manager-interface into 748b815 on casbin:master.

@hsluoyz
Copy link
Member

hsluoyz commented Aug 29, 2017

I have several questions:

  1. I saw newRoleManagerFunc in model.go is a global variable. So two Casbin enforcers will affect each other. I think it should be a member of Enforcer to avoid the conflicts.

  2. defaultRoleManager can be refactored out to default_role_manager.go. role_manager.go will only contain the interface.

  3. The function comments in RoleManager interface didn't describe the domain parameter.

@kenjones-cisco
Copy link
Contributor Author

If the RoleManagerConstructor is added to the Enforcer, that means that func (model Model) BuildRoleLinks() and func (ast *Assertion) buildRoleLinks() would need to accept a RoleManagerConstructor as input given that func (ast *Assertion) buildRoleLinks() creates a new RoleManager instance on each execution.

Any concerns with change the signatures of those methods?

@kenjones-cisco kenjones-cisco force-pushed the feature/role-manager-interface branch from 8514f98 to cd99ceb Compare August 29, 2017 18:11
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 91.551% when pulling cd99ceb on kenjones-cisco:feature/role-manager-interface into 748b815 on casbin:master.

@hsluoyz
Copy link
Member

hsluoyz commented Aug 30, 2017

The code looks good to me. Can you add a working example (e.g. TestRBACModelWithCustomRoleManager()) to model_test.go? In it, you can start with the RBAC example, then set a custom role manager. The role manager can be very simple, such as using hard-coded values. Then call testEnforce to show how the new role manager affects the enforcing results.

func TestRBACModel(t *testing.T) {
	e := NewEnforcer("examples/rbac_model.conf", "examples/rbac_policy.csv")

	testEnforce(t, e, "alice", "data1", "read", true)
	testEnforce(t, e, "alice", "data1", "write", false)
	testEnforce(t, e, "alice", "data2", "read", true)
	testEnforce(t, e, "alice", "data2", "write", true)
	testEnforce(t, e, "bob", "data1", "read", false)
	testEnforce(t, e, "bob", "data1", "write", false)
	testEnforce(t, e, "bob", "data2", "read", false)
	testEnforce(t, e, "bob", "data2", "write", true)
}

Define RoleManager as interface, and rename existing RoleManager struct
to defaultRoleManager to provide a default implementation and backwards
compatability.

Defines RoleManagerConstructor to allow specifying a function for
creating a new RoleManager.

Adds SetRoleManagerConstructor method to Model to allow setting the
function to call for a new instance of RoleManager.
Adds implementation of RoleManagerConstructor to Model that will create
an instance of the defaultRoleManager just like before.

Updates Assertion to reference RoleManager interface instead of an
instance. Updates Assertions.buildRoleLinks() to use newRoleManagerFunc
that holds the RoleManagerConstructor.

All test are passing with the only change being *RoleManager to
RoleManager.
@kenjones-cisco kenjones-cisco force-pushed the feature/role-manager-interface branch from cd99ceb to ef81f10 Compare August 30, 2017 03:01
@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 91.812% when pulling ef81f10 on kenjones-cisco:feature/role-manager-interface into 748b815 on casbin:master.

@hsluoyz hsluoyz merged commit 24e6518 into apache:master Aug 30, 2017
@kenjones-cisco kenjones-cisco deleted the feature/role-manager-interface branch August 30, 2017 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants