Not really sure about this formatting here. Maybe:

assert(1 == list(len(conn.search_s(...))))

This shouldn't be on one line....

Isn't there a test_user_create function you can use on userAccount?

Shouldn't this be a "d = Domain(DEFAULT_SUFFIX); l = d.get('aci'); d.set('aci', l)"?
That would avoid ensure_bytes. Generally, if you see "ensure bytes" it probably means you are doing something incorrectly, as we shouldnt' have this at a high level like tests.

Please don't use raw modify_s, there is a "object.delete('attr')" instead you can use.

Because you shtouldn't ever need raw "b'str'" types ...

If you use Domain.add here, you don't need ensure_bytes.

I'm sorry @aborah But I want you to self review this patch you just provided with my comments again. I looked really quickly and I still see formating and incorrect api usage changes. I really appreciate you want these changes merged, but when I provide comments they aren't just for single lines, but I provide them so that they apply to the whole patch. This means if I point out a change in one location I expect you to apply it across the whole file.

I think you should review this patch again with my comments previously in mind, and please update it. Additionally, I think the "please merge if ok" is maybe not the right language to use in a project like this. The team will make the decision when we are ready. Thanks,

This style of formatting is something I raised in my review, and it's not been corrected.

You don't need a list comprehension here, just use conn.search_s as it already returns a list .....

Can you comment what this is trying to achieve (I think it's checking aci's show different number of users ...)

This indentation is a bit weird I think.

Why uniquemember and not member? Group in ds should be groupOfNames not groupOfUniqueNames ....

You define dn twice. Consider format strings instead "ou=%s,%s" % (thing, suffix)

Create test user already exists in the UserAccount class I think ....

Create test user already exists in the UserAccount class I think ....

Yes there is one , but while creating user with Create test user already exists in the UserAccount class, user will be like uid='', but for some of the test cases i need users with cn='',
thats why i am using this one .

Why uniquemember and not member? Group in ds should be groupOfNames not groupOfUniqueNames ....

as test case says test_deny_access_to_group_should_deny_access_to_all_uniquemember , thats why i am adding users as uniquemember

Why do you have seperate create test functions here? We can add more to core lib389 if needed that are generic over a variety of tests instead of adding them to every suite individually.

@firstyear , i have removed whole working_constans.py now only generic create_test functions are there .

You shouldn't need raw search, there are other ways to check this.

I am only going to comment on this that you have again, used a raw search_s() function. There are other ways to achieve this. For example:

users = UserAccounts(inst, DEFAULT_SUFFIX)
users.list()

That's it! Note the 's', which pluralises the object type. UserAccounts represents "all possible userAccounts" on the directory Server, and thus, allows you to wrap and search based on who the inst is bound as.

I am only going to comment on this that you have again, used a raw search_s() function. There are other ways to achieve this. For example:
users = UserAccounts(inst, DEFAULT_SUFFIX)
users.list()

That's it! Note the 's', which pluralises the object type. UserAccounts represents "all possible userAccounts" on the directory Server, and thus, allows you to wrap and search based on who the inst is bound as.

For bellow case what will be the solution , as per the test case aci allows only targetattr="mail"

Domain(topo.standalone, DEFAULT_SUFFIX).replace("aci", '(target="ldap:///{}")(targetattr="mail")''(version 3.0; acl "Test";allow (read,search,compare) ''(userdn = "ldap:///anyone"); )'.format(DEFAULT_SUFFIX))

conn = UserAccount(topo.standalone, "").bind("")
# aci will allow only mail targetattr
assert 3 == len(conn.search_s(DEFAULT_SUFFIX, ldap.SCOPE_SUBTREE, "mail=*"))
# aci will allow only mail targetattr
assert 0 == len(conn.search_s(DEFAULT_SUFFIX, ldap.SCOPE_SUBTREE, "cn=*"))

@aborah

user_conn = UserAccount(inst, "").bind("") # See my rcomments about making an anonymous type
AnonUserView = UserAccounts(use_conn, DEFAULT_SUFFIX)
users = AnonUserView.list() # Should return no entries because cn is not searchable

We'll need to add a generic filter type to handle the mail=* case. This is a perfect example of "if we do extra work today to fix it properly" we never have to fix it ever ever again.

PS: This means, open a new ticket for "Add generic filter to DSLdapObjects" ticket, and you will implement that first and separately, in isolation, with tests, then we'll rebase onto that work from here.

@firstyear where should i put it , any suggestion how to approach , like how to do the job done

You already opened a ticket for this? https://pagure.io/389-ds-base/issue/50219 So I guess that's where...

@firstyear we have already implemented filter and changes are done in the scripts accordingly . Please check

I'll check monday if that's okay.

@aborah, I would like to run the tests would it be possible to set the aci logs for all tests.
Should be something like instance.config.loglevel((ErrorLog.LOG_ACL_SUMMARY,))"

@aborah, I would like to validate some testcase to be sure we are testing what we expect to validate. Will do it next week

You know you don't need this syntax? The following is cleaner:

ous = OrganizationalUnits(topo.standalone, DEFAULT_SUFFIX)
for i in ['a', 'b']: #lazy names
    ous.create(properties={'ou': i})

Learn to use the multiple versions of dsldapobjects. Every "type" (with a handful of exceptions, mainly config) has a pair of types.

There is the DSLdapObject (singular) that represents "a single entry in the directory" and manipulations of that.

There is DSLdapObjects (plural) that represents "the concept of all of the entry type in the directory" and interactions with all of them.

The idea is that you have say "Groups" which represents "all possible groups that exist", and a "Group" which is one group, that really really exists and can be interacted with.

So when you are creating a new group, you say "Groups.create()", because you are saying "To the set of all possible groups, I create a new group". Because Groups "knows" what a group should look like, and where, it creates it in the right spot for you.

In this case, OU's plural always creates "directly under default suffix", so using the pattern above is nicer

Now continuing the lesson, here you do this horrid looking weird "name,ou=..." syntax. When what you actually want is:

users = UserAccounts(topo.standalone, basedn='ou=Product Development,DEFAULT_SUFFIX')
users.create(properties={
    'uid': 'jeff',
    ...
})

In a similar way to above, this UserAccounts (plural), is saying "for the concept of all users that could possible exist under ou=Product Development,..." then of course, the .create knows how to put the jeff object in exactly the right place!

Remember, you use for loops to eliminate repeated work, but here you are actually doing different work (there are two ou's you create in), so just do the above segment twice. Over optimisation leads to situations like this.

Don't use accounts here, it will do something you don't expect. OU's are VALID ACCOUNTS. which is absoluely insane and broken, but that's LDAP for you. You should be more specific to the type you are accessing like UserAccounts or something.

Now continuing the lesson, here you do this horrid looking weird "name,ou=..." syntax. When what you actually want is:
users = UserAccounts(topo.standalone, basedn='ou=Product Development,DEFAULT_SUFFIX')
users.create(properties={
'uid': 'jeff',
...
})

In a similar way to above, this UserAccounts (plural), is saying "for the concept of all users that could possible exist under ou=Product Development,..." then of course, the .create knows how to put the jeff object in exactly the right place!
Remember, you use for loops to eliminate repeated work, but here you are actually doing different work (there are two ou's you create in), so just do the above segment twice. Over optimisation leads to situations like this.

You did not check the rdn (ou) value both are different:

for i in ['Jeff Vedder,ou=Product Development', 'Sam Carter,ou=Accounting']:
UserAccount(topo.standalone, "cn={},{}".format(i, DEFAULT_SUFFIX)).create(properties={

anyway i have changed it to UserAccounts()

The tests looks good to me.
Please to simplify the read of the testcase, rename 'USER_DELADD" into "USER_ANUJ" and "USER_WITH_ACI_DELADD" into "USER_ANANDA"

@aborah, I review and check validity of few TC. Note I can not review/verify all of them.
You have my ACK.
Please wait for @firstyear apporval before merging

@firstyear , this one got @tbordaz approval please check

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/3235

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.