New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support snap #155

Merged
merged 3 commits into from Apr 3, 2018

Conversation

Projects
None yet
2 participants
@alexlarsson
Member

alexlarsson commented Feb 8, 2018

This adds support for snaps, based on the code in #136 by @jhenstridge. I did not actually test this, so it needs testing before it can be commited.

This PR includes the changes from #154, but we should probably land those separately first.

This should handle the basics of portals, but there are two things still missing:

  • xdp_get_app_info_from_pid() needs support to look up a snap AppInfo from the pid
  • app_has_file_access() in document-portal.c currently only supports flatpak, it would be interesting to have a version of this for snaps too.
@alexlarsson

This comment has been minimized.

Member

alexlarsson commented Feb 13, 2018

Rebased this on master with #154 merged

@jhenstridge

Sorry for taking so long with this. After I made the change to aa_is_enabled, the portal detected snap confined applications correctly.

For testing, I used a snapped version of portal-test:

http://people.canonical.com/~jamesh/portal-test_1_amd64.snap

I'm also running the following patch to snapd: https://github.com/snapcore/snapd/compare/master...jhenstridge:xdg-desktop-portal-support?expand=1

(I don't currently have document-portal support rolled in: that's still coming).

I used the screenshot test because there is currently an http/https scheme handler in the snap environment, which is picked before trying the portal.

The portal prompted me to grant permission to make the screenshot available. Like my original PR, we don't have any app info apart from the ID at the moment, so the UI shows the app name as "(null)". That seems to be more about how xdg-desktop-portal-gtk looks up the application info though, which should be solvable.

proposed = g_strdup_printf ("%s/apparmor", mntpt->mnt_dir);
if (stat (proposed, &statbuf) == 0)
{
apparmor_enabled = FALSE;

This comment has been minimized.

@jhenstridge

jhenstridge Feb 16, 2018

Contributor

I needed to change this to TRUE for the snap detection code to do anything.

This comment has been minimized.

@alexlarsson
/* This is the same as flatpak apps, except we also allow
names to start with digits, so that ids of the form
snap.pkg.$snapname is allowed for all snap names. */

This comment has been minimized.

@jhenstridge

jhenstridge Feb 16, 2018

Contributor

I used "snap.pkg.*" as the prefix to work around the fact that xdp_is_valid_app_id required at least three dotted components.

I can keep using that, but if we're modifying the validation rules what do you think of allowing two dotted components and shortening it to "snap.$snapname"?

This comment has been minimized.

@alexlarsson

alexlarsson Mar 15, 2018

Member

I'm fine with allowing two components.

@alexlarsson

This comment has been minimized.

Member

alexlarsson commented Mar 15, 2018

I just merged #163 which could probably be useful for this too.

alexlarsson added some commits Feb 8, 2018

Broaden what are valid app ids to allow snap.$PKGNAME
We now allow numbers as the first character in an app id segment.
We also allow names with two components.
This allows us to map any valid snap package name to the
form snap.$PKGNAME.

This also means we could technically create a document for an
that is not a valid flatpak app id. However, flatpaks would just
never look these up, so this is not a problem.
XdpAppInfo: Centralize handling of xdp_app_info_new_host ()
In the flatpak case, handle the not-a-flatpak case by returning
NULL (with no error set) so that we can continue trying any
other alternatives, with a single last fallback for the uncontained
host case.
Add support for snap packages
This adds initial support for detecting snaps. The code is based on
#136 by @jhenstridge
@alexlarsson

This comment has been minimized.

Member

alexlarsson commented Apr 3, 2018

This is just the initial work, but lets merge this for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment