[apparmor] [patch] rule class - split out common parts from is_covered()

[Steve Beattie]

On Fri, Dec 19, 2014 at 12:24:02AM +0100, Christian Boltz wrote:
> Hello,
> 
> this patch splits is_covered() in capability.py into
> - is_covered_localparts() for rule-specific code
> - is_covered() for common code - located in __init__.py

I'm not keen on the naming convention xxxxx_localvars() that the code
actually uses, the xxxxx_localparts() listed above is makes me cringe
slightly less.

> The object type comparison now uses type(self) and a slightly different
> error message to make it usable everywhere.
> 
> It also renames rule_obj to other_rule which is more self-explaining 
> (inspired by the parameter name in the is_covered() dummy in __init__.py).
> 
> 
> [ rule_split_is_covered.diff ]
> 
> === modified file 'utils/apparmor/rule/__init__.py'
> --- utils/apparmor/rule/__init__.py     2014-12-16 22:17:33 +0000
> +++ utils/apparmor/rule/__init__.py     2014-12-18 23:16:13 +0000
> @@ -68,6 +68,29 @@
>          else:
>              return self.get_clean(depth)
>  
> +    def is_covered(self, other_rule, check_allow_deny=True, check_audit=False):
> +        '''check if other_rule is covered by this rule object'''
> +
> +        if not type(other_rule) == type(self):
> +            raise AppArmorBug('Passes %s instead of %s' % (str(other_rule),self.__class__.__name__))
> +
> +        if check_allow_deny and self.deny != other_rule.deny:
> +            return False
> +
> +        if check_audit and other_rule.audit != self.audit:
> +            return False
> +
> +        if other_rule.audit and not self.audit:
> +            return False

I recognize that you are merely pushing these checks around, but do you
recall why we do the second audit test after the first? I don't quite
get the reasoning...

> +
> +        # still here? -> then the common part is covered, check rule-specific things now
> +        return self.is_covered_localvars(other_rule, check_allow_deny, check_audit)
> +
> +    # @abstractmethod  FIXME - uncomment when python3 only
> +    def is_covered_localvars(self, other_rule, check_allow_deny=True, check_audit=False):

If check_allow_deny and check_audit are verified in the BaseRule method,
is there a reason to pass them on to the subclass version of the method,
since they should be irrelevant to them?

> +        '''check if the rule-specific parts of other_rule is covered by this rule object'''
> +        raise AppArmorBug("'%s' needs to implement is_covered_localvars(), but didn't" % (str(self)))
> +
>      def is_equal(self, rule_obj, strict=False):
>          '''compare if rule_obj == self
>             Calls is_equal_localvars() to compare rule-specific variables'''
> @@ -85,11 +108,6 @@
>          return self.is_equal_localvars(rule_obj)
>  
>      # @abstractmethod  FIXME - uncomment when python3 only
> -    def is_covered(self, other_rule, check_allow_deny=True, check_audit=False):
> -        '''check if other_rule is covered by this rule object'''
> -        raise AppArmorBug("'%s' needs to implement is_covered(), but didn't" % (str(self)))
> -
> -    # @abstractmethod  FIXME - uncomment when python3 only
>      def is_equal_localvars(self, other_rule):
>          '''compare if rule-specific variables are equal'''
>          raise AppArmorBug("'%s' needs to implement is_equal_localvars(), but didn't" % (str(self)))
> 
> === modified file 'utils/apparmor/rule/capability.py'
> --- utils/apparmor/rule/capability.py   2014-12-16 22:17:33 +0000
> +++ utils/apparmor/rule/capability.py   2014-12-18 22:50:11 +0000
> @@ -95,30 +95,18 @@
>              else:
>                  raise AppArmorBug("Empty capability rule")
>  
> -    def is_covered(self, rule_obj, check_allow_deny=True, check_audit=False):
> -        '''check if rule_obj is covered by this rule object'''
> -
> -        if not type(rule_obj) == CapabilityRule:
> -            raise AppArmorBug('Passes non-capability rule: %s' % str(rule_obj))
> -
> -        if check_allow_deny and self.deny != rule_obj.deny:
> -            return False
> +    def is_covered_localvars(self, other_rule, check_allow_deny=True, check_audit=False):
> +        '''check if other_rule is covered by this rule object'''
>  
> -        if not rule_obj.capability and not rule_obj.all_caps:
> +        if not other_rule.capability and not other_rule.all_caps:
>              raise AppArmorBug('No capability specified')
>  
>          if not self.all_caps:
> -            if rule_obj.all_caps:
> +            if other_rule.all_caps:
>                  return False
> -            if not rule_obj.capability.issubset(self.capability):
> +            if not other_rule.capability.issubset(self.capability):
>                  return False
>  
> -        if check_audit and rule_obj.audit != self.audit:
> -            return False
> -
> -        if rule_obj.audit and not self.audit:
> -            return False
> -
>          # still here? -> then it is covered
>          return True
>  
> 
> 
> 
> Regards,
> 
> Christian Boltz
> -- 
> Please resolve this as NOT A BUG and USER SHOULD HAVE MORE COFFEE BEFORE
> FILING BUGS.  I apologize for taking up valuable developer time!
> [Jon Nelson in https://bugzilla.novell.com/show_bug.cgi?id=776271#c2]
> 
> 
> -- 
> AppArmor mailing list
> AppArmor at lists.ubuntu.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor

-- 
Steve Beattie
<sbeattie at ubuntu.com>
http://NxNW.org/~steve/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20150113/51275e6c/attachment.pgp>