PR#50512: Issue 50511 - lib389 PosixGroups type can not handle rdn properly - 389-ds-base

389-ds-base

#50512 Issue 50511 - lib389 PosixGroups type can not handle rdn properly

Merged 6 months ago by spichugi. Opened 6 months ago by aborah.

aborah/389-ds-base possix into master

Issue 50511 - lib389 PosixGroups type can not handle rdn properly

Anuj Borah • 6 months ago

9ea5b9b

src/lib389/lib389/idm/group.py

file modified

+7 -6

		`@@ -95,10 +95,10 @@`
		`]`
		`self._filterattrs = [RDN]`
		`self._childobject = Group`
		`- if rdn:`
		`- self._basedn = '{},{}'.format(ensure_str(rdn), ensure_str(basedn))`
		`- else:`
		`+ if rdn is None:`
		`self._basedn = ensure_str(basedn)`
		`+ else:`
		`+ self._basedn = '{},{}'.format(ensure_str(rdn), ensure_str(basedn))`


		`class UniqueGroup(DSLdapObject):`
		`@@ -137,10 +137,11 @@`
		`]`
		`self._filterattrs = [RDN]`
		`self._childobject = UniqueGroup`
		`- if rdn:`
		`- self._basedn = '{},{}'.format(ensure_str(rdn), ensure_str(basedn))`
		`- else:`
		`+ if rdn is None:`
		`self._basedn = ensure_str(basedn)`
		`+ else:`
		`+ self._basedn = '{},{}'.format(ensure_str(rdn), ensure_str(basedn))`
		`+`

src/lib389/lib389/idm/posixgroup.py

file modified

+5 -4

		`@@ -7,7 +7,7 @@`
		`# --- END COPYRIGHT BLOCK ---`

		`from lib389._mapped_object import DSLdapObject, DSLdapObjects`
		`- from lib389.utils import ds_is_older`
		`+ from lib389.utils import ds_is_older, ensure_str`

		`MUST_ATTRIBUTES = [`
		`'cn',`
		`@@ -76,6 +76,7 @@`
		`]`
		`self._filterattrs = [RDN]`
		`self._childobject = PosixGroup`
		`- self._basedn = '{},{}'.format(rdn, basedn)`
		`-`
		`-`
		`+ if rdn is None:`
		`+ self._basedn = ensure_str(basedn)`
		`+ else:`
		`+ self._basedn = '{},{}'.format(ensure_str(rdn), ensure_str(basedn))`

aborah commented 6 months ago

Description: lib389 PosixGroups type can not handle rdn properly

Fixes: https://pagure.io/389-ds-base/issue/50511

Author: aborah

Reviewed by: ???

spichugi commented on line 8 of src/lib389/lib389/idm/posixgroup.py 6 months ago

It is better to compare it with None. More safe

mhonek commented 6 months ago

It is better to compare it with None. More safe

The original Issue considers e.g. an empty string to be the same case as None. Therefore, the suggestion would be invalid. However, a question would be if the original request is itself valid; IDK.

aborah commented 6 months ago

@spichugi if we compare with None . it will make it

    if rdn is None:
        self._basedn = basedn
    else:
        self._basedn = '{},{}'.format(rdn, basedn)

(Pdb) PosixGroups(topo_m4.all_insts.get('master1'), SUBSUFFIX, rdn=None)._basedn
'dc=SubSuffix,dc=autoMembers,dc=com' ---- right
(Pdb) PosixGroups(topo_m4.all_insts.get('master1'), SUBSUFFIX, rdn='')._basedn
',dc=SubSuffix,dc=autoMembers,dc=com' ---- wrong

with the current version:

    if rdn:
        self._basedn = '{},{}'.format(rdn, basedn)
    else:
        self._basedn = basedn

so we have to go with current version

rebased onto 2c0ab1e780df9c43283c4923785475b894b994e1

6 months ago

aborah commented 6 months ago

@spichugi changes are done , as per pour suggestion .

spichugi commented 6 months ago

Please, fix other groups.py functionality too as stated in the issue.

rebased onto 0b0d360adc8db5507024d45de9492f26640dd5c7

6 months ago

aborah commented 6 months ago

@spichugi changes are done

spichugi commented 6 months ago

I think it is better to preserve ensure_str(rdn), ensure_str(basedn) part because it is safer.
Sorry if I created confusion by me first comment here.

rebased onto c64030836b87014146a4617ed0e97dd2bc397abb

6 months ago

aborah commented 6 months ago

@spichugi changes are done

spichugi commented 6 months ago

You haven't applied the same logic for PosixGroups... Was it intentional?

rebased onto 9ea5b9b

6 months ago

aborah commented 6 months ago