[Freeipa-devel] [PATCHES] 0464-0466 Multivalued targetfilter

[Martin Kosek]

On 02/19/2014 10:44 AM, Petr Viktorin wrote:
> On 02/18/2014 08:02 PM, Petr Viktorin wrote:
>> On 02/18/2014 09:42 AM, Martin Kosek wrote:
>>> On 02/13/2014 01:12 PM, Petr Viktorin wrote:
>>>> Hello,
>>>> These patches fix https://fedorahosted.org/freeipa/ticket/4074
>>>> Design:
>>>> http://www.freeipa.org/page/V3/Multivalued_target_filters_in_permissions
>>>>
>>>>
>>>> Since --type, affects only targetfilter values in the form
>>>> "(objectclass=...)"
>>>> and leaves others alone, and in the -mod command we don't fetch the
>>>> existing
>>>> entry until the pre_callback, I had to put the adds/removes in the
>>>> context. I
>>>> don't think the approach is too terrible given the limitations.
>>>
>>> 464:
>>>
>>> 1) Internal Error:
>>>
>>> # ipa permission-mod can_write2 --filter='foo=bar'
>>> ipa: ERROR: an internal error has occurred
>>
>> Thanks for the catch! Fixed & added to tests.
>>
>>> 2) ACI target filter
>>>
>>> I would relabel targetfilter from "ACI target filter" to "Target
>>> filter" to
>>> make it consistent with other ACI attributes. We are sort of hiding
>>> there are
>>> any underlying ACIs anyway...
>>
>> Fixed.
>>
>>> 3) I am now thinking about the behavior of --memberof and --filter
>>> options and
>>> how they interact. It looks OK except for the case when I set filter
>>> to None:
>>
>> The same would happen when setting --filter to any other value(s) that
>> don't include existing memberOf filters.
>>
>>> # ipa permission-mod can_write2 --memberof=bar
>>> --------------------------------
>>> Modified permission "can_write2"
>>> --------------------------------
>>>    Permission name: can_write2
>>>    Permissions: write
>>>    Effective attributes: description
>>>    Bind rule type: permission
>>>    Subtree: dc=example,dc=com
>>>    ACI target filter: (foo=bar),
>>>
>>> (memberOf=cn=bar,cn=groups,cn=accounts,dc=example,dc=com)
>>>    Member of group: bar
>>> # ipa permission-mod can_write2 --filter=
>>> --------------------------------
>>> Modified permission "can_write2"
>>> --------------------------------
>>>    Permission name: can_write2
>>>    Permissions: write
>>>    Effective attributes: description
>>>    Bind rule type: permission
>>>    Subtree: dc=example,dc=com
>>>
>>> Then both memberOf and filter is erased. Are we ok with that?
>>> Shouldn't we keep
>>> memberOf part until that is set to None? I am not insisting, just
>>> trying to
>>> discuss the best behavior.
>>
>> Memberof affects the filter; this is even pointed out in --help output.
>> The alternative would be to make --filter= exclude filters affected by
>> other options, which I think would be even more confusing.
>> Keep in mind --type sets (objectclass=...) filters in exactly the same
>> way as --memberof works for (memberof=...).
>> The --memberof, --targetgroup, --type options are just shortcuts for
>> setting other permission attributes. I'm hoping we can get this message
>> across, in --help, and in the docs, well enough to reduce the confusion.
>>
>>> 465: I know this was already discussed previously, but I am now having
>>> second
>>> thoughts - should we use posixAccount as THE objectclass for user
>>> targetfilter?
>>>
>>> # ipa permission-add can_write --permissions write --attrs=description
>>> --type=user
>>> ----------------------------
>>> Added permission "can_write"
>>> ----------------------------
>>>    Permission name: can_write
>>>    Permissions: write
>>>    Effective attributes: description
>>>    Bind rule type: permission
>>>    Subtree: cn=users,cn=accounts,dc=example,dc=com
>>>    ACI target filter: (objectclass=posixaccount)
>>>    Type: user
>>>
>>> What if we add system users at some point which would miss the
>>> posixaccount
>>> objectclass? Wouldn't it be better to use the inetOrgPerson structural
>>> objectclass instead of posixAccount? Simo or Ludwig, any opinion on this?
>>
>> I'm not opposed.
>> (I don't think it should block this patchset though. We'll have to add
>> canonical objectclasses to all the types that don't currently have them.
>> Deciding it for `user` can be a part of that effort.)
> 
> Apologies, I've sent a slightly wrong version of the tests. Here's the fixed
> patchset.
> 

Thanks. New check works fine, but the attribute name in the error message seems
inconsistent:

# ipa permission-mod can_write --filter=foo=bar
ipa: ERROR: invalid 'filter': must be enclosed in parentheses

# ipa permission-mod can_write --filter="(foo=bar))"
ipa: ERROR: invalid 'ipapermtargetfilter': Bad search filter

If you fix that, it's ACK for all.

Martin