On 12/13/2013 10:49 AM, Martin Kosek wrote:
On 12/12/2013 05:17 PM, Petr Viktorin wrote:
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] ad...@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




Looks better, this is hopefully the last issue:

1) There is some problem with DNS zone permissions:

# ipa dnszone-add-permission example.com
-----------------------------------------------------
Added system permission "Manage DNS zone example.com"
-----------------------------------------------------

# ipa permission-show 'Manage DNS zone example.com' --all --raw
ipa: ERROR: The ACI for permission Manage DNS zone example.com was not found in
dc=idm,dc=example,dc=com

Thanks for the catch. Fixed in an additional patch.

# ipa privilege-add-permission foo --permissions foo
   Privilege name: foo
   Description: foo
   Failed members:
     permission: foo: missing attribute "ipaPermLocation" required by object
class "ipaPermissionV2"
-----------------------------
Number of permissions added 0
-----------------------------

Could not reproduce. This one used a permission created by the previous set of patches, where ipaPermLocation was not set when it was $SUFFIX. This is incompatible with the new schema. From the last update ipaPermLocation is in MUST, and is always added. I did write some additional tests before I asked Martin to explain this, so I'm also including these.

Apply these on top of the patches sent earlier.

--
PetrĀ³

From 1d4dc710e4dd73f1f1d8e994ad3e7e5232f38629 Mon Sep 17 00:00:00 2001
From: Petr Viktorin <pvikt...@redhat.com>
Date: Fri, 13 Dec 2013 11:10:28 +0100
Subject: [PATCH] Make sure SYSTEM permissions can be retreived with --all
 --raw

Part of the work for: https://fedorahosted.org/freeipa/ticket/4034
---
 ipalib/plugins/permission.py            | 12 ++++++++++--
 ipatests/test_xmlrpc/test_dns_plugin.py | 18 ++++++++++++++++--
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/ipalib/plugins/permission.py b/ipalib/plugins/permission.py
index f3f001b7493d03bd8cc957c131b090916a3ea06d..bdde3e32eb6005b5ec37334356b8ed36b262f07c 100644
--- a/ipalib/plugins/permission.py
+++ b/ipalib/plugins/permission.py
@@ -310,8 +310,16 @@ def postprocess_result(self, entry, options):
 
         if options.get('raw'):
             # Retreive the ACI from LDAP to ensure we get the real thing
-            acientry, acistring = self._get_aci_entry_and_string(entry)
-            entry.single_value['aci'] = acistring
+            try:
+                acientry, acistring = self._get_aci_entry_and_string(entry)
+            except errors.NotFound:
+                if list(entry.get('ipapermissiontype')) == ['SYSTEM']:
+                    # SYSTEM permissions don't have normal ACIs
+                    pass
+                else:
+                    raise
+            else:
+                entry.single_value['aci'] = acistring
 
         if not client_has_capability(options['version'], 'permissions2'):
             # Legacy clients expect some attributes as a single value
diff --git a/ipatests/test_xmlrpc/test_dns_plugin.py b/ipatests/test_xmlrpc/test_dns_plugin.py
index 8dbdec6ba8c1b76908272ce402841bb80d1f4aee..d301458d7aa7f1850dbcf8a8a28fda93381c9813 100644
--- a/ipatests/test_xmlrpc/test_dns_plugin.py
+++ b/ipatests/test_xmlrpc/test_dns_plugin.py
@@ -1349,7 +1349,6 @@ def setUpClass(cls):
                 '"%s" already exists' % dnszone1_permission)
         ),
 
-
         dict(
             desc='Make sure the permission was created %r' % dnszone1,
             command=(
@@ -1367,6 +1366,22 @@ def setUpClass(cls):
             ),
         ),
 
+        dict(
+            desc='Retrieve the permission %r with --all --raw' % dnszone1,
+            command=(
+                'permission_show', [dnszone1_permission], {}
+            ),
+            expected=dict(
+                value=dnszone1_permission,
+                summary=None,
+                result={
+                    'dn': dnszone1_permission_dn,
+                    'cn': [dnszone1_permission],
+                    'objectclass': objectclasses.system_permission,
+                    'ipapermissiontype': [u'SYSTEM'],
+                },
+            ),
+        ),
 
         dict(
             desc='Try to remove per-zone permission for unknown zone',
@@ -1374,7 +1389,6 @@ def setUpClass(cls):
             expected=errors.NotFound(reason=u'does.not.exist: DNS zone not found')
         ),
 
-
         dict(
             desc='Remove per-zone permission for zone %r' % dnszone1,
             command=(
-- 
1.8.3.1

From 0017a5ed1673a3fb92c64a2818cb75c27e930f5b Mon Sep 17 00:00:00 2001
From: Petr Viktorin <pvikt...@redhat.com>
Date: Fri, 13 Dec 2013 11:28:22 +0100
Subject: [PATCH] Test adding noaci/system permissions to privileges

Part of the work for: https://fedorahosted.org/freeipa/ticket/4034
---
 ipatests/test_xmlrpc/test_permission_plugin.py | 42 +++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/ipatests/test_xmlrpc/test_permission_plugin.py b/ipatests/test_xmlrpc/test_permission_plugin.py
index 6259fc831d59d8553e000ba72bb4a0d3b1790bcf..e1a7cd8ee15349b353979c3cfca0c5b027580ee3 100644
--- a/ipatests/test_xmlrpc/test_permission_plugin.py
+++ b/ipatests/test_xmlrpc/test_permission_plugin.py
@@ -2055,6 +2055,27 @@ def _make_permission_flag_tests(flags, expected_message):
         ),
 
         dict(
+            desc='Add %r to %r' % (permission1, privilege1),
+            command=('privilege_add_permission', [privilege1],
+                     {'permission': permission1}),
+            expected=dict(
+                completed=1,
+                failed=dict(
+                    member=dict(
+                        permission=[],
+                    ),
+                ),
+                result={
+                    'dn': privilege1_dn,
+                    'cn': [privilege1],
+                    'description': [u'privilege desc. 1'],
+                    'memberof_permission': [permission1],
+                    'objectclass': objectclasses.privilege,
+                }
+            ),
+        ),
+
+        dict(
             desc='Delete %r with --force' % permission1,
             command=('permission_del', [permission1], {'force': True}),
             expected=dict(
@@ -2070,9 +2091,28 @@ class test_permission_flags(Declarative):
     """Test that permission flags are handled correctly"""
     cleanup_commands = [
         ('permission_del', [permission1], {'force': True}),
+        ('privilege_del', [privilege1], {}),
     ]
 
-    tests = (
+    tests = [
+        dict(
+            desc='Create %r' % privilege1,
+            command=('privilege_add', [privilege1],
+                dict(description=u'privilege desc. 1')
+            ),
+            expected=dict(
+                value=privilege1,
+                summary=u'Added privilege "%s"' % privilege1,
+                result=dict(
+                    dn=privilege1_dn,
+                    cn=[privilege1],
+                    description=[u'privilege desc. 1'],
+                    objectclass=objectclasses.privilege,
+                ),
+            ),
+        ),
+
+    ] + (
         _make_permission_flag_tests(
             [u'SYSTEM'],
             'A SYSTEM permission may not be modified or removed') +
-- 
1.8.3.1

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

Reply via email to