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 <[email protected]> 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 <[email protected]> http://NxNW.org/~steve/
signature.asc
Description: Digital signature
-- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
