Merge lp:~phablet-team/media-hub/add-unity8-exception into lp:media-hub

[Jim Hodapp]

Add apparmor exception to media-hub for unity8

Add apparmor exception to media-hub for unity8

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:170
http://jenkins.qa.ubuntu.com/job/media-hub-ci/392/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-amd64-ci/232
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-armhf-ci/232
        deb: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-armhf-ci/232/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-i386-ci/232

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/media-hub-ci/392/rebuild

review: Approve (continuous-integration)

[Paweł Stołowski (stolowski)]

Thanks for that Jim! There is one problem though, it throws early in Context constructor, because there is no package name set for unity8-dash:

apparmor profile name: unity8-dash, is_unconfined(): false, has_package_name(): false
terminate called after throwing an instance of 'std::logic_error'
  what(): apparmor::ubuntu::Context: Invalid profile name unity8-dash

If this is because unity8-dash doesn't come from a click package, then I think the ctor of Context also needs an exception for unity8-dash (making unity8-dash a .click is not in our plans).
Please let me know if there is any other way for me to make this constraint happy on my side.

[lp:~phablet-team/media-hub/add-unity8-exception]

171. By Jim Hodapp on 2016-02-11

Add an exception for unity to not require a package_name since it will not have one

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:171
http://jenkins.qa.ubuntu.com/job/media-hub-ci/393/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-amd64-ci/233
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-armhf-ci/233
        deb: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-armhf-ci/233/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-i386-ci/233

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/media-hub-ci/393/rebuild

review: Approve (continuous-integration)

[Paweł Stołowski (stolowski)]

Yep, that helped, thanks!

[Paweł Stołowski (stolowski)]

Hi Jim, I think there is one more case - I'm getting this when pressing play button from the confined Dash:

Adding track file:///home/phablet/Music/1.mp3
Failed to add track ' "file:///home/phablet/Music/1.mp3" ' to playlist: mpris.TrackList.Error.InsufficientPermissionsToAddTrack: Warning: Not adding track file:///home/phablet/Music/1.mp3 to TrackList because of inadequate client apparmor permissions.

[lp:~phablet-team/media-hub/add-unity8-exception]

172. By Jim Hodapp on 2016-02-18

Add debugging output

[Jim Hodapp (jhodapp)]

@Pawel: try rebuilding and get me a clean media-hub.log. I added a couple of debug statements that I want to see the output for since I should already have a working exception (but obviously isn't).

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:172
http://jenkins.qa.ubuntu.com/job/media-hub-ci/395/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-amd64-ci/235
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-armhf-ci/235
        deb: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-armhf-ci/235/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-i386-ci/235

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/media-hub-ci/395/rebuild

review: Approve (continuous-integration)

[Paweł Stołowski (stolowski)]

Hi Jim, here you are:

virtual void core::ubuntu::media::TrackListImplementation::reset()
Updating MPRIS TrackList properties; Tracks: 0, has_previous: false, has_next: false
*** void core::ubuntu::media::TrackListSkeleton::Private::handle_add_tracks_with_uri_at(const Ptr&)
apparmor profile name: unity8-dash, is_unconfined(): false, has_package_name(): false
Warning: Not adding track file:///home/phablet/Music/5.mp3 to TrackList because of inadequate client apparmor permissions.
void core::ubuntu::media::PlayerImplementation<Parent>::stop() [with Parent = core::ubuntu::media::PlayerSkeleton]
Current player state is already stopped - no need to change state to stopped
virtual void core::ubuntu::media::TrackListImplementation::reset()
Updating MPRIS TrackList properties; Tracks: 0, has_previous: false, has_next: false
*** void core::ubuntu::media::TrackListSkeleton::Private::handle_add_tracks_with_uri_at(const Ptr&)
apparmor profile name: unity8-dash, is_unconfined(): false, has_package_name(): false
Warning: Not adding track file:///home/phablet/Music/5.mp3 to TrackList because of inadequate client apparmor permissions.
void core::ubuntu::media::PlayerImplementation<Parent>::stop() [with Parent = core::ubuntu::media::PlayerSkeleton]
Current player state is already stopped - no need to change state to stopped
virtual void core::ubuntu::media::TrackListImplementation::reset()
Updating MPRIS TrackList properties; Tracks: 0, has_previous: false, has_next: false
*** void core::ubuntu::media::TrackListSkeleton::Private::handle_add_tracks_with_uri_at(const Ptr&)
apparmor profile name: unity8-dash, is_unconfined(): false, has_package_name(): false
Warning: Not adding track file:///home/phablet/Music/5.mp3 to TrackList because of inadequate client apparmor permissions.

[Paweł Stołowski (stolowski)]

Ok, this one has the debug you added:

Session created by request of: :1.121, key: 2, uuid: 2533cb52-bf0b-44b9-98a2-603563ecfac8, path:/core/ubuntu/media/Service/sessions/2

audio_sink: pulsesink
video_sink: mirsink
Audio stream role: props,media.role=multimedia
apparmor profile name: unity8-dash, is_unconfined(): false, has_package_name(): false
operator()():149 -- app_name='unity8-dash', attached
void core::ubuntu::media::PlayerImplementation<Parent>::stop() [with Parent = core::ubuntu::media::PlayerSkeleton]
bool gstreamer::Playbin::set_state_and_wait(GstState): requested state change.
Setting state for parent: 0xabe37254
virtual bool gstreamer::Engine::stop()
Emiting playback_status_changed signal: PlaybackStatus::stopped
virtual void core::ubuntu::media::TrackListImplementation::reset()
Updating MPRIS TrackList properties; Tracks: 0, has_previous: false, has_next: false
virtual void core::ubuntu::media::TrackListImplementation::reset()
Updating MPRIS TrackList properties; Tracks: 0, has_previous: false, has_next: false
*** void core::ubuntu::media::TrackListSkeleton::Private::handle_add_tracks_with_uri_at(const Ptr&)
apparmor profile name: unity8-dash, is_unconfined(): false, has_package_name(): false
context.profile_name(): -
parsed_uri.path: /home/phablet/Music/1.mp3
Warning: Not adding track file:///home/phablet/Music/1.mp3 to TrackList because of inadequate client apparmor permissions.
void core::ubuntu::media::PlayerImplementation<Parent>::stop() [with Parent = core::ubuntu::media::PlayerSkeleton]
Current player state is already stopped - no need to change state to stopped
virtual void core::ubuntu::media::TrackListImplementation::reset()
Updating MPRIS TrackList properties; Tracks: 0, has_previous: false, has_next: false
*** void core::ubuntu::media::TrackListSkeleton::Private::handle_add_tracks_with_uri_at(const Ptr&)
apparmor profile name: unity8-dash, is_unconfined(): false, has_package_name(): false
context.profile_name(): -
parsed_uri.path: /home/phablet/Music/5.mp3
Warning: Not adding track file:///home/phablet/Music/5.mp3 to TrackList because of inadequate client apparmor permissions.

[lp:~phablet-team/media-hub/add-unity8-exception]

173. By Jim Hodapp on 2016-02-22

Add unity8 exception for calculating the profile_name

[Jim Hodapp (jhodapp)]

I uploaded what should be a fix (untested). Please go ahead and build in your silo and give it a try.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:173
http://jenkins.qa.ubuntu.com/job/media-hub-ci/396/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-amd64-ci/236
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-armhf-ci/236
        deb: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-armhf-ci/236/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-i386-ci/236

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/media-hub-ci/396/rebuild

review: Approve (continuous-integration)

[Paweł Stołowski (stolowski)]

> I uploaded what should be a fix (untested). Please go ahead and build in your
> silo and give it a try.

Yeah, playback works now, thanks!

[Jim Hodapp (jhodapp)]

No problem, glad it's working now! I'll have Alfonso do a quick code review so that we can get this landed when you're ready with your silo.

[lp:~phablet-team/media-hub/add-unity8-exception]

174. By Jim Hodapp on 2016-02-23

Use the const unity_name everywhere

[Paweł Stołowski (stolowski)]

+1 from me, feel free to land it, thanks!

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:174
http://jenkins.qa.ubuntu.com/job/media-hub-ci/397/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-amd64-ci/237
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-armhf-ci/237
        deb: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-armhf-ci/237/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-i386-ci/237

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/media-hub-ci/397/rebuild

review: Approve (continuous-integration)

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

LGTM

review: Approve

[lp:~phablet-team/media-hub/add-unity8-exception]

175. By Jim Hodapp on 2016-02-23

Merged with trunk

=== modified file 'src/core/media/apparmor/ubuntu.cpp'
--- src/core/media/apparmor/ubuntu.cpp	2016-01-15 15:44:04 +0000
+++ src/core/media/apparmor/ubuntu.cpp	2016-02-23 16:29:54 +0000
@@ -72,6 +72,7 @@
 
 static constexpr std::size_t index_package{1};
 static constexpr std::size_t index_app{2};
+static const std::string unity_name{"unity8-dash"};
 
 // Returns true if the context name is a valid Ubuntu app id.
 // If it is, out is populated with the package and app name.
@@ -83,7 +84,7 @@
     static const std::regex full_re{"(.*)_(.*)_(.*)"};
     static const std::regex trust_store_re{"(.*)-(.*)"};
 
-    if (s == "messaging-app"
+    if ((s == "messaging-app" or s == unity_name)
             and std::regex_match(s, out, trust_store_re))
     {
         pkg_name = s;
@@ -103,15 +104,17 @@
 apparmor::ubuntu::Context::Context(const std::string& name)
     : apparmor::Context{name},
       unconfined_{str() == ubuntu::unconfined},
+      unity_{name == unity_name},
       has_package_name_{process_context_name(str(), match_, pkg_name_)}
 {
     std::cout << "apparmor profile name: " << name;
     std::cout << ", is_unconfined(): " << is_unconfined();
     std::cout << ", has_package_name(): " << has_package_name() << std::endl;
-    if (not is_unconfined() && not has_package_name()) throw std::logic_error
-    {
-        "apparmor::ubuntu::Context: Invalid profile name " + str()
-    };
+    if (not is_unconfined() and not is_unity() and not has_package_name())
+        throw std::logic_error
+        {
+            "apparmor::ubuntu::Context: Invalid profile name " + str()
+        };
 }
 
 bool apparmor::ubuntu::Context::is_unconfined() const
@@ -119,6 +122,11 @@
     return unconfined_;
 }
 
+bool apparmor::ubuntu::Context::is_unity() const
+{
+    return unity_;
+}
+
 bool apparmor::ubuntu::Context::has_package_name() const
 {
     return has_package_name_;
@@ -155,6 +163,9 @@
 
     Uri parsed_uri = parse_uri(uri);
 
+    std::cout << "context.profile_name(): " << context.profile_name() << std::endl;
+    std::cout << "parsed_uri.path: " << parsed_uri.path << std::endl;
+
     // All confined apps can access their own files
     if (parsed_uri.path.find(std::string(".local/share/" + context.package_name() + "/")) != std::string::npos ||
         parsed_uri.path.find(std::string(".cache/" + context.package_name() + "/")) != std::string::npos)
@@ -194,7 +205,8 @@
     // Check in ~/Music and ~/Videos
     // TODO: when the trust store lands, check it to see if this app can access the dirs and
     // then remove the explicit whitelist of the music-app, and gallery-app
-    else if ((context.package_name() == "com.ubuntu.music" || context.package_name() == "com.ubuntu.gallery") &&
+    else if ((context.package_name() == "com.ubuntu.music" || context.package_name() == "com.ubuntu.gallery" ||
+              context.profile_name() == unity_name) &&
             (parsed_uri.path.find(std::string("Music/")) != std::string::npos ||
              parsed_uri.path.find(std::string("Videos/")) != std::string::npos ||
              parsed_uri.path.find(std::string("/media")) != std::string::npos))

=== modified file 'src/core/media/apparmor/ubuntu.h'
--- src/core/media/apparmor/ubuntu.h	2016-01-15 15:44:04 +0000
+++ src/core/media/apparmor/ubuntu.h	2016-02-23 16:29:54 +0000
@@ -67,6 +67,9 @@
     // Returns true iff the context is unconfined.
     virtual bool is_unconfined() const;
 
+    // Returns true iff the context matches Unity.
+    virtual bool is_unity() const;
+
     // Returns true iff the context contains a package name.
     virtual bool has_package_name() const;
 
@@ -79,6 +82,7 @@
     std::smatch match_;
     std::string pkg_name_;
     const bool unconfined_;
+    const bool unity_;
     const bool has_package_name_;
 };