search

389ds / 389-ds-base

The enterprise-class Open Source LDAP server for Linux
https://www.port389.org/
Other
210 stars 89 forks source link

PR - Issue: 50170 - composable object types for nsRole in lib389 #3230

Closed 389-ds-bot closed 4 years ago

389-ds-bot commented 4 years ago

Cloned from Pagure Pull-Request: https://pagure.io/389-ds-base/pull-request/50171

Composable object types for nsRole in lib389

Resolves: #3229

Reviewed by: ???

389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2019-01-16 17:26:08

are you creating a nsroledefinition for a filtered role ? But then you would need to specify a nsRoleFilter to determine which users will have this role

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-01-17 00:06:55

I think that @elkris is pointing out this part here. I think (but can not remember correctly), but you may only need nsFilteredRole OR nsComplexRole, but not both.

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-01-17 00:08:05

Hey @aborah-sudo

I put an example of how to do this on the mailing list. I think the nsRole parts need to be in an nsRole class, then nsUserAccountRole needs to subclass both nsRole and nsUserAccount. That way we ccan reuse nsRole in say "Account" or "UserAccount" etc. If we did it like this, we'd need to duplicate all the nsRole functionality in every place we want to use it.

389-ds-bot commented 4 years ago

Comment from aborah (@aborah-sudo) at 2019-01-18 06:14:00

rebased onto ef85226282386e0a7133f22dd3f6a1222ddc26fb

389-ds-bot commented 4 years ago

Comment from aborah (@aborah-sudo) at 2019-01-18 06:23:32

This small change in user.py of lib389 is done as we cant add user accounts with object class nsFilteredRoleDefinition with preexisting nsUserAccounts and UserAccounts as 'uid' is not supported by nsFilteredRoleDefinition . You cant add account and nsFilteredRoleDefinition object class all together as account object class needs 'uid' which is not supported by nsFilteredRoleDefinition.

Now we can add user with object class nsFilteredRoleDefinition with newly added nsUserAccountRole and nsUserAccountRoles(manly used for search operation).

This change is done to work with aciattr.py which requires users with nsRole filer which need nsFilteredRoleDefinition object class .

Kindly merge it , so that i can go ahead with my porting stuff .

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-01-18 07:23:07

Please don't add anything with ldapsubentry, because it hides it from administration and searches unless you request it.

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-01-18 07:23:59

Okay, this looks better,but this is the kind of code where an associated test to show it's execution and usage would be great. It also means we are testing we don't regress on the lib389 feature itself. It would be good to see this please :)

389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2019-01-18 09:36:12

Now we can add user with object class nsFilteredRoleDefinition with newly added nsUserAccountRole and nsUserAccountRoles(manly used for search operation). This change is done to work with aciattr.py which requires users with nsRole filer which need nsFilteredRoleDefinition object class .

Again, this is not how roles work, please read: https://access.redhat.com/documentation/en-us/red_hat_directory_server/10/html/administration_guide/advanced_entry_management-using_roles#Managing_Roles_Using_the_Console-Creating_a_Filtered_Role

The role definition is NOT in the user entry, the the role difinition is a separate entry with the roles objectclasses, for a filtered role you then have to specify an "nsRoleFilter" attribute, eg.

nsRoleFilter: o=myrole

and then all user accounts matching this filter will have the role

389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2019-01-18 09:44:19

Please don't add anything with ldapsubentry, because it hides it from administration and searches unless you request it.

since the role definitions are separate entries just performing a hidden mechanism as assigning roles, but are in the tree it might be ok to hide them from normal searches. They will only be needed to administrate roles

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-01-18 10:00:18

Please don't add anything with ldapsubentry, because it hides it from administration and searches unless you request it.

since the role definitions are separate entries just performing a hidden mechanism as assigning roles, but are in the tree it might be ok to hide them from normal searches. They will only be needed to administrate roles

I'm still a bit cautious of hiding things where there isn't an explicit requirement to do the hiding as this can cause administrative traps due to the fact that people may not know what ldapsubentry is or does (even when I was an ldap admin, it took me years to find out about this ...).

So I'm not "intent" to say no to this, but I think there is a benefit to not having ldapsubentry here in admin transparency.

389-ds-bot commented 4 years ago

Comment from aborah (@aborah-sudo) at 2019-01-18 10:00:24

Please don't add anything with ldapsubentry, because it hides it from administration and searches unless you request it.

since the role definitions are separate entries just performing a hidden mechanism as assigning roles, but are in the tree it might be ok to hide them from normal searches. They will only be needed to administrate roles

By default these entries will hidden entry . where you define , with ldapsubentry object class or not . I will show with usages

389-ds-bot commented 4 years ago

Comment from aborah (@aborah-sudo) at 2019-01-18 10:03:07

Now we can add user with object class nsFilteredRoleDefinition with newly added nsUserAccountRole and nsUserAccountRoles(manly used for search operation). This change is done to work with aciattr.py which requires users with nsRole filer which need nsFilteredRoleDefinition object class .

Again, this is not how roles work, please read: https://access.redhat.com/documentation/en-us/red_hat_directory_server/10/html/administration_guide/advanced_entry_management-using_roles#Managing_Roles_Using_the_Console-Creating_a_Filtered_Role The role definition is NOT in the user entry, the the role difinition is a separate entry with the roles objectclasses, for a filtered role you then have to specify an "nsRoleFilter" attribute, eg. nsRoleFilter: o=myrole and then all user accounts matching this filter will have the role

I am keeping this for user_props={'cn':'Anuj', 'nsRoleFilter':'cn=*'} , user must define , user_props before creating any entry , i will give some usage example .

389-ds-bot commented 4 years ago

Comment from aborah (@aborah-sudo) at 2019-01-18 10:24:32

rebased onto e5f5b81f20e0e03506bf20512f5e921ba536d374

389-ds-bot commented 4 years ago

Comment from aborah (@aborah-sudo) at 2019-01-18 10:31:28

Usages has been added , and for regression test , you can run dirsrvtests/tests/suites/acl/acivattr_test.py from PR: "50113 Issue: 50112 Port ACI test suit from TET to python3 (ACI Attr) "

Dont forget to take user.py from this PR(download raw user.py to local directory ), you have to change it on working_contstants.py.

use: from user import instead of: from lib389.idm.user import

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-01-20 23:16:17

@aborah-sudo Have you setup roles by hand before? It would be good to do this first, learn how it works and go from there. Perhaps in my explanations I have confused you about the lib389 parts, because I think that this isn't quite the solution that @elkris probably has in mind. So I apologise if I have led you astray here.

So. Here is what I would like to see, because I think that it will help you to write better tests. I'd like to see you write a "howto-roles" as a text file, which we could put onto the wiki even. Setup roles by hand and follow the documentation that was linked by elkris. Ask questions about it, and how it works, and we'll help you to make sure you understand it properly.

After that, then lets come back to the lib389 code. We'll write a nsRoles for lib389 and make it composable, along with some examples of usage.

Then finally, the tests can work with it.

I know this will take a bit longer, and may seem a bit more work, but it's really important that we do this the right way, and we learn and share knowledge so that we can all do the best work we can. @elkris and I are happy to help and explain any questions you have,

Thanks,

389-ds-bot commented 4 years ago

Comment from aborah (@aborah-sudo) at 2019-01-21 04:37:18

rebased onto 19efea6c9aa7faff72a09028b1b99989962ac0e9

389-ds-bot commented 4 years ago

Comment from aborah (@aborah-sudo) at 2019-01-21 04:59:34

One small correction here :

using newly created nsFilterAccountRole and nsFilterAccountRoles ( Will be used only to create filter role ) , i am creating filter roles only . This is the confusion here , we should remember filter roles are nothing but entries with o='something'. I am not touching any user here , but i am creating roles and these roles are covering the users automatically as elkris said earlier. example-

dn: cn=FILTERROLEENGROLE,o=acivattr1,dc=example,dc=com cn: FILTERROLEENGROLE nsRoleFilter: cn=* objectClass: top objectClass: LDAPsubentry objectClass: nsRoleDefinition objectClass: nsComplexRoleDefinition objectClass: nsFilteredRoleDefinition

This above entry is nothing but filter role entry , which will cover all users in 'o=acivattr1' which has sub entries that begins with 'cn'. And this is the property of filter role .

Yes , i must say that newly created nsFilterAccountRole and nsFilterAccountRoles will only cover filter role as you cant create Filter role and Manage role all together . You cant create other roles like Managed Roles which needs nsManagedRoleDefinition object class . For my porting stuff newly created nsFilterAccountRole and nsFilterAccountRoles is more than enough because i need filter roles only .

Hope it clears all of your doubt .

389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2019-01-21 08:58:22

I am travellinh this week and will bemostly in meetings, so only can look at it later.

Could you please follow William's suggestion to write up a doc what the tests are supposed to do, it would really help

389-ds-bot commented 4 years ago

Comment from aborah (@aborah-sudo) at 2019-02-06 19:12:29

rebased onto f4f31642751175c0d47450608f0f51e1d9a18496

389-ds-bot commented 4 years ago

Comment from aborah (@aborah-sudo) at 2019-02-06 19:14:25

rebased onto 711540b1773d6b9295a6c26be16c6850056a012b

389-ds-bot commented 4 years ago

Comment from aborah (@aborah-sudo) at 2019-02-06 19:16:20

Just make some changes , with DSLdapObject, DSLdapObjects it will create and search nsfilter role entries

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-02-11 08:44:47

I don't think we should add ldapsubentry here.

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-02-11 08:44:56

As above

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-02-11 08:45:16

This rdn probably should not be here.

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-02-11 08:45:30

And because you have no rdn, you don't need the special handling here.

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-02-11 08:46:18

Is there a simple test demonstrating this code to go with this? For example, dirsrvtests/suites/roles/basic.py? It would be good to see this inuse with basic verification of correctness.

389-ds-bot commented 4 years ago

Comment from aborah (@aborah-sudo) at 2019-02-11 09:43:35

rebased onto 60a5f3b20debd90b9221bf4962f88ae7d612a5a3

389-ds-bot commented 4 years ago

Comment from aborah (@aborah-sudo) at 2019-02-11 09:47:00

@Firstyear changes has been done as per your suggestion , i will put some example in dirsrvtests/suites/roles/basic.py if this code looks good to you

389-ds-bot commented 4 years ago

Comment from tbordaz (@tbordaz) at 2019-02-11 10:30:30

The filterRole is described in 6.2.2.2 (https://access.redhat.com/documentation/en-us/red_hat_directory_server/9.0/html/administration_guide/advanced_entry_management-using_roles#Managing_Roles_Using_the_Console-Creating_a_Filtered_Role)

I think this example may be confusing how to use filterrole. A role is independent of an account. It can exist a role even if no account is granted that role. So if you create a filter role it should not have a DN that looks like a user account (cn=tuser1).

The key of a filterrole is the 'nsroleFilter' attribute (i.e. nsRoleFilter: cn=*). With such filter, filterRole DN should rather be 'cn=AccountsWIthCN,cn=people,dc=example,dc=com'). So the userAccount 'cn=tuser1,ou=People,dc=example,dc=com' will be granted the role (nsRole) 'cn=AccountsWIthCN,cn=people,dc=example,dc=com'

389-ds-bot commented 4 years ago

Comment from aborah (@aborah-sudo) at 2019-02-11 12:49:09

rebased onto 174bed3191eff02f4d4b3bd4baed9730d803dca2

389-ds-bot commented 4 years ago

Comment from aborah (@aborah-sudo) at 2019-02-11 12:49:53

@tbordaz , made changes to the role example as per your suggestion

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-02-11 23:05:48

Can you also add a simple roles test suite in dirsrvtests to show that this is working as expected?

389-ds-bot commented 4 years ago

Comment from aborah (@aborah-sudo) at 2019-02-12 05:40:53

@Firstyear #3172#request_diff , test_nsrole is perfect example of this is working as expected, I am going add 3 more script (100 test cases) after this one get approved

389-ds-bot commented 4 years ago

Comment from aborah (@aborah-sudo) at 2019-02-12 05:45:04

rebased onto d1337715d5789963e6bd038c7a7395cd3f75fd08

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-02-12 07:03:51

The test of the functionality in this pr, should be in the same pr. The tests in your "tet pull" are about testing the server functionality, this pr should have a test to show the lib389 parts work as we expect.

So either move the test_nsrole from that pr to this one in a new test file in suites somewhere, or make a new test here for the suites please.

389-ds-bot commented 4 years ago

Comment from aborah (@aborah-sudo) at 2019-02-12 07:27:07

rebased onto d5ba70443b30a6586c006dfa0240e6ee47757719

389-ds-bot commented 4 years ago

Comment from aborah (@aborah-sudo) at 2019-02-12 07:28:12

@Firstyear as per your suggestion , dirsrvtests/tests/suites/roles/basic_test.py is created

389-ds-bot commented 4 years ago

Comment from aborah (@aborah-sudo) at 2019-02-12 07:29:06

rebased onto 9547dff2a7605f7bf14e2a2b02791219edabdfd5

389-ds-bot commented 4 years ago

Comment from aborah (@aborah-sudo) at 2019-02-12 07:31:24

rebased onto 58ff948f4835374783899be250d654c8a28bf719

389-ds-bot commented 4 years ago

Comment from tbordaz (@tbordaz) at 2019-02-12 09:17:36

Thanks for this very nice test case. Few comments

The testcase add an aci in o=acivattr, but I think this finalizer removes all aci under . If it is, It is a bit overkill. Also, I would prefer to let the standard aci at suffix level because some others basic test may rely on it

389-ds-bot commented 4 years ago

Comment from tbordaz (@tbordaz) at 2019-02-12 09:19:36

Do you really need this aci ? I can not find where the client binds as enguser1. Also if the test is run as DM, you may safely ignore aci.

389-ds-bot commented 4 years ago

Comment from tbordaz (@tbordaz) at 2019-02-12 09:21:48

The helper nsFilterAccountRole creates a role. But the roles will apply not only on Account but on any entry matching the filters. Why not naming the help function like nsFilterRole ?

389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2019-02-12 09:22:05

Do you really need this aci ? I can not find where the client binds as enguser1. Also if the test is run as DM, you may safely ignore aci.

if an aci test is run as DM, you can ignore the test :-(

389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2019-02-12 09:23:17

Thanks for this very nice test case. Few comments The testcase add an aci in o=acivattr, but I think this finalizer removes all aci under . If it is, It is a bit overkill. Also, I would prefer to let the standard aci at suffix level because some others basic test may rely on it

An aci test always should only have the acis it needs for testing, having default acis around will allow too many side effects.

389-ds-bot commented 4 years ago

Comment from tbordaz (@tbordaz) at 2019-02-12 09:24:06

Same remark as above, I would prefer nsFilterRole than nsFilterAccountRole because the filter role is not limited to user Account

389-ds-bot commented 4 years ago

Comment from aborah (@aborah-sudo) at 2019-02-12 09:38:45

rebased onto c109d5688c63e1e343acb854913947dbee10d890

389-ds-bot commented 4 years ago

Comment from aborah (@aborah-sudo) at 2019-02-12 09:39:34

rebased onto a1e4984e02a1a4354bc20f8abfd44e4c28fef61e

389-ds-bot commented 4 years ago

Comment from aborah (@aborah-sudo) at 2019-02-12 09:43:13

rebased onto 5ded19d33175dc8196926c2b0c7ef4462b80bcaa

389-ds-bot commented 4 years ago

Comment from aborah (@aborah-sudo) at 2019-02-12 09:44:20

@tbordaz all changes are done as per your suggestion

389-ds-bot commented 4 years ago

Comment from tbordaz (@tbordaz) at 2019-02-12 15:39:48

@aborah-sudo thanks for your continuous effort on that improvement. The patch looks good to me. Please wait for @Firstyear and @elkris approvals as they also raise some comments.