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

[Martin Kosek]

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.

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.


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?


4) Typo in refactored permission plugin:

+            errors.NotFound('ACI of to permission %s was not found' %
+                            keys[0])



Martin