On Thu, May 28, 2015 at 09:51:31PM +0200, Christian Boltz wrote: > Am Mittwoch, 27. Mai 2015 schrieb Steve Beattie: > > This is problematic in the situation where paths begin with an @{} > > variable (e.g. 'change_profile @{MY_PROGRAM} -> my_program_profile,'). > > We can prepare for variables nevertheless ;-) > > For the moment, I added an ... or execcond.startswith('@') > > On the long term, maybe a regex check would be better - but that's > something for a follow-up patch.
Sure, ideally the specifics of the validity of an fs path would be hidden within a function or method call. > Given that there are variables, *, **, ?, [...] and {...} (did I miss > something?), maybe we'll even end up with a separate class for paths > that can be used in various rule types. The parser only accepts path rules that begin with / or @{. But yeah, there may need to be a class for path elements. > > > + else: > > > + raise AppArmorBug('Empty exec condition in > > > change_profile rule') + else: > > > + raise AppArmorBug('Passed unknown object to > > > ChangeProfileRule: %s' % str(execcond)) > > That said, I also agree with Seth's comment, it might be nice to > > restructure the if else clauses like so: > > [...quoting of overlong lines destroyed by KMail...] > > > Otherwise, this is looking okay. > > :-) > > > Here's the updated patch - changes are > - s/network/change_profile/ in a comment > - the if conditions discussed above changed to avoid a nesting level: > > + if execcond == ChangeProfileRule.ALL: > + self.all_execconds = True > + elif type(execcond) == str: > + if not execcond.strip(): > + raise AppArmorBug('Empty exec condition in change_profile > rule') > + elif execcond.startswith('/') or execcond.startswith('@'): > + self.execcond = execcond > + else: > + raise AppArmorException('Exec condition in change_profile > rule does not start with /: %s' % str(execcond)) > + else: > + raise AppArmorBug('Passed unknown object to ChangeProfileRule: > %s' % str(execcond)) > > I know assigning self.execcond is still inside the elif block, but IMHO > that's more readable than lots of 'not' condtions - especially with > two or'ed conditions (which would become 'and not'). Yeah, just trying to not let the raised exceptions get too far away from the tests that triggered them. Thanks. > I can (and probably will) change that when we have some common code or > even a class for handling paths. > > > [ 01-add-ChangeProfileRule.diff ] Acked-by: Steve Beattie <st...@nxnw.org> -- Steve Beattie <sbeat...@ubuntu.com> http://NxNW.org/~steve/
signature.asc
Description: Digital signature
-- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor