cmd/snap: implement userd command as replacement for snapd-xdg-open #3260

Merged
merged 84 commits into from Aug 28, 2017

Conversation

Projects
None yet
9 participants
Contributor

morphis commented May 2, 2017

In https://forum.snapcraft.io/t/integrate-snapd-xdg-open-into-snapd-repository/100 we decided to implement a snap userd command which replaces snapd-xdg-open.

Based on discussion from https://forum.snapcraft.io/t/integrate-snapd-xdg-open-into-snapd-repository/100

daemon/user/daemon.go
+type Daemon struct {
+ tomb tomb.Tomb
+ conn *dbus.Conn
+ ifaces []registeredInterface
@zyga

zyga May 2, 2017

Contributor

The overlord of the word interface is unfortunate.

@morphis

morphis May 2, 2017

Contributor

True, but we deal with dbus interfaces here ..

daemon/user/safelauncher.go
+)
+
+var (
+ allowedUrlSchemes = []string{"http", "https", "mailto"}
@ogra1

ogra1 May 2, 2017

Contributor

we audit this already inside the core snap, before sending the call to the dbus ... i think it would be nice to agree to only do this auditing in one place (either at the start or at the end of the chain) so you do not need to change two places (core snap as well as snapd code) when adding or removing a protocol handler (i dont care which side we drop, but having a single place will prevent errors)

@morphis

morphis May 2, 2017

Contributor

Where are we doing this? The xdg-open script in the core snap has only:

$ cat /snap/core/current/usr/local/bin/xdg-open                
#!/bin/sh
dbus-send --print-reply --session --dest=com.canonical.SafeLauncher / com.canonical.SafeLauncher.OpenURL string:"$1"
@ogra1

ogra1 May 3, 2017

Contributor

in the mimetype handler of the .desktop file we install inside core: https://github.com/snapcore/core/blob/master/live-build/hooks/500-create-xdg-wrapper.binary#L23
and in the mimeapps.list we implement:
https://github.com/snapcore/core/blob/master/live-build/hooks/500-create-xdg-wrapper.binary#L28
protocols not defined in there will simply be ignored. as i said above i'm not opposed to drop this if it doesnt break things, i'd just like if we have only one place defining it.

@zyga

zyga May 8, 2017

Contributor

Anything inside the core snap is untrusted as it can be altered by the attacker. This filtering really does belond on this side.

@@ -112,3 +112,12 @@ Section: oldlibs
Pre-Depends: dpkg (>= 1.15.7.2)
Description: Transitional package for snapd
This is a transitional dummy package. It can safely be removed.
+
+Package: snapd-xdg-open
@ogra1

ogra1 May 2, 2017

Contributor

keeping this as separate package will break for users using only "apt-get upgrade" (vs dist-upgrade) if snapd gets a dependency on it.
...it was one of the main reasons to actually start discussing to move snapd-xdg-open into snapd in the beginning to actually get it into distros that have not seeded it (like elementary) without introducing a package dependency.

@ogra1

ogra1 May 2, 2017

Contributor

similar to snap-confine btw ...

@morphis

morphis May 2, 2017

Contributor

It will stay a separate package but produced by the snapd source package so we can migrate people away from the other snapd-xdg-open deb package. That was the idea. Didn't tested yet this works well.

@ogra1

ogra1 May 3, 2017

Contributor

well, but that means you have to either add a recommends (in which case the package will never be installed for users only doing "apt-get upgrade") or you add a hard dependency in which case users of "apt-get upgrade" will get an upgrade error. we have tons of bugs for that case from snap-confine which was one of the reasons to include it without a separate package, keeping a separate binary package looks to me like we are exactly re-introducing the same problem again just with another package now.

@morphis

morphis May 3, 2017

Contributor

We need one or another way to get the snapd-xdg-open package removed from installations out there. If we have a binary package produced by two different source packages but the second one has a higher version as the first one isn't then the one from the first source package overwritten?

The Recommends is for sure missing. Any other ways how we can achieve this?

@ogra1

ogra1 May 3, 2017

Contributor

yes, include the binaries in snapd and have the proper breaks/replaces lines (and perhaps a provides) in the snapd package.

@morphis

morphis May 3, 2017

Contributor

Ok, including the binaries in the snapd package was the plan. Will add the Breaks/Replaces line.

@morphis

morphis May 4, 2017

Contributor

Done.

@zyga

zyga May 15, 2017

Contributor

CC @mvo5 - I think we just want to have this in the main snapd package and have snapd-xdg-open as an empty transitional package.

@morphis

morphis May 15, 2017

Contributor

@zyga that is what we already do here. But leaving this to @mvo5 to approve.

@morphis morphis changed the title from WIP: Implement --user instance for snapd to daemon: implement --user instance for snapd May 4, 2017

Contributor

zyga commented May 8, 2017

Personally, given the scope of this branch, I'd like to re-target a smaller subset. Let's start with the basic mechanics (user session daemon) even if it does nothing. Then follow up with more things.

Some more comments.

data/upstart/Makefile
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+LIBEXECDIR := /usr/lib
@zyga

zyga May 8, 2017

Contributor

I think this should come from the outside. Ideally we'd have one makefile for everything in data/

@zyga

zyga May 15, 2017

Contributor

My point is that := is stronger than ?=

@morphis

morphis May 15, 2017

Contributor

Got it, will change to ?=

@morphis

morphis May 29, 2017

Contributor

Done.

packaging/ubuntu-14.04/rules
@@ -103,6 +103,8 @@ override_dh_auto_build:
cd cmd && ( autoreconf -i -f )
cd cmd && ( ./configure --prefix=/usr --libexecdir=/usr/lib/snapd $(VENDOR_ARGS))
$(MAKE) -C cmd all
+ # Build any available upstart configurations
+ $(MAKE) -C data/upstart all
@zyga

zyga May 8, 2017

Contributor

We could convey things like LIBEXECDIR and anything else through make variables.

@morphis

morphis May 8, 2017

Contributor

Aren't we doing that already? We just take the defaults here as we do similar for the systemd build step. This works similar like with default values for configure scripts.

@zyga

zyga May 8, 2017

Contributor

Well, not with LIBEXECDIR := /usr/lib that you can see in data/upstart/Makefile

@morphis

morphis May 9, 2017

Contributor

Isn't that already a variable set in the Makefile which can be optionally overridden by the caller?

packaging/ubuntu-14.04/rules
@@ -142,6 +144,10 @@ override_dh_install:
# and now the normal install rules
install --mode=0644 debian/snapd.system-shutdown.service debian/snapd/$(SYSTEMD_UNITS_DESTDIR)
$(MAKE) -C cmd install DESTDIR=$(CURDIR)/debian/tmp
+
+ # install any available upstart jobs
+ $(MAKE) -C data/upstart install DESTDIR=$(CURDIR)/debian/snapd/
@zyga

zyga May 8, 2017

Contributor

If you go with one makefile you could just roll that into the existing make install on data.

@morphis

morphis May 8, 2017

Contributor

We can do a single makefile but this would make it a bit harder for people to skip the installation of the upstart files.

Some comments, good luck! :-)

cmd/snapd/main.go
- if err := run(); err != nil {
+
+ parser := flags.NewParser(&opts, flags.HelpFlag|flags.PassDoubleDash|flags.PassAfterNonOption)
+ _, err := parser.ParseArgs(os.Args[1:])
@zyga

zyga May 15, 2017

Contributor

You can combine this into one line:

if _, err := parser.ParseArgs(os.Args[1:]); err != nil {
...
}
@morphis

morphis May 15, 2017

Contributor

Can combine that. The code was exactly this way before.

cmd/snapd/main.go
+ if opts.User {
+ d, err = user.NewDaemon()
+ } else {
+ httputil.SetUserAgentFromVersion(cmd.Version)
@zyga

zyga May 15, 2017

Contributor

Can we call SetUserAgentFromVersion unconditionally? I suspect we can and it would not hurt in any way.

@morphis

morphis May 15, 2017

Contributor

It was done this way before so I guess it's ok.

daemon/user/daemon.go
+import (
+ "bytes"
+
+ "github.com/godbus/dbus"
@zyga

zyga May 15, 2017

Contributor

Good luck a packaging that everywhere ^_^

Quick question: how many dependencies does godbus itself pull in?

@morphis

morphis May 15, 2017

Contributor

I only had to add godbus itself to our vendoring setup, no more. And the good thing is, it is already on Fedora, the only distro where this would hurt.

@zyga

zyga May 16, 2017

Contributor

What about Debian?

@morphis

morphis May 16, 2017

Contributor

Somebody would have to submit a package but our story for debian is basically that we rely on re-execution to work. For unstable and next testing this would be a thing somebody has to look into but I guess that will be a natural thing once we task somebody to package a more recent snapd version for debian.

@zyga

zyga May 16, 2017

Contributor

I think this somebody is you and me We should pick this up in anticipating of this thing landing.

@morphis

morphis May 16, 2017

Contributor

I've talked with Michael and Steve in the past and they weren't much interested as re-execution is supposed to handle this and we can't really update snapd in stable or testing. So unstable is our only possible landing bed. You're a debian developer? If you're I would prefer you do this as I am not one.

@zyga

zyga Aug 23, 2017

Contributor

I'm a DM, in either case we must package this for Sid.

daemon/user/daemon.go
+ "fmt"
+
+ "github.com/godbus/dbus/introspect"
+ tomb "gopkg.in/tomb.v2"
@zyga

zyga May 15, 2017

Contributor

I think you can skip tomb as it will be imported like that anyway.

@morphis

morphis May 15, 2017

Contributor

Will try.

@morphis

morphis May 29, 2017

Contributor

Done.

daemon/user/daemon.go
+)
+
+const (
+ busName = "com.canonical.SafeLauncher"
@zyga

zyga May 15, 2017

Contributor

Nitpick: I know this is what we currently implement but I'd like us to listen on both io.snapcraft.SomethingNice and com.canonical.SafeLauncher (the second one can be deprecated later)

@morphis

morphis May 15, 2017

Contributor

I would not like to open another gate here with the upcoming xdg portal working which will introduce a very similar API. Lets keep it to what it is today and then refine / rework in a follow up PR.

daemon/user/daemon.go
+
+// Stop shuts down the Daemon
+func (d *Daemon) Stop() error {
+ d.tomb.Kill(nil)
@zyga

zyga May 15, 2017

Contributor

I'm not versed in the tomb semantics to review this. @pedronis perhaps?

@morphis

morphis May 29, 2017

Contributor

@pedronis any comments from your side?

daemon/user/daemon_test.go
+ c.Assert(d, NotNil)
+
+ d.Start()
+ time.Sleep(time.Second * 1)
@zyga

zyga May 15, 2017

Contributor

See below

daemon/user/daemon_test.go
+ c.Assert(conn.requestNameData.name, Equals, "com.canonical.SafeLauncher")
+ c.Assert(conn.requestNameData.flags, Equals, dbus.NameFlagDoNotQueue)
+
+ c.Assert(len(conn.exportData), Equals, 2)
@zyga

zyga May 15, 2017

Contributor

You may use DeepEquals and pass full objects as arguments.

@morphis

morphis May 29, 2017

Contributor

Done.

daemon/user/daemon_test.go
+ c.Assert(conn.requestNameData.name, Equals, "com.canonical.SafeLauncher")
+ c.Assert(conn.requestNameData.flags, Equals, dbus.NameFlagDoNotQueue)
+
+ c.Assert(len(conn.exportData), Equals, 2)
@zyga

zyga May 15, 2017

Contributor

You may use DeepEquals and pass full objects as arguments.

@morphis

morphis May 29, 2017

Contributor

Done.

daemon/user/daemon_test.go
+ c.Assert(d, NotNil)
+
+ d.Start()
+ time.Sleep(time.Second * 1)
@zyga

zyga May 15, 2017

Contributor

See below

daemon/user/daemon_test.go
+ c.Assert(d, NotNil)
+
+ d.Start()
+ time.Sleep(time.Second * 1)
@zyga

zyga May 15, 2017

Contributor

I think we can do better than sleeping. Perhaps add a notification channel/callback to notify when we're ready?

@zyga

zyga Aug 23, 2017

Contributor

@morphis can you please comment on this.

daemon/user/daemon_test.go
+ c.Assert(d, NotNil)
+
+ d.Start()
+ time.Sleep(time.Second * 1)
@zyga

zyga May 15, 2017

Contributor

See above

daemon/user/export_test.go
+package user
+
+func MockSessionBus(conn DBusConnection, err error) {
+ connectSessionBus = func() (DBusConnection, error) {
@zyga

zyga May 15, 2017

Contributor

Typically such functions return a restore func() that undoes the effect.

@morphis

morphis May 29, 2017

Contributor

Done

daemon/user/safelauncher.go
+ "github.com/godbus/dbus"
+)
+
+const (
@zyga

zyga May 15, 2017

Contributor

In cases like that just do:

const safeLauncherIntrospectionXML = `
...
`
@morphis

morphis May 29, 2017

Contributor

Done

daemon/user/safelauncher.go
+ allowedURLSchemes = []string{"http", "https", "mailto"}
+)
+
+// SafeLauncher implements the 'com.canonical.SafeLauncher' interface
@zyga

zyga May 15, 2017

Contributor

Can you please always disambiguate DBus interface vs snapd interface?

daemon/user/safelauncher.go
+ if !validScheme {
+ return &dbus.Error{
+ Name: "org.freedesktop.DBus.Error.AccessDenied",
+ Body: []interface{}{fmt.Sprintf("Supplied URL scheme '%s' is not allowed", u.Scheme)},
@zyga

zyga May 15, 2017

Contributor

Please use %q, as '%s' does not quote properly.

data/Makefile
@@ -0,0 +1,11 @@
+all:
@zyga

zyga May 15, 2017

Contributor

Please use $(MAKE) instead of make below.

@morphis

morphis May 29, 2017

Contributor

Done

data/Makefile
+ make -C systemd
+ make -C upstart
+
+install:
@zyga

zyga May 15, 2017

Contributor

You can combine all three rules:

all install clean:
    $(MAKE) -C systemd $@
    $(MAKE) -C upstart $@
@morphis

morphis May 29, 2017

Contributor

Done

data/systemd/Makefile
SYSTEMD_UNITS_GENERATED := $(patsubst %.service.in,%.service,$(wildcard *.service.in))
-SYSTEMD_UNITS := ${SYSTEMD_UNITS_GENERATED} snapd.refresh.timer snapd.socket
+SYSTEMD_UNITS := snapd.autoimport.service snapd.refresh.service snapd.refresh.timer snapd.socket snapd.service snapd.system-shutdown.service
+SYSTEMD_USER_UNITS := snapd-user.service
@zyga

zyga May 15, 2017

Contributor

Do we need a new FESCO request to allow us to use a user service?

@morphis

morphis May 15, 2017

Contributor

Maybe, didn't checked with Neal yet.

data/systemd/snapd-user.service.in
@@ -0,0 +1,10 @@
+[Unit]
@zyga

zyga May 15, 2017

Contributor

I bet I'm missing something but isn't it the case that user services have this funky syntax including @ in the service name?

@morphis

morphis May 15, 2017

Contributor

They don't have to. @ basically allows to pass arguments to the systemd unit which we don't need in this case as systemd will ensure we're started as the right user and in the right scope.

@zyga

zyga Aug 23, 2017

Contributor

Aha, I see, thank you!

data/upstart/Makefile
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+LIBEXECDIR := /usr/lib
@zyga

zyga May 8, 2017

Contributor

I think this should come from the outside. Ideally we'd have one makefile for everything in data/

@zyga

zyga May 15, 2017

Contributor

My point is that := is stronger than ?=

@morphis

morphis May 15, 2017

Contributor

Got it, will change to ?=

@morphis

morphis May 29, 2017

Contributor

Done.

data/upstart/Makefile
+UPSTART_SERVICES := $(patsubst %.conf.in,%.conf,$(wildcard *.conf.in))
+
+%.conf: %.conf.in
+ cat $< | \
@zyga

zyga May 15, 2017

Contributor

Nitpick, this fits on one line nicely:

@morphis

morphis May 29, 2017

Contributor

Done

data/upstart/Makefile
+
+%.conf: %.conf.in
+ cat $< | \
+ sed s:@libexecdir@:${LIBEXECDIR}:g | \
@zyga

zyga May 15, 2017

Contributor

Please use single quote around this

@morphis

morphis May 15, 2017

Contributor

Will do

@@ -112,3 +112,12 @@ Section: oldlibs
Pre-Depends: dpkg (>= 1.15.7.2)
Description: Transitional package for snapd
This is a transitional dummy package. It can safely be removed.
+
+Package: snapd-xdg-open
@ogra1

ogra1 May 2, 2017

Contributor

keeping this as separate package will break for users using only "apt-get upgrade" (vs dist-upgrade) if snapd gets a dependency on it.
...it was one of the main reasons to actually start discussing to move snapd-xdg-open into snapd in the beginning to actually get it into distros that have not seeded it (like elementary) without introducing a package dependency.

@ogra1

ogra1 May 2, 2017

Contributor

similar to snap-confine btw ...

@morphis

morphis May 2, 2017

Contributor

It will stay a separate package but produced by the snapd source package so we can migrate people away from the other snapd-xdg-open deb package. That was the idea. Didn't tested yet this works well.

@ogra1

ogra1 May 3, 2017

Contributor

well, but that means you have to either add a recommends (in which case the package will never be installed for users only doing "apt-get upgrade") or you add a hard dependency in which case users of "apt-get upgrade" will get an upgrade error. we have tons of bugs for that case from snap-confine which was one of the reasons to include it without a separate package, keeping a separate binary package looks to me like we are exactly re-introducing the same problem again just with another package now.

@morphis

morphis May 3, 2017

Contributor

We need one or another way to get the snapd-xdg-open package removed from installations out there. If we have a binary package produced by two different source packages but the second one has a higher version as the first one isn't then the one from the first source package overwritten?

The Recommends is for sure missing. Any other ways how we can achieve this?

@ogra1

ogra1 May 3, 2017

Contributor

yes, include the binaries in snapd and have the proper breaks/replaces lines (and perhaps a provides) in the snapd package.

@morphis

morphis May 3, 2017

Contributor

Ok, including the binaries in the snapd package was the plan. Will add the Breaks/Replaces line.

@morphis

morphis May 4, 2017

Contributor

Done.

@zyga

zyga May 15, 2017

Contributor

CC @mvo5 - I think we just want to have this in the main snapd package and have snapd-xdg-open as an empty transitional package.

@morphis

morphis May 15, 2017

Contributor

@zyga that is what we already do here. But leaving this to @mvo5 to approve.

Simon Fels added some commits May 2, 2017

Contributor

morphis commented May 29, 2017

@mvo5 @ogra1 @zyga @niemeyer Can you guys have another look at this?

Simon Fels added some commits May 29, 2017

The general feeling I have of the whole PR is that there's quite a bit of future thinking and perhaps premature abstraction into what is actually a simple change we need to do right now.

Another feeling that comes from reading the code is that this is not really about snapd. This is abstracting away the snapd command and its daemon interface, but the actual sharing of code and functionality with snapd itself is very close to zero. Instead, this is really a client to snapd, and I expect it will soon need to call into the actual snapd, using its sockets, etc. As such, it'd be closer to the "snap" command. Perhaps we should have "snap userd" or similar?

Happy to discuss this live when you have a moment.

I'd also go straight to the point instead of trying to organize and abstract away what is actually a pretty small amount of code. For example, how simple would we be able to make such a userd command inside the current snap command infrastructure?

Then, as a last side-note, let's please take this chance to rename the interface to io.snapcraft.SafeLauncher or similar.

Contributor

morphis commented May 30, 2017

Happy to discuss this live when you have a moment.

Lets hang on after the SU today.

tests/main/snapd-user/task.yaml
+ echo $! > /tmp/snapd-user-pid
+
+ # Give snapd --user a moment to start everything
+ sleep 1
@morphis

morphis May 30, 2017

Contributor

Let me rework that.

Contributor

morphis commented May 30, 2017

Agreed with @niemeyer that I will move the daemon into snap userd

@diddledan diddledan referenced this pull request in baedert/corebird Jun 2, 2017

Open

Add snap package instructions to documentation #732

Simon Fels added some commits Jun 2, 2017

codecov-io commented Jun 19, 2017

Codecov Report

Merging #3260 into master will decrease coverage by 0.2%.
The diff coverage is 17.24%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3260      +/-   ##
==========================================
- Coverage   75.86%   75.65%   -0.21%     
==========================================
  Files         403      407       +4     
  Lines       34835    35020     +185     
==========================================
+ Hits        26427    26494      +67     
- Misses       6535     6647     +112     
- Partials     1873     1879       +6
Impacted Files Coverage Δ
interfaces/builtin/unity7.go 67.85% <ø> (ø) ⬆️
userd/userd.go 0% <0%> (ø)
testutil/dbustest.go 0% <0%> (ø)
userd/launcher.go 0% <0%> (ø)
cmd/snap/cmd_userd.go 83.33% <83.33%> (ø)
cmd/snap/cmd_aliases.go 93.33% <0%> (-1.67%) ⬇️
daemon/api.go 72.45% <0%> (-0.18%) ⬇️
cmd/snap/cmd_snap_op.go 63.63% <0%> (-0.11%) ⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3aa2420...278ac67. Read the comment docs.

@morphis morphis changed the title from daemon: implement --user instance for snapd to cmd/snap: implement userd command as replacement for snapd-xdg-open Jun 19, 2017

Simon Fels added some commits Jun 19, 2017

Member

chipaca commented Jul 10, 2017

The spread test failures are germane.
What's the status of this PR?

@chipaca chipaca added the Decaying label Jul 10, 2017

Contributor

morphis commented Jul 10, 2017

@chipaca I know, systemd-run doesn't exist on 14.04 so I am adding upstart variant currently.

Contributor

morphis commented Jul 11, 2017

@niemeyer @chipaca @zyga This is ready for another review round. If the spread test still fails for 14.04 ignore that for the moment, I am working on getting it to pass.

Simon Fels and others added some commits Aug 1, 2017

A few comments, I think this is very close now. Let's please iterate on this.

cmd/snap/cmd_userd.go
+ }
+
+ if reply != dbus.RequestNameReplyPrimaryOwner {
+ err = fmt.Errorf("Failed to request bus name '%s'", busName)
@zyga

zyga Aug 8, 2017

Contributor

Perhaps fmt.Errorf("cannot obtain bus name %q", busName)

@@ -0,0 +1,3 @@
+all install clean:
+ $(MAKE) -C systemd $@
@zyga

zyga Aug 8, 2017

Contributor

Thank you for using $(MAKE)

data/dbus/Makefile
+SERVICES := ${SERVICES_GENERATED}
+
+%.service: %.service.in
+ cat $< | sed s:@bindir@:${BINDIR}:g | cat > $@
@zyga

zyga Aug 1, 2017

Contributor

Why not just sed < > ?

@zyga

zyga Aug 1, 2017

Contributor

Please quote the argument with single quotes.

@morphis

morphis Aug 1, 2017

Contributor

@Sed This is using the same as in data/systemd/Makefile as @mvo5 initially suggested :-)

Let me do the quoting.

@morphis

morphis Aug 1, 2017

Contributor

You mean quoting all of s:...:g? That wont work as it wont replace ${BINDIR}

@zyga

zyga Aug 8, 2017

Contributor

It will because it is make that is doing the string expansion, not sh.

@mvo5

mvo5 Aug 23, 2017

Collaborator

Thanks, added the quoting, fwiw, while sed < > works I like the cat better because the shell < and the make $< in one line make my eyes twitch ;)

dbus/safelauncher.go
+ allowedURLSchemes = []string{"http", "https", "mailto"}
+)
+
+// SafeLauncher implements the 'io.snapcraft.SafeLauncher' DBus interface
@zyga

zyga Aug 8, 2017

Contributor

Nitpick: it would be nice if those would end with a dot like proper sentence.

dbus/safelauncher.go
+ if !strutil.ListContains(allowedURLSchemes, u.Scheme) {
+ return &dbus.Error{
+ Name: "org.freedesktop.DBus.Error.AccessDenied",
+ Body: []interface{}{fmt.Sprintf("Supplied URL scheme %q is not allowed", u.Scheme)},
@zyga

zyga Aug 8, 2017

Contributor

I think this could be wrapped in a nice helper.

dbus/safelauncher.go
+ }
+
+ if err = xdgOpenCommand(addr); err != nil {
+ return dbus.MakeFailedError(fmt.Errorf("Can not open supplied URL"))
@zyga

zyga Aug 8, 2017

Contributor

cannot open supplied URL

dbus/safelauncher_test.go
+
+ "github.com/snapcore/snapd/dbus"
+
+ "fmt"
@zyga

zyga Aug 8, 2017

Contributor

"fmt" belongs upstairs with "testing"

dbus/safelauncher_test.go
+
+ "fmt"
+
+ . "gopkg.in/check.v1"
@zyga

zyga Aug 8, 2017

Contributor

I think this should be swapped with the snapd import above. Maybe my memory is skewed towards python experience but imports are (stdlib, 3rd party, this project)

dbus/safelauncher_test.go
+func (s *safeLauncherSuite) TestOpenURLWithNotAllowedScheme(c *C) {
+ err := s.launcher.OpenURL("tel://049112233445566")
+ c.Assert(err, NotNil)
+ c.Assert(err, ErrorMatches, "Supplied URL scheme \"tel\" is not allowed")
@zyga

zyga Aug 8, 2017

Contributor

I think you can skip the NotNil test since ErrorMatches already expresses it.

dbus/safelauncher_test.go
+
+ err = s.launcher.OpenURL("aabbccdd0011")
+ c.Assert(err, NotNil)
+ c.Assert(err, ErrorMatches, "Supplied URL scheme \"\" is not allowed")
@zyga

zyga Aug 8, 2017

Contributor

Same

dbus/safelauncher_test.go
+ c.Assert(s.args, IsNil)
+}
+
+func (s *safeLauncherSuite) TestOpenURLWithAllowedSchemeHTTP(c *C) {
@zyga

zyga Aug 8, 2017

Contributor

The next few tests could be pulled into one table-driven test.

dbus/safelauncher_test.go
+ })
+ err := s.launcher.OpenURL("https://snapcraft.io")
+ c.Assert(err, NotNil)
+ c.Assert(err, ErrorMatches, "Can not open supplied URL")
@zyga

zyga Aug 8, 2017

Contributor

This should be updated along with the earlier comment about Can not

interfaces/builtin/unity7.go
+ path=/io/snapcraft/SafeLauncher
+ interface=io.snapcraft.SafeLauncher
+ member=OpenURL
+ peer=(label=unconfined),
@zyga

zyga Aug 8, 2017

Contributor

I would love if we confined the safe launcher eventually :-)

@@ -64,8 +64,8 @@ Depends: adduser,
systemd,
${misc:Depends},
${shlibs:Depends}
-Replaces: ubuntu-snappy (<< 1.9), ubuntu-snappy-cli (<< 1.9), snap-confine (<< 2.23), ubuntu-core-launcher (<< 2.22)
-Breaks: ubuntu-snappy (<< 1.9), ubuntu-snappy-cli (<< 1.9), snap-confine (<< 2.23), ubuntu-core-launcher (<< 2.22)
+Replaces: ubuntu-snappy (<< 1.9), ubuntu-snappy-cli (<< 1.9), snap-confine (<< 2.23), ubuntu-core-launcher (<< 2.22), snapd-xdg-open (<< 0.0.0)
@zyga

zyga Aug 8, 2017

Contributor

Finally, thank you again for doing this :-)

@Conan-Kudo

Conan-Kudo Aug 25, 2017

Contributor

That is an insane number of Breaks/Replaces directives!

+ # here instead of debian/snapd.install because the
+ # ubuntu/14.04 release branch adds/changes bits here
+ $(MAKE) -C data install DESTDIR=$(CURDIR)/debian/snapd/ \
+ SYSTEMDSYSTEMUNITDIR=$(SYSTEMD_UNITS_DESTDIR)
@zyga

zyga Aug 8, 2017

Contributor

I wish we had one spelling for this variable.

+
+systems:
+ # Not supposed to work on Ubuntu Core systems as we don't have
+ # a user session environment there
@zyga

zyga Aug 8, 2017

Contributor

I think this will change in time but this is fine for now.

+ distro_install_package dbus-x11 xdg-utils
+
+ dbus-launch > dbus.env
+ export $(cat dbus.env | xargs)
@zyga

zyga Aug 8, 2017

Contributor

Hmm, what? I admit I don't get the xargs part.

@mvo5

mvo5 Aug 23, 2017

Collaborator

This just moves the "foo=bar\nbar=bar" to "foo=bar bar=baz", i.e. removes the newlines

tests/main/snap-userd/task.yaml
+
+ # Wait until the service is up and running
+ while ! systemctl is-active test-snap-userd.scope ; do
+ sleep .1
@zyga

zyga Aug 8, 2017

Contributor

We could use the https://dbus.freedesktop.org/doc/dbus-java/api/org/freedesktop/DBus.Peer.html#Ping() method to wait properly. I'd rather not do a sleep here as we have plenty of races already.

@mvo5

mvo5 Aug 23, 2017

Collaborator

The ping seems to be java specific, but I did something in a similar spirit in the branch now.

@zyga

zyga Aug 25, 2017

Contributor

The ping method is not specific to Java: https://dbus.freedesktop.org/doc/dbus-specification.html

On receipt of the METHOD_CALL message org.freedesktop.DBus.Peer.Ping, an application should do nothing other than reply with a METHOD_RETURN as usual. It does not matter which object path a ping is sent to. The reference implementation handles this method automatically.

@Ads20000 Ads20000 referenced this pull request in snapcrafters/discord Aug 19, 2017

Closed

Cannot open link #1

@mvo5 mvo5 added this to the 2.28 milestone Aug 23, 2017

mvo5 added some commits Aug 23, 2017

Use testutil.MockCommand() in safelauncher_test.go
Also write the tests a little more compact.

mvo5 approved these changes Aug 23, 2017

Thanks a lot for doing this work! I like this PR a lot. I took the liberty to fix some of the nitpicks I had.

cmd/snap/cmd_userd.go
+ "github.com/snapcore/snapd/i18n"
+ "github.com/snapcore/snapd/logger"
+
+ "github.com/godbus/dbus"
@mvo5

mvo5 Aug 23, 2017

Collaborator

(nitpick) The convention is usually 1. go lib imports, 2. external libraries 3. snapd stuff. I'll fix that.

cmd/snap/cmd_userd.go
+
+var connectSessionBus = connectSessionBusImpl
+
+type registeredDBusInterface interface {
@mvo5

mvo5 Aug 23, 2017

Collaborator

Nice!

cmd/snap/cmd_userd.go
+ x.tomb.Kill(nil)
+ if x.conn != nil {
+ x.conn.Close()
+
@mvo5

mvo5 Aug 23, 2017

Collaborator

(nitpick) extra newline.

data/dbus/Makefile
+SERVICES := ${SERVICES_GENERATED}
+
+%.service: %.service.in
+ cat $< | sed s:@bindir@:${BINDIR}:g | cat > $@
@zyga

zyga Aug 1, 2017

Contributor

Why not just sed < > ?

@zyga

zyga Aug 1, 2017

Contributor

Please quote the argument with single quotes.

@morphis

morphis Aug 1, 2017

Contributor

@Sed This is using the same as in data/systemd/Makefile as @mvo5 initially suggested :-)

Let me do the quoting.

@morphis

morphis Aug 1, 2017

Contributor

You mean quoting all of s:...:g? That wont work as it wont replace ${BINDIR}

@zyga

zyga Aug 8, 2017

Contributor

It will because it is make that is doing the string expansion, not sh.

@mvo5

mvo5 Aug 23, 2017

Collaborator

Thanks, added the quoting, fwiw, while sed < > works I like the cat better because the shell < and the make $< in one line make my eyes twitch ;)

dbus/export_test.go
+
+package dbus
+
+func MockXdgOpenCommand(cmd func(args ...string) error) {
@mvo5

mvo5 Aug 23, 2017

Collaborator

A restore functions is missing here, this is the idiomatic way we do this kind of mocking.

packaging/ubuntu-16.04/rules
@@ -119,8 +119,8 @@ override_dh_auto_build:
cd cmd && ( ./configure --prefix=/usr --libexecdir=/usr/lib/snapd $(VENDOR_ARGS))
$(MAKE) -C cmd all
- # Generate the real systemd units out of the available templates
- $(MAKE) -C data/systemd all
+ # Generate the real systemd/upstart config files
@mvo5

mvo5 Aug 23, 2017

Collaborator

lol@upstart - I assume you mean dbus here?

@zyga

zyga Aug 23, 2017

Contributor

No, we need both the upstart (for 16.04 session) and systemd config files here.

@mvo5

mvo5 Aug 23, 2017

Collaborator

@zyga but we don't generate any upstart files, do we? We generate a dbus service file that will auto-start. Or am I missing something?

Collaborator

mvo5 commented Aug 23, 2017

One more concern - cmd_userd.go lacks unit tests, I will look/think about this next.

cmd/snap/cmd_userd.go
+
+ // Listen to keep our thread up and running. All DBus bits
+ // are running in the background
+ select {}
@zyga

zyga Aug 23, 2017

Contributor

Do we want to / need to do something to exit cleanly on a signal? Does it behave correctly when the session is shutting down?

@mvo5

mvo5 Aug 23, 2017

Collaborator

Nice catch, yes we do.

Some comments need responses (mainly about tests and sleeping). I'd like to get a confirmation from somebody that they actually tested this on Ubuntu Artful.

I'll review again when pinged.

mvo5 added some commits Aug 23, 2017

A few trivials and one relevant question. Probably the last pass.

cmd/snap/cmd_userd.go
+ dbusIfaces []registeredDBusInterface
+}
+
+var shortUserdHelp = i18n.G("Start the snap userd service")
@niemeyer

niemeyer Aug 23, 2017

Contributor

s/snap//

cmd/snap/cmd_userd.go
+}
+
+var shortUserdHelp = i18n.G("Start the snap userd service")
+var longUserdHelp = i18n.G("The userd command starts the snap user session service.")
@niemeyer

niemeyer Aug 23, 2017

Contributor

s/snap//; this is really the user session service. The fact it's about snaps is implied in the context. The association in the sentence is also slightly unclear ("snap user session service" are four nouns in a row, which can be mentally aggregated in different ways.. e.g. "snap user"). Just removing the "snap" word here as well should be enough.

dbus/safelauncher.go
+ *
+ */
+
+package dbus
@niemeyer

niemeyer Aug 23, 2017

Contributor

This should be "userd" I think. Besides being inconvenient and confusing to have two packages named dbus that need to be used next to each other, the content here is really about a particular package leveraging dbus rather than being a door into dbus.

dbus/safelauncher.go
+)
+
+const safeLauncherIntrospectionXML = `
+<interface name='io.snapcraft.SafeLauncher'>
@niemeyer

niemeyer Aug 23, 2017

Contributor

Can we please rename this type to Launcher and the file to launcher.go. Originally it was important to have Safe in here because it was a general namespace. The safety implications are now implied with this being under snapcraft.io, in that safety is a concern across all such external interactions, so we don't need to call it out explicitly.

We may also end up putting this behind an interface some day, so the name would also be less right in that case.

dbus/safelauncher.go
+ return makeAccessDeniedError(fmt.Errorf("Supplied URL scheme %q is not allowed", u.Scheme))
+ }
+
+ if err = exec.Command("xdg-open", addr).Run(); err != nil {
@niemeyer

niemeyer Aug 23, 2017

Contributor

I'm slightly concerned here because the real xdg-open is a shell application with a pretty extensive and hard to review implementation.

Isn't there a standard dbus service these days to take such URLs?

@chipaca

chipaca Aug 24, 2017

Member

if there was, this whole chunk of work wouldn't've been necessary :-)

@morphis

morphis Aug 24, 2017

Contributor

The xdg-desktop-portal work brings something like this, yes, but that isn't an option cross-distro. Maybe in a few years when everyone has settled on xdg-desktop-portal but as long as that doesn't happen we have to live with xdg-open. I don't see a big problem with xdg-open, it's in main (so comes with security updates from our side), an official upstream project (https://cgit.freedesktop.org/xdg/xdg-utils/) etc.

I can understand that it being a shell script and hard to read is suboptimal but this is something which doesn't play into a discussion for the current implementation.

We started with just bringing snapd-xdg-open into the tree which then became a whole rewrite. Lets not start another rewrite in the same thread. If xdg-open is still suboptimal from a snapd perspective lets figure that out in another thread but lets finish this one first. It's already going on for too long :-)

@mvo5

mvo5 Aug 24, 2017

Collaborator

I'm not aware of anything that is more portable than xdg-open :/

@mvo5

mvo5 Aug 24, 2017

Collaborator

We could check which of gvfs-open, kde-open, exo-open to use ourself, but that would duplicate the detection logic in xdg-open of course.

mvo5 added a commit to mvo5/core that referenced this pull request Aug 24, 2017

Collaborator

mvo5 commented Aug 24, 2017

I also created the companion PR for the core side: snapcore/core#54

@@ -0,0 +1,3 @@
+[D-BUS Service]
+Name=io.snapcraft.Launcher
+Exec=@bindir@/snap userd
@zyga

zyga Aug 25, 2017

Contributor

Feels like libexecdir thing to me.

@mvo5

mvo5 Aug 25, 2017

Collaborator

This is the regular snap binary that provides the userd subcommand. So we need bindir here unless I miss something.

@Conan-Kudo

Conan-Kudo Aug 25, 2017

Contributor

This is correct, though I expected this to be implemented in a libexecdir command, as users shouldn't usually do things here. Not a biggie, though.

Hey

Thank you for picking this up mvo. Two nitpicks, one about libexecdir vs bindir (no manual page for us to write) and one more about ping. I really really want to use the ping method because it's the right thing to do in this context.

tests/main/snap-userd/task.yaml
+ --type=method_call \
+ --print-reply \
+ /io/snapcraft/Launcher \
+ io.snapcraft.Launcher.OpenURL string:"ping-ping-ping:" \
@zyga

zyga Aug 25, 2017

Contributor

Please use the regular peer ping method here.

@mvo5

mvo5 Aug 25, 2017

Collaborator

Unfortuantely ping is not implemented by us, I can dig a bit more into this but it looks like a limiation/missing feature of the godbus library which is not very high-level.

@mvo5

mvo5 Aug 25, 2017

Collaborator

I changed the code to use "ping" now.

zyga approved these changes Aug 25, 2017

mvo5 added some commits Aug 25, 2017

Looks great, but a few comments.

@@ -0,0 +1,3 @@
+[D-BUS Service]
+Name=io.snapcraft.Launcher
+Exec=@bindir@/snap userd
@zyga

zyga Aug 25, 2017

Contributor

Feels like libexecdir thing to me.

@mvo5

mvo5 Aug 25, 2017

Collaborator

This is the regular snap binary that provides the userd subcommand. So we need bindir here unless I miss something.

@Conan-Kudo

Conan-Kudo Aug 25, 2017

Contributor

This is correct, though I expected this to be implemented in a libexecdir command, as users shouldn't usually do things here. Not a biggie, though.

+ path=/io/snapcraft/Launcher
+ interface=io.snapcraft.Launcher
+ member=OpenURL
+ peer=(label=unconfined),
@Conan-Kudo

Conan-Kudo Aug 25, 2017

Contributor

👍 🎉

packaging/fedora/snapd.spec
@@ -380,12 +380,11 @@ autoreconf --force --install --verbose
popd
# Build systemd units
-pushd ./data/systemd
-make BINDIR="%{_bindir}" LIBEXECDIR="%{_libexecdir}" \
+make -C data \
@Conan-Kudo

Conan-Kudo Aug 25, 2017

Contributor

Please refrain from making unrelated changes to the spec. Changing from pushd / popd to using make -C is a different type of change.

@mvo5

mvo5 Aug 25, 2017

Collaborator

Sorry, I just inherited this change, I can revert this back if you prefer.

packaging/fedora/snapd.spec
@@ -441,13 +440,11 @@ rm -fv %{buildroot}%{_bindir}/ubuntu-core-launcher
popd
# Install all systemd units
-pushd ./data/systemd
-%make_install SYSTEMDSYSTEMUNITDIR="%{_unitdir}" BINDIR="%{_bindir}" LIBEXECDIR="%{_libexecdir}"
+%make_install -C data SYSTEMDSYSTEMUNITDIR="%{_unitdir}" BINDIR="%{_bindir}" LIBEXECDIR="%{_libexecdir}"
@Conan-Kudo

Conan-Kudo Aug 25, 2017

Contributor

Ditto here.

@mvo5

mvo5 Aug 25, 2017

Collaborator

Same as above.

@@ -64,8 +64,8 @@ Depends: adduser,
systemd,
${misc:Depends},
${shlibs:Depends}
-Replaces: ubuntu-snappy (<< 1.9), ubuntu-snappy-cli (<< 1.9), snap-confine (<< 2.23), ubuntu-core-launcher (<< 2.22)
-Breaks: ubuntu-snappy (<< 1.9), ubuntu-snappy-cli (<< 1.9), snap-confine (<< 2.23), ubuntu-core-launcher (<< 2.22)
+Replaces: ubuntu-snappy (<< 1.9), ubuntu-snappy-cli (<< 1.9), snap-confine (<< 2.23), ubuntu-core-launcher (<< 2.22), snapd-xdg-open (<< 0.0.0)
@zyga

zyga Aug 8, 2017

Contributor

Finally, thank you again for doing this :-)

@Conan-Kudo

Conan-Kudo Aug 25, 2017

Contributor

That is an insane number of Breaks/Replaces directives!

mvo5 added some commits Aug 25, 2017

@niemeyer niemeyer merged commit 047538e into snapcore:master Aug 28, 2017

6 of 7 checks passed

xenial-ppc64el autopkgtest finished (failure)
Details
artful-amd64 autopkgtest finished (success)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
xenial-amd64 autopkgtest finished (success)
Details
xenial-i386 autopkgtest finished (success)
Details
yakkety-amd64 autopkgtest finished (success)
Details
zesty-amd64 autopkgtest finished (success)
Details
Contributor

niemeyer commented Aug 28, 2017

Thanks for the long work here @morphis, thanks for taking this over @mvo, and thanks for the reviews everybody else!

Thank you guys for this!

So, snaps that call xdg-open will work out of the box without any dependences when 2.28 gets released?

Collaborator

mvo5 commented Aug 29, 2017

@feikname correct :)

Contributor

morphis commented Aug 29, 2017

@mvo5 I owe you a beer or something else on the next sprint for taking this over and finishing it!

Conan-Kudo added a commit to Conan-Kudo/snapd that referenced this pull request Sep 6, 2017

packaging/fedora: Add missing godbus dependencies
This should have been caught when snapcore/snapd#3260 was being worked on.

Signed-off-by: Neal Gompa <ngompa13@gmail.com>

mvo5 added a commit to mvo5/snappy that referenced this pull request Sep 11, 2017

packaging/fedora: Add missing godbus dependencies
This should have been caught when snapcore/snapd#3260 was being worked on.

Signed-off-by: Neal Gompa <ngompa13@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment