[Date Prev][Date Next] [Thread Prev][Thread Next]
Re: [Freeipa-devel] [PATCHES] 0322-0327 New permissions system
- From: Martin Kosek <mkosek redhat com>
- To: Petr Viktorin <pviktori redhat com>, freeipa-devel <freeipa-devel redhat com>
- Subject: Re: [Freeipa-devel] [PATCHES] 0322-0327 New permissions system
- Date: Mon, 02 Dec 2013 13:04:16 +0100
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
> 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
> 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.
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]
[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
[Mon Dec 02 10:49:11.972430 2013] [:error] [pid 14181]
[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
[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.
2) In filter check:
+ # Rough filter validation by a search
+ if self.env.in_server and 'ipapermtargetfilter' in options:
+ ldap = self.Backend.ldap2
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.
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
Bind rule type: all
ACI target DN: cn=*,cn=groups,cn=accounts,dc=example,dc=com
# ipa privilege-add-permission foo --permissions "Write Group Description 2"
Privilege name: foo
Permissions: write group description 2
Number of permissions added 1
Is this a bug or do you already plan to fix it later?
4) Typo in refactored permission plugin:
+ errors.NotFound('ACI of to permission %s was not found' %
[Date Prev][Date Next] [Thread Prev][Thread Next]