Code review comment for lp:~jamesh/mediascanner2/dbus-apparmor

> * AppArmor related review comments that need fixing
> - media-hub limits access for the "com.ubuntu.music" application to
> /home/phablet/{Music,Videos}
> + http://bazaar.launchpad.net/~phablet-team/media-hub/media-hub-
> condensed/view/head:/src/core/media/player_skeleton.cpp#L200
> + MediaScanner should probably do the same thing
> + However, note that mediahub is currently not doing this quite right
> since
> the path should *equal* /home/phablet/Music or /home/phablet/Vidoes,
> rather than just containing the strings Music/ or Videos/

Currently the media scanner is only scanning ~/Music and ~/Videos and external media. Most of the APIs are centred around media types rather than locations though. I'm not sure what the best answer is here.

>
> * AppArmor related review comments that should be fixed but are non-blockers
> - You may want to make the handling of
> GetConnectionAppArmorSecurityContext()
> more robust
> + When AppArmor is disabled in the kernel or in dbus-daemon,
> GetConnectionAppArmorSecurityContext() will return the
> DBUS_ERROR_APPARMOR_SECURITY_CONTEXT_UNKNOWN error
> + In the current merge proposal, this will always result in the client
> being denied access
> + We may want to link against libapparmor to call aa_is_enabled() and, if
> AppArmor is disabled, always grant access before even calling
> GetConnectionAppArmorSecurityContext()
> - does_client_have_access() is very Ubuntu Touch centric
> + Some confined clients may not be Click Apps, so they may not contains
> '_'
> chars in their AppArmor confinement context
> + This is fine for now, but in the future we will want to handle
> applications that are applications that are confined by AppArmor but are
> not Click Apps

I've updated the code to call aa_is_enabled(), and treat peers as unconfined if it returns false. That seems to help get the code through Jenkins (and presumably also Launchpad's builder).

I agree that it would be good to externalise the security policy, but this seems to be the kind of thing media-hub is also doing for now. I'd be happy to change this once we've got an idea of how to do that.

>
> * Minor things that caught my eye
> - The bus variable in get_client_apparmor_context() is unused
> - The MediaType parameter in does_client_have_access() is unused
> + Because of this, the MediaType parameter of check_access is also unused

I've switched the policy over to allowing com.ubuntu.music to access AudioMedia, so the MediaType parameter is now used (I guess I missed this when I got distracted by the Jenkins failure). I've removed the unused bus variable in get_client_apparmor_context().