usersession/userd: separate bus name ownership from defining interfaces #9370
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously bus names and interface names were conflated, which would encourage anyone extending userd with new interfaces to also have it acquire new bus names. That is not necessary or desirable.
jhenstridge
force-pushed
the
userd-bus-names
branch
from
60ffc9b
to
0477187
Compare
anonymouse64
approved these changes
Sep 28, 2020
pedronis
referenced
this pull request
May 31, 2021
…ps (#8699) Add a new interface desktop-launch that allows shells to read .desktop files from /var/lib/snapd/desktop/applications/ and call io.snapcraft.PrivilegedDesktopLauncher.OpenDesktopEntry. Add io.snapcraft.PrivilegedDesktopLauncher.OpenDesktopEntry support to userd that implements support for this on Classic systems. The result is that a confined desktop shell can identify other snaps and launch them with (for example) the WAYLAND_DESKTOP environment variable needed for the client to connect to the correct desktop. Follows on from: Discussion at forum.snapcraft.io; and, #7490 (rebased and updated) * "shell-support" interface * Spike OpenDesktopEntry method * Add OpenDesktopEntryEnv to permit setting environment variables * Switch to Desktop File IDs * Extract desktopFileIdToFilename() * Extract readExecCommandFromDesktopFile() * Clearer use of whitespace * More robust logic in readExecCommandFromDesktopFile() * Document the processing of the exec command * > Missing high-level test for interface 'shell-support'. Please add to: > * tests/lib/snaps/test-snapd-policy-app-consumer/meta/snap.yaml * Handle shell quoting in the exec command * Drop the `OpenDesktopEntry()` method * Better handling of exec variables * deny-auto-connection: true * Use the `MockConnectedPlug` and `MockConnectedSlot` helpers * Use free functions * Rename `shell-support` => `app-launch` * Report error if desktop file not found * Search all the paths that can be formed by the desktop ID * We don't need github.com/google/shlex, we have github.com/snapcore/snapd/strutil/shlex * Don't use error to indicate whether a desktop file is found * Update comments referring to desktop-entry-spec-latest.html * Don't ignore errors from os.Stat() * Restrict the environment variables that may be set to those used to describe the shell to toolkits. * Comment to explain the code * Use dirs.SnapDesktopFilesDir, not a hard coded path * First cut at some internal tests * Test parsing of Exec command * Use the shell to launch the app to avoid becoming a parent and/or leaving a zombie process * Fix "usersession/userd/launcher.go:154:13: undefined: strings.ReplaceAll" in CI * Drop implicitOnCore as it isn't supportable (yet) * Remove "unknown field 'reservedForOS' in struct literal of type commonInterface" * Rename `app-launch` => `desktop-launch` * Renames to conform to convention * Use a scanner instead of reading lines "by hand" * Use the language better * Update naming * Use check.v1 * Less evil hack to avoid zombie processes * Make interface superprivileged * Add TestStaticInfo() to interface tests * Reworked comments and added sanity for review * Additional "hardening" suggested in review * gofmt -s -w * Add BaseDeclarationPlugs to desktopLaunchSuite.TestStaticInfo * Document allowedEnvVars * Update comment * Document and correct check on desktop file & path * gofmt * Add test for foo-bar_foo-bar.desktop * A comment to explain test strategy * Check the desktopFile path all the way down from "/" * Comment on the recursion in findDesktopFile * Use err to indicate failure instead of null pointer to string * Clearer table of test cases in TestParseExecCommandSucceedsWithValidEntry * Extract verifyDesktopFileLocation() from readExecCommandFromDesktopFile() * Tests for readExecCommandFromDesktopFile() * First draft of TestOpenDesktopEntryEnvSucceedsWithGoodDesktopId * Hack the production code to make the tests pass * Test some error paths * Use camelCase * tests: add a basic spread test for the dbus-launch interface * Update tests/main/interfaces-desktop-launch/test-app/bin/app.sh Co-authored-by: James Henstridge <james@jamesh.id.au> * Add access to /var/lib/snapd/desktop/icons * Add RegularFileExists() to osutil * We don't want /foo2 to be treated as under /foo * Drop contentious checks * Error out on unexpected exec variables * Reword comment * Update test Exec lines with exec variables we do support * go fmt * Comments adjusted to match code * Use systemd-run to launch apps * Don't pass environment * Failed PrivilegedDesktopLauncher * Make path match interface * Update to reflect snapcore#9370 * Drop OpenDesktopEntry from io.snapcraft.Launcher * Split out PrivilegedDesktopLauncher tests * Use export_test.go to access internal functions * Use dirs.SnapBinariesDir * Fix and move repetitive test setup to SetUpTest() * go fmt * Support for %i * Correct desktop-launch launcher.sh script * Revert accidental move of fdToFilename from launcher.go * userd: delete unused PrivilegedDesktopLauncher.OpenFile D-Bus method * userd: clean up PrivilegedDesktopLauncher code based on review from @pedronis * userd: simplify how the mock fileExists handler is injected for testing * Don't try to pass environment in interfaces-desktop-launch * Add "internal error: " to what is currently a logic error * Use the new regularFileExists signature * Use `--collect` with systemd-run * unnecessary whitespace Co-authored-by: Ian Johnson <person.uwsome@gmail.com> * we should keep the error and include it in the message Co-authored-by: Ian Johnson <person.uwsome@gmail.com> * change to a TODO Co-authored-by: Ian Johnson <person.uwsome@gmail.com> * Remove spurious comma * usersession/userd: only pass --collect if we have a new enough systemd * u/userd: test error message from desktopFileIDToFilename * usersession/userd: apply a regexp to validate desktop file IDs * usersession/userd: update copyright notices * usersession/userd: fix up some error assertions in tests, and catch some more invalid desktop file IDs * usersession/userd: follow the XDG Base Dir spec in resolving desktop file IDs * tests: ensure XDG_DATA_DIRS is set in spread test * usersession/userd: don't reuse the object path of the existing launcher interface for PrivilegedDesktopLauncher This reinforces that the API is not in the same security domain as those exported on /io/snapcraft/Launcher. * usersession/userd: add a direct test for DesktopFileIDToFilename without mocked stat calls * usersession/userd: add test demonstrating that launching non-snap desktop files fails * usersession/userd: more fixups based on review comments * usersession/userd: reject desktop files with multiple [Desktop Entry] sections * Address latest review feedback * Fix accidental paste. * Update tests to match * Update tests/main/interfaces-desktop-launch/task.yaml Co-authored-by: Ian Johnson <person.uwsome@gmail.com> * Update tests/main/interfaces-desktop-launch/task.yaml Co-authored-by: Ian Johnson <person.uwsome@gmail.com> Co-authored-by: James Henstridge <james@jamesh.id.au> Co-authored-by: Ian Johnson <person.uwsome@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This branch is prompted by PR #8699, where Alan is extending userd.
Previously bus names and interface names were conflated, which would encourage anyone extending userd with new interfaces to also have it acquire new bus names. That is not necessary or desirable. There's no need for userd to have multiple well known bus names, and adding new service activation files requires updates in multiple locations.
This branch keeps the two names userd currently claims, but separates them from the exporting of new methods on object paths. I also took the opportunity to change the nomenclature to more closely match what D-Bus's terminology to avoid future confusion.