Re: [Freeipa-devel] [PATCHES] 0558-0561 Read ACI fixes

2014-05-29 Thread Martin Kosek
On 05/28/2014 03:40 PM, Petr Viktorin wrote:
 Hello,
 Some of IPA plugins assume that everyone has access to everything. Here are
 some fixes for that.
 
 Patch 0560 adds a new permission for the UPG Definition, which is required to
 add users correctly.

558:

Crash is now removed, though I am thinking that the output may be confusing for
users as there is no output:

# ipa krbtpolicy-show
# echo $?
0

I need to use --all to see anything:

# ipa krbtpolicy-show --all
  dn: cn=MKOSEK-FEDORA20.TEST,cn=kerberos,dc=mkosek-fedora20,dc=test
  cn: MKOSEK-FEDORA20.TEST
  objectclass: krbrealmcontainer, top, krbticketpolicyaux

Would it make sense to raise ACIError if user cannot any Kerberos policy
attributes?

559: ACK
560: ACK
561:

functionally works fine, tested with migrate-ds. When looking at the code,
would it make sense to replace this section:

+disable_attr = '(objectclass=disable)'
+org_filter = upg_entries[0].single_value['originfilter']
+return not re.search(r'%s' % disable_attr, org_filter)

with

+origin_filter = upg_entries[0].single_value['originfilter']
+return '(objectclass=disable)' not in origin_filter

I am not sure why RE is used in this case at all.

Martin

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


Re: [Freeipa-devel] [PATCHES] 0558-0561 Read ACI fixes

2014-05-29 Thread Petr Viktorin

On 05/29/2014 10:03 AM, Martin Kosek wrote:

On 05/28/2014 03:40 PM, Petr Viktorin wrote:

Hello,
Some of IPA plugins assume that everyone has access to everything. Here are
some fixes for that.

Patch 0560 adds a new permission for the UPG Definition, which is required to
add users correctly.


558:

Crash is now removed, though I am thinking that the output may be confusing for
users as there is no output:

# ipa krbtpolicy-show
# echo $?
0

I need to use --all to see anything:

# ipa krbtpolicy-show --all
   dn: cn=MKOSEK-FEDORA20.TEST,cn=kerberos,dc=mkosek-fedora20,dc=test
   cn: MKOSEK-FEDORA20.TEST
   objectclass: krbrealmcontainer, top, krbticketpolicyaux

Would it make sense to raise ACIError if user cannot any Kerberos policy
attributes?


Hm, actually there's a bigger problem -- if the user policy is not 
readable, the command will actually lie.
I think we'll need to check attributelevelrights here to see if the 
attributes are really unset or just unreadable. And I'll go through all 
the other commands more carefully, to see if distinction between not 
readable and not existing makes significant difference.


I withdraw the patch for now.


559: ACK
560: ACK
561:

functionally works fine, tested with migrate-ds. When looking at the code,
would it make sense to replace this section:

+disable_attr = '(objectclass=disable)'
+org_filter = upg_entries[0].single_value['originfilter']
+return not re.search(r'%s' % disable_attr, org_filter)

with

+origin_filter = upg_entries[0].single_value['originfilter']
+return '(objectclass=disable)' not in origin_filter

I am not sure why RE is used in this case at all.


Good point, thanks. Update attached.


--
PetrĀ³

From 6de342ee9e00d4cfb5a4b62ddb3de7896bd337a6 Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Tue, 27 May 2014 16:41:43 +0200
Subject: [PATCH] aci plugin: Fix internal error when ACIs are not readable

Part of the work for: https://fedorahosted.org/freeipa/ticket/3566
---
 ipalib/plugins/aci.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ipalib/plugins/aci.py b/ipalib/plugins/aci.py
index 4821089f164106f195e9c59d5c9a379d8ffadf27..158909837040a4f43c50bf0e36ae62f2c61041e6 100644
--- a/ipalib/plugins/aci.py
+++ b/ipalib/plugins/aci.py
@@ -546,7 +546,7 @@ def execute(self, aciname, **kw):
 raise errors.DuplicateEntry()
 
 newaci_str = unicode(newaci)
-entry['aci'].append(newaci_str)
+entry.setdefault('aci', []).append(newaci_str)
 
 if not kw.get('test', False):
 ldap.update_entry(entry)
-- 
1.9.0

From c328cc4e4230b389c698e67a258d302361ca97f7 Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Wed, 28 May 2014 12:42:02 +0200
Subject: [PATCH] Add managed read permission for the UPG Definition

Since user_add checks the UPG definition to see if UPG is enabled,
user admins need read access to add users correctly.

All attributes are allowed since UPG Definition is an extensibleObject;
the needed attributes are not in the schema.

Part of the work for: https://fedorahosted.org/freeipa/ticket/3566
---
 ipalib/plugins/user.py | 17 +
 1 file changed, 17 insertions(+)

diff --git a/ipalib/plugins/user.py b/ipalib/plugins/user.py
index 604502ee77bc7d10a951335b423526949154392b..27ad36b7fdbee5b2c0dba10f1c381233e8059150 100644
--- a/ipalib/plugins/user.py
+++ b/ipalib/plugins/user.py
@@ -111,6 +111,12 @@
 ),
)
 
+UPG_DEFINITION_DN = DN(('cn', 'UPG Definition'),
+   ('cn', 'Definitions'),
+   ('cn', 'Managed Entries'),
+   ('cn', 'etc'),
+   api.env.basedn)
+
 # characters to be used for generating random user passwords
 user_pwdchars = string.digits + string.ascii_letters + '_,.@+-='
 
@@ -319,6 +325,17 @@ class user(LDAPObject):
 'memberof',
 },
 },
+'System: Read UPG Definition': {
+# Required for adding users
+'replaces_global_anonymous_aci': True,
+'non_object': True,
+'ipapermlocation': UPG_DEFINITION_DN,
+'ipapermtarget': UPG_DEFINITION_DN,
+'ipapermbindruletype': 'permission',
+'ipapermright': {'read', 'search', 'compare'},
+'ipapermdefaultattr': {'*'},
+'default_privileges': {'User Administrators'},
+},
 }
 
 label = _('Users')
-- 
1.9.0

From b853299f3d03dc6397f5ea18ab63c8f39354d9c9 Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Tue, 27 May 2014 17:07:13 +0200
Subject: [PATCH] ldap2.has_upg: Raise an error if the UPG definition is not
 found

The UPG Definition is always present in IPA; if it can not be read
it's usually caused by insufficient privileges.
Previously the code assumed the absence of the entry meant that
UPG is disabled. With granular read permissions, this would mean
that users that 

Re: [Freeipa-devel] [PATCHES] 0558-0561 Read ACI fixes

2014-05-29 Thread Martin Kosek
On 05/29/2014 11:04 AM, Petr Viktorin wrote:
 On 05/29/2014 10:03 AM, Martin Kosek wrote:
 On 05/28/2014 03:40 PM, Petr Viktorin wrote:
 Hello,
 Some of IPA plugins assume that everyone has access to everything. Here are
 some fixes for that.

 Patch 0560 adds a new permission for the UPG Definition, which is required 
 to
 add users correctly.

 558:

 Crash is now removed, though I am thinking that the output may be confusing 
 for
 users as there is no output:

 # ipa krbtpolicy-show
 # echo $?
 0

 I need to use --all to see anything:

 # ipa krbtpolicy-show --all
dn: cn=MKOSEK-FEDORA20.TEST,cn=kerberos,dc=mkosek-fedora20,dc=test
cn: MKOSEK-FEDORA20.TEST
objectclass: krbrealmcontainer, top, krbticketpolicyaux

 Would it make sense to raise ACIError if user cannot any Kerberos policy
 attributes?
 
 Hm, actually there's a bigger problem -- if the user policy is not readable,
 the command will actually lie.
 I think we'll need to check attributelevelrights here to see if the attributes
 are really unset or just unreadable. And I'll go through all the other 
 commands
 more carefully, to see if distinction between not readable and not 
 existing
 makes significant difference.
 
 I withdraw the patch for now.
 
 559: ACK
 560: ACK
 561:

 functionally works fine, tested with migrate-ds. When looking at the code,
 would it make sense to replace this section:

 +disable_attr = '(objectclass=disable)'
 +org_filter = upg_entries[0].single_value['originfilter']
 +return not re.search(r'%s' % disable_attr, org_filter)

 with

 +origin_filter = upg_entries[0].single_value['originfilter']
 +return '(objectclass=disable)' not in origin_filter

 I am not sure why RE is used in this case at all.
 
 Good point, thanks. Update attached.
 
 

Thanks, works fine. ACK for all these 3 patches.

Martin

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