On 02/12/2014 04:57 PM, Martin Kosek wrote:
On 02/10/2014 04:53 PM, Petr Viktorin wrote:
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.


Looks good to me:

# ipa permission-show testperm
   Permission name: testperm
   Permissions: write
   Effective attributes: cn, o, sn
   Included attributes: sn
   Excluded attributes: l
   Default attributes: l, o, cn
   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

ACK to all 5 patches (the last patch had 2-fuzz chunk).

Martin


Thank you! Pushed to master: 3db08227e8c760c688b8886e0b3b072e9b6dd94d

--
PetrĀ³

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to