[Freeipa-devel] [PATCHES] 0322-0327 New permissions system

Fri Dec 6 10:49:54 UTC 2013

On 12/02/2013 01:04 PM, Martin Kosek wrote:
> On 12/01/2013 11:46 PM, Petr Viktorin wrote:
>> This seems to work now. Please tell me what I missed.
>>
>> Design: http://www.freeipa.org/page/V3/Permissions_V2
>> Ticket: https://fedorahosted.org/freeipa/ticket/4034
>>
>>
>> 0322 Allow sets for initialization of frozenset-typed Param keywords
>> because my OCD compels me to use sets instead of lists when the order does not
>> matter.
>>
>>
>> 0323 Allow Declarative test classes to specify the API version
>> For the next patch, I want to test how the rewrite handles old clients. To make
>> that easy I made the default API version a testclass attribute
>>
>>
>> 0324 Add tests for permission plugin with older clients
>> These tests will not pass yet, but comparing this file with the old
>> test_permission_plugin.py will can serve as a nice summary of API changes. A
>> summary of the summary:
>> - Lots of new attributes will be added for output
>> - The `type` and `subtree` options now interact in a different way: setting one
>> affects the other. Same with `type`/`filter` and `memberof`/`targetfilter`.
>> (Some change here was necessary for https://fedorahosted.org/freeipa/ticket/2355)
>> - Validation will be stricter (and/or done in different order)
>> - Some error messages will change (hopefully for the better)
>> - `subtree` must now point to an existing entry
>> - Permission names may now contain '.' (this is to allow names of DNS
>> permissions that were previously internal)
>>
>> P.S. a handy command for listing the changes (once this patch is applied):
>> git diff ipa-3-3:ipatests/test_xmlrpc/test_permission_plugin.py
>> ipatests/test_xmlrpc/test_old_permission_plugin.py
>>
>>
>> 0325 Add new permission schema
>> Introducing the new OIDs
>>
>>
>> 0326 Rewrite the Permission plugin
>> See the design for what this does.
>>
>> The new permission plugin does not use aci plugin at all. The plan is to retire
>> the aci plugin when the time comes to also refactor delegation & selfservice.
>> It does use ipalib's ACI class, mainly for parsing (needed for
>> upgrading/showing old ACIs).
>>
>> The permission-find command is a bit faster than the old one, but still
>> painfully slow (5s instead of 7s on my box). The good news is that it now
>> scales with the number of *old* permissions, so as you upgrade it'll get faster.
>>
>> Tests are updated, including privilege and DNS tests that worked with permissions.
>>
>>
>> 0327 Verify ACIs are added correctly in tests
>> Right after saying I want to get rid of it, I found a new use for the aci
>> plugin: an tested code path for getting at ACIs (Declaratrive tests can only
>> use the API, they don't play well with LDAP connections).
>> Now we can be sure the ACIs are actually changed when we play with permissions.
>>
>
> Great job!
>
> I read through the code and did few tests, this is what I've found so far:
>
> 1) When I tried to move ACI to non-existent DN, I got a general error + the
> underlying ACI was gone:
>
> # ipa permission-mod "Write Group Description" --subtree=foo=bar
> ipa: ERROR: no such entry
>
>
> When I then tried to delete it, I got Internal Error:
> # ipa permission-del "Write Group Description"
> ipa: ERROR: an internal error has occurred
>
> ...
> /python2.7/site-packages/ipalib/plugins/permission.py", line 681, in pre_callback
> [Mon Dec 02 10:49:11.972422 2013] [:error] [pid 14181]
> self.obj.remove_aci(entry)
> [Mon Dec 02 10:49:11.972426 2013] [:error] [pid 14181]   File
> "/usr/lib/python2.7/site-packages/ipalib/plugins/permission.py", line 374, in
> remove_aci
> [Mon Dec 02 10:49:11.972430 2013] [:error] [pid 14181]
> self._replace_aci(permission_entry)
> [Mon Dec 02 10:49:11.972434 2013] [:error] [pid 14181]   File
> "/usr/lib/python2.7/site-packages/ipalib/plugins/permission.py", line 392, in
> _replace_aci
> [Mon Dec 02 10:49:11.972438 2013] [:error] [pid 14181]     acidn = acientry.dn
>   # pylint: disable=E1103
> [Mon Dec 02 10:49:11.972441 2013] [:error] [pid 14181] AttributeError: 'dict'
> object has no attribute 'dn'
>
> I think we should add a check for that + at least try to rollback if we fail to
> move the ACI.

Fixed. I did in in 2 additional patches for clarity:
- 0331 adds rollback
- 0332 adds the check (you can test the rollback without this applied)

> 2) In filter check:
>
> +        # Rough filter validation by a search
> +        if self.env.in_server and 'ipapermtargetfilter' in options:
> +            ldap = self.Backend.ldap2
> +            try:
> +                ldap.find_entries(
> +                    filter=options['ipapermtargetfilter'],
> +                    base_dn=self.env.basedn,
> +                    size_limit=0)
>
> This is great for starters, but I would make it less costly and only search
> with BASE scope and sizelimit==1 to avoid costly, possibly unindexed searches
> across whole database.

Agreed, fixed.

> 3) I see that I can add ALL bind type permission as a member to a privilege:
>
> # ipa permission-show "Write Group Description 2"
>    Permission name: Write Group Description 2
>    Permissions: write
>    Attributes: description
>    Bind rule type: all
>    Subtree: cn=groups,cn=accounts,dc=example,dc=com
>    ACI target DN: cn=*,cn=groups,cn=accounts,dc=example,dc=com
>    Type: group
>
> # ipa privilege-add-permission foo --permissions "Write Group Description 2"
>    Privilege name: foo
>    Description: foo
>    Permissions: write group description 2
> -----------------------------
> Number of permissions added 1
> -----------------------------
>
> Is this a bug or do you already plan to fix it later?

Yes, I plan to fix that soon (#4032).
I've modified the patch to allow "permission" only. I'll re-introduce 
the others when I add the necessary checks.

> 4) Typo in refactored permission plugin:
>
> +            errors.NotFound('ACI of to permission %s was not found' %
> +                            keys[0])

Fixed, thanks for the catch!

-- 
Petr³

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0322.2-Allow-sets-for-initialization-of-frozenset-typed-Par.patch
Type: text/x-patch
Size: 1154 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131206/4a84dda6/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0323.2-Allow-Declarative-test-classes-to-specify-the-API-ve.patch
Type: text/x-patch
Size: 1268 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131206/4a84dda6/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0324.2-Add-tests-for-permission-plugin-with-older-clients.patch
Type: text/x-patch
Size: 44634 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131206/4a84dda6/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0325.2-Add-new-permission-schema.patch
Type: text/x-patch
Size: 4364 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131206/4a84dda6/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0326.2-Rewrite-the-Permission-plugin.patch
Type: text/x-patch
Size: 135946 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131206/4a84dda6/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0327.2-Verify-ACIs-are-added-correctly-in-tests.patch
Type: text/x-patch
Size: 20000 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131206/4a84dda6/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0331.2-Roll-back-ACI-changes-on-failed-permission-updates.patch
Type: text/x-patch
Size: 10122 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131206/4a84dda6/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0332.2-permission-plugin-Ensure-ipapermlocation-subtree-alw.patch
Type: text/x-patch
Size: 3912 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131206/4a84dda6/attachment-0007.bin>