[Freeipa-devel] [PATCHES] 0322-0327 New permissions system
Petr Viktorin
pviktori at redhat.com
Thu Dec 12 16:17:24 UTC 2013
On 12/12/2013 02:00 PM, Martin Kosek wrote:
> On 12/06/2013 11:49 AM, Petr Viktorin wrote:
>> 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!
>>
>
>
> 1) 0352.2: I think that ipaPermTargetFilter and ipaPermTarget should be
> SINGLE-VALUE'd as well
Thanks, changed.
> 2) More about the schema, I think we should move the attributes that permission
> plugin always depends on to MUST list, I think this should affect:
> * ipapermright
> * ipapermbindruletype
OK. This means that SYSTEM permissions stay without the new objectclass,
which means removing most options from permission_add_noaci.
> I was pondering about ipapermallowedattr, but ACI works without it well, it is
> just NOOP. Other attributes are just different combinations of targetting the
> entries to protect, so they may stay MAY.
Optional ipapermallowedattr will be required for managed permissions.
Also, add/delete permissions could be specified without an attr filter.
> 3)326.2: shouldn't
>
> + StrEnum(
> + 'ipapermright*',
> + cli_name='permissions',
>
> rather read 'ipapermright+'? This is what I get if I omit the permissions:
>
> # ipa permission-add foo --attrs=description --type group
> ipa: ERROR: an internal error has occurred
>
> Same with update and other attributes:
> # ipa permission-mod foo3 --permissions ''
> ipa: ERROR: an internal error has occurred
>
> # ipa permission-mod foo3 --memberof=''
> ipa: ERROR: an internal error has occurred
>
> The later one is only reproducible if there is not memberof part set.
Unfortunately it can't be required because it can be specified by a
different name via the old API. But, it is now checked.
> 4) Internall error when entering blank subtree
> # ipa permission-add foo3 --permissions write --subtree ''
> ipa: ERROR: an internal error has occurred
>
> 5) Internal error when entering non-blank subtree and nothing else:
>
> # ipa permission-add foo3 --permissions write --subtree 'cn=otp,dc=example,dc=com'
> ipa: ERROR: an internal error has occurred
>
> [Thu Dec 12 13:18:04.635874 2013] [:error] [pid 17259] raise SyntaxError,
> "target must be a non-empty dictionary"
>
> It seems this case should translate into error "there should be at least one
> target entry specifier".
>
> 6) permission-find gives 0 results when searching in the default DN and it was
> not explicitly set:
>
> # ipa permission-find --subtree dc=example,dc=com
> ---------------------
> 0 permissions matched
> ---------------------
> ----------------------------
> Number of entries returned 0
> ----------------------------
4-6: Thanks for the catches, fixed & added to tests.
> 7) Web UI list of permissions returns weird results (just 2 of my new testing
> permissions). This is what it calls:
>
> [Thu Dec 12 13:41:01.473157 2013] [:error] [pid 17258] ipa: INFO:
> [jsonserver_session] admin at IDM.LAB.ENG.BRQ.REDHAT.COM: permission_find(u'',
> sizelimit=0, pkey_only=True): SUCCESS
>
> But as I see, Web UI is broken in many other aspects, so it needs to be adapted
> to the new output. As I do not want to stop development of the server framework
> part (there is a lot to do) it can be done in other patches after yours are
> merged, so that we have at least the server part in. We just need to fix it
> before 3.4 release.
Right. Here's a ticket for it: https://fedorahosted.org/freeipa/ticket/4079
> This is all I found in the second round of review, these are mostly corner
> cases, the core of the patch set seems to be nice.
>
> Martin
>
--
Petr³
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0322.3-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/20131212/3a2770be/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0323.3-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/20131212/3a2770be/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0324.3-Add-tests-for-permission-plugin-with-older-clients.patch
Type: text/x-patch
Size: 44831 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131212/3a2770be/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0325.3-Add-new-permission-schema.patch
Type: text/x-patch
Size: 4397 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131212/3a2770be/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0326.3-Rewrite-the-Permission-plugin.patch
Type: text/x-patch
Size: 142665 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131212/3a2770be/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0327.3-Verify-ACIs-are-added-correctly-in-tests.patch
Type: text/x-patch
Size: 20621 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131212/3a2770be/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0331.3-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/20131212/3a2770be/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0332.3-permission-plugin-Ensure-ipapermlocation-subtree-alw.patch
Type: text/x-patch
Size: 3013 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131212/3a2770be/attachment-0007.bin>
More information about the Freeipa-devel
mailing list