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

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

Hey @aborah

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.

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 .

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

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 :)

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

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

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.

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

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 .

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 *

@aborah 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 @lkrispen 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 lkrispen. 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. @lkrispen and I are happy to help and explain any questions you have,

Thanks,

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 lkrispen 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 .

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

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

I don't think we should add ldapsubentry here.

As above

This rdn probably should not be here.

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

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.

@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

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'

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

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

@firstyear https://pagure.io/389-ds-base/pull-request/50113#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

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.

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

Thanks for this very nice test case.
Few comments

The testcase add an aci in o=acivattr,<suffix> but I think this finalizer removes all aci under <suffix>. 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

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.

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 ?

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 :-(

Thanks for this very nice test case.
Few comments
The testcase add an aci in o=acivattr,<suffix> but I think this finalizer removes all aci under <suffix>. 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.

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

@tbordaz all changes are done as per your suggestion

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

Do we really need all these imports as "*"? We should try to import only what we use.

What do you assert about this created user? Should they be a member of the role?

Similar comment here? What are you asserting?

Okay, I think my confusion here is you create the users/roles in the fixture, an assert in the test. I think this isn't right. I think you should have the create users with the asserts in the test, because it's the very act of creating the user that is testing the role, and the you assert that.

Imagine I wrote another test in this file today. Do I need all the users you have created? Is that really part of the fixture? Or is that part of the test? That will help guide you on where to put your code (fixtures are intended to be re-usable).

@firstyear all changes are done as per your suggestion .

get_attr_val_utf8 will prevent the need for b"" string here. It's really well worth your time to read the entire _mapped_object.py file, because that's the core of lib389, and most problems you think you have are "already solved" there in interesting and correct ways.

Minor, but string format is better than concat to read. You also are using string concat when not needed ('cn=' + 'sale...'. Why?)

This whole line is better as:

 "cn=salesmanager1,ou=sales,o=acivattr,%s" % DEFAULT_SUFFIX

Please really consider your usage of concat and why you are doing it, and if it is constructive.

We can probably reduce this to the minimal set of needed attributes. I don't see you using telephonenumber in any of the role definitions, so we probably don't need this extraneous copy paste data.

Probably nice to have some white space around this role create parts to help make it clearer and easier to read. Also, its "roles" as the variable, because you are using the plural nsFilterRoles (not nsFilterRole). Though I will admit, I deeply regret the use of plurals in lib389, it is a great source of confusion.

@firstyear all changes are done as per your suggestion

^ String concat ... I know sometimes I put the comment in a single place, but often broad comments apply to the full file please.

after that is fixed, I will ack and merge.

@firstyear all changes are done as per your suggestion

@aborah , @firstyear , just to say you did a great job here :)

Agreed: I know it took a long time, but I appreciate your patience. Everyone here cares about making this project the best, so we hold everyone to a high standard :)

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3230

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.