On 05/30/2014 11:02 AM, Petr Viktorin wrote:
On 05/29/2014 07:13 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
When investigating this issue I became very annoyed by the star import
hiding where names come from, so I did some cleanup first.


In krbtpolicy, an ACIError is now raised if:
- the user doesn't have permission to read any one of the ticket policy
   attributes on the requested entry
   (checked using attribute-level rights)
- any ticket policy attribute from the default policy is not available
   (either not readable, or not there at all)
   (only checked if these are accessed, i.e. when the user entry doesn't
    override all of the defaults, or when requesting the global policy)

That means if the user is not available at all, you get a NotFound, but
if global policy is not found it's assumed that it's just unreadable.

That seems reasonable to me.

I also noticed a typo, ddoesn't

Fixed, thanks.

In the lower-level code, ldap2.py, we have some help
can_[read|write|etc] for managing rights. Would doing something similar
in baseldap be better than embedding the logic into each plugins?

So instead of this:

                     if rights is None:
                         rights = baseldap.get_effective_rights(
                             ldap, dn, self.obj.default_attributes)
                     if 'r' not in rights.get(attrname.lower(), ''):
                         raise errors.ACIError(
                             info=_('Ticket policy for %s could not be
read') %
                                 keys[-1])

You'd have this:

                     if not baseldap.can_read(ldap, dn, attrname):
                         raise errors.ACIError(
                             info=_('Ticket policy for %s could not be
read') %
                                 keys[-1])

The second is definitely better.

This may end up fetching the rights multiple times depending on how many
things need to be read, so perhaps passing that in if you have it
already.

This however means doing the get_effective_rights call anyway to get all
the needed attrs, at which point most of the readability benefits are gone.

Actually I'd like to add some good attrlevelrights handling right into
ipaldap. Let's do this correctly rather than add a quick fix to the
helpers just so we can use them.
Issue here: https://fedorahosted.org/freeipa/ticket/4362

Once more, with patches

--
PetrĀ³

From 4760edee0db8dd7f1d24daeee0b2501c485dc828 Mon Sep 17 00:00:00 2001
From: Petr Viktorin <pvikt...@redhat.com>
Date: Thu, 29 May 2014 15:39:26 +0200
Subject: [PATCH] krbtpolicy plugin: Code cleanup

- Use the new plugin registration API
  See: http://www.freeipa.org/page/Coding_Best_Practices#Decorator-based_plugin_registration

- Remove the star import from baseldap
  Part of the work for: https://fedorahosted.org/freeipa/ticket/2653
---
 ipalib/plugins/krbtpolicy.py | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/ipalib/plugins/krbtpolicy.py b/ipalib/plugins/krbtpolicy.py
index 22a961a4ab6005b138d0fb261d2a90527b373c48..a3b971e14697470790f8cd01fb45b194223a8226 100644
--- a/ipalib/plugins/krbtpolicy.py
+++ b/ipalib/plugins/krbtpolicy.py
@@ -17,10 +17,12 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-from ipalib import api
+from ipalib import api, errors, output, _
 from ipalib import Int, Str
-from ipalib.plugins.baseldap import *
-from ipalib import _
+from ipalib.plugins import baseldap
+from ipalib.plugins.baseldap import entry_to_dict, pkey_to_value
+from ipalib.plugable import Registry
+from ipapython.dn import DN
 
 __doc__ = _("""
 Kerberos ticket policy
@@ -60,6 +62,8 @@
   ipa krbtpolicy-mod admin --maxlife=3600
 """)
 
+register = Registry()
+
 # FIXME: load this from a config file?
 _default_values = {
     'krbmaxticketlife': 86400,
@@ -67,7 +71,8 @@
 }
 
 
-class krbtpolicy(LDAPObject):
+@register()
+class krbtpolicy(baseldap.LDAPObject):
     """
     Kerberos Ticket Policy object
     """
@@ -141,10 +146,9 @@ def get_dn(self, *keys, **kwargs):
             return self.api.Object.user.get_dn(*keys, **kwargs)
         return DN(self.container_dn, api.env.basedn)
 
-api.register(krbtpolicy)
 
-
-class krbtpolicy_mod(LDAPUpdate):
+@register()
+class krbtpolicy_mod(baseldap.LDAPUpdate):
     __doc__ = _('Modify Kerberos ticket policy.')
 
     def pre_callback(self, ldap, dn, entry_attrs, attrs_list, *keys, **options):
@@ -155,10 +159,9 @@ def pre_callback(self, ldap, dn, entry_attrs, attrs_list, *keys, **options):
             options['all'] = False
         return dn
 
-api.register(krbtpolicy_mod)
 
-
-class krbtpolicy_show(LDAPRetrieve):
+@register()
+class krbtpolicy_show(baseldap.LDAPRetrieve):
     __doc__ = _('Display the current Kerberos ticket policy.')
 
     def pre_callback(self, ldap, dn, attrs_list, *keys, **options):
@@ -180,10 +183,9 @@ def post_callback(self, ldap, dn, entry_attrs, *keys, **options):
                     entry_attrs.setdefault(a, res['result'][a])
         return dn
 
-api.register(krbtpolicy_show)
 
-
-class krbtpolicy_reset(LDAPQuery):
+@register()
+class krbtpolicy_reset(baseldap.LDAPQuery):
     __doc__ = _('Reset Kerberos ticket policy to the default values.')
 
     has_output = output.standard_entry
@@ -217,5 +219,3 @@ def execute(self, *keys, **options):
         entry_attrs = entry_to_dict(entry_attrs, **options)
 
         return dict(result=entry_attrs, value=pkey_to_value(keys[-1], options))
-
-api.register(krbtpolicy_reset)
-- 
1.9.0

From 3b3b2fdfac7d1283c6c88e1fae693e1971021787 Mon Sep 17 00:00:00 2001
From: Petr Viktorin <pvikt...@redhat.com>
Date: Tue, 27 May 2014 16:22:33 +0200
Subject: [PATCH] krbtpolicy plugin: Fix internal error when global policy is
 not readable

An ACIError is now raised if:
- the user doesn't have permission to read any one of the ticket policy
  attributes on the requested entry
  (checked using attribute-level rights)
- any ticket policy attribute from the default policy is not available
  (either not readable, or not there at all)
  (only checked if these are accessed, i.e. when the user entry doesn't
   override all of the defaults, or when requesting the global policy)

https://fedorahosted.org/freeipa/ticket/4354
---
 ipalib/plugins/krbtpolicy.py | 36 +++++++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/ipalib/plugins/krbtpolicy.py b/ipalib/plugins/krbtpolicy.py
index a3b971e14697470790f8cd01fb45b194223a8226..8ddc3b08e44280259c81fdf5e52880fdb13366da 100644
--- a/ipalib/plugins/krbtpolicy.py
+++ b/ipalib/plugins/krbtpolicy.py
@@ -172,15 +172,33 @@ def pre_callback(self, ldap, dn, attrs_list, *keys, **options):
             options['all'] = False
         return dn
 
-    def post_callback(self, ldap, dn, entry_attrs, *keys, **options):
-        assert isinstance(dn, DN)
-        if keys[-1] is not None:
-            # if policy for a specific user isn't set, display global values
-            if 'krbmaxticketlife' not in entry_attrs or \
-                'krbmaxrenewableage' not in entry_attrs:
-                res = self.api.Command.krbtpolicy_show()
-                for a in self.obj.default_attributes:
-                    entry_attrs.setdefault(a, res['result'][a])
+    def post_callback(self, ldap, dn, entry, *keys, **options):
+        default_entry = None
+        rights = None
+        for attrname in self.obj.default_attributes:
+            if attrname not in entry:
+                if keys[-1] is not None:
+                    # User entry doesn't override the attribute.
+                    # Check if this is caused by insufficient read rights
+                    if rights is None:
+                        rights = baseldap.get_effective_rights(
+                            ldap, dn, self.obj.default_attributes)
+                    if 'r' not in rights.get(attrname.lower(), ''):
+                        raise errors.ACIError(
+                            info=_('Ticket policy for %s could not be read') %
+                                keys[-1])
+                    # Fallback to the default
+                    if default_entry is None:
+                        try:
+                            default_dn = self.obj.get_dn(None)
+                            default_entry = ldap.get_entry(default_dn)
+                        except errors.NotFound:
+                            default_entry = {}
+                    if attrname in default_entry:
+                        entry[attrname] = default_entry[attrname]
+            if attrname not in entry:
+                raise errors.ACIError(
+                    info=_('Default ticket policy could not be read'))
         return dn
 
 
-- 
1.9.0

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

Reply via email to