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
