PR#50314: Issue: 50313 - Making an NestedRoleDefinition type in src/lib389/lib389/idm/role.py - 389-ds-base

389-ds-base

#50314 Issue: 50313 - Making an NestedRoleDefinition type in src/lib389/lib389/idm/role.py

Closed 3 years ago by spichugi. Opened 5 years ago by aborah.

aborah/389-ds-base nested into master

Issue: 50313 - Add a NestedRole type to lib389

Anuj Borah • 5 years ago

9724e8b

dirsrvtests/tests/suites/roles/basic_test.py

file modified

+135 -31

		`@@ -6,13 +6,20 @@`
		`# See LICENSE for details.`
		`# --- END COPYRIGHT BLOCK ---`

		`- import pytest, os`
		`+`
		`+ """`
		`+ Importing necessary Modules.`
		`+ """`
		`+`
		`+ import os`
		`+ import pytest`
		`+`
		`from lib389._constants import PW_DM, DEFAULT_SUFFIX`
		`from lib389.idm.user import UserAccount, UserAccounts`
		`from lib389.idm.organization import Organization`
		`from lib389.idm.organizationalunit import OrganizationalUnit`
		`from lib389.topologies import topology_st as topo`
		`- from lib389.idm.role import FilterRoles, ManagedRole, ManagedRoles`
		`+ from lib389.idm.role import FilterRoles, ManagedRoles, NestedRoles`
		`from lib389.idm.domain import Domain`


		`@@ -45,11 +52,11 @@`
		`'ou': 'eng',`
		`}`

		`- ou = OrganizationalUnit(topo.standalone, "ou=eng,o=acivattr,{}".format(DEFAULT_SUFFIX))`
		`- ou.create(properties=properties)`
		`+ ou_ou = OrganizationalUnit(topo.standalone, "ou=eng,o=acivattr,{}".format(DEFAULT_SUFFIX))`
		`+ ou_ou.create(properties=properties)`
		`properties = {'ou': 'sales'}`
		`- ou = OrganizationalUnit(topo.standalone, "ou=sales,o=acivattr,{}".format(DEFAULT_SUFFIX))`
		`- ou.create(properties=properties)`
		`+ ou_ou = OrganizationalUnit(topo.standalone, "ou=sales,o=acivattr,{}".format(DEFAULT_SUFFIX))`
		`+ ou_ou.create(properties=properties)`

		`roles = FilterRoles(topo.standalone, DNBASE)`
		`roles.create(properties={'cn': 'FILTERROLEENGROLE', 'nsRoleFilter': 'cn=eng*'})`
		`@@ -64,7 +71,8 @@`
		`'homeDirectory': '/home/' + 'salesuser1',`
		`'userPassword': PW_DM`
		`}`
		`- user = UserAccount(topo.standalone, 'cn=salesuser1,ou=sales,o=acivattr,{}'.format(DEFAULT_SUFFIX))`
		`+ user = UserAccount(topo.standalone,`
		`+ 'cn=salesuser1,ou=sales,o=acivattr,{}'.format(DEFAULT_SUFFIX))`
		`user.create(properties=properties)`

		`properties = {`
		`@@ -76,7 +84,8 @@`
		`'homeDirectory': '/home/' + 'salesmanager1',`
		`'userPassword': PW_DM,`
		`}`
		`- user = UserAccount(topo.standalone, 'cn=salesmanager1,ou=sales,o=acivattr,{}'.format(DEFAULT_SUFFIX))`
		`+ user = UserAccount(topo.standalone,`
		`+ 'cn=salesmanager1,ou=sales,o=acivattr,{}'.format(DEFAULT_SUFFIX))`
		`user.create(properties=properties)`

		`properties = {`
		`@@ -88,7 +97,8 @@`
		`'homeDirectory': '/home/' + 'enguser1',`
		`'userPassword': PW_DM`
		`}`
		`- user = UserAccount(topo.standalone,'cn=enguser1,ou=eng,o=acivattr,{}'.format(DEFAULT_SUFFIX))`
		`+ user = UserAccount(topo.standalone,`
		`+ 'cn=enguser1,ou=eng,o=acivattr,{}'.format(DEFAULT_SUFFIX))`
		`user.create(properties=properties)`

		`properties = {`
		`@@ -100,24 +110,29 @@`
		`'homeDirectory': '/home/' + 'engmanager1',`
		`'userPassword': PW_DM`
		`}`
		`- user = UserAccount(topo.standalone, 'cn=engmanager1,ou=eng,o=acivattr,{}'.format(DEFAULT_SUFFIX))`
		`+ user = UserAccount(topo.standalone,`
		`+ 'cn=engmanager1,ou=eng,o=acivattr,{}'.format(DEFAULT_SUFFIX))`
		`user.create(properties=properties)`

		`- #user with cn=sales* will automatically memeber of nsfilterrole cn=filterrolesalesrole,o=acivattr,dc=example,dc=com`
		`- assert UserAccount(topo.standalone, 'cn=salesuser1,ou=sales,o=acivattr,dc=example,dc=com').get_attr_val_utf8(`
		`- 'nsrole') == 'cn=filterrolesalesrole,o=acivattr,dc=example,dc=com'`
		`+ # user with cn=sales* will automatically memeber of nsfilterrole`
		`+ # cn=filterrolesalesrole,o=acivattr,dc=example,dc=com`
		`+ assert UserAccount(topo.standalone,`
		`+ 'cn=salesuser1,ou=sales,o=acivattr,dc=example,dc=com').\`
		`+ get_attr_val_utf8('nsrole') == 'cn=filterrolesalesrole,o=acivattr,dc=example,dc=com'`
		`# same goes to SALES_MANAGER`
		`assert UserAccount(topo.standalone, SALES_MANAGER).get_attr_val_utf8(`
		`'nsrole') == 'cn=filterrolesalesrole,o=acivattr,dc=example,dc=com'`
		`- # user with cn=eng* will automatically memeber of nsfilterrole cn=filterroleengrole,o=acivattr,dc=example,dc=com`
		`- assert UserAccount(topo.standalone, 'cn=enguser1,ou=eng,o=acivattr,dc=example,dc=com').get_attr_val_utf8(`
		`- 'nsrole') == 'cn=filterroleengrole,o=acivattr,dc=example,dc=com'`
		`+ # user with cn=eng* will automatically memeber of nsfilterrole`
		`+ # cn=filterroleengrole,o=acivattr,dc=example,dc=com`
		`+ assert UserAccount(topo.standalone, 'cn=enguser1,ou=eng,o=acivattr,dc=example,dc=com').\`
		`+ get_attr_val_utf8('nsrole') == 'cn=filterroleengrole,o=acivattr,dc=example,dc=com'`
		`# same goes to ENG_MANAGER`
		`assert UserAccount(topo.standalone, ENG_MANAGER).get_attr_val_utf8(`
		`'nsrole') == 'cn=filterroleengrole,o=acivattr,dc=example,dc=com'`
		`- for DN in [ENG_USER, SALES_UESER, ENG_MANAGER, SALES_MANAGER, FILTERROLESALESROLE, FILTERROLEENGROLE, ENG_OU,`
		`- SALES_OU, DNBASE]:`
		`- UserAccount(topo.standalone, DN).delete()`
		`+ for dn_dn in [ENG_USER, SALES_UESER, ENG_MANAGER, SALES_MANAGER,`
		`+ FILTERROLESALESROLE, FILTERROLEENGROLE, ENG_OU,`
		`+ SALES_OU, DNBASE]:`
		`+ UserAccount(topo.standalone, dn_dn).delete()`


		`def test_managedrole(topo):`
		`@@ -140,14 +155,14 @@`
		`# Create user and Assign the role to the entry`
		`uas = UserAccounts(topo.standalone, DEFAULT_SUFFIX, rdn=None)`
		`uas.create(properties={`
		`- 'uid': 'Fail',`
		`- 'cn': 'Fail',`
		`- 'sn': 'user',`
		`- 'uidNumber': '1000',`
		`- 'gidNumber': '2000',`
		`- 'homeDirectory': '/home/' + 'Fail',`
		`- 'nsRoleDN': role.dn,`
		`- 'userPassword': PW_DM`
		`+ 'uid': 'Fail',`
		`+ 'cn': 'Fail',`
		`+ 'sn': 'user',`
		`+ 'uidNumber': '1000',`
		`+ 'gidNumber': '2000',`
		`+ 'homeDirectory': '/home/' + 'Fail',`
		`+ 'nsRoleDN': role.dn,`
		`+ 'userPassword': PW_DM`
		`})`

		`# Create user and do not Assign any role to the entry`
		`@@ -163,20 +178,23 @@`
		`})`

		`# Assert that Manage role entry is created and its searchable`
		`- assert ManagedRoles(topo.standalone, DEFAULT_SUFFIX).list()[0].dn == 'cn=ROLE1,dc=example,dc=com'`
		`+ assert ManagedRoles(topo.standalone, DEFAULT_SUFFIX).list()[0].dn \`
		`+ == 'cn=ROLE1,dc=example,dc=com'`

		`# Set an aci that will deny ROLE1 manage role`
		`- Domain(topo.standalone, DEFAULT_SUFFIX).add('aci', '(targetattr=*)(version 3.0; aci "role aci"; deny(all) roledn="ldap:///{}";)'.format(role.dn),)`
		`+ Domain(topo.standalone, DEFAULT_SUFFIX).\`
		`+ add('aci', '(targetattr=*)(version 3.0; aci "role aci";'`
		`+ ' deny(all) roledn="ldap:///{}";)'.format(role.dn),)`

		`# Crate a connection with cn=Fail which is member of ROLE1`
		`conn = UserAccount(topo.standalone, "uid=Fail,{}".format(DEFAULT_SUFFIX)).bind(PW_DM)`
		`# Access denied to ROLE1 members`
		`- assert 0 == len(ManagedRoles(conn, DEFAULT_SUFFIX).list())`
		`+ assert not ManagedRoles(conn, DEFAULT_SUFFIX).list()`

		`# Now create a connection with cn=Success which is not a member of ROLE1`
		`conn = UserAccount(topo.standalone, "uid=Success,{}".format(DEFAULT_SUFFIX)).bind(PW_DM)`
		`# Access allowed here`
		`- assert 1 == len(ManagedRoles(conn, DEFAULT_SUFFIX).list())`
		`+ assert ManagedRoles(conn, DEFAULT_SUFFIX).list()`

		`for i in uas.list():`
		`i.delete()`
		`@@ -185,6 +203,92 @@`
		`i.delete()`


		`+ @pytest.fixture(scope="function")`
		`+ def _final(request, topo):`
		`+ """`
		`+ Removes and Restores ACIs after the test.`
		`+ """`
		`+ aci_list = Domain(topo.standalone, DEFAULT_SUFFIX).get_attr_vals('aci')`
		`+`
		`+ def finofaci():`
		`+ """`
		`+ Removes and Restores ACIs and other users after the test.`
		`+ """`
		`+ domain = Domain(topo.standalone, DEFAULT_SUFFIX)`
		`+ domain.remove_all('aci')`
		`+`
		`+ managed_roles = ManagedRoles(topo.standalone, DEFAULT_SUFFIX)`
		`+ nested_roles = NestedRoles(topo.standalone, DEFAULT_SUFFIX)`
		`+ users = UserAccounts(topo.standalone, DEFAULT_SUFFIX)`
		`+`
		`+ for i in managed_roles.list() + nested_roles.list() + users.list():`
		`+ i.delete()`
		`+`
		`+ for i in aci_list:`
		`+ domain.add("aci", i)`
		`+`
		`+ request.addfinalizer(finofaci)`
		`+`
		`+`
		`+ def test_nestedrole(topo, _final):`
		`+ """`
		`+ :id: d52a9cw0-3bg6-11e9-9b7b-8c16451d917t`
		`+ :setup: Standalone server`
		`+ :steps:`
		`+ 1. Add test entry`
		`+ 2. Add ACI`
		`+ 3. Search managed role entries`
		`+ :expectedresults:`
		`+ 1. Entry should be added`
		`+ 2. Operation should succeed`
		`+ 3. Operation should succeed`
		`+ """`
		`+ # Create Managed role entry`
		`+ managed_roles = ManagedRoles(topo.standalone, DEFAULT_SUFFIX)`
		`+ managed_role1 = managed_roles.create(properties={"cn": 'managed_role1'})`
		`+ managed_role2 = managed_roles.create(properties={"cn": 'managed_role2'})`
		`+`
		`+ # Create nested role entry`
		`+ nested_roles = NestedRoles(topo.standalone, DEFAULT_SUFFIX)`
		`+ nested_role = nested_roles.create(properties={"cn": 'nested_role',`
		`+ "nsRoleDN": [managed_role1.dn, managed_role2.dn]})`
		`+`
		`+ # Create user and assign managed role to it`
		`+ users = UserAccounts(topo.standalone, DEFAULT_SUFFIX)`
		`+ user1 = users.create_test_user(uid=1, gid=1)`
		`+ user1.set('nsRoleDN', managed_role1.dn)`
		`+ user1.set('userPassword', PW_DM)`
		`+`
		`+ # Create another user and assign managed role to it`
		`+ user2 = users.create_test_user(uid=2, gid=2)`
		`+ user2.set('nsRoleDN', managed_role2.dn)`
		`+ user2.set('userPassword', PW_DM)`
		`+`
		`+ # Create another user and do not assign any role to it`
		`+ user3 = users.create_test_user(uid=3, gid=3)`
		`+ user3.set('userPassword', PW_DM)`
		`+`
		`+ # Create a ACI with deny access to nested role entry`
		`+ Domain(topo.standalone, DEFAULT_SUFFIX).\`
		`+ add('aci', f'(targetattr=*)(version 3.0; aci '`
		`+ f'"role aci"; deny(all) roledn="ldap:///{nested_role.dn}";)')`
		`+`
		`+ # Create connection with 'uid=test_user_1,ou=People,dc=example,dc=com' member of managed_role1`
		`+ # and search while bound as the user`
		`+ conn = users.get('test_user_1').bind(PW_DM)`
		`+ assert not UserAccounts(conn, DEFAULT_SUFFIX).list()`
		`+`
		`+ # Create connection with 'uid=test_user_2,ou=People,dc=example,dc=com' member of managed_role2`
		`+ # and search while bound as the user`
		`+ conn = users.get('test_user_2').bind(PW_DM)`
		`+ assert not UserAccounts(conn, DEFAULT_SUFFIX).list()`
		`+`
		`+ # Create connection with 'uid=test_user_3,ou=People,dc=example,dc=com' and`
		`+ # search while bound as the user`
		`+ conn = users.get('test_user_3').bind(PW_DM)`
		`+ assert UserAccounts(conn, DEFAULT_SUFFIX).list()`
		`+`
		`+`
		`if __name__ == "__main__":`
		`CURRENT_FILE = os.path.realpath(__file__)`
		`pytest.main("-s -v %s" % CURRENT_FILE)`

src/lib389/lib389/idm/role.py

file modified

+49 -1

		`@@ -112,4 +112,52 @@`
		`]`
		`self._filterattrs = ['cn']`
		`self._basedn = basedn`
		`- self._childobject = ManagedRole`
		`\ No newline at end of file`
		`+ self._childobject = ManagedRole`
		`+`
		`+`
		`+ class NestedRole(DSLdapObject):`
		`+ """A single instance of NestedRole entry to create NestedRole role.`
		`+`
		`+ :param instance: An instance`
		`+ :type instance: lib389.DirSrv`
		`+ :param dn: Entry DN`
		`+ :type dn: str`
		`+`
		`+ """`
		`+ def __init__(self, instance, dn=None):`
		`+ super(NestedRole, self).__init__(instance, dn)`
		`+ self._must_attributes = ['cn', 'nsRoleDN']`
		`+ self._rdn_attribute = 'cn'`
		`+ self._create_objectclasses = [`
		`+ 'top',`
		`+ 'nsRoleDefinition',`
		`+ 'nsComplexRoleDefinition',`
		`+ 'ldapSubEntry',`
		`+ 'nsNestedRoleDefinition'`
		`+ ]`
		`+`
		`+`
		`+ class NestedRoles(DSLdapObjects):`
		`+ """DSLdapObjects that represents all NestedRoles entries in suffix.`
		`+`
		`+ This instance is used mainly for search operation NestedRoles role`
		`+`
		`+ :param instance: An instance`
		`+ :type instance: lib389.DirSrv`
		`+ :param basedn: Suffix DN`
		`+ :type basedn: str`
		`+ :param rdn: The DN that will be combined wit basedn`
		`+ :type rdn: str`
		`+ """`
		`+ def __init__(self, instance, basedn):`
		`+ super(NestedRoles, self).__init__(instance)`
		`+ self._objectclasses = [`
		`+ 'top',`
		`+ 'nsRoleDefinition',`
		`+ 'nsComplexRoleDefinition',`
		`+ 'ldapSubEntry',`
		`+ 'nsNestedRoleDefinition'`
		`+ ]`
		`+ self._filterattrs = ['cn']`
		`+ self._basedn = basedn`
		`+ self._childobject = NestedRole`

aborah commented 5 years ago

Making an NestedRoleDefinition type in src/lib389/lib389/idm/role.py

https://pagure.io/389-ds-base/issue/50313

Reviewed by: ???

spichugi commented on line 23 of src/lib389/lib389/idm/role.py 5 years ago

Looks like we also need LDAPsubentry objectClass here.

spichugi commented on line 11 of src/lib389/lib389/idm/role.py 5 years ago

If you are mentioning the objectClass here - it should be nsNestedRoleDefinition

spichugi commented on line 21 of src/lib389/lib389/idm/role.py 5 years ago

We also need 'self._must_attributes' here which should contain nsRoleDN and cn

spichugi commented on line 38 of dirsrvtests/tests/suites/roles/basic_test.py 5 years ago

Typo. And variable should be lowercase. Please, run pytest-pylint here, also

spichugi commented on line 57 of dirsrvtests/tests/suites/roles/basic_test.py 5 years ago

Instead of UserAccount(topo.standalone, f'uid=test_user_1,ou=People,{DEFAULT_SUFFIX}') you can use existing UserAccounts instance and get users from it by selector.

spichugi commented on line 56 of dirsrvtests/tests/suites/roles/basic_test.py 5 years ago

The goal here is not to create a connection but to search while bound as the user

aborah commented 5 years ago

Looks like we also need LDAPsubentry objectClass here.

That will not be required , as it will come by default

aborah commented 5 years ago

If you are mentioning the objectClass here - it should be nsNestedRoleDefinition

In previous reviews @tbordaz asked me to remove all the 'ns' (except from object classes ) , for this one i will change it to NestedRole

rebased onto b1450d43b64f7b372aa19170d846b90817a065bd

5 years ago

aborah commented 5 years ago

@spichugi , all other changes are done as per your suggestion

spichugi commented on line 21 of src/lib389/lib389/idm/role.py 5 years ago

Please, add another attribute I've asked to add. The definition won't work without it.

spichugi commented on line 75 of dirsrvtests/tests/suites/roles/basic_test.py 5 years ago

I think it makes sense to join the lists and run delete() operation on all of them. It will be 2 lines instead of 6.

spichugi commented 5 years ago

Issue: 50313 - Making an NestedRoleDefinition type - is an unneccessary long line, I think.
And follow the basic commit message rules, please.
It should be something like - Issue: 50313 - Add a NestedRole type to lib389

And the commit message body should have some info about the test you've added too.

spichugi commented 5 years ago

Typo. And variable should be lowercase. Please, run pytest-pylint here, also

Please, don't ignore my comments. :) You still have a bunch of pylint issues.
The code will be much more maintainable if we follow basic lint rules.

tbordaz commented on line 46 of src/lib389/lib389/idm/role.py 5 years ago

Just notice this, I would expect a role definition to be a subentry. This applies to managed/filtered/nested roles.
Could you confirm that the 'objectclass: ldapsubentry' was missing in original role definitions ?

tbordaz commented on line 76 of dirsrvtests/tests/suites/roles/basic_test.py 5 years ago

I think that once the test is completed, all items added by the test should be deleted (roles (managed and nested), users and ACI).
This deletion should occur if the test succeeds or not (so may be using a python directtive like 'finally')

aborah commented 5 years ago

Just notice this, I would expect a role definition to be a subentry. This applies to managed/filtered/nested roles.
Could you confirm that the 'objectclass: ldapsubentry' was missing in original role definitions ?

objectclass , ldapsubentry will come by default . No need to put differently

aborah commented 5 years ago

I think that once the test is completed, all items added by the test should be deleted (roles (managed and nested), users and ACI).
This deletion should occur if the test succeeds or not (so may be using a python directtive like 'finally')

I have added one fixture to clean up the (roles (managed and nested), users and ACI).

rebased onto 7220c724858c1c54b21cc8df00d1ae28cab5f4be

5 years ago

rebased onto abc70b1ffdeb18ba7f39846e61d34f157c8af62d

5 years ago

aborah commented 5 years ago

@spichugi , all changes are done as per your suggestion

aborah commented 5 years ago

@tbordaz , @spichugi , checkout the out put for ldapsubentry , its coming by default without putting it separately

(Pdb) NestedRoles(topo.standalone, DEFAULT_SUFFIX).list()
[<role.NestedRole object at 0x7f7b38225470>]
(Pdb) NestedRoles(topo.standalone, DEFAULT_SUFFIX).list()[0].dn
'cn=nested_role,dc=example,dc=com'
(Pdb) NestedRoles(topo.standalone, DEFAULT_SUFFIX).list()[0]._unsafe_raw_entry()
dn: cn=nested_role,dc=example,dc=com
cn: nested_role
objectClass: top
objectClass: nsRoleDefinition
objectClass: nsComplexRoleDefinition
objectClass: nsNestedRoleDefinition
objectClass: ldapSubEntry

spichugi commented 5 years ago

Even though it is created automatically, we better have it as a part of the DSLdapObject and DSLdapObjects.
The explicitness will ensure that everything is in order.

rebased onto c5c0ef2472d5d98eff838ace2e27f682388fd81a

5 years ago

aborah commented 5 years ago

Even though it is created automatically, we better have it as a part of the DSLdapObject and DSLdapObjects.
The explicitness will ensure that everything is in order. '

Done

tbordaz commented 5 years ago

@spichugi , just to be sure. Did ldapsubentry come from DSLdapObject or DSLdapObjects ?

spichugi commented 5 years ago

@spichugi , just to be sure. Did ldapsubentry come from DSLdapObject or DSLdapObjects ?

DSLdapObject._create_objectclasses sets the objectClasses that will be created in a new entry
DSLdapObjects._objectclasses sets the objectClasses that will be used as a filter for the searches.

spichugi commented on line 20 of dirsrvtests/tests/suites/roles/basic_test.py 5 years ago

ManagedRole is an unused import

spichugi commented 5 years ago

The commit message body should follow the same consistent imperative rule as the subject. And the class name should be consistent too. Something like this:

Add the NestedRole and the NestedRoles classes to src/lib389/lib389/idm/role.py
Add one test case that will test that the new class NestedRoles is
working fine.

rebased onto 9724e8b

5 years ago

aborah commented 5 years ago

@spichugi , all changes are done as per your suggestion

spichugi commented 5 years ago

LGTM. Ack!