From 499f71729b8689d40608d9c99db703eb2c00a934 Mon Sep 17 00:00:00 2001 From: Rob Crittenden Date: Thu, 23 Jun 2022 10:32:24 -0400 Subject: [PATCH] Warn for permissions with read/write/search/compare and no attrs An ACI with rights of read, write, search and/or compare without attributes to apply the rights to is effectively a no-op. Allow the ACI to be created but include a warning. Ignore the add and delete rights. While they make no sense in the context of the other rights we should still warn that they are a no-op with no attributes. Use the existing make_aci() object method to create the message and update the add/mod callers to capture and add the message to the result if one is provided. When updating an existing ACI the effective attributes will not be included so fall back to the attributes in the resulting permission. Prior to checking for rights and attributes convert any deprecated names for older clients into the newer values needed by make_aci This is exercised by existing xmlrpc permission tests that create such permissions without attributes. https://pagure.io/freeipa/issue/9188 Signed-off-by: Rob Crittenden Reviewed-By: Alexander Bokovoy Reviewed-By: Florence Blanc-Renaud --- ipalib/messages.py | 14 +- ipaserver/plugins/permission.py | 49 +++++- .../test_xmlrpc/test_old_permission_plugin.py | 108 +++++++++++++ .../test_xmlrpc/test_permission_plugin.py | 144 ++++++++++++++++++ ipatests/test_xmlrpc/test_privilege_plugin.py | 12 ++ makeaci.in | 2 +- 6 files changed, 320 insertions(+), 9 deletions(-) diff --git a/ipalib/messages.py b/ipalib/messages.py index f7df9c28d..732de7cb9 100644 --- a/ipalib/messages.py +++ b/ipalib/messages.py @@ -18,7 +18,7 @@ # along with this program. If not, see . """ -Custom message (debug, info, wraning) classes passed through RPC. +Custom message (debug, info, warning) classes passed through RPC. These are added to the "messages" entry in a RPC response, and printed to the user as log messages. @@ -27,7 +27,7 @@ Each message class has a unique numeric "errno" attribute from the 10000-10999 range, so that it does not clash with PublicError numbers. Messages also have the 'type' argument, set to one of 'debug', 'info', -'warning', 'error'. This determines the severity of themessage. +'warning', 'error'. This determines the severity of the message. """ from __future__ import print_function @@ -496,6 +496,16 @@ class LightweightCACertificateNotAvailable(PublicMessage): format = _("The certificate for %(ca)s is not available on this server.") +class MissingTargetAttributesinPermission(PublicMessage): + """ + **13032** A permission was added with no target attributes + """ + errno = 13032 + type = "warning" + format = _("The permission has %(right)s rights but no attributes " + "are set.") + + def iter_messages(variables, base): """Return a tuple with all subclasses """ diff --git a/ipaserver/plugins/permission.py b/ipaserver/plugins/permission.py index afd913756..b75187191 100644 --- a/ipaserver/plugins/permission.py +++ b/ipaserver/plugins/permission.py @@ -28,6 +28,7 @@ from .privilege import validate_permission_to_privilege from ipalib import errors from ipalib.parameters import Str, StrEnum, DNParam, Flag from ipalib import api, _, ngettext +from ipalib import messages from ipalib.plugable import Registry from ipalib.capabilities import client_has_capability from ipalib.aci import ACI @@ -553,6 +554,7 @@ class permission(baseldap.LDAPObject): def make_aci(self, entry): """Make an ACI string from the given permission entry""" + msg = None aci_parts = [] name = entry.single_value['cn'] @@ -560,6 +562,18 @@ class permission(baseldap.LDAPObject): attrs = self.get_effective_attrs(entry) if attrs: aci_parts.append("(targetattr = \"%s\")" % ' || '.join(attrs)) + else: + # If we are re-parsing an entry for validation then + # the effective attrs may be empty. + if entry.get('attrs') is None: + rights = set(['read', 'write', 'search', 'compare']) + permrights = set(entry['ipapermright']) + for right in ('add', 'delete'): + permrights.discard(right) + if permrights and permrights.issubset(rights): + msg = messages.MissingTargetAttributesinPermission( + right=', '.join(sorted(entry['ipapermright'])) + ) # target ipapermtarget = entry.single_value.get('ipapermtarget') @@ -608,12 +622,12 @@ class permission(baseldap.LDAPObject): aci_parts.append('(version 3.0;acl "permission:%s";allow (%s) %s;)' % ( name, ','.join(sorted(entry['ipapermright'])), bindrule)) - return ''.join(aci_parts) + return ''.join(aci_parts), msg def add_aci(self, permission_entry): """Add the ACI coresponding to the given permission entry""" ldap = self.api.Backend.ldap2 - acistring = self.make_aci(permission_entry) + acistring, _msg = self.make_aci(permission_entry) location = permission_entry.single_value.get('ipapermlocation', self.api.env.basedn) @@ -641,7 +655,7 @@ class permission(baseldap.LDAPObject): - entry - removed ACI string, or None if none existed previously """ - new_acistring = self.make_aci(permission_entry) + new_acistring, _msg = self.make_aci(permission_entry) return self._replace_aci(permission_entry, old_name, new_acistring) def _replace_aci(self, permission_entry, old_name=None, new_acistring=None): @@ -672,6 +686,25 @@ class permission(baseldap.LDAPObject): logger.debug('No changes to ACI') return acientry, acistring + def check_attrs(self, result, *keys, **options): + """Re-build the ACI to determine if there are rights that + only work when there are attributes defined.""" + ldap = self.backend + + dn = self.get_dn(*keys, **options) + entry = ldap.make_entry(dn, result['result']) + for old_name, new_name in _DEPRECATED_OPTION_ALIASES.items(): + if old_name in entry: + entry[new_name] = entry[old_name] + del entry[old_name] + _acistring, msg = self.make_aci(entry) + if msg: + messages.add_message( + options['version'], + result, + msg, + ) + def _get_aci_entry_and_string(self, permission_entry, name=None, notfound_ok=False, cached_acientry=None): """Get the entry and ACI corresponding to the permission entry @@ -776,7 +809,7 @@ class permission(baseldap.LDAPObject): target_entry['ipapermlocation'] = [self.api.env.basedn] # Make sure we're not losing *any info* by the upgrade - new_acistring = self.make_aci(target_entry) + new_acistring, _msg = self.make_aci(target_entry) if not ACI(new_acistring).isequal(aci): raise ValueError('Cannot convert ACI, %r != %r' % (new_acistring, acistring)) @@ -992,7 +1025,9 @@ class permission_add(baseldap.LDAPCreate): # the whole command, not just the callbacks def execute(self, *keys, **options): self.obj.preprocess_options(options, merge_targetfilter=True) - return super(permission_add, self).execute(*keys, **options) + result = super(permission_add, self).execute(*keys, **options) + self.obj.check_attrs(result, *keys, **options) + return result def get_args(self): for arg in super(permission_add, self).get_args(): @@ -1094,7 +1129,9 @@ class permission_mod(baseldap.LDAPUpdate): def execute(self, *keys, **options): context.filter_ops = self.obj.preprocess_options( options, return_filter_ops=True) - return super(permission_mod, self).execute(*keys, **options) + result = super(permission_mod, self).execute(*keys, **options) + self.obj.check_attrs(result, *keys, **options) + return result def get_options(self): for opt in super(permission_mod, self).get_options(): diff --git a/ipatests/test_xmlrpc/test_old_permission_plugin.py b/ipatests/test_xmlrpc/test_old_permission_plugin.py index 600e44942..e898e6bba 100644 --- a/ipatests/test_xmlrpc/test_old_permission_plugin.py +++ b/ipatests/test_xmlrpc/test_old_permission_plugin.py @@ -152,6 +152,18 @@ class test_old_permission(Declarative): expected=dict( value=permission1, summary=u'Added permission "%s"' % permission1, + messages=( + { + 'message': ('The permission has write rights but no ' + 'attributes are set.'), + 'code': 13032, + 'type': 'warning', + 'name': 'MissingTargetAttributesinPermission', + 'data': { + 'right': 'write', + } + }, + ), result=dict( dn=permission1_dn, cn=[permission1], @@ -493,6 +505,18 @@ class test_old_permission(Declarative): expected=dict( value=permission2, summary=u'Added permission "%s"' % permission2, + messages=( + { + 'message': ('The permission has write rights but no ' + 'attributes are set.'), + 'code': 13032, + 'type': 'warning', + 'name': 'MissingTargetAttributesinPermission', + 'data': { + 'right': 'write', + } + }, + ), result=dict( dn=permission2_dn, cn=[permission2], @@ -801,6 +825,18 @@ class test_old_permission(Declarative): expected=dict( value=permission1, summary=u'Modified permission "%s"' % permission1, + messages=( + { + 'message': ('The permission has read rights but no ' + 'attributes are set.'), + 'code': 13032, + 'type': 'warning', + 'name': 'MissingTargetAttributesinPermission', + 'data': { + 'right': 'read', + } + }, + ), result=dict( dn=permission1_dn, cn=[permission1], @@ -921,6 +957,18 @@ class test_old_permission(Declarative): expected=dict( value=permission1_renamed, summary=u'Modified permission "%s"' % permission1_renamed, + messages=( + { + 'message': ('The permission has write rights but no ' + 'attributes are set.'), + 'code': 13032, + 'type': 'warning', + 'name': 'MissingTargetAttributesinPermission', + 'data': { + 'right': 'write', + } + }, + ), result={ 'dn': permission1_renamed_ucase_dn, 'cn': [permission1_renamed_ucase], @@ -947,6 +995,18 @@ class test_old_permission(Declarative): expected=dict( value=permission1_renamed_ucase, summary=u'Modified permission "%s"' % permission1_renamed_ucase, + messages=( + { + 'message': ('The permission has write rights but no ' + 'attributes are set.'), + 'code': 13032, + 'type': 'warning', + 'name': 'MissingTargetAttributesinPermission', + 'data': { + 'right': 'write', + } + }, + ), result=dict( dn=permission1_renamed_ucase_dn, cn=[permission1_renamed_ucase], @@ -1175,6 +1235,18 @@ class test_old_permission(Declarative): expected=dict( value=permission1, summary=u'Added permission "%s"' % permission1, + messages=( + { + 'message': ('The permission has write rights but no ' + 'attributes are set.'), + 'code': 13032, + 'type': 'warning', + 'name': 'MissingTargetAttributesinPermission', + 'data': { + 'right': 'write', + } + }, + ), result=dict( dn=permission1_dn, cn=[permission1], @@ -1206,6 +1278,18 @@ class test_old_permission(Declarative): expected=dict( value=permission1, summary=u'Modified permission "%s"' % permission1, + messages=( + { + 'message': ('The permission has write rights but no ' + 'attributes are set.'), + 'code': 13032, + 'type': 'warning', + 'name': 'MissingTargetAttributesinPermission', + 'data': { + 'right': 'write', + } + }, + ), result=dict( dn=permission1_dn, cn=[permission1], @@ -1229,6 +1313,18 @@ class test_old_permission(Declarative): ), expected=dict( summary=u'Modified permission "%s"' % permission1, + messages=( + { + 'message': ('The permission has write rights but no ' + 'attributes are set.'), + 'code': 13032, + 'type': 'warning', + 'name': 'MissingTargetAttributesinPermission', + 'data': { + 'right': 'write', + } + }, + ), value=permission1, result=dict( dn=permission1_dn, @@ -1266,6 +1362,18 @@ class test_old_permission(Declarative): expected=dict( value=permission1, summary=u'Added permission "%s"' % permission1, + messages=( + { + 'message': ('The permission has write rights but no ' + 'attributes are set.'), + 'code': 13032, + 'type': 'warning', + 'name': 'MissingTargetAttributesinPermission', + 'data': { + 'right': 'write', + } + }, + ), result=dict( dn=permission1_dn, cn=[permission1], diff --git a/ipatests/test_xmlrpc/test_permission_plugin.py b/ipatests/test_xmlrpc/test_permission_plugin.py index 12c50339a..218c893d5 100644 --- a/ipatests/test_xmlrpc/test_permission_plugin.py +++ b/ipatests/test_xmlrpc/test_permission_plugin.py @@ -3167,6 +3167,18 @@ class test_permission_bindtype(Declarative): expected=dict( value=permission1, summary=u'Added permission "%s"' % permission1, + messages=( + { + 'message': ('The permission has write rights but no ' + 'attributes are set.'), + 'code': 13032, + 'type': 'warning', + 'name': 'MissingTargetAttributesinPermission', + 'data': { + 'right': 'write', + } + }, + ), result=dict( dn=permission1_dn, cn=[permission1], @@ -3228,6 +3240,18 @@ class test_permission_bindtype(Declarative): expected=dict( value=permission1, summary=u'Modified permission "%s"' % permission1, + messages=( + { + 'message': ('The permission has write rights but no ' + 'attributes are set.'), + 'code': 13032, + 'type': 'warning', + 'name': 'MissingTargetAttributesinPermission', + 'data': { + 'right': 'write', + } + }, + ), result=dict( dn=permission1_dn, cn=[permission1], @@ -3319,6 +3343,18 @@ class test_permission_bindtype(Declarative): expected=dict( value=permission1, summary=u'Modified permission "%s"' % permission1, + messages=( + { + 'message': ('The permission has write rights but no ' + 'attributes are set.'), + 'code': 13032, + 'type': 'warning', + 'name': 'MissingTargetAttributesinPermission', + 'data': { + 'right': 'write', + } + }, + ), result=dict( dn=permission1_renamed_dn, cn=[permission1_renamed], @@ -3350,6 +3386,18 @@ class test_permission_bindtype(Declarative): expected=dict( value=permission1_renamed, summary=u'Modified permission "%s"' % permission1_renamed, + messages=( + { + 'message': ('The permission has write rights but no ' + 'attributes are set.'), + 'code': 13032, + 'type': 'warning', + 'name': 'MissingTargetAttributesinPermission', + 'data': { + 'right': 'write', + } + }, + ), result=dict( dn=permission1_renamed_dn, cn=[permission1_renamed], @@ -3379,6 +3427,18 @@ class test_permission_bindtype(Declarative): expected=dict( value=permission1_renamed, summary=u'Modified permission "%s"' % permission1_renamed, + messages=( + { + 'message': ('The permission has write rights but no ' + 'attributes are set.'), + 'code': 13032, + 'type': 'warning', + 'name': 'MissingTargetAttributesinPermission', + 'data': { + 'right': 'write', + } + }, + ), result=dict( dn=permission1_dn, cn=[permission1], @@ -4010,6 +4070,18 @@ class test_permission_filters(Declarative): expected=dict( value=permission1, summary=u'Added permission "%s"' % permission1, + messages=( + { + 'message': ('The permission has write rights but no ' + 'attributes are set.'), + 'code': 13032, + 'type': 'warning', + 'name': 'MissingTargetAttributesinPermission', + 'data': { + 'right': 'write', + } + }, + ), result=dict( dn=permission1_dn, cn=[permission1], @@ -4054,6 +4126,18 @@ class test_permission_filters(Declarative): expected=dict( value=permission1, summary=u'Modified permission "%s"' % permission1, + messages=( + { + 'message': ('The permission has write rights but no ' + 'attributes are set.'), + 'code': 13032, + 'type': 'warning', + 'name': 'MissingTargetAttributesinPermission', + 'data': { + 'right': 'write', + } + }, + ), result=dict( dn=permission1_dn, cn=[permission1], @@ -4093,6 +4177,18 @@ class test_permission_filters(Declarative): expected=dict( value=permission1, summary=u'Modified permission "%s"' % permission1, + messages=( + { + 'message': ('The permission has write rights but no ' + 'attributes are set.'), + 'code': 13032, + 'type': 'warning', + 'name': 'MissingTargetAttributesinPermission', + 'data': { + 'right': 'write', + } + }, + ), result=dict( dn=permission1_dn, cn=[permission1], @@ -4132,6 +4228,18 @@ class test_permission_filters(Declarative): expected=dict( value=permission1, summary=u'Modified permission "%s"' % permission1, + messages=( + { + 'message': ('The permission has write rights but no ' + 'attributes are set.'), + 'code': 13032, + 'type': 'warning', + 'name': 'MissingTargetAttributesinPermission', + 'data': { + 'right': 'write', + } + }, + ), result=dict( dn=permission1_dn, cn=[permission1], @@ -4172,6 +4280,18 @@ class test_permission_filters(Declarative): expected=dict( value=permission1, summary=u'Modified permission "%s"' % permission1, + messages=( + { + 'message': ('The permission has write rights but no ' + 'attributes are set.'), + 'code': 13032, + 'type': 'warning', + 'name': 'MissingTargetAttributesinPermission', + 'data': { + 'right': 'write', + } + }, + ), result=dict( dn=permission1_dn, cn=[permission1], @@ -4205,6 +4325,18 @@ class test_permission_filters(Declarative): expected=dict( value=permission1, summary=u'Modified permission "%s"' % permission1, + messages=( + { + 'message': ('The permission has write rights but no ' + 'attributes are set.'), + 'code': 13032, + 'type': 'warning', + 'name': 'MissingTargetAttributesinPermission', + 'data': { + 'right': 'write', + } + }, + ), result=dict( dn=permission1_dn, cn=[permission1], @@ -4254,6 +4386,18 @@ class test_permission_filters(Declarative): expected=dict( value=permission1, summary=u'Added permission "%s"' % permission1, + messages=( + { + 'message': ('The permission has write rights but no ' + 'attributes are set.'), + 'code': 13032, + 'type': 'warning', + 'name': 'MissingTargetAttributesinPermission', + 'data': { + 'right': 'write', + } + }, + ), result=dict( dn=permission1_dn, cn=[permission1], diff --git a/ipatests/test_xmlrpc/test_privilege_plugin.py b/ipatests/test_xmlrpc/test_privilege_plugin.py index 49f6c80b9..aefff6cb8 100644 --- a/ipatests/test_xmlrpc/test_privilege_plugin.py +++ b/ipatests/test_xmlrpc/test_privilege_plugin.py @@ -255,6 +255,18 @@ class test_privilege(Declarative): expected=dict( value=permission2, summary=u'Added permission "%s"' % permission2, + messages=( + { + 'message': ('The permission has write rights but no ' + 'attributes are set.'), + 'code': 13032, + 'type': 'warning', + 'name': 'MissingTargetAttributesinPermission', + 'data': { + 'right': 'write', + } + }, + ), result=dict( dn=permission2_dn, cn=[permission2], diff --git a/makeaci.in b/makeaci.in index 47b237e25..11013f294 100644 --- a/makeaci.in +++ b/makeaci.in @@ -61,7 +61,7 @@ def generate_aci_lines(api): anonymous_read_aci=None, is_new=True, ) - aci = perm_plugin.make_aci(entry) + aci, _msg = perm_plugin.make_aci(entry) yield 'dn: %s\n' % entry.single_value['ipapermlocation'] yield f'aci: {aci}\n'