[apparmor] [patch] Change abstract methods in BaseRule to use NotImplementedError

Kshitij Gupta kgupta8592 at gmail.com
Thu Oct 29 20:27:12 UTC 2015


Hi,

On Thu, Oct 29, 2015 at 6:09 PM, Christian Boltz <apparmor at cboltz.de> wrote:

> Hello,
>
> as Kshitij mentioned, abstract methods should use NotImplementedError
> instead of AppArmorBug.
>
> While changing this, I noticed that __repr__() needs to be robust against
> NotImplementedError because get_raw() is not available in BaseRule.
> Therefore the patch changes __repr__() to catch NotImplementedError.
>
> Of course the change to NotImplementedError also needs several
> adjustments in the tests.
>
> Thanks for the short turn-around time with this patch and helping make
things uniform and consistent.

>
> [ 19-baserule-use-NotImplementedError.diff ]
>
> === modified file ./utils/apparmor/rule/__init__.py
> --- utils/apparmor/rule/__init__.py     2015-10-29 13:11:10.731878625 +0100
> +++ utils/apparmor/rule/__init__.py     2015-10-29 13:12:50.523041470 +0100
> @@ -52,7 +52,11 @@
>
>      def __repr__(self):
>          classname = self.__class__.__name__
> -        return '<%s> ' % classname + self.get_raw()
> +        try:
> +            raw_content = self.get_raw()  # will fail for BaseRule
> +            return '<%s> %s' % (classname, raw_content)
> +        except NotImplementedError:
> +            return '<%s (NotImplementedError - get_clean() not
> implemented?)>' % classname
>
>      @classmethod
>      def match(cls, raw_rule):
> @@ -69,7 +73,7 @@
>      @classmethod
>      def _match(cls, raw_rule):
>          '''parse raw_rule and return regex match object'''
> -        raise AppArmorBug("'%s' needs to implement _match(), but didn't"
> % (str(cls)))
> +        raise NotImplementedError("'%s' needs to implement _match(), but
> didn't" % (str(cls)))
>
>      @classmethod
>      def parse(cls, raw_rule):
> @@ -83,12 +87,12 @@
>      def _parse(cls, raw_rule):
>          '''returns a Rule object created from parsing the raw rule.
>             required to be implemented by subclasses; raise exception if
> not'''
> -        raise AppArmorBug("'%s' needs to implement _parse(), but didn't"
> % (str(cls)))
> +        raise NotImplementedError("'%s' needs to implement _parse(), but
> didn't" % (str(cls)))
>
>      # @abstractmethod  FIXME - uncomment when python3 only
>      def get_clean(self, depth=0):
>          '''return clean rule (with default formatting, and leading
> whitespace as specified in the depth parameter)'''
> -        raise AppArmorBug("'%s' needs to implement get_clean(), but
> didn't" % (str(self.__class__)))
> +        raise NotImplementedError("'%s' needs to implement get_clean(),
> but didn't" % (str(self.__class__)))
>
>      def get_raw(self, depth=0):
>          '''return raw rule (with original formatting, and leading
> whitespace in the depth parameter)'''
> @@ -121,7 +125,7 @@
>      # @abstractmethod  FIXME - uncomment when python3 only
>      def is_covered_localvars(self, other_rule):
>          '''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)))
> +        raise NotImplementedError("'%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
> @@ -142,7 +146,7 @@
>      # @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)))
> +        raise NotImplementedError("'%s' needs to implement
> is_equal_localvars(), but didn't" % (str(self)))
>
>      def severity(self, sev_db):
>          '''return severity of this rule, which can be:
> @@ -178,7 +182,7 @@
>      def logprof_header_localvars(self):
>          '''return the headers (human-readable version of the rule) to
> display in aa-logprof for this rule object
>             returns {'label1': 'value1', 'label2': 'value2'} '''
> -        raise AppArmorBug("'%s' needs to implement logprof_header(), but
> didn't" % (str(self)))
> +        raise NotImplementedError("'%s' needs to implement
> logprof_header(), but didn't" % (str(self)))
>
>      def modifiers_str(self):
>          '''return the allow/deny and audit keyword as string, including
> whitespace'''
> @@ -336,7 +340,7 @@
>      def get_glob_ext(self, path_or_rule):
>          '''returns the next possible glob with extension (for file rules
> only).
>             For all other rule types, raise an exception'''
> -        raise AppArmorBug("get_glob_ext is not available for this rule
> type!")
> +        raise NotImplementedError("get_glob_ext is not available for this
> rule type!")
>
>
>  def parse_comment(matches):
> === modified file ./utils/test/test-baserule.py
> --- utils/test/test-baserule.py 2015-10-29 13:11:10.802874470 +0100
> +++ utils/test/test-baserule.py 2015-10-29 12:12:19.971584508 +0100
> @@ -20,34 +20,34 @@
>
>  class TestBaserule(AATest):
>      def test_abstract__parse(self):
> -        with self.assertRaises(AppArmorBug):
> +        with self.assertRaises(NotImplementedError):
>              BaseRule._parse('foo')
>
>      def test_abstract__parse_2(self):
> -        with self.assertRaises(AppArmorBug):
> +        with self.assertRaises(NotImplementedError):
>              BaseRule.parse('foo')
>
>      def test_abstract__match(self):
> -        with self.assertRaises(AppArmorBug):
> +        with self.assertRaises(NotImplementedError):
>              BaseRule._match('foo')
>
>      def test_abstract__match2(self):
> -        with self.assertRaises(AppArmorBug):
> +        with self.assertRaises(NotImplementedError):
>              BaseRule.match('foo')
>
>      def test_abstract_get_clean(self):
>          obj = BaseRule()
> -        with self.assertRaises(AppArmorBug):
> +        with self.assertRaises(NotImplementedError):
>              obj.get_clean()
>
>      def test_is_equal_localvars(self):
>          obj = BaseRule()
> -        with self.assertRaises(AppArmorBug):
> +        with self.assertRaises(NotImplementedError):
>              obj.is_equal_localvars(BaseRule())
>
>      def test_is_covered_localvars(self):
>          obj = BaseRule()
> -        with self.assertRaises(AppArmorBug):
> +        with self.assertRaises(NotImplementedError):
>              obj.is_covered_localvars(None)
>
>      def test_parse_modifiers_invalid(self):
> @@ -65,7 +65,7 @@
>
>      def test_logprof_header_localvars(self):
>          obj = BaseRule()
> -        with self.assertRaises(AppArmorBug):
> +        with self.assertRaises(NotImplementedError):
>              obj.logprof_header_localvars()
>
>
> === modified file ./utils/test/test-capability.py
> --- utils/test/test-capability.py       2015-10-29 13:11:10.802874470 +0100
> +++ utils/test/test-capability.py       2015-10-29 12:21:13.288600004 +0100
> @@ -635,7 +635,7 @@
>          self.assertEqual(self.ruleset.get_glob('capability net_raw,'),
> 'capability,')
>
>      def test_glob_ext(self):
> -        with self.assertRaises(AppArmorBug):
> +        with self.assertRaises(NotImplementedError):
>              self.ruleset.get_glob_ext('capability net_raw,')
>
>  class CapabilityDeleteTest(AATest):
> === modified file ./utils/test/test-change_profile.py
> --- utils/test/test-change_profile.py   2015-10-29 13:11:10.803874412 +0100
> +++ utils/test/test-change_profile.py   2015-10-29 12:16:05.183514365 +0100
> @@ -449,7 +449,7 @@
>      #     self.assertEqual(self.ruleset.get_glob('change_profile /foo ->
> /bar,'), 'change_profile -> /bar,')
>
>      def test_glob_ext(self):
> -        with self.assertRaises(AppArmorBug):
> +        with self.assertRaises(NotImplementedError):
>              # get_glob_ext is not available for change_profile rules
>              self.ruleset.get_glob_ext('change_profile /foo -> /bar,')
>
> === modified file ./utils/test/test-network.py
> --- utils/test/test-network.py  2015-10-29 13:11:10.844872012 +0100
> +++ utils/test/test-network.py  2015-10-29 12:19:48.363533637 +0100
> @@ -441,7 +441,7 @@
>      #     self.assertEqual(self.ruleset.get_glob('network inet raw,'),
> 'network inet,')
>
>      def test_glob_ext(self):
> -        with self.assertRaises(AppArmorBug):
> +        with self.assertRaises(NotImplementedError):
>              # get_glob_ext is not available for network rules
>              self.ruleset.get_glob_ext('network inet raw,')
>
> === modified file ./utils/test/test-rlimit.py
> --- utils/test/test-rlimit.py   2015-10-29 13:11:10.844872012 +0100
> +++ utils/test/test-rlimit.py   2015-10-29 12:19:13.128581461 +0100
> @@ -411,7 +411,7 @@
>      #     self.assertEqual(self.ruleset.get_glob('rlimit /foo -> /bar,'),
> 'rlimit -> /bar,')
>
>      def test_glob_ext(self):
> -        with self.assertRaises(AppArmorBug):
> +        with self.assertRaises(NotImplementedError):
>              # get_glob_ext is not available for rlimit rules
>              self.ruleset.get_glob_ext('set rlimit cpu <= 100,')
>
> === modified file ./utils/test/test-signal.py
> --- utils/test/test-signal.py   2015-10-29 13:11:10.844872012 +0100
> +++ utils/test/test-signal.py   2015-10-29 12:20:30.940059830 +0100
> @@ -581,7 +581,7 @@
>      #     self.assertEqual(self.ruleset.get_glob('signal send raw,'),
> 'signal send,')
>
>      def test_glob_ext(self):
> -        with self.assertRaises(AppArmorBug):
> +        with self.assertRaises(NotImplementedError):
>              # get_glob_ext is not available for signal rules
>              self.ruleset.get_glob_ext('signal send set=int,')
>
> Looks good.

Thanks for the patch.

Acked-by: Kshitij Gupta <kgupta8592 at gmail.com>.

>
>
> Regards,
>
> Christian Boltz
> --
> > > Gibt's mehr Gehalt? ;)
> > Du glaubst doch nicht wirklich, dass ich über solche Dinge öffentlich
> > rede, oder?
> Du brauchst ja keine Summe nennen. Es reicht wenn Du uns alle hier mal
> auf eine Pizza einlädst ;-)))
> [>> David Haller, > Philipp Thomas und Thorsten Körner in suse-linux]
>
>
> --
> AppArmor mailing list
> AppArmor at lists.ubuntu.com
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/apparmor
>



-- 
Regards,

Kshitij Gupta
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20151030/c88f645b/attachment-0001.html>


More information about the AppArmor mailing list