PR#50178: Issue:50112 - Port ACI test suit from TET to python3(misc and syntex) - 389-ds-base

389-ds-base

#50178 Issue:50112 - Port ACI test suit from TET to python3(misc and syntex)

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

aborah/389-ds-base misc_n_syntex into master

Issue:50112 - Port ACI test suit from TET to python3(misc and syntax)

Anuj Borah • 5 years ago

24f8b6d

dirsrvtests/tests/suites/acl/misc_test.py

file added

+413

		`@@ -0,0 +1,413 @@`
		`+ """`
		`+ # --- BEGIN COPYRIGHT BLOCK ---`
		`+ # Copyright (C) 2019 RED Hat, Inc.`
		`+ # All rights reserved.`
		`+ #`
		`+ # License: GPL (version 3 or any later version).`
		`+ # See LICENSE for details.`
		`+ # --- END COPYRIGHT BLOCK ----`
		`+ """`
		`+`
		`+ import os`
		`+ import pytest`
		`+`
		`+ from lib389._constants import DEFAULT_SUFFIX, PW_DM`
		`+ from lib389.idm.user import UserAccount, UserAccounts`
		`+ from lib389._mapped_object import DSLdapObject`
		`+ from lib389.idm.account import Accounts, Anonymous`
		`+ from lib389.idm.organizationalunit import OrganizationalUnit, OrganizationalUnits`
		`+ from lib389.idm.group import Group, Groups`
		`+ from lib389.topologies import topology_st as topo`
		`+ from lib389.idm.domain import Domain`
		`+ from lib389.plugins import ACLPlugin`
		`+`
		`+ import ldap`
		`+`
		`+`
		`+ PEOPLE = "ou=PEOPLE,{}".format(DEFAULT_SUFFIX)`
		`+ DYNGROUP = "cn=DYNGROUP,{}".format(PEOPLE)`
		`+ CONTAINER_1_DELADD = "ou=Product Development,{}".format(DEFAULT_SUFFIX)`
		`+ CONTAINER_2_DELADD = "ou=Accounting,{}".format(DEFAULT_SUFFIX)`
		`+`
		`+`
		`+ @pytest.fixture(scope="function")`
		`+ def aci_of_user(request, topo):`
		`+ """`
		`+ :param request:`
		`+ :param topo:`
		`+ """`
		`+ aci_list = Domain(topo.standalone, DEFAULT_SUFFIX).get_attr_vals('aci')`
		`+`
		`+ def finofaci():`
		`+ """`
		`+ Removes and Restores ACIs after the test.`
		`+ """`
		`+ domain = Domain(topo.standalone, DEFAULT_SUFFIX)`
		`+ domain.remove_all('aci')`
		`+ for i in aci_list:`
		`+ domain.add("aci", i)`
		`+`
		`+ request.addfinalizer(finofaci)`
		`+`
		`+`
		`+ @pytest.fixture(scope="function")`
		`+ def clean(request, topo):`
		`+ """`
		`+ :param request:`
		`+ :param topo:`
		`+ """`
		`+ ous = OrganizationalUnits(topo.standalone, DEFAULT_SUFFIX)`
		`+ try:`
		`+ for i in ['Product Development', 'Accounting']:`
		`+ ous.create(properties={'ou': i})`
		`+ except ldap.ALREADY_EXISTS as eoor_eoor:`
		`+ topo.standalone.log.info("Exception (expected): %s" % type(eoor_eoor).__name__)`
		`+`
		`+ def fin():`
		`+ """`
		`+ Deletes entries after the test.`
		`+ """`
		`+ for scope_scope in [CONTAINER_1_DELADD, CONTAINER_2_DELADD, PEOPLE]:`
		`+ try:`
		`+ DSLdapObject(topo.standalone, scope_scope).delete()`
		`+ except ldap.ALREADY_EXISTS as eoor_eoor:`
		`+ topo.standalone.log.info("Exception (expected): %s" % type(eoor_eoor).__name__)`
		`+`
		`+ request.addfinalizer(fin)`
		`+`
		`+`
		`+ def test_accept_aci_in_addition_to_acl(topo, clean, aci_of_user):`
		`+ """`
		`+ Misc Test 2 accept aci in addition to acl`
		`+ :id:8e9408fa-7db8-11e8-adaa-8c16451d917b`
		`+ :setup: Standalone Instance`
		`+ :steps:`
		`+ 1. Add test entry`
		`+ 2. Add ACI`
		`+ 3. User should follow ACI role`
		`+ :expectedresults:`
		`+ 1. Entry should be added`
		`+ 2. Operation should succeed`
		`+ 3. Operation should succeed`
		`+ """`
		`+ uas = UserAccounts(topo.standalone, DEFAULT_SUFFIX, rdn='ou=product development')`
		`+ user = uas.create_test_user()`
		`+ for i in [('mail', 'anujborah@okok.com'), ('givenname', 'Anuj'), ('userPassword', PW_DM)]:`
		`+ user.set(i[0], i[1])`
		`+`
		`+ aci_target = "(targetattr=givenname)"`
		`+ aci_allow = ('(version 3.0; acl "Name of the ACI"; deny (read, search, compare, write)')`
		`+ aci_subject = 'userdn="ldap:///anyone";)'`
		`+ Domain(topo.standalone, CONTAINER_1_DELADD).add("aci", aci_target + aci_allow + aci_subject)`
		`+`
		`+ conn = Anonymous(topo.standalone).bind()`
		`+ # aci will block targetattr=givenname to anyone`
		`+ user = UserAccount(conn, user.dn)`
		`+ with pytest.raises(AssertionError):`
		`+ assert user.get_attr_val_utf8('givenname') == 'Anuj'`
		`+ # aci will allow targetattr=uid to anyone`
		`+ assert user.get_attr_val_utf8('uid') == 'test_user_1000'`
		`+`
		`+ for i in uas.list():`
		`+ i.delete()`
		`+`
		`+`
		`+ @pytest.mark.bz334451`
		`+ def test_more_then_40_acl_will_crash_slapd(topo, clean, aci_of_user):`
		`+ """`
		`+ bug 334451 : more then 40 acl will crash slapd`
		`+ superseded by Bug 772778 - acl cache overflown problem with > 200 acis`
		`+ :id:93a44c60-7db8-11e8-9439-8c16451d917b`
		`+ :setup: Standalone Instance`
		`+ :steps:`
		`+ 1. Add test entry`
		`+ 2. Add ACI`
		`+ 3. User should follow ACI role`
		`+ :expectedresults:`
		`+ 1. Entry should be added`
		`+ 2. Operation should succeed`
		`+ 3. Operation should succeed`
		`+ """`
		`+ uas = UserAccounts(topo.standalone, DEFAULT_SUFFIX, rdn='ou=Accounting')`
		`+ user = uas.create_test_user()`
		`+`
		`+ aci_target = '(target ="ldap:///{}")(targetattr !="userPassword")'.format(CONTAINER_1_DELADD)`
		`+ # more_then_40_acl_will not crash_slapd`
		`+ for i in range(40):`
		`+ aci_allow = '(version 3.0;acl "ACI_{}";allow (read, search, compare)'.format(i)`
		`+ aci_subject = 'userdn="ldap:///anyone";)'`
		`+ aci_body = aci_target + aci_allow + aci_subject`
		`+ Domain(topo.standalone, CONTAINER_1_DELADD).add("aci", aci_body)`
		`+ conn = Anonymous(topo.standalone).bind()`
		`+ assert UserAccount(conn, user.dn).get_attr_val_utf8('uid') == 'test_user_1000'`
		`+`
		`+ for i in uas.list():`
		`+ i.delete()`
		`+`
		`+ @pytest.mark.bz345643`
		`+ def test_search_access_should_not_include_read_access(topo, clean, aci_of_user):`
		`+ """`
		`+ bug 345643`
		`+ Misc Test 4 search access should not include read access`
		`+ :id:98ab173e-7db8-11e8-a309-8c16451d917b`
		`+ :setup: Standalone Instance`
		`+ :steps:`
		`+ 1. Add test entry`
		`+ 2. Add ACI`
		`+ 3. User should follow ACI role`
		`+ :expectedresults:`
		`+ 1. Entry should be added`
		`+ 2. Operation should succeed`
		`+ 3. Operation should succeed`
		`+ """`
		`+ assert Domain(topo.standalone, DEFAULT_SUFFIX).present('aci')`
		`+ Domain(topo.standalone, DEFAULT_SUFFIX)\`
		`+ .add("aci", [f'(target ="ldap:///{DEFAULT_SUFFIX}")(targetattr !="userPassword")'`
		`+ '(version 3.0;acl "anonymous access";allow (search)'`
		`+ '(userdn = "ldap:///anyone");)',`
		`+ f'(target="ldap:///{DEFAULT_SUFFIX}") (targetattr = "*")(version 3.0; '`
		`+ 'acl "allow self write";allow(write) '`
		`+ 'userdn = "ldap:///self";)',`
		`+ f'(target="ldap:///{DEFAULT_SUFFIX}") (targetattr = "*")(version 3.0; '`
		`+ 'acl "Allow all admin group"; allow(all) groupdn = "ldap:///cn=Directory '`
		`+ 'Administrators, {}";)'])`
		`+`
		`+ conn = Anonymous(topo.standalone).bind()`
		`+ # search_access_should_not_include_read_access`
		`+ suffix = Domain(conn, DEFAULT_SUFFIX)`
		`+ with pytest.raises(AssertionError):`
		`+ assert suffix.present('aci')`
		`+`
		`+`
		`+ def test_only_allow_some_targetattr(topo, clean, aci_of_user):`
		`+ """`
		`+ Misc Test 5 only allow some targetattr (1/2)`
		`+ :id:9d27f048-7db8-11e8-a71c-8c16451d917b`
		`+ :setup: Standalone Instance`
		`+ :steps:`
		`+ 1. Add test entry`
		`+ 2. Add ACI`
		`+ 3. User should follow ACI role`
		`+ :expectedresults:`
		`+ 1. Entry should be added`
		`+ 2. Operation should succeed`
		`+ 3. Operation should succeed`
		`+ """`
		`+`
		`+ uas = UserAccounts(topo.standalone, DEFAULT_SUFFIX, rdn=None)`
		`+ for i in range(1, 3):`
		`+ user = uas.create_test_user(uid=i, gid=i)`
		`+ user.replace_many(('cn', 'Anuj1'), ('mail', 'annandaBorah@anuj.com'))`
		`+`
		`+ Domain(topo.standalone, DEFAULT_SUFFIX).\`
		`+ replace("aci", '(target="ldap:///{}")(targetattr="mail\|\|objectClass")'`
		`+ '(version 3.0; acl "Test";allow (read,search,compare) '`
		`+ '(userdn = "ldap:///anyone"); )'.format(DEFAULT_SUFFIX))`
		`+`
		`+ conn = Anonymous(topo.standalone).bind()`
		`+ accounts = Accounts(conn, DEFAULT_SUFFIX)`
		`+`
		`+ # aci will allow only mail targetattr`
		`+ assert len(accounts.filter('(mail=*)')) == 2`
		`+ # aci will allow only mail targetattr`
		`+ assert not accounts.filter('(cn=*)')`
		`+ # with root no , blockage`
		`+ assert len(Accounts(topo.standalone, DEFAULT_SUFFIX).filter('(uid=*)')) == 2`
		`+`
		`+ for i in uas.list():`
		`+ i.delete()`
		`+`
		`+`
		`+ def test_only_allow_some_targetattr_two(topo, clean, aci_of_user):`
		`+ """`
		`+ Misc Test 6 only allow some targetattr (2/2)"`
		`+ :id:a188239c-7db8-11e8-903e-8c16451d917b`
		`+ :setup: Standalone Instance`
		`+ :steps:`
		`+ 1. Add test entry`
		`+ 2. Add ACI`
		`+ 3. User should follow ACI role`
		`+ :expectedresults:`
		`+ 1. Entry should be added`
		`+ 2. Operation should succeed`
		`+ 3. Operation should succeed`
		`+ """`
		`+ uas = UserAccounts(topo.standalone, DEFAULT_SUFFIX, rdn=None)`
		`+ for i in range(5):`
		`+ user = uas.create_test_user(uid=i, gid=i)`
		`+ user.replace_many(('mail', 'anujborah@anujborah.com'),`
		`+ ('cn', 'Anuj'), ('userPassword', PW_DM))`
		`+`
		`+ user1 = uas.create_test_user()`
		`+ user1.replace_many(('mail', 'anujborah@anujborah.com'), ('userPassword', PW_DM))`
		`+`
		`+ Domain(topo.standalone, DEFAULT_SUFFIX).\`
		`+ replace("aci", '(target="ldap:///{}") (targetattr="mail\|\|objectClass")'`
		`+ '(targetfilter="cn=Anuj") (version 3.0; acl "$tet_thistest"; '`
		`+ 'allow (compare,read,search) '`
		`+ '(userdn = "ldap:///anyone"); )'.format(DEFAULT_SUFFIX))`
		`+`
		`+ conn = UserAccount(topo.standalone, user.dn).bind(PW_DM)`
		`+ # aci will allow only mail targetattr but only for cn=Anuj`
		`+ account = Accounts(conn, DEFAULT_SUFFIX)`
		`+ assert len(account.filter('(mail=*)')) == 5`
		`+ assert not account.filter('(cn=*)')`
		`+`
		`+ for i in account.filter('(mail=*)'):`
		`+ assert i.get_attr_val_utf8('mail') == 'anujborah@anujborah.com'`
		`+`
		`+`
		`+ conn = Anonymous(topo.standalone).bind()`
		`+ # aci will allow only mail targetattr but only for cn=Anuj`
		`+ account = Accounts(conn, DEFAULT_SUFFIX)`
		`+ assert len(account.filter('(mail=*)')) == 5`
		`+ assert not account.filter('(cn=*)')`
		`+`
		`+ for i in account.filter('(mail=*)'):`
		`+ assert i.get_attr_val_utf8('mail') == 'anujborah@anujborah.com'`
		`+`
		`+ # with root no blockage`
		`+ assert len(Accounts(topo.standalone, DEFAULT_SUFFIX).filter('(mail=*)')) == 6`
		`+`
		`+ for i in uas.list():`
		`+ i.delete()`
		`+`
		`+`
		`+`
		`+ @pytest.mark.bz326000`
		`+ def test_memberurl_needs_to_be_normalized(topo, clean, aci_of_user):`
		`+ """`
		`+ Non-regression test for BUG 326000: MemberURL needs to be normalized`
		`+ :id:a5d172e6-7db8-11e8-aca7-8c16451d917b`
		`+ :setup: Standalone Instance`
		`+ :steps:`
		`+ 1. Add test entry`
		`+ 2. Add ACI`
		`+ 3. User should follow ACI role`
		`+ :expectedresults:`
		`+ 1. Entry should be added`
		`+ 2. Operation should succeed`
		`+ 3. Operation should succeed`
		`+ """`
		`+ ou_ou = OrganizationalUnit(topo.standalone, "ou=PEOPLE,{}".format(DEFAULT_SUFFIX))`
		`+ ou_ou.set('aci', '(targetattr= *)'`
		`+ '(version 3.0; acl "tester"; allow(all) '`
		`+ 'groupdn = "ldap:///cn =DYNGROUP,ou=PEOPLE, {}";)'.format(DEFAULT_SUFFIX))`
		`+`
		`+ groups = Groups(topo.standalone, DEFAULT_SUFFIX, rdn='ou=PEOPLE')`
		`+ groups.create(properties={"cn": "DYNGROUP",`
		`+ "description": "DYNGROUP",`
		`+ 'objectClass': 'groupOfURLS',`
		`+ 'memberURL': "ldap:///ou=PEOPLE,{}??sub?"`
		`+ "(uid=test_user_2)".format(DEFAULT_SUFFIX)})`
		`+`
		`+ uas = UserAccounts(topo.standalone, DEFAULT_SUFFIX)`
		`+ for demo1 in [(1, "Entry to test rights on."), (2, "Member of DYNGROUP")]:`
		`+ user = uas.create_test_user(uid=demo1[0], gid=demo1[0])`
		`+ user.replace_many(('description', demo1[1]), ('userPassword', PW_DM))`
		`+`
		`+ ##with normal aci`
		`+ conn = UserAccount(topo.standalone, uas.list()[1].dn).bind(PW_DM)`
		`+ harry = UserAccount(conn, uas.list()[1].dn)`
		`+ harry.add('sn', 'FRED')`
		`+`
		`+ ##with abnomal aci`
		`+ dygrp = Group(topo.standalone, DYNGROUP)`
		`+ dygrp.remove('memberurl', "ldap:///ou=PEOPLE,{}??sub?(uid=test_user_2)".format(DEFAULT_SUFFIX))`
		`+ dygrp.add('memberurl', "ldap:///ou=PEOPLE,{}??sub?(uid=tesT_UsEr_2)".format(DEFAULT_SUFFIX))`
		`+ harry.add('sn', 'Not FRED')`
		`+`
		`+ for i in uas.list():`
		`+ i.delete()`
		`+`
		`+ @pytest.mark.bz624370`
		`+ def test_greater_than_200_acls_can_be_created(topo, clean, aci_of_user):`
		`+ """`
		`+ Misc 10, check that greater than 200 ACLs can be created. Bug 624370`
		`+ :id:ac020252-7db8-11e8-8652-8c16451d917b`
		`+ :setup: Standalone Instance`
		`+ :steps:`
		`+ 1. Add test entry`
		`+ 2. Add ACI`
		`+ 3. User should follow ACI role`
		`+ :expectedresults:`
		`+ 1. Entry should be added`
		`+ 2. Operation should succeed`
		`+ 3. Operation should succeed`
		`+ """`
		`+ # greater_than_200_acls_can_be_created`
		`+ uas = UserAccounts(topo.standalone, DEFAULT_SUFFIX)`
		`+ for i in range(200):`
		`+ user = uas.create_test_user(uid=i, gid=i)`
		`+ user.set('aci', '(targetattr = "description")'`
		`+ '(version 3.0;acl "foo{}"; allow (read, search, compare)'`
		`+ '(userdn="ldap:///anyone");)'.format(i))`
		`+`
		`+ assert user.\`
		`+ get_attr_val_utf8('aci') == '(targetattr = "description")' \`
		`+ '(version 3.0;acl "foo{}"; allow ' \`
		`+ '(read, search, compare)' \`
		`+ '(userdn="ldap:///anyone");)'.format(i)`
		`+ for i in uas.list():`
		`+ i.delete()`
		`+`
		`+`
		`+ @pytest.mark.bz624453`
		`+ def test_server_bahaves_properly_with_very_long_attribute_names(topo, clean, aci_of_user):`
		`+ """`
		`+ Make sure the server bahaves properly with very long attribute names. Bug 624453.`
		`+ :id:b0d31942-7db8-11e8-a833-8c16451d917b`
		`+ :setup: Standalone Instance`
		`+ :steps:`
		`+ 1. Add test entry`
		`+ 2. Add ACI`
		`+ 3. User should follow ACI role`
		`+ :expectedresults:`
		`+ 1. Entry should be added`
		`+ 2. Operation should succeed`
		`+ 3. Operation should succeed`
		`+ """`
		`+ users = UserAccounts(topo.standalone, DEFAULT_SUFFIX)`
		`+ users.create_test_user()`
		`+ users.list()[0].set('userpassword', PW_DM)`
		`+`
		`+ user = UserAccount(topo.standalone, 'uid=test_user_1000,ou=People,{}'.format(DEFAULT_SUFFIX))`
		`+ with pytest.raises(ldap.INVALID_SYNTAX):`
		`+ user.add("aci", "a" * 9000)`
		`+`
		`+`
		`+ def test_do_bind_as_201_distinct_users(topo, clean, aci_of_user):`
		`+ """`
		`+ Do bind as 201 distinct users`
		`+ Increase the nsslapd-aclpb-max-selected-acls in cn=ACL Plugin,cn=plugins,cn=config`
		`+ Restart the server`
		`+ Do bind as 201 distinct users`
		`+ :id:c0060532-7db8-11e8-a124-8c16451d917b`
		`+ :setup: Standalone Instance`
		`+ :steps:`
		`+ 1. Add test entry`
		`+ 2. Add ACI`
		`+ 3. User should follow ACI role`
		`+ :expectedresults:`
		`+ 1. Entry should be added`
		`+ 2. Operation should succeed`
		`+ 3. Operation should succeed`
		`+ """`
		`+ uas = UserAccounts(topo.standalone, DEFAULT_SUFFIX)`
		`+ for i in range(50):`
		`+ user = uas.create_test_user(uid=i, gid=i)`
		`+ user.set('userPassword', PW_DM)`
		`+`
		`+ for i in range(len(uas.list())):`
		`+ uas.list()[i].bind(PW_DM)`
		`+`
		`+ ACLPlugin(topo.standalone).replace("nsslapd-aclpb-max-selected-acls", '220')`
		`+ topo.standalone.restart()`
		`+`
		`+ for i in range(len(uas.list())):`
		`+ uas.list()[i].bind(PW_DM)`
		`+`
		`+`
		`+ if __name__ == "__main__":`
		`+ CURRENT_FILE = os.path.realpath(__file__)`
		`+ pytest.main("-s -v %s" % CURRENT_FILE)`

dirsrvtests/tests/suites/acl/syntax_test.py

file added

+258

		`@@ -0,0 +1,258 @@`
		`+ """`
		`+ # --- BEGIN COPYRIGHT BLOCK ---`
		`+ # Copyright (C) 2019 Red Hat, Inc.`
		`+ # All rights reserved.`
		`+ #`
		`+ # License: GPL (version 3 or any later version).`
		`+ # See LICENSE for details.`
		`+ # --- END COPYRIGHT BLOCK ----`
		`+ """`
		`+`
		`+ import os`
		`+ import pytest`
		`+`
		`+ from lib389._constants import DEFAULT_SUFFIX`
		`+ from lib389.idm.domain import Domain`
		`+ from lib389.topologies import topology_st as topo`
		`+`
		`+ import ldap`
		`+`
		`+ INVALID = [('test_targattrfilters_1',`
		`+ f'(targattrfilters ="add=title:title=fred),del=cn:(cn!=harry)")'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute (all)userdn="ldap:///anyone";)'),`
		`+ ('test_targattrfilters_2',`
		`+ f'(targattrfilters ="add=:(title=fred),del=cn:(cn!=harry)")'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute (all)userdn="ldap:///anyone";)'),`
		`+ ('test_targattrfilters_3',`
		`+ f'(targattrfilters ="add=:(title=fred),del=cn:(cn!=harry))'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute (all)userdn="ldap:///anyone";)'),`
		`+ ('test_targattrfilters_4',`
		`+ f'(targattrfilters ="add=title:(title=fred),=cn:(cn!=harry")'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute (all)userdn="ldap:///anyone";)'),`
		`+ ('test_targattrfilters_5',`
		`+ f'(targattrfilters ="add=title:(\|(title=fred)(cn=harry)),del=cn:(cn=harry)")'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute (all)userdn="ldap:///anyone";)'),`
		`+ ('test_targattrfilters_6',`
		`+ f'(targattrfilters ="add=title:(\|(title=fred)(title=harry)),del=cn:(title=harry)")'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute (all)userdn="ldap:///anyone";)'),`
		`+ ('test_targattrfilters_7',`
		`+ f'(targattrfilters ="add=title:(cn=architect), '`
		`+ f'del=title:(title=architect) && l:(l=cn=Meylan,dc=example,dc=com")")'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute (all)userdn="ldap:///anyone";)'),`
		`+ ('test_targattrfilters_8',`
		`+ f'(targattrfilters ="add=title:(cn=architect)")'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute (all)userdn="ldap:///anyone";)'),`
		`+ ('test_targattrfilters_9',`
		`+ f'(targattrfilters ="add=title:(cn=arch*)")'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute (all)userdn="ldap:///anyone";)'),`
		`+ ('test_targattrfilters_10',`
		`+ f'(targattrfilters ="add=title:(cn >= 1)")'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute (all)userdn="ldap:///anyone";)'),`
		`+ ('test_targattrfilters_11',`
		`+ f'(targattrfilters ="add=title:(cn <= 1)")'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute (all)userdn="ldap:///anyone";)'),`
		`+ ('test_targattrfilters_12',`
		`+ f'(targattrfilters ="add=title:(cn ~= 1)")'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute (all)userdn="ldap:///anyone";)'),`
		`+ ('test_targattrfilters_13',`
		`+ f'(targattrfilters ="add=title:(!(cn ~= 1))")'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute (all)userdn="ldap:///anyone";)'),`
		`+ ('test_targattrfilters_14',`
		`+ f'(targattrfilters ="add=title:(&(cn=fred)(cn ~= 1))")'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute (all)userdn="ldap:///anyone";)'),`
		`+ ('test_targattrfilters_15',`
		`+ f'(targattrfilters ="add=title:(\|(cn=fred)(cn ~= 1))")'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute (all)userdn="ldap:///anyone";)'),`
		`+ ('test_targattrfilters_16',`
		`+ f'(targattrfilters ="add=title:(&(\|(title=fred)(title=harry))(cn ~= 1))")'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute (all)userdn="ldap:///anyone";)'),`
		`+ ('test_targattrfilters_17',`
		`+ f'\(targattrfilters ="add=title:(&(\|(&(title=harry)(title=fred))'`
		`+ f'(title=harry))(title ~= 1))")'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute (all)userdn="ldap:///anyone";)'),`
		`+ ('test_targattrfilters_19',`
		`+ f'(target = ldap:///cn=Jeff Vedder,ou=Product Development,{DEFAULT_SUFFIX})'`
		`+ f'(targetattr=*)'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny(write)gropdn="ldap:///anyone";)'),`
		`+ ('test_targattrfilters_21',`
		`+ f'(target = ldap:///cn=Jeff Vedder,ou=Product Development,{DEFAULT_SUFFIX})'`
		`+ f'(targetattr=*)'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny(rite)userdn="ldap:///anyone";)'),`
		`+ ('test_targattrfilters_22',`
		`+ f'(targt = ldap:///cn=Jeff Vedder,ou=Product Development,{DEFAULT_SUFFIX})'`
		`+ f'(targetattr=*)'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute (all)userdn="ldap:///anyone";)'),`
		`+ ('test_targattrfilters_23',`
		`+ f'(target = ldap:///cn=Jeff Vedder,ou=Product Development,{DEFAULT_SUFFIX})'`
		`+ f'(targetattr=*)'`
		`+ f'(version 3.0; acl "Name of the ACI"; absolute (all)userdn="ldap:///anyone";)'),`
		`+ ('test_Missing_acl_mispel',`
		`+ f'(target = ldap:///cn=Jeff Vedder,ou=Product Development,{DEFAULT_SUFFIX})'`
		`+ f'(targetattr=*)'`
		`+ f'(version 3.0; alc "Name of the ACI"; deny absolute (all)userdn="ldap:///anyone";)'),`
		`+ ('test_Missing_acl_string',`
		`+ f'(target = ldap:///cn=Jeff Vedder,ou=Product Development,{DEFAULT_SUFFIX})'`
		`+ f'(targetattr=*)'`
		`+ f'(version 3.0; "Name of the ACI"; deny absolute (all)userdn="ldap:///anyone";)'),`
		`+ ('test_Wrong_version_string',`
		`+ f'(target = ldap:///cn=Jeff Vedder,ou=Product Development,{DEFAULT_SUFFIX})'`
		`+ f'(targetattr=*)'`
		`+ f'(version 2.0; acl "Name of the ACI"; deny absolute (all)userdn="ldap:///anyone";)'),`
		`+ ('test_Missing_version_string',`
		`+ f'(target = ldap:///cn=Jeff Vedder,ou=Product Development,{DEFAULT_SUFFIX})'`
		`+ f'(targetattr=*)'`
		`+ f'(; acl "Name of the ACI"; deny absolute (all)userdn="ldap:///anyone";)'),`
		`+ ('test_Authenticate_statement',`
		`+ f'(target = ldap:///cn=Jeff Vedder,ou=Product Development,{DEFAULT_SUFFIX})'`
		`+ f'(targetattr != "uid")'`
		`+ f'(targetattr=*)(version 3.0; acl "Name of the ACI"; deny absolute (all)'`
		`+ f'userdn="ldap:///anyone";)'),`
		`+ ('test_Multiple_targets',`
		`+ f'(target = ldap:///ou=Product Development,{DEFAULT_SUFFIX})'`
		`+ f'(target = ldap:///ou=Product Testing,{DEFAULT_SUFFIX})(targetattr=*)'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute (all)userdn="ldap:///anyone";)'),`
		`+ ('test_Target_set_to_self',`
		`+ f'(target = ldap:///self)(targetattr=*)'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute (all)userdn="ldap:///anyone";)'),`
		`+ ('test_target_set_with_ldap_instead_of_ldap',`
		`+ f'(target = ldap:\\\{DEFAULT_SUFFIX})(targetattr=*)'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute (all)userdn="ldap:///anyone";)'),`
		`+ ('test_target_set_with_more_than_three',`
		`+ f'(target = ldap:////{DEFAULT_SUFFIX})(targetattr=*)'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute (all)userdn="ldap:///anyone";)'),`
		`+ ('test_target_set_with_less_than_three',`
		`+ f'(target = ldap://{DEFAULT_SUFFIX})(targetattr=*)'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute (all)userdn="ldap:///anyone";)'),`
		`+ ('test_bind_rule_set_with_less_than_three',`
		`+ f'(target = ldap:///{DEFAULT_SUFFIX})(targetattr=*)'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute (all)userdn="ldap:/anyone";)'),`
		`+ ('test_Use_semicolon_instead_of_comma_in_permission',`
		`+ f'(target = ldap:///{DEFAULT_SUFFIX})(targetattr=*)'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny '`
		`+ f'(read; search; compare; write)userdn="ldap:///anyone";)'),`
		`+ ('test_Use_double_equal_instead_of_equal_in_the_target',`
		`+ f'(target == ldap:///{DEFAULT_SUFFIX})(targetattr=*)'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute (all)userdn="ldap:///anyone";)'),`
		`+ ('test_use_double_equal_instead_of_equal_in_user_and_group_access',`
		`+ f'(target = ldap:///{DEFAULT_SUFFIX})'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute (all)'`
		`+ f'userdn == "ldap:///anyone";)'),`
		`+ ('test_donot_cote_the_name_of_the_aci',`
		`+ f'(target = ldap:///{DEFAULT_SUFFIX})'`
		`+ f'(version 3.0; acl Name of the ACI ; deny absolute (all)userdn = "ldap:///anyone";)'),`
		`+ ('test_extra_parentheses_case_1',`
		`+ f'( )(target = ldap:///{DEFAULT_SUFFIX}) (targetattr=*)'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute (all)userdn = "ldap:///anyone";)'),`
		`+ ('test_extra_parentheses_case_2',`
		`+ f'(((((target = ldap:///{DEFAULT_SUFFIX})(targetattr=*)'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute (all)'`
		`+ f'userdn == "ldap:///anyone";)'),`
		`+ ('test_extra_parentheses_case_3',`
		`+ f'(((target = ldap:///{DEFAULT_SUFFIX}) (targetattr=*)'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute '`
		`+ f'(all)userdn = "ldap:///anyone";)))'),`
		`+ ('test_no_semicolon_at_the_end_of_the_aci',`
		`+ f'(target = ldap:///{DEFAULT_SUFFIX}) (targetattr=*)'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute (all)userdn = "ldap:///anyone")'),`
		`+ ('test_a_character_different_of_a_semicolon_at_the_end_of_the_aci',`
		`+ f'(target = ldap:///{DEFAULT_SUFFIX}) (targetattr=*)'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute (all)userdn = "ldap:///anyone"%)'),`
		`+ ('test_bad_filter',`
		`+ f'(target = ldap:///{DEFAULT_SUFFIX}) '`
		`+ f'(targetattr="cn \|&\| sn \|(\|) uid")'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute (all)userdn = "ldap:///anyone";)'),`
		`+ ('test_Use_double_equal_instead_of_equal_in_the_targattrfilters',`
		`+ f'(target = ldap:///{DEFAULT_SUFFIX})(targattrfilters== "add=title:(title=architect)")'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute (all)userdn="ldap:///anyone";)'),`
		`+ ('test_Use_double_equal_instead_of_equal_inside_the_targattrfilters',`
		`+ f'(target = ldap:///{DEFAULT_SUFFIX})(targattrfilters="add==title:(title==architect)")'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute (all)userdn="ldap:///anyone";)'),]`
		`+`
		`+`
		`+ FAILED = [('test_targattrfilters_18',`
		`+ f'(target = ldap:///cn=Jeff Vedder,ou=Product Development,{DEFAULT_SUFFIX})'`
		`+ f'(targetattr=*)'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny(write)userdn="ldap:///{"123" * 300}";)'),`
		`+ ('test_targattrfilters_20',`
		`+ f'(target = ldap:///cn=Jeff Vedder,ou=Product Development,{DEFAULT_SUFFIX})'`
		`+ f'(targetattr=*)'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny(write)userdns="ldap:///anyone";)'),`
		`+ ('test_bind_rule_set_with_more_than_three',`
		`+ f'(target = ldap:///{DEFAULT_SUFFIX})(targetattr=*)'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute (all)'`
		`+ f'userdn="ldap:////////anyone";)'),`
		`+ ('test_Use_double_equal_instead_of_equal_in_the_targetattr',`
		`+ f'(target = ldap:///{DEFAULT_SUFFIX})(targetattr==*)'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute (all)userdn="ldap:///anyone";)'),`
		`+ ('test_Use_double_equal_instead_of_equal_in_the_targetfilter',`
		`+ f'(target = ldap:///{DEFAULT_SUFFIX})(targetfilter==*)'`
		`+ f'(version 3.0; acl "Name of the ACI"; deny absolute '`
		`+ f'(all)userdn="ldap:///anyone";)'), ]`
		`+`
		`+`
		`+ @pytest.mark.xfail(reason='https://bugzilla.redhat.com/show_bug.cgi?id=1691473')`
		`+ @pytest.mark.parametrize("real_value", [a[1] for a in FAILED],`
		`+ ids=[a[0] for a in FAILED])`
		`+ def test_aci_invalid_syntax_fail(topo, real_value):`
		`+ """`
		`+`
		`+ Try to set wrong ACI syntax.`
		`+`
		`+ :id: d544d09a-6ed1-11e8-8872-8c16451d917b`
		`+ :setup: Standalone Instance`
		`+ :steps:`
		`+ 1. Create ACI`
		`+ 2. Try to setup the ACI with Instance`
		`+ :expectedresults:`
		`+ 1. It should pass`
		`+ 2. It should not pass`
		`+ """`
		`+ domain = Domain(topo.standalone, DEFAULT_SUFFIX)`
		`+ with pytest.raises(ldap.INVALID_SYNTAX):`
		`+ domain.add("aci", real_value)`
		`+`
		`+`
		`+ @pytest.mark.parametrize("real_value", [a[1] for a in INVALID],`
		`+ ids=[a[0] for a in INVALID])`
		`+ def test_aci_invalid_syntax(topo, real_value):`
		`+ """`
		`+`
		`+ Try to set wrong ACI syntax.`
		`+`
		`+ :id: d544d09a-6ed1-11e8-8872-8c16451d917b`
		`+ :setup: Standalone Instance`
		`+ :steps:`
		`+ 1. Create ACI`
		`+ 2. Try to setup the ACI with Instance`
		`+ :expectedresults:`
		`+ 1. It should pass`
		`+ 2. It should not pass`
		`+ """`
		`+ domain = Domain(topo.standalone, DEFAULT_SUFFIX)`
		`+ with pytest.raises(ldap.INVALID_SYNTAX):`
		`+ domain.add("aci", real_value)`
		`+`
		`+`
		`+ def test_target_set_above_the_entry_test(topo):`
		`+ """`
		`+ Try to set wrong ACI syntax.`
		`+`
		`+ :id: d544d09a-6ed1-11e8-8872-8c16451d917b`
		`+ :setup: Standalone Instance`
		`+ :steps:`
		`+ 1. Create ACI`
		`+ 2. Try to setup the ACI with Instance`
		`+ :expectedresults:`
		`+ 1. It should pass`
		`+ 2. It should not pass`
		`+ """`
		`+ domain = Domain(topo.standalone, "ou=People,{}".format(DEFAULT_SUFFIX))`
		`+ with pytest.raises(ldap.INVALID_SYNTAX):`
		`+ domain.add("aci", f'(target = ldap:///{DEFAULT_SUFFIX})'`
		`+ f'(targetattr=*)(version 3.0; acl "Name of the ACI"; deny absolute '`
		`+ f'(all)userdn="ldap:///anyone";)')`
		`+`
		`+`
		`+ if __name__ == "__main__":`
		`+ CURRENT_FILE = os.path.realpath(__file__)`
		`+ pytest.main("-s -v %s" % CURRENT_FILE)`

aborah commented 5 years ago

Port ACI test suit from TET to python3(misc and syntex)

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

Reviewed by: ???

rebased onto cc5e2412c55600c1cfc1125e105632a6a418212a

5 years ago

rebased onto fcba321ab72d96cc9cf3c90d6f2390f0f54b3cb2

5 years ago

rebased onto 201b45afbafd831ddbb4d950632453b2f6d6f7a1

5 years ago

rebased onto 2be1e20ceddc790c8f4589330b93104ab4380d1e

5 years ago

rebased onto fd4c4f4a1097431af5b2082164628d84107121e1

5 years ago

rebased onto af58836878e50c4fcf412764f606d93a48d8fe65

5 years ago

rebased onto 07bf7b9c3595d56b8a208b0d2715fef19bd5332f

5 years ago

rebased onto ccd5d9f62c889aee3d0963081abd66621936968b

5 years ago

rebased onto 817c696552e60bd1a33079be9cebadd785a9ccba

5 years ago

rebased onto eb8088184ce86332d195b7500550db251abd730a

5 years ago

rebased onto c8f34bb956b87792aab90922db0086b949444614

5 years ago

tbordaz commented on line 92 of dirsrvtests/tests/suites/acl/misc_test.py 5 years ago

please use DEFAULT_SUFFIX instead of hardcoded suffix value

tbordaz commented on line 130 of dirsrvtests/tests/suites/acl/misc_test.py 5 years ago

I do not understand that tests/description.
An aci denies anonymous read/write on 'givenname' attribute. My understanding is that the above line does a base search read of the entry and get the 'uid'.
This call does not select/test the aci.
I should rather expect a test where you create an entry with a 'givenname', search the entry and check that the returned entry does not contain a givenname.

tbordaz commented on line 169 of dirsrvtests/tests/suites/acl/misc_test.py 5 years ago

Does that check that it exist a value for 'uid' attribute in the entry ?

rebased onto 381e384d6ff7493253017527b03f612d2b069299

5 years ago

rebased onto 5f95a6f796a721e33ad298ac04445c8d19259d00

5 years ago

rebased onto 5b5bc03faab2c39cfd2550195310c2e7f5c164eb

5 years ago

firstyear commented on line 59 of dirsrvtests/tests/suites/acl/misc_test.py 5 years ago

What have I said about "use the right type for the right object?".

At the least, if you do something generic like this, use "DSLdapObject(topo.standalone, DN).delete()". Even though I hate it, it's at least "correct".

firstyear commented on line 80 of dirsrvtests/tests/suites/acl/misc_test.py 5 years ago

See comments on different ticket about "UserAccounts" plura form.

firstyear commented on line 389 of dirsrvtests/tests/suites/acl/misc_test.py 5 years ago

You can use python generators here. "a" * 9000.

firstyear commented on line 28 of dirsrvtests/tests/suites/acl/syntax_test.py 5 years ago

If you don't do some special ou behaviour, why make this and do crazy stuff? Just leave it as ou=People, andchange the aci's to suit?

firstyear commented on line 376 of dirsrvtests/tests/suites/acl/syntax_test.py 5 years ago

How is this test different to "test all together 1".

rebased onto 5474a734ea86b7919cb79261a8d9963b3a55d20a

5 years ago

rebased onto ca2d3bb9631af9f35b5f2aefc7f33a8a7e81a9ce

5 years ago

rebased onto 82a87a2642eeac955526609bae89131671b1a39b

5 years ago

rebased onto 1766259fd7de726d9a9608ac1167f9d5c13591e3

5 years ago

firstyear commented on line 43 of dirsrvtests/tests/suites/acl/misc_test.py 5 years ago

domain.remove_all('aci')

firstyear commented on line 54 of dirsrvtests/tests/suites/acl/misc_test.py 5 years ago

Did ... you read the comment about OrganisationalUnits (plural) as a constructor?

rebased onto 9a979550bc0d92fb511a3d1d63d3ad6f0a798437

5 years ago

rebased onto 64f6ec845cd3e265fc020319a792aa2764efdd4a

5 years ago

rebased onto 78a28f79f9036b2b8c7a026b4f50dcb25fa0eae3

5 years ago

rebased onto 1d0de867fcd80ea8b36bc64e4ab6d8a81e90b987

5 years ago

rebased onto 8e1c98e29c6cb8a4fbe0aecd03190ec057cb3b31

5 years ago

rebased onto dfa3c56b99170fe14fa385ec8a9fe63c4217d0ee

5 years ago

rebased onto 4f7bdbac9367a1bed1c3ceef86584aa054dc2342

5 years ago

firstyear commented on line 297 of dirsrvtests/tests/suites/acl/misc_test.py 5 years ago

I just scrolled to a random part, and found this. Code should be:

conn = UserAccount(topo.standalone, HARRY).bind(PW_DM)
harry = UserAccount(conn, HARRY)
harry.add('sn', 'FRED')
dyngroup = Group(topo.standalone, DYNGROUP)
dyngroup.remove(...)
dyngroup.add(...)
harry.add('sn', 'Not Fred')

Can you please self review and follow my advice on this? It's now nearly 3 months and you still are not using objects (classes) correctly, and you still are not using the typed groups properly.

rebased onto 3501c5fd2664c23d5b89cdea7e23642f8a7d25d8

5 years ago

rebased onto 4c1a163701d495e74770257675704297433273fb

5 years ago

rebased onto 338e8f171926302586a5213535b9b2c823740c34

5 years ago

rebased onto b73361fd30eed4b9029537303859ee54d44c2dfc

5 years ago

aborah commented 5 years ago

@firstyear self review done and all changes are done as per your suggestion , please check

rebased onto 4896974e43875a39a1ababc72ba6b442909be0c0

5 years ago

firstyear commented 5 years ago

Are you deleting your comments and reposting them to get me to review this .... ?

rebased onto 28782e0d56fa7aa2866c5f873437bf6dd0ca7e83

5 years ago

rebased onto 621e663c755e80c89c5ac7c9ae438814e58d3bed

5 years ago

aborah commented 5 years ago

@tbordaz all changes are done , Please check .

tbordaz commented 5 years ago

@aborah. THe patch looks good to me and you have my ACK. Please wait for @firstyear ACK as well.

Just an external comment. It is quite difficult to evaluate to validity of ACI tests.
If you intend to port others tests, I would prefer, whenever it is possible, that you do a PR per testcase.

aborah commented 5 years ago

@tbordaz Thanks for ack , from my next PR i will limit test cases to 5 per PR , i have got around 2000 Test cases which yet to push to pagure . Thanks

firstyear commented 5 years ago

@aborah I'm waiting to review this until you answer my question I asked a few days ago - are you deleting and re-posting comments on these threads?

aborah commented 5 years ago

@firstyear , few days back i was commenting "@firstyear all Changes are done. Please check"
But i was not getting any response from anybody . And i was waiting for this PR to be complete so that i can concentrate on other PRs .

While commenting for 5-6 days the same comment "@firstyear all Changes are done. Please check" , i see the same comment is appearing here continually which does not look good and have same meaning . So i have removed continuous 3 "@firstyear all Changes are done. Please check" comments.

firstyear commented 5 years ago

The thing to remember @aborah is that this is the upstream project - there are different interests and time pressures on people, besides your own. It's common for me to have patches that go un-reviewed for weeks at a time, and it's not malicious, it's just that the others in the team have concerns you may not know about or see. For example, this last week I have been exclusively dealing with internal issues at SUSE, and have not had the capacity to review such a large change. (Remember what I said about making changes smaller ...) . I know that Mark often has admin duties or packaging business at RH, I know Simon probably does a lot of assistance to qe, and Matus is spread over a few projects. Thierry has to do a lot with IPA too, and he often is busy too. We all are.

Continually poking daily is a bit rude - I have not forgotten about this. I'd say, remind once a week might be fine, and certainly, deleting comments is not okay here.

In the meantime, everyone else in the project, myself included, when we wait on reviews, we can go work on other things. Please be patient and kind with people.

I will review this, when I am ready, and when I have the capacity and time to give it the attention it deserves.

aborah commented 5 years ago

@firstyear , agreed . Will remember in future. Thanks

tbordaz commented 5 years ago

@aborah, in my mind QE is a place where a POC becomes a product. So you can not imagine how much I valuate your effort to bring back to life former tests in lib389.

Now all tests are not equivalent. an ACI test is sensitive from a security POV but also difficult to write and to check the validity of it. So it is normal that it takes time to review, and the more there are tests to review the more difficult it is because as @firstyear said, we continuously switch from a priority to an other.

We can try making smaller review to see if it helps. I would hope more frequent ack and progress, than currently a single long pending ack.

mhonek commented 5 years ago

I have to agree with William and Thierry here. Especially that with smaller patches you get quicker turnaround (roughly exponentially quicker!) -- for reviews and therefore also for your learning path.

Maybe it would make sense in dirsrvtests/tests/suites/acl/syntax_test.py not duplicating the ACI names three times each. Maybe try to rewrite to the following structure (using f-strings):

tests = [
    ('name-of-the-aci-case', f'(target = ldap:///{DEFAULT_SUFFIX}) (...'),
    ...
]

@pytest.mark.paramterize("realvalue", [a[1] for a in tests], ids=[a[0] for a in tests])
def test_all_together_part2(topo, realvalue):
    ...

It would make the content of the file more error-prone.

I think it would make sense to rename the test_all_together_part2 to something more meaningful, like test_aci_invalid_syntax.

Unless there will be more ACIs to test, with test_target_set_above_the_entry_test it does not make sense to parametrize the test, just put the value in the function.

The test_more_then_40_acl_will_crash_slapd failed for me once but I could not reproduce since. I guess no-op for now, just saying.

Please note I have not thoroughly investigated the content of the tests, so don't take this as an ack right away.

mhonek commented 5 years ago

Aaaand a nit pick: it's 'syntAx' not 'syntEx'; in the commit message. :)

rebased onto 48d433e487554e5ea476ca5e8cdb9ac9077e4768

5 years ago

aborah commented 5 years ago

I have to agree with William and Thierry here. Especially that with smaller patches you get quicker turnaround (roughly exponentially quicker!) -- for reviews and therefore also for your learning path.
Maybe it would make sense in dirsrvtests/tests/suites/acl/syntax_test.py not duplicating the ACI names three times each. Maybe try to rewrite to the following structure (using f-strings):
tests = [
('name-of-the-aci-case', f'(target = ldap:///{DEFAULT_SUFFIX}) (...'),
...
]

@pytest.mark.paramterize("realvalue", [a[1] for a in tests], ids=[a[0] for a in tests])
def test_all_together_part2(topo, realvalue):
...

It would make the content of the file more error-prone.
I think it would make sense to rename the test_all_together_part2 to something more meaningful, like test_aci_invalid_syntax.
Unless there will be more ACIs to test, with test_target_set_above_the_entry_test it does not make sense to parametrize the test, just put the value in the function.
The test_more_then_40_acl_will_crash_slapd failed for me once but I could not reproduce since. I guess no-op for now, just saying.
Please note I have not thoroughly investigated the content of the tests, so don't take this as an ack right away.

@pytest.mark.paramterize("realvalue", [a[1] for a in tests], ids=[a[0] for a in tests])

It took me 20 mints to find out whats wrong with my new script . Typo(paramterize)

All changes are done as per your suggestion including the commit part . Thanks

spichugi commented on line 52 of dirsrvtests/tests/suites/acl/misc_test.py 5 years ago

Please, catch exactly the exception that you are expecting (ldap.ALEARDY_EXISTS ?). Also, log something in that case.

And, please, apply the change to all similar cases in this script and future.

mhonek commented 5 years ago

Thanks for the updates. Just a couple of notes:
- IMHO the blank lines in the list are not necessary; they don't improve readablity
- You removed the test_target_set_above_the_entry_test test function and merged it into the other test case. However, notice what was the previous way of instantiating the Domain, it used a different suffix; I think that one particular test case should come back. In my previous comment the only thing I suggested to do to this function was to drop the decorator and plug the ACI filter right into the function body.

spichugi commented on line 100 of dirsrvtests/tests/suites/acl/misc_test.py 5 years ago

Instead of this line, you can use the instance in uas.create(properties={. Like user = uas.create(properties={

And, please, apply the change to all similar cases in this script and future.

spichugi commented on line 170 of dirsrvtests/tests/suites/acl/misc_test.py 5 years ago

You can use Domain here.

spichugi commented on line 70 of dirsrvtests/tests/suites/acl/misc_test.py 5 years ago

A standalone instance will be more descriptive, I think

spichugi commented on line 291 of dirsrvtests/tests/suites/acl/misc_test.py 5 years ago

ou=PEOPLE is the default RDN for UserAccounts

spichugi commented on line 333 of dirsrvtests/tests/suites/acl/misc_test.py 5 years ago

You use default properties everywhere. Why not to use create_test_user function?

spichugi commented on line 410 of dirsrvtests/tests/suites/acl/misc_test.py 5 years ago

We have ACLPlugin object for this.

spichugi commented 5 years ago

Please, run all your test cases with pytest-pylint as was advised before...

You can install it with:

pip3 install pytest-pylint

And then run with:

py.test --pylint -v dirsrvtests/tests/suites/acl/misc_test.py

You can ignore the errors regarding the fixtures like:

Redefining name 'topo' from outer scope (line 16) (redefined-outer-name)
Redefining name 'clean' from outer scope (line 46) (redefined-outer-name)
Redefining name 'aci_of_user' from outer scope (line 33) (redefined-outer-name)
Unused argument 'clean' (unused-argument)
Unused argument 'aci_of_user' (unused-argument)
Unused topology_st imported from lib389.topologies as topo (unused-import)

The rest... It will be nice to fix it. It will make the code more readable and PEP8 compliant.

rebased onto 602a00ccacb30821a288ce9f1e0fa2c71eb1c3ab

5 years ago

aborah commented 5 years ago

Thanks for the updates. Just a couple of notes:
- IMHO the blank lines in the list are not necessary; they don't improve readablity
- You removed the test_target_set_above_the_entry_test test function and merged it into the other test case. However, notice what was the previous way of instantiating the Domain, it used a different suffix; I think that one particular test case should come back. In my previous comment the only thing I suggested to do to this function was to drop the decorator and plug the ACI filter right into the function body.

I have made the changes as per your suggestion , now it look like i have found 6 bugs here .

Please check : http://pastebin.test.redhat.com/741730

lkrispen commented 5 years ago

ok, so the next steps are:
- verify that the acis failing the test really do violate the syntax
- create a ticket listing the acis where syntax violation is not checked. (not just a reference to this test)

I think aci syntax evaluation is a bit sloppy in many cases, eg it checks if the bind rules are using know kewywards likle "userdn", but also accepting "userdns" or "userdnxx" - this should be fixed, but we need a ticket providing te details.

aborah commented 5 years ago

ok, so the next steps are:
- verify that the acis failing the test really do violate the syntax
- create a ticket listing the acis where syntax violation is not checked. (not just a reference to this test)
I think aci syntax evaluation is a bit sloppy in many cases, eg it checks if the bind rules are using know kewywards likle "userdn", but also accepting "userdns" or "userdnxx" - this should be fixed, but we need a ticket providing te details.

https://bugzilla.redhat.com/show_bug.cgi?id=1691473 is raised for this issue .

lkrispen commented 5 years ago

yes, but you reported 4 acis in the bz and you said that 6 tests were failing.

and I asked you not just to dump the test to a bugzilla but analyze and determine why the synatx is wrong, but you just did that

aborah commented 5 years ago

yes, but you reported 4 acis in the bz and you said that 6 tests were failing.
and I asked you not just to dump the test to a bugzilla but analyze and determine why the synatx is wrong, but you just did that

Bellow are the reasons for these failure:

for first aci: userdn="ldap:///{"123" * 300}";)
for second aci: userdns="ldap:///anyone";)
for third aci: userdn="ldap:////////anyone";)
for fourth aci : targetattr==*

Also mentioned in the Bug

Edited 5 years ago by aborah

rebased onto 1ca49734312737de58bacec4791163dbbc927b6b

5 years ago

aborah commented 5 years ago

@spichugi , all changes are done as per your suggestion

mhonek commented 5 years ago

IIUC in 48d433e you removed four(six?) cases that were failing:
1. AFAICT they were not failing before the structural(!) change I requested? So where's the catch?
2. Do not remove the cases right out just because they fail, especially when you're just migrating them. Or, at least, make it clear you removed them at the rebase, with an explanation.

Also, in the same rebase you renamed some of the cases. Now, some are off by one, and some lost a meaningful name to a number. What's the reason?

aborah commented 5 years ago

IIUC in 48d433e you removed four(six?) cases that were failing:
1. AFAICT they were not failing before the structural(!) change I requested? So where's the catch?
2. Do not remove the cases right out just because they fail, especially when you're just migrating them. Or, at least, make it clear you removed them at the rebase, with an explanation.
Also, in the same rebase you renamed some of the cases. Now, some are off by one, and some lost a meaningful name to a number. What's the reason?

I have removed one case from test_aci_invalid_syntax as it is the same case that is present in test_target_set_above_the_entry_test

firstyear commented 5 years ago

It's okay to leave failing cases, but mark them as pytest.xfail. It's good to have these here to show "yes, we really do expect this to go wrong".

spichugi commented 5 years ago

@spichugi , all changes are done as per your suggestion

But it is not true...

I've asked to add some logging for the try-except:pass (instead of empty pass)...
You haven't fixed the issues from pytest-pylint
You haven't replaced uas.create(properties) with create_test_user functions (don't forget that you can add attributes after the creation, for example, you can add mail attribute)

rebased onto 4482c014dc7d87f91e048e1bfb6cdb2ae313183e

5 years ago

aborah commented 5 years ago

@spichugi , all changes are done as per your suggestion

But it is not true...

I've asked to add some logging for the try-except:pass (instead of empty pass)...
You haven't fixed the issues from pytest-pylint
You haven't replaced uas.create(properties) with create_test_user functions (don't forget that you can add attributes after the creation, for example, you can add mail attribute)

All the changes are done as per your suggestion , i have cleaned up as per as possible for pytest-pylint part

rebased onto 8694bd312e77758c8d6e4bd4b80d69823a847e24

5 years ago

aborah commented 5 years ago

It's okay to leave failing cases, but mark them as pytest.xfail. It's good to have these here to show "yes, we really do expect this to go wrong".

I have marked the failed test cases as xfail

rebased onto efcf96ed4f901941e4e0b80ebfa929f09c2aabc3

5 years ago

spichugi commented 5 years ago

@spichugi , all changes are done as per your suggestion
But it is not true...
I've asked to add some logging for the try-except:pass (instead of empty pass)...
You haven't fixed the issues from pytest-pylint
You haven't replaced uas.create(properties) with create_test_user functions (don't forget that you can add attributes after the creation, for example, you can add mail attribute)

All the changes are done as per your suggestion , i have cleaned up as per as possible for pytest-pylint part

You missed a couple of suggestions... Examples (there are more of them in the report):

Variable name "ou" doesn't conform to snake_case naming style (invalid-name)
standard import "import os" should be placed before "import pytest" (wrong-import-order)
third party import "from lib389._constants import DEFAULT_SUFFIX, PW_DM" should be placed before "import ldap" (wrong-import-order)
Duplicate string formatting argument 'DEFAULT_SUFFIX', consider passing as named argument (duplicate-string-formatting-argument)
Comparison should be len(accounts.filter('(mail=*)')) == 2 (misplaced-comparison-constant)

It will make the code more readable and PEP8 compliant, as I mentioned before

rebased onto 7c69a7741abb60e491549e773dec49c3e4073af5

5 years ago

aborah commented 5 years ago

@spichugi , all changes are done as per your suggestion
But it is not true...
I've asked to add some logging for the try-except:pass (instead of empty pass)...
You haven't fixed the issues from pytest-pylint
You haven't replaced uas.create(properties) with create_test_user functions (don't forget that you can add attributes after the creation, for example, you can add mail attribute)
All the changes are done as per your suggestion , i have cleaned up as per as possible for pytest-pylint part

You missed a couple of suggestions... Examples (there are more of them in the report):
Variable name "ou" doesn't conform to snake_case naming style (invalid-name)
standard import "import os" should be placed before "import pytest" (wrong-import-order)
third party import "from lib389._constants import DEFAULT_SUFFIX, PW_DM" should be placed before "import ldap" (wrong-import-order)
Duplicate string formatting argument 'DEFAULT_SUFFIX', consider passing as named argument (duplicate-string-formatting-argument)
Comparison should be len(accounts.filter('(mail=*)')) == 2 (misplaced-comparison-constant)

It will make the code more readable and PEP8 compliant, as I mentioned before

Now its all cleaned up as per the pylint .

spichugi commented 5 years ago

Now its all cleaned up as per the pylint .

Oh, and could you please make the same operation on another file you have dirsrvtests/tests/suites/acl/syntax_test.py? And please, use the technic in the future PRs (and on all of the PRs that are already on review)

Overwise, very nice job fixing it! :)

P.S. you have a typo in FaileValues. And realvalue should be real_value because it is more readable.

rebased onto 9f92203ef3edc61602b11a9e9ca62294cc9c3590

5 years ago

aborah commented 5 years ago

Now its all cleaned up as per the pylint .

Oh, and could you please make the same operation on another file you have dirsrvtests/tests/suites/acl/syntax_test.py? And please, use the technic in the future PRs (and on all of the PRs that are already on review)
Overwise, very nice job fixing it! :)
P.S. you have a typo in FaileValues. And realvalue should be real_value because it is more readable.

dirsrvtests/tests/suites/acl/syntax_test.py is also cleaned up as pylint . and typo corrected

rebased onto 0d645daeb70f95b26b120471349c3099b955340d

5 years ago

firstyear commented 5 years ago

Ack from me, @spichugi do you want to do a final check and merge?

spichugi commented 5 years ago

LGTM

rebased onto 074b5794d46ddd7eacf026f2df4f0003c7ee8b13

5 years ago

rebased onto 24f8b6d

5 years ago