{snap,wrappers}: improve desktop file to snap app matching

desktop/desktopentry/desktopentry.go

@@ -39,6 +39,9 @@ type DesktopEntry struct {
 	Icon     string
 	Exec     string
 
+	SnapInstanceName string
+	SnapAppName      string
+
 	Hidden                bool
 	OnlyShowIn            []string
 	NotShownIn            []string
@@ -52,6 +55,8 @@ type Action struct {
 	Name string
 	Icon string
 	Exec string
+
+	SnapAppName string
 }
 
 type groupState int
@@ -155,6 +160,10 @@ func parse(filename string, r io.Reader) (*DesktopEntry, error) {
 				de.GnomeAutostartEnabled = value == "true"
 			case "Actions":
 				actions = splitList(value)
+			case "X-SnapInstanceName":
+				de.SnapInstanceName = value
+			case "X-SnapAppName":
+				de.SnapAppName = value
 			default:
 				// Ignore all other keys
 			}
@@ -166,6 +175,8 @@ func parse(filename string, r io.Reader) (*DesktopEntry, error) {
 				currentAction.Icon = value
 			case "Exec":
 				currentAction.Exec = value
+			case "X-SnapAppName":
+				currentAction.SnapAppName = value
 			default:
 				// Ignore all other keys
 			}

desktop/desktopentry/desktopentry_test.go

@@ -39,9 +39,11 @@ var _ = Suite(&desktopentrySuite{})
 
 const browserDesktopEntry = `
 [Desktop Entry]
+X-SnapInstanceName=browser
 Version=1.0
 Type=Application
 Name=Web Browser
+X-SnapAppName=browser-app
 Exec=browser %u
 Icon = ${SNAP}/default256.png
 Actions=NewWindow;NewPrivateWindow;
@@ -53,10 +55,12 @@ Exec=not-the-executable
 # A comment
 [Desktop Action NewWindow]
 Name = Open a New Window
+X-SnapAppName=browser-app
 Exec=browser -new-window
 
 [Desktop Action NewPrivateWindow]
 Name=Open a New Private Window
+X-SnapAppName=browser-app
 Exec=browser -private-window
 Icon=${SNAP}/private.png
 `
@@ -69,17 +73,21 @@ func (s *desktopentrySuite) TestParse(c *C) {
 	c.Check(de.Name, Equals, "Web Browser")
 	c.Check(de.Icon, Equals, "${SNAP}/default256.png")
 	c.Check(de.Exec, Equals, "browser %u")
+	c.Check(de.SnapInstanceName, Equals, "browser")
+	c.Check(de.SnapAppName, Equals, "browser-app")
 	c.Check(de.Actions, HasLen, 2)
 
 	c.Assert(de.Actions["NewWindow"], NotNil)
 	c.Check(de.Actions["NewWindow"].Name, Equals, "Open a New Window")
 	c.Check(de.Actions["NewWindow"].Icon, Equals, "")
 	c.Check(de.Actions["NewWindow"].Exec, Equals, "browser -new-window")
+	c.Check(de.Actions["NewWindow"].SnapAppName, Equals, "browser-app")
 
 	c.Assert(de.Actions["NewPrivateWindow"], NotNil)
 	c.Check(de.Actions["NewPrivateWindow"].Name, Equals, "Open a New Private Window")
 	c.Check(de.Actions["NewPrivateWindow"].Icon, Equals, "${SNAP}/private.png")
 	c.Check(de.Actions["NewPrivateWindow"].Exec, Equals, "browser -private-window")
+	c.Check(de.Actions["NewPrivateWindow"].SnapAppName, Equals, "browser-app")
 }
 
 func (s *desktopentrySuite) TestParseBad(c *C) {

overlord/snapstate/snapstate_test.go

@@ -9144,6 +9144,7 @@ X-SnapInstanceName=test-snap`)
 [Desktop Entry]
 X-SnapInstanceName=test-snap
 Name=test
+X-SnapAppName=test-snap
 Exec=env BAMF_DESKTOP_FILE_HINT=%s/test-snap_test-snap.desktop %s/test-snap
 `[1:], dirs.SnapDesktopFilesDir, dirs.SnapBinariesDir)
 

snap/info.go

@@ -31,7 +31,9 @@
 	"strings"
 	"time"
 
+	"github.com/snapcore/snapd/desktop/desktopentry"
 	"github.com/snapcore/snapd/dirs"
+	"github.com/snapcore/snapd/logger"
 	"github.com/snapcore/snapd/metautil"
 	"github.com/snapcore/snapd/osutil"
 	"github.com/snapcore/snapd/osutil/sys"
@@ -1425,6 +1427,34 @@
 // DesktopFile returns the path to the installed optional desktop file for the
 // application.
 func (app *AppInfo) DesktopFile() string {
+	desktopFileIDs, err := app.Snap.DesktopPlugFileIDs()
+	if err != nil || len(desktopFileIDs) == 0 {
+		// fallback to a simple heuristic "$PREFIX_$APP.desktop"
+		return filepath.Join(dirs.SnapDesktopFilesDir, fmt.Sprintf("%s_%s.desktop", app.Snap.DesktopPrefix(), app.Name))
+	}
+	// Loop through desktop-file-ids desktop interface plug attribute in order to
+	// have deterministic output
+	for _, desktopFileID := range desktopFileIDs {
+		desktopFile := filepath.Join(dirs.SnapDesktopFilesDir, desktopFileID+".desktop")
+		if !osutil.FileExists(desktopFile) {
+			continue
+		}
+		// No need to also check instance name because we already filter by the
+		// snap's desktop file ids
+		de, err := desktopentry.Read(desktopFile)
+		if err != nil {
+			// Errors when reading indicates either an issue opening the desktop
+			// file or a malformed desktop file, both of which are internal
+			// errors caused somewhere else.
+			// Let's log for debugging and try the next desktop file id.
+			logger.Debugf("internal error: failed to read %q: %v", desktopFile, err)
+			continue
+		}
+		if de.SnapAppName == app.Name {
+			return desktopFile
+		}
+	}
+	// fallback to a simple heuristic "$PREFIX_$APP.desktop"
 	return filepath.Join(dirs.SnapDesktopFilesDir, fmt.Sprintf("%s_%s.desktop", app.Snap.DesktopPrefix(), app.Name))
 }
 

snap/info_test.go

@@ -1352,6 +1352,91 @@ func (s *infoSuite) TestAppDesktopFile(c *C) {
 	c.Check(snapInfo.DesktopPrefix(), Equals, "sample+instance")
 }
 
+func (s *infoSuite) testAppDesktopFileWithDesktopFileIDs(c *C, isParallelInstance bool) {
+	const sampleDesktopFileIDsYaml = `
+name: sample
+version: 1
+apps:
+ app:
+   command: foo
+ app2:
+   command: bar
+ sample:
+   command: foobar
+   command-chain: [chain]
+plugs:
+  desktop:
+    desktop-file-ids:
+      - org.example
+      - org.example.Foo
+      - org.example.Bar
+`
+
+	snaptest.MockSnap(c, sampleDesktopFileIDsYaml, &snap.SideInfo{})
+	snapInfo, err := snap.ReadInfo("sample", &snap.SideInfo{})
+	c.Assert(err, IsNil)
+	c.Assert(snapInfo.Plugs["desktop"], NotNil)
+
+	if isParallelInstance {
+		snapInfo.InstanceKey = "instance"
+	}
+
+	c.Assert(os.MkdirAll(dirs.SnapDesktopFilesDir, 0755), IsNil)
+	mockDesktopFile := func(app *snap.AppInfo, path string) {
+		const mockDesktopFileTemplate = `[Desktop Entry]
+X-SnapInstanceName=%s
+Name=foo
+X-SnapAppName=%s
+Exec=env BAMF_DESKTOP_FILE_HINT=foo.desktop %s
+`
+		content := fmt.Sprintf(mockDesktopFileTemplate, snapInfo.InstanceName(), app.Name, app.WrapperPath())
+		c.Assert(os.WriteFile(path, []byte(content), 0644), IsNil)
+	}
+
+	// Given the configuration below:
+	//   - org.example     -> app
+	//   - org.example.Foo -> sample
+	//   - org.example.Bar -> sample
+	mockDesktopFile(snapInfo.Apps["app"], filepath.Join(dirs.SnapDesktopFilesDir, "org.example.desktop"))
+	mockDesktopFile(snapInfo.Apps["sample"], filepath.Join(dirs.SnapDesktopFilesDir, "org.example.Foo.desktop"))
+	mockDesktopFile(snapInfo.Apps["sample"], filepath.Join(dirs.SnapDesktopFilesDir, "org.example.Bar.desktop"))
+
+	// Desktop file detected for "app" should be org.example.desktop
+	c.Check(snapInfo.Apps["app"].DesktopFile(), Matches, `.*/var/lib/snapd/desktop/applications/org.example.desktop`)
+
+	// Desktop file detected for "sample" should be org.example.Foo.desktop because
+	// it comes before org.example.Bar in the desktop-file-ids attribute
+	c.Check(snapInfo.Apps["sample"].DesktopFile(), Matches, `.*/var/lib/snapd/desktop/applications/org.example.Foo.desktop`)
+
+	// When org.example.Foo.desktop is removed, esktop file detected for "sample" should be org.example.Bar.desktop
+	c.Assert(os.Remove(filepath.Join(dirs.SnapDesktopFilesDir, "org.example.Foo.desktop")), IsNil)
+	c.Check(snapInfo.Apps["sample"].DesktopFile(), Matches, `.*/var/lib/snapd/desktop/applications/org.example.Bar.desktop`)
+
+	// When no desktop-file-id desktop files exist, fallback to the "$PREFIX_$APP.desktop" heuristic
+	c.Assert(os.Remove(filepath.Join(dirs.SnapDesktopFilesDir, "org.example.desktop")), IsNil)
+	c.Assert(os.Remove(filepath.Join(dirs.SnapDesktopFilesDir, "org.example.Bar.desktop")), IsNil)
+	prefix := "sample"
+	if isParallelInstance {
+		prefix = `sample\+instance`
+	}
+	c.Check(snapInfo.Apps["app"].DesktopFile(), Matches, ".*/var/lib/snapd/desktop/applications/"+prefix+"_app.desktop")
+	c.Check(snapInfo.Apps["sample"].DesktopFile(), Matches, ".*/var/lib/snapd/desktop/applications/"+prefix+"_sample.desktop")
+
+	// When X-SnapAppName is not found, also fallback to the "$PREFIX_$APP.desktop" heuristic
+	c.Assert(os.WriteFile(filepath.Join(dirs.SnapDesktopFilesDir, "org.example.desktop"), nil, 0644), IsNil)
+	c.Check(snapInfo.Apps["app"].DesktopFile(), Matches, ".*/var/lib/snapd/desktop/applications/"+prefix+"_app.desktop")
+}
+
+func (s *infoSuite) TestAppDesktopFileWithDesktopFileIDs(c *C) {
+	const isParallelInstance = false
+	s.testAppDesktopFileWithDesktopFileIDs(c, isParallelInstance)
+}
+
+func (s *infoSuite) TestAppDesktopFileWithDesktopFileIDsParallelInstance(c *C) {
+	const isParallelInstance = true
+	s.testAppDesktopFileWithDesktopFileIDs(c, isParallelInstance)
+}
+
 const coreSnapYaml = `name: core
 version: 0
 type: os

tests/main/install-sideload/task.yaml

@@ -59,11 +59,12 @@ execute: |
     echo "Sideload desktop snap"
     SNAP_MOUNT_DIR="$(os.paths snap-mount-dir)"
     snap install --dangerous ./basic-desktop_1.0_all.snap
-    diff -u <(head -n5 /var/lib/snapd/desktop/applications/basic-desktop_echo.desktop) - <<-EOF
+    diff -u <(head -n6 /var/lib/snapd/desktop/applications/basic-desktop_echo.desktop) - <<-EOF
     	[Desktop Entry]
     	X-SnapInstanceName=basic-desktop
     	Name=Echo
     	Comment=It echos stuff
+    	X-SnapAppName=echo
     	Exec=env BAMF_DESKTOP_FILE_HINT=/var/lib/snapd/desktop/applications/basic-desktop_echo.desktop $SNAP_MOUNT_DIR/bin/basic-desktop.echo
     EOF
 

tests/main/parallel-install-desktop/task.yaml

@@ -32,11 +32,12 @@ execute: |
         expected="^basic-desktop_$instance 1.0 installed\$"
         "$TESTSTOOLS"/snaps-state install-local-as basic-desktop "basic-desktop_$instance" | MATCH "$expected"
 
-        diff -u <(head -n5 "/var/lib/snapd/desktop/applications/basic-desktop+${instance}_echo.desktop") - <<-EOF
+        diff -u <(head -n6 "/var/lib/snapd/desktop/applications/basic-desktop+${instance}_echo.desktop") - <<-EOF
     [Desktop Entry]
     X-SnapInstanceName=basic-desktop_${instance}
     Name=Echo
     Comment=It echos stuff
+    X-SnapAppName=echo
     Exec=env BAMF_DESKTOP_FILE_HINT=/var/lib/snapd/desktop/applications/basic-desktop+${instance}_echo.desktop $SNAP_MOUNT_DIR/bin/basic-desktop_$instance.echo
     EOF
 

wrappers/desktop.go

@@ -29,6 +29,7 @@ import (
 	"regexp"
 	"strings"
 
+	"github.com/snapcore/snapd/desktop/desktopentry"
 	"github.com/snapcore/snapd/dirs"
 	"github.com/snapcore/snapd/logger"
 	"github.com/snapcore/snapd/osutil"
@@ -103,8 +104,9 @@ var isValidDesktopFileLine = regexp.MustCompile(strings.Join([]string{
 	"^TargetEnvironment=",
 }, "|")).Match
 
-// rewriteExecLine rewrites a "Exec=" line to use the wrapper path for snap application.
-func rewriteExecLine(s *snap.Info, desktopFile, line string) (string, error) {
+// detectAppAndRewriteExecLine parses snap app name from passed "Exec=" line and rewrites it
+// to use the wrapper path for snap application.
+func detectAppAndRewriteExecLine(s *snap.Info, desktopFile, line string) (appName string, execLine string, err error) {
 	env := fmt.Sprintf("env BAMF_DESKTOP_FILE_HINT=%s ", desktopFile)
 
 	cmd := strings.SplitN(line, "=", 2)[1]
@@ -121,9 +123,9 @@ func rewriteExecLine(s *snap.Info, desktopFile, line string) (string, error) {
 		// this is ok because desktop files are not run through sh
 		// so we don't have to worry about the arguments too much
 		if cmd == validCmd {
-			return "Exec=" + env + wrapper, nil
+			return app.Name, "Exec=" + env + wrapper, nil
 		} else if strings.HasPrefix(cmd, validCmd+" ") {
-			return fmt.Sprintf("Exec=%s%s%s", env, wrapper, line[len("Exec=")+len(validCmd):]), nil
+			return app.Name, fmt.Sprintf("Exec=%s%s%s", env, wrapper, line[len("Exec=")+len(validCmd):]), nil
 		}
 	}
 
@@ -137,10 +139,10 @@ func rewriteExecLine(s *snap.Info, desktopFile, line string) (string, error) {
 	if ok {
 		newExec := fmt.Sprintf("Exec=%s%s", env, app.WrapperPath())
 		logger.Noticef("rewriting desktop file %q to %q", desktopFile, newExec)
-		return newExec, nil
+		return app.Name, newExec, nil
 	}
 
-	return "", fmt.Errorf("invalid exec command: %q", cmd)
+	return "", "", fmt.Errorf("invalid exec command: %q", cmd)
 }
 
 func rewriteIconLine(s *snap.Info, line string) (string, error) {
@@ -188,11 +190,14 @@ func sanitizeDesktopFile(s *snap.Info, desktopFile string, rawcontent []byte) []
 		// rewrite exec lines to an absolute path for the binary
 		if bytes.HasPrefix(bline, []byte("Exec=")) {
 			var err error
-			line, err := rewriteExecLine(s, desktopFile, string(bline))
+			appName, line, err := detectAppAndRewriteExecLine(s, desktopFile, string(bline))
 			if err != nil {
 				// something went wrong, ignore the line
 				continue
 			}
+			// Add metadata entry to associate the exec line with a snap app
+			newContent.Write([]byte("X-SnapAppName=" + appName + "\n"))
+
 			bline = []byte(line)
 		}
 
@@ -274,26 +279,6 @@ func deriveDesktopFilesContent(s *snap.Info) (map[string]osutil.FileState, error
 	return content, nil
 }
 
-// TODO: Merge desktop file helpers into desktop/desktopentry package
-func snapInstanceNameFromDesktopFile(desktopFile string) (string, error) {
-	file, err := os.Open(desktopFile)
-	if err != nil {
-		return "", err
-	}
-	defer file.Close()
-
-	scanner := bufio.NewScanner(file)
-	for i := 0; scanner.Scan(); i++ {
-		bline := scanner.Text()
-		if !strings.HasPrefix(bline, "X-SnapInstanceName=") {
-			continue
-		}
-		return strings.TrimPrefix(bline, "X-SnapInstanceName="), nil
-	}
-
-	return "", fmt.Errorf("cannot find X-SnapInstanceName entry in %q", desktopFile)
-}
-
 // forAllDesktopFiles loops over all installed desktop files under
 // dirs.SnapDesktopFilesDir.
 //
@@ -306,15 +291,15 @@ func forAllDesktopFiles(cb func(base, instanceName string) error) error {
 	}
 
 	for _, desktopFile := range installedDesktopFiles {
-		instanceName, err := snapInstanceNameFromDesktopFile(desktopFile)
-		if err != nil {
+		de, err := desktopentry.Read(desktopFile)
+		if err != nil || de.SnapInstanceName == "" {
 			// cannot read instance name from desktop file, ignore
 			logger.Noticef("cannot read instance name: %s", err)
 			continue
 		}
 
 		base := filepath.Base(desktopFile)
-		if err := cb(base, instanceName); err != nil {
+		if err := cb(base, de.SnapInstanceName); err != nil {
 			return err
 		}
 	}