PR#49944: Ticket 49943 - rfc3673_all_oper_attrs_test is not strict enough - 389-ds-base

389-ds-base

#49944 Ticket 49943 - rfc3673_all_oper_attrs_test is not strict enough

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

mhonek/389-ds-base fix-test-rfc3673_all_oper_attrs_test into master

Ticket 49943 - rfc3673_all_oper_attrs_test is not strict enough

Matúš Honěk • 5 years ago

e2810e7

dirsrvtests/tests/suites/filter/rfc3673_all_oper_attrs_test.py

file modified

+9 -10

		`@@ -84,7 +84,10 @@`
		`under whole suffix`
		`"""`

		`- ACI_BODY = ensure_bytes('(targetattr= "objectClass \|\| cn \|\| sn \|\| mail \|\| uid \|\| uidNumber \|\| gidNumber \|\| homeDirectory \|\| creatorsName \|\| createTimestamp \|\| modifyTimestamp \|\| nsUniqueId \|\| parentid \|\| entryid \|\| entrydn \|\| ou \|\| numSubordinates")(version 3.0; acl "Allow read for user"; allow (read,search,compare) userdn = "ldap:///%s";)' % TEST_USER_DN)`
		`+ ACI_TARGET = '(targetattr= "modifiersName")'`
		`+ ACI_RULE = ('(version 3.0; acl "Deny modifiersName for user"; deny (read)'`
firstyear commented 5 years ago We shouldn't be using deny ACI's anyway, they should just be "lack of an allow" ... even in tests.
mhonek commented 5 years ago Well, for some reason, with the 'allow' rule before, although the 'modifiersName' was not mentioned in it it was present in the result (which it should be not) - I guess some implicit rules took place.
		`+ ' userdn = "ldap:///%s";)' % TEST_USER_DN)`
		`+ ACI_BODY = ensure_bytes(ACI_TARGET + ACI_RULE)`
		`topology_st.standalone.modify_s(DEFAULT_SUFFIX, [(ldap.MOD_ADD, 'aci', ACI_BODY)])`


		`@@ -142,24 +145,20 @@`
		`topology_st.standalone.simple_bind_s(DN_DM, ensure_bytes(PASSWORD))`

		`search_filter = ['+']`
		`+ expected_attrs = oper_attr_list`
		`if add_attr:`
		`search_filter.append(add_attr)`
		`- expected_attrs = sorted(oper_attr_list + ['objectClass'])`
		`- else:`
		`- expected_attrs = sorted(oper_attr_list)`
		`+ expected_attrs += ['objectClass']`

		`entries = topology_st.standalone.search_s(search_suffix, ldap.SCOPE_BASE,`
		`'(objectclass=*)',`
		`search_filter)`
		`- found_attrs = sorted(entries[0].data.keys())`
		`+ found_attrs = entries[0].data.keys()`

		`if add_attr == '*':`
		`- # Check that found attrs contain both operational`
		`- # and non-operational attributes`
		`- assert all(attr in found_attrs`
		`- for attr in ['objectClass', expected_attrs[0]])`
		`+ assert set(expected_attrs) - set(found_attrs) == set()`
		`else:`
		`- assert set(expected_attrs).issubset(set(found_attrs))`
		`+ assert set(expected_attrs) == set(found_attrs)`


		`if __name__ == '__main__':`

mhonek commented 5 years ago

Bug Description:
Test suites/filter/rfc3673_all_oper_attrs_test.py::test_search_basic
does not reach constraints extensively. The asserts are too
benevolent.

The commit 6ef4eb5 changed 'normal user' ACIs, however these changes
introduced new attr 'modifiersName' which was supposed to be missing
when searching.

In the first case, assert checks only for 'objectClass' and
pseudo-randomly one more attr to be present which is not sufficient.

In the second case, recently changed assert introduced weaker check
than the one present before.

Fix Description:
Bring back previous ACI to explicitly test the difference when binding
as normal user and the DM.

In case of add_attr == '*', test for all expected_attrs to be in
found_attrs. In the other case bring back the strict comparison as
there used to be before.

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

Author: mhonek

Review by: ???

rebased onto 57fe32b0fa0e5dca468237fec0065df4263fcbf7

5 years ago

firstyear commented on line 7 of dirsrvtests/tests/suites/filter/rfc3673_all_oper_attrs_test.py 5 years ago

We shouldn't be using deny ACI's anyway, they should just be "lack of an allow" ... even in tests.

firstyear commented 5 years ago

I'm not sure I understand the change here, would you mind explaining it a bit more? I don't think that it checks randomly, I thought it checked the set difference?

mhonek commented on line 7 of dirsrvtests/tests/suites/filter/rfc3673_all_oper_attrs_test.py 5 years ago

Well, for some reason, with the 'allow' rule before, although the 'modifiersName' was not mentioned in it it was present in the result (which it should be not) - I guess some implicit rules took place.

mhonek commented 5 years ago

I'm not sure I understand the change here, would you mind explaining it a bit more? I don't think that it checks randomly, I thought it checked the set difference?

So for the positive if branch, yes, it was checking for a set difference. However, only for objectClass and expected_attrs[0] to be present where expected_attrs[0] is only the first attribute name (of type str) of the list expected_attrs (hence "pseudo-randomly", not just "randomly" ;)). That behaviour is too weak to notice the incorrectly present modifiersName attribute.

For the negative if branch, two commits ago there was cmp(a,b)==0 which was (due to Python3 incompatibility) changed to a.issubset(b) which is (incorrectly in this context) weaker. So I changed it to the set(a)==set(b) (which I believe is in this context sufficient as we do not need to account for duplicates).

Please, correct my statements if they are in any sense incorrect.