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

Revision history for this message
Tyler Hicks (tyhicks) wrote :

* 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/

* 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

* 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

review: Needs Fixing

« Back to merge proposal