[Freeipa-devel] [PATCHES] 0447-0449 Add support for managed permissions

[Simo Sorce]

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.

-- 
Simo Sorce * Red Hat, Inc * New York