[apparmor] [patch] Add severity() to BaseRule class

[Steve Beattie]

On Wed, Jun 03, 2015 at 11:57:00PM +0200, Christian Boltz wrote:
> Am Mittwoch, 3. Juni 2015 schrieb Christian Boltz:
> > So here's the updated patch that adds a NOT_IMPLEMENTED constant to 
> > severity.py and changes the code to use it.
> > 
> > See [1] for an interdiff.
> 
> Argh, I should also update the comment in rule/__init__.py ;-)
> 
> Add severity() to BaseRule class
> 
> severity() will, surprise!, return the severity of a rule, or
> sev_db.NOT_IMPLEMENTED if a *Rule class doesn't implement the severity()
> function.
> 
> Also add the NOT_IMPLEMENTED constant to severity.py, and a test to
> test-baserule.py that checks the return value in BaseRule.
> 
> 
> [ 19-baserule-add-severity.diff ]

Acked-by: Steve Beattie <steve at nxnw.org> as is. Some comments:

> === modified file utils/apparmor/rule/__init__.py
> --- utils/apparmor/rule/__init__.py     2015-06-03 23:24:34.798948576 +0200
> +++ utils/apparmor/rule/__init__.py     2015-06-03 23:25:46.638698441 +0200
> @@ -135,6 +135,12 @@
>          '''compare if rule-specific variables are equal'''
>          raise AppArmorBug("'%s' needs to implement is_equal_localvars(), but didn't" % (str(self)))
>  
> +    def severity(self, sev_db):
> +        '''return severity of this rule (a number between 0 and 10, where 0 means harmless and 10 means critical),
> +           or sev_db.NOT_IMPLEMENTED if no severity check is implemented for this rule type.

Should the comment here say anything about possibly returning
sev_db.unknown?

> +           sev_db must be an apparmor.severity.Severity object.'''
> +        return sev_db.NOT_IMPLEMENTED
> +
>      def modifiers_str(self):
>          '''return the allow/deny and audit keyword as string, including whitespace'''
>  
> === modified file utils/apparmor/severity.py
> --- utils/apparmor/severity.py  2015-06-03 23:24:34.789949109 +0200
> +++ utils/apparmor/severity.py  2015-06-03 23:27:30.412600284 +0200
> @@ -20,6 +20,7 @@
>      def __init__(self, dbname=None, default_rank=10):
>          """Initialises the class object"""
>          self.PROF_DIR = '/etc/apparmor.d'  # The profile directory
> +        self.NOT_IMPLEMENTED = '_-*not*implemented*-_'  # used for rule types that don't have severity ratings
>          self.severity = dict()
>          self.severity['DATABASENAME'] = dbname
>          self.severity['CAPABILITIES'] = {}
> === modified file utils/test/test-baserule.py
> --- utils/test/test-baserule.py 2015-06-03 23:24:34.798948576 +0200
> +++ utils/test/test-baserule.py 2015-06-03 23:29:26.556777257 +0200
> @@ -14,6 +14,7 @@
>  
>  from apparmor.common import AppArmorBug
>  from apparmor.rule import BaseRule, parse_modifiers
> +import apparmor.severity as severity
>  
>  import re
>  
> @@ -51,6 +52,11 @@
>          with self.assertRaises(AppArmorBug):
>              parse_modifiers(matches)
>  
> +    def test_default_severity(self):
> +        sev_db = severity.Severity('severity.db', 'unknown')
> +        obj = BaseRule()
> +        rank = obj.severity(sev_db)

From an object-oriented perspective, it feels a little odd to me to
ask a rule what the severity of that rule is given a severity db. It
seems like the more natural conceptual structure would be to ask the
severity db what it thinks the severity of a given rule object is. Not
an objection to this patch, but just one of those things that feels
quirky about the division of responsibilities.

> +        self.assertEqual(rank, sev_db.NOT_IMPLEMENTED)
>  
>  
>  setup_all_loops(__name__)
> 

Thanks.

-- 
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/20150605/5b67bca1/attachment.pgp>