Seems okay to me, but should probably get @spichugi or @firstyear to look it over as well.

A lot of issues that you have here, already mentioned by @firstyear in previous PRs.
I'll go once again through the stuff - just to be more explicit...

But. Let's first fix small issues regarding the code. It will increase the readability and it covers 80% of the issues we usually mention in the review.

Please, run this and fix the issues it points out:

pip3 install pytest-pylint
py.test --pylint -m pylint dirsrvtests/tests/suites/acl/keywords_part2_test.py dirsrvtests/tests/suites/acl/keywords_test.py

Also, reach to me or any of us if you have any doubts about the report.

This may not work the way you want. Imagine I have my hosts file setup as:

172.24.10.1     systemhostname.localdomain

So socket.gethostname will give me my hostname, then it will give me the 172.24.x.x address BUT we are connected to the the test instance via localhost.

So I think this will have issues in containers and other host configurations.

Possibly the best thing to do here is actually just make ip '127.0.0.1', because in tests we generally connect to localhost only, and we should only be using that. Check topologies.py maybe.

'(target = "ldap:///{}")(targetattr=*)(version 3.0; aci "IP aci"; allow(all) userdn = "ldap:///{}" and ip != "{}" ;)'

This could be turned into an ACI_TEMPLATE constant that you populate into, and use named formats, not positional, for clarity.

You have this mod_seealso, but it only replaces one line? You also by doing the _mod_seealso are sneaky violating a rule I told you: ONLY test the exceptions
from a single function.

Your modsee also is initing the object with the conn and doing the read, when you actually, should have the object create outside of the "with pytest.raises".

Remember, all your code must follow this pattern. "correct, simple, fast". you have targetted "simple, correct, fast". As a result, you have broken a correctness rule by prematurely trying to over simplify.

This is not a good test because you are relying on external state of the system to then check the TIME, and determine results.

Consider the CI server. It only runs at about 12:00 my time AEST, so that's probably about 02:00AM UTC at a guess. So then this test in CI will ONLY ever
attempt the MORNING path, and will never actually check the AFTERNOON path. Before you say "but there is a test data in the afternoon ..." time is continually flowing, and so between this test and the afternoon test what happens if it goes from morning to afternoon? Now what? There are cases left untested.

A better solution is to take the current time, +- 2 hours either side as your allow window, then check you are allowed. Then have a denied test where you have +12 (+- 2 hours) from now, and assert it's denied.

No timezone issues, now flow issues, no risks.

A better solution is LD_PRELOAD a time shim into the server but that's wayyyyy out of scope.

ALso, this is probably a really ineffective way to get a datetime. You actually want:

>>> from datetime import datetime
>>> datetime.now()
>>> datetime.now().hour

I don't really know why you chose to code this like this, but it seems like instead of taking the time to research the right way to do it, you chose to rush and bruteforce a solution.

This is not a clear explanation, and your test code isn't clear what the ACI is testing due to your use of constants. I think you need a much better comment here about what this is attempting to do.

Similar: this is not a good comment. WHY can they not access the data? How is the ACI constructed to not allow the access?

Why?

Email says @aborah has commented, but they are not appearing.

ip = socket.gethostbyname(socket.gethostname()) --->>> This is the real ip of my machine.
If i have to follow you point , i have to assign '127.0.0.1' to my machine or test will fail .

socket.gethostbyname(socket.gethostname()) is the universal way to detect any ip address . If
your machine have ip of "127.0.0.1", socket.gethostbyname(socket.gethostname()) will detect it
and will assign it to aci .

I'm sorry, but this is incorrect in this context. gethostname get's you the primary interface of the machine, and only if your hosts file is filled correctly. Imagine my machine hostname was "candice" but /etc/hosts didn't have that entry? Then socket.gethostbyname will fail because there is no host candice.

When you are writing tests like this, the only ip you can guarantee will exist is 127.0.0.1. That's it. As well, you are always connecting by localhost (because that's what DirSrv does in tests!). Which means if you want to test an ACI by ip, you have to test by the ip we are connecting on which is always localhost. There is no way for you to avoid that.

Perhaps to make this more clear, here the output of ip from my docker env:

1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN group default qlen 1
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
    inet 127.0.0.1/8 scope host lo
       valid_lft forever preferred_lft forever
12: eth0@if13: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP group default
    link/ether 02:42:ac:11:00:04 brd ff:ff:ff:ff:ff:ff link-netnsid 0
    inet 172.17.0.4/16 brd 172.17.255.255 scope global eth0
       valid_lft forever preferred_lft forever

So here, socket gethostname will give me 'ldapkdc.example.com' for the container, that then resolves to '172.17.0.4', but dirsrv will connect via localhost, so will bypass the aci anyway.

For a different example, my laptop:

lo0: flags=8049<UP,LOOPBACK,RUNNING,MULTICAST> mtu 16384
    options=1203<RXCSUM,TXCSUM,TXSTATUS,SW_TIMESTAMP>
    inet 127.0.0.1 netmask 0xff000000
    inet6 ::1 prefixlen 128
    inet6 fe80::1%lo0 prefixlen 64 scopeid 0x1
    nd6 options=201<PERFORMNUD,DAD>
...
en0: flags=8863<UP,BROADCAST,SMART,RUNNING,SIMPLEX,MULTICAST> mtu 1500
    ether 38:f9:d3:51:eb:07
    inet6 fe80::1c5e:d5d5:c994:6652%en0 prefixlen 64 secured scopeid 0xa
    inet 192.168.1.119 netmask 0xffffff00 broadcast 192.168.1.255
    nd6 options=201<PERFORMNUD,DAD>
    media: autoselect
    status: active

>>> import socket
>>> socket.gethostname()
'amethyst'
>>> socket.gethostbyname('amethyst')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
socket.gaierror: [Errno 8] nodename nor servname provided, or not known

So the test would FAIL on my machine here, because my hosts is:

127.0.0.1   localhost
255.255.255.255 broadcasthost
::1             localhost
fe80::1%lo0 localhost

Please, just use localhost. I'm offering a simpler, more robust way for you to conduct the test case here (just use localhost), but instead you seem to be arguing for a more complex, less portable, more fragile solution? I'm won't accept it I'm sorry.

Changes are done , Please check

@spichugi , @firstyear , please review. @mreynolds already has already ack for this one

The comments like this look weird. Also I don't see any sense in them if we already have the test function name and docstrings. They just repeat.

The docstring doesn't make much sense...

dirsrvtests/tests/suites/acl/keywords_part2_test.py::test_access_from_certain_network_only_ip 
dirsrvtests/tests/suites/acl/keywords_part2_test.py::test_timeofday_keyword

These tests have failed for me on OpenStack machine (1minutetip).
Exceptions - DID NOT RAISE

The comments like this look weird. Also I don't see any sense in them if we already have the test function name and docstrings. They just repeat.

I've asked him to write comments explaining what the tests do, but I think these comments need work as you point out ... They should explain more about the intent, and why we test it the way we do, and why things are structured as they are for example.

These tests have failed for me on OpenStack machine (1minutetip).
Exceptions - DID NOT RAISE

This worked for me, what do you have in the logs?

@spichugi , @firstyear , @vashirov All changes are done , please check

It should not be a docstring. Please, check other files for the example

You've just created the ou. You can use the DN from the DSLdapOject itself. Less hardcoding is better :)

You create 'Authmethod', 'DNS' but you don't use them... I can't see any reason why (except it was copy-pasted)

Please, don't use a bare DSLdapObject without a legitimate reason.
You can operate with right DSLdapObject types here and group the removal separately for ou, useraccount, etc..

We have special methods for log processing. Please, use it.
https://fedorapeople.org/~spichugi/html/dirsrv_log.html

You can fetch the log by restarting the instance.
The operation you have used is dangerous.

It fails for me on 1minutetip machine here
Failed: DID NOT RAISE

if I recall correctly, you can put the fistures in a separate file (conftest.py) and call them here.
This way you'll get rid of the redundancy.

if I recall correctly, you can put the fistures in a separate file (conftest.py) and call them here.
This way you'll get rid of the redundancy.

I was doing exactly same as you are suggesting while pushing ACI test suits . But somebody may be firstyear or viktor advised me not to create separate files for one test suit . Just create independent test script , which can be run independently .Thats why i am not doing it .

All other changes are done as per your suggestion

It should be the docstring for the whole module. In this case, for the whole test suite

You also created 5 OUs. Shouldn't we remove them also?

As I said, please, use the restart operation for flushing logs

Please, use the ds_access_log fully here (it has an option for matching)

@spichugi all changes are done as per your suggestion

It looks a bit weird and implicit. What are you doing here?

It looks a bit weird and implicit. What are you doing here?

(Pdb) for i in ous_next.list(): i.dn
'ou=Keywords,dc=example,dc=com'
'ou=Dayofweek,ou=Keywords,dc=example,dc=com'
'ou=DNS,ou=Keywords,dc=example,dc=com'
'ou=IP,ou=Keywords,dc=example,dc=com'
'ou=Timeofday,ou=Keywords,dc=example,dc=com'

It contains 'ou=Keywords,dc=example,dc=com' , if you try to delete

for i in ous_next.list():
i.delete()

you will get non_leaf error , thats why i have removed it first then deleted the sub entries first
then deleted the 'ou=Keywords,dc=example,dc=com' separately.
ous = ous_next.list()
ous.pop(0)

  for i in ous:
        i.delete()

ou_origin.delete()

It looks a bit weird and implicit. What are you doing here?

(Pdb) for i in ous_next.list(): i.dn
'ou=Keywords,dc=example,dc=com'
'ou=Dayofweek,ou=Keywords,dc=example,dc=com'
'ou=DNS,ou=Keywords,dc=example,dc=com'
'ou=IP,ou=Keywords,dc=example,dc=com'
'ou=Timeofday,ou=Keywords,dc=example,dc=com'

You can sort the list in a way the parent will be at the end of the list.
The way you have it is too implicit and hard to follow. Also, it is hacky which we should avoid.

I was doing exactly same as you are suggesting while pushing ACI test suits . But somebody may be firstyear or viktor advised me not to create separate files for one test suit . Just create independent test script , which can be run independently .Thats why i am not doing it .

Still, I think we should put it to conftest.pt or to __init__.py. It doesn't make sense to put it to lib389 but it is redundant to have it duplicated in the both files.

Should be get_attr_vals_utf8

@spichugi all changes are done as per your suggestion

We should have the copyright here too

All of the imports should be separated to all of the test suite files accordingly to its usage. So please, clean up them. You shouldn't import all of them in the conftest.py

Please, sort the list.
The returned order matters.

Please, sort the list.
The returned order matters.

Can you please point it out again , i missed it

Please, sort the list.
The returned order matters.

Can you please point it out again , i missed it

(Pdb) ous.sort

(Pdb) ous.sort()
*** TypeError: '<' not supported between instances of 'OrganizationalUnit' and 'OrganizationalUnit'
(Pdb) ous.reverse()

it can be reversed , but cant be sorted

@spichugi all changes are done as per your suggestion

Please, sort the list.
The returned order matters.
Can you please point it out again , i missed it

(Pdb) ous.sort

(Pdb) ous.sort()
*** TypeError: '<' not supported between instances of 'OrganizationalUnit' and 'OrganizationalUnit'
(Pdb) ous.reverse()
it can be reversed , but cant be sorted

The return order of the ldapsearch (and search_s) is not defined.
To be sure, we should sort the objects.

OrganizationalUnit doesn't have sort but you still can sort.
For that, you can use sorted() function - set key to the OrganizationalUnit.dn length using a lambda function. And set reverse to True.
Then you can safely run through the list and delete the objects.

@spichugi all changes are done as per your suggestion

You can sort directly the ous_next.list(). For that, you should set the key to the lambda function for len(i.dn).

@spichugi all changes are done as per your suggestion

Looks good. Small nitpick though - could you please change the i to something like ou? It is better to use meaningful names always.
Otherwise - very good job! Great

The test case still fails on 1minutetip machine.
As @viktor pointed out, it happens because of the race condition. You can see it on fast machines while no special logging is enabled.
Could you please check it (reproduce) and then fix it with a flush of the logs or some other sane way?

@spichugi , till i figure out why that specific test case is failing (on fast machines), can you please start reviewing this PR : https://pagure.io/389-ds-base/pull-request/50192

@spichugi all changes are done as per your suggestion , i have done some changes in failed test cases , hopefully these test cases will not fail now

Just realised, dn_dn looks pretty weird. What is it?
I suggested to use ou here because it represents OrganisationalUnit object. It is not a DN, it is a DSLdapObject.

Could you please fix it here and in other places? dn_dn is really confusing.

Please, check the documentation about conftest.py. You don't need to import it. It is used by pytest by default.

dirsrvtests/tests/suites/acl/keywords_part2_test.py::test_timeofday_keyword still fails for me.

Also dirsrvtests/tests/suites/acl/keywords_part2_test.py::test_connectin_from_an_unauthorized_network has strated to fail.

>       ip_ip = topo.standalone.ds_access_log.match('.* connection from ')[0].split()[-1]
E       IndexError: list index out of range

@spichugi all changes are done as per your suggestion, Can you please rerun the tests

dirsrvtests/tests/suites/acl/keywords_part2_test.py::test_connectin_from_an_unauthorized_network is still failing but with another error.

        # now user can access data
>       org.replace("seeAlso", "cn=1")
E         ldap.INSUFFICIENT_ACCESS: {'desc': 'Insufficient access', 'info': "Insufficient 'write' privilege to the 'seeAlso' attribute of entry 'ou=ip,ou=keywords,dc=example,dc=com'.\n"}

And about dirsrvtests/tests/suites/acl/keywords_part2_test.py::test_timeofday_keyword...
Why did you remove the failing part of the test case instead of fixing it?
I've mentioned a possible cause of the failure here - https://pagure.io/389-ds-base/pull-request/50180#comment-85197

@spichugi all changes are done as per your suggestion, Can you please rerun the tests

LGTM! Thanks!
Ack

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

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.