[Freeipa-devel] [PATCHES] 0455-0459 Add support for managed permissions
Petr Viktorin
pviktori at redhat.com
Mon Feb 10 15:53:44 UTC 2014
On 01/31/2014 01:43 PM, Martin Kosek wrote:
> On 01/24/2014 04:48 PM, Petr Viktorin wrote:
>> On 01/23/2014 02:42 PM, Simo Sorce wrote:
>>> On Thu, 2014-01-23 at 13:23 +0100, Petr Viktorin wrote:
>>>> On 01/23/2014 12:24 PM, Martin Kosek wrote:
>>>>> On 01/22/2014 10:27 AM, Petr Viktorin wrote:
>>>>>> On 01/08/2014 04:49 PM, Petr Viktorin wrote:
>>>>>>> Hello,
>>>>>>> This adds "managed" permissions, the framework that will make our
>>>>>>> default permissions merge IPA updates and user changes sanely.
>>>>>>>
>>>>>>> There is no updater yet, nor does this add any actual managed
>>>>>>> permissions, so there's no user-visible change (beyond help text and a
>>>>>>> disabled option). To test the patch you might need to touch LDAP directly.
>>>>>>>
>>>>>>> Ticket: https://fedorahosted.org/freeipa/ticket/4033
>>>>>>> Design (no updater & plugin changes yet):
>>>>>>> http://www.freeipa.org/page/V3/Managed_Read_permissions
>>>>>>>
>>>>>>> 0447 - Minor fixes.
>>>>>>> 0448 - Since you can't create managed permissions through the API, I
>>>>>>> needed to get creative with the declarative tests. The tests will need a
>>>>>>> custom function that adds a managed perm.
>>>>>>> 0449 - The change itself.
>>>>>>
>>>>>> ping; any thoughts on this one?
>>>>>>
>>>>>>
>>>>>
>>>>> 1) 449, the comment:
>>>>>
>>>>> +Deleting or renaming a managed permission, as well as changing its target,
>>>>> +is not supported.
>>>>> +""") + _("""
>>>>>
>>>>> I am not sure that the phrase "not supported" is the right one. It sounds
>>>>> to me
>>>>> like this is something we want to allow, just not implemented yet. IMO "is not
>>>>> allowed" would be better.
>>>>
>>>> Makes sense.
>>>>
>>>>> 2) Can you add allow_mod_for_managed flag description to parameters.py?
>>>>>
>>>>> + flags={'no_create', 'allow_mod_for_managed'},
>>>>>
>>>>> So far we try to add all flag descriptions there.
>>>>
>>>> OK
>>>>
>>>>> 3) When I updated the test to not delete the testperm, I tried to show the
>>>>> managed permission and it is not entirely clear, see:
>>>>>
>>>>> # ipa permission-show testperm
>>>>> Permission name: testperm
>>>>> Permissions: write
>>>>> * Attributes: cn, o, sn
>>>>> * Excluded attributes: cn, sn
>>>>> Bind rule type: all
>>>>> Subtree: cn=users,cn=accounts,dc=example,dc=com
>>>>> ACI target DN: uid=*,cn=users,cn=accounts,dc=example,dc=com
>>>>> Type: user
>>>>> * Default attributes: l, o, cn
>>>>> * Effective attributes: l, o
>>>>
>>>> Well, this is a tradeoff between presenting what's stored in LDAP and
>>>> what's in the ACI.
>>>>
>>>>> The "Attributes" mean actually attributes explicitly allowed by user, but
>>>>> it is
>>>>> not obvious from the output.
>>>>>
>>>>> Maybe it would be better to return only "Effective attributes" by default and
>>>>> return the 3 source lists only when --all is passed. But this would require us
>>>>> to let Command override LDAPObject's default_attributes, which framework
>>>>> cannot do.
>>>>
>>>> Modifying default_attributes would not work because the 3 lists need to
>>>> be loaded from LDAP to determine the effective attributes.
>>>> It's possible to remove the extra attributes in the post_callback,
>>>> postprocess_result already does similar output manipulation.
>>>>
>>>>> Alternatively, we may choose to use the attributes differently with managed
>>>>> permissions:
>>>>> - we add the new attributeType "ipaPermIncludedAttr". It would be used for the
>>>>> user-specified whitelist of attributes instead of ipaPermAllowedAttr
>>>>> - we do not use the ipaPermAllowedAttr with managed attributes at all or
>>>>> use it
>>>>> for the "Effective attributes" list
>>>>>
>>>>> My point is that the semantics of ipaPermAllowedAttr is different for managed
>>>>> and non-managed permission, so it may confuse people.
>>>>
>>>> Well, the semantics are always the same (effective = (default | allowed)
>>>> - excluded). I agree that it can be confusing; perhaps I'm in too deep
>>>> to judge how it looks from the outside.
>>>>
>>>>> For example, you may want
>>>>> to search for all permissions that allow attribute "sn":
>>>>>
>>>>> # ipa permission-find --attrs sn
>>>>> ---------------------
>>>>> 4 permissions matched
>>>>> ---------------------
>>>>> Permission name: anon
>>>>> Permissions: read
>>>>> Attributes: sn
>>>>> Bind rule type: anonymous
>>>>> Subtree: cn=users,cn=accounts,dc=example,dc=com
>>>>> ACI target DN: uid=*,cn=users,cn=accounts,dc=example,dc=com
>>>>> Type: user
>>>>> ...
>>>>> Permission name: testperm
>>>>> Permissions: write
>>>>> Attributes: cn, o, sn
>>>>> Excluded attributes: cn, sn
>>>>> Bind rule type: anonymous
>>>>> Subtree: cn=users,cn=accounts,dc=example,dc=com
>>>>> ACI target DN: uid=*,cn=users,cn=accounts,dc=example,dc=com
>>>>> Type: user
>>>>> Default attributes: l, o, cn
>>>>> Effective attributes: l, o
>>>>> ...
>>>>>
>>>>> As you see, it matched both testperm and anon even though testperm does not
>>>>> really allow sn as it excluded.
>>>>>
>>>>> Thoughts?
>>>>
>>>> Well, we could have default, included, excluded attributes stored in
>>>> LDAP as now (using the name "included" instead of "allowed"), and make
>>>> effective attributes (--attrs) into an updatable virtual attribute: when
>>>> setting it, IPA would consult the "default" attributes and update
>>>> "included"/"excluded" accordingly. (With non-managed permissions
>>>> "default" is empty, so only "included" would be updated.) And searching
>>>> on --attrs would construct an appropriate filter.
>>>>
>>>> I thought about this approach earlier but thought that it obscured
>>>> what's actually stored in LDAP. Given recent discussions I'm now
>>>> thinking I shouldn't have rejected it.
>>>
>>>
>>> I would take a consistent approach indeed.
>>> I do not much care for the virtual attribute part in LDAP, as long as
>>> our tool show prominently the effective list.
>>> And also as long as all permissions behave the same way in the general
>>> mechanism in LDAP.
>>>
>>> Simo.
>>>
>>
>> All right. Here are patches; I'll start updating the design page.
>>
>> **NOTE**
>> This renames the 'ipaPermAllowedAttr' attribute to 'ipaPermIncludedAttr'.
>> Upgrades from master will fail, so please install a new server. Of course no
>> released versions of FreeIPA are affected.
>> I assume there's no clean way to rename an attribute without changing the OID?
>> Is it OK to break master this way?
>>
>> As before three source lists are stored in LDAP:
>> - ipaPermDefaultAttr
>> - ipaPermIncludedAttr (--includedattrs)
>> - ipaPermExcludedAttr (--excludedattrs)
>>
>> Setting --attrs ("Effective Attributes") will set the included/excluded
>> attributes based on the default set.
>> For normal permissions, default & excluded will be empty, and in this case only
>> effective attributes will be displayed on output (unless --all or --raw is given).
>>
>>
>>
>> I've added some preparatory patches for #4074 to this batch, mostly to prevent
>> rebase conflicts with that work. Re-numbering for a sane order.
>>
>> Apply on top of my patch 0452
>>
>> 0455 - minor fixes, unchanged from 0447
>>
>> 0456 - converting options on the server it will allow us to consult the entry's
>> existing state. Arguably it's also cleaner to use execute than
>> args_options_2_params for this.
>>
>> 0457 - generate ACIs in the plugin. This is the next step in obsoleting the ACI
>> class, which in the end should only be necessary for updating old-style ACIs.
>> The one-way function should be easier to maintain and extend.
>>
>> 0458 - allow callables in declarative tests, unchanged from 0448
>>
>> 0459 - managed permissions
>>
>>
>> Or: git pull https://github.com/encukou/freeipa 3566-managed-perms
>> commit 51bb7f27516202a062ffa25fedae18d0e9f302b6
>>
>
> 1) (cosmetic) Wouldn't it better to move ipapermdefaultattr to takes_params
> with ['no_create', 'no_update'] flags to:
>
> a) Have better ordering of output params. Now it looks like:
>
> # ipa permission-show testperm
> Permission name: testperm
> Permissions: write
> Effective attributes: cn, l, o, sn
> Included attributes: sn
> Bind rule type: all
> Subtree: cn=users,cn=accounts,dc=idm,dc=lab,dc=eng,dc=brq,dc=redhat,dc=com
> ACI target DN:
> uid=*,cn=users,cn=accounts,dc=idm,dc=lab,dc=eng,dc=brq,dc=redhat,dc=com
> Type: user
> Default attributes: l, o, cn
>
>
> I think it would be better if all the "* attributes" parameters are together.:
>
> Effective attributes: bla, bla
> Included attributes: bla, bla
> Excluded attributes: bla, bla
> Default attributes: bla, bla
>
> so that it is easier to read the output.
>
> b) If ipapermdefaultattr is in takes_params, one would be able to do
> permission-search for default attributes. Even if we don't allow user to change
> them, we should allow him to search them.
>
> 2) (also cosmetic) Given we have ipapermincludedattr described as
> doc=_('User-specified attributes to which the permission applies'),
> shouldn't we also describe ipapermexcludedattr as
> doc=_('User-specified attributes to which the permission explicitly '
> 'does not apply'),
> ? I think it would be more consistent.
>
>
> Besides these 2 cosmetic issues, I think the new patchset works pretty nice -
> good job!
Thanks for the review, fixed.
I've also fixed the search filter for --attrs, and added more tests for
that.
--
Petr³
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0455.2-Permission-plugin-fixes.patch
Type: text/x-patch
Size: 3959 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140210/3053566d/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0456.2-permission-plugin-Convert-options-in-execute-not-arg.patch
Type: text/x-patch
Size: 3198 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140210/3053566d/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0457.2-permission-plugin-Generate-ACIs-in-the-plugin.patch
Type: text/x-patch
Size: 3125 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140210/3053566d/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0458.2-Make-it-possible-to-call-custom-functions-in-Declara.patch
Type: text/x-patch
Size: 1798 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140210/3053566d/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0459.2-Add-support-for-managed-permissions.patch
Type: text/x-patch
Size: 78150 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140210/3053566d/attachment-0004.bin>
More information about the Freeipa-devel
mailing list