Skip to content
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

Set permissions for flatpaks #60

Merged
merged 19 commits into from Jun 4, 2020

Conversation

meisenzahl
Copy link
Member

Description

As described in the AppCenter for Everyone campaign, you want to use Flatpak as a format for apps in the future. So I thought it would be a good idea to be able to set the permissions of the apps. By the way: Congratulations on the success of the campaign 🎉

For the implementation I have oriented myself on Flatseal. Great foundation!

The extension of the plug now offers the same functionality as Flatseal, except for the setting of customized paths. But I am already working on that ;)

I created the pull request to get feedback from you.

Current state

Screenshot von 2020-02-21 09 40 28

Screenshot von 2020-02-21 09 42 21

The permissions are immediately overwritten when toggling a switch. For example, Games can't start anymore if I revoke the permission to access X11:

Gtk-Message: 09:42:00.497: Failed to load module "pantheon-filechooser-module"
Unable to init server: Could not connect: Connection refused

(org.gnome.Games:2): Gtk-WARNING **: 09:42:00.506: cannot open display:

Screenshot von 2020-02-21 09 42 28

Clicking the Reset button restores the default permissions.

Tasks

  • Redesign the GUI
    • Minimize the Reset button
    • Minimize the size of the switches
  • Refactor the code

Questions

  • I created an implementation of a Map because GLib.HashTable changes the order of the keys. Would it be okay to add Gee as a dependency?

I am looking forward to your feedback!

@davidmhewitt
Copy link
Member

Wow, this is a really interesting starting point! Thank you for this.

It would be fine to add Gee as a dependency, we use this nearly everywhere so one more place won't hurt 😄

One of the things that we're going to have to think about at the sprint is a way to write the description of all of these permissions so that they're clear to the average user. For example, I shouldn't have to know what "inter-process communications" means. I should just be told it means that apps may be able to talk to, control and read information from other apps or system components.

Another thing worth considering for us is that we're going to need to at least display these permissions in at least 3 applications (Sideload, AppCenter and here in the applications plug), so we may consider building a small library for reading, translating/describing, and setting these permissions rather than having this code duplicated in 3+ places.

I think it's unlikely we'll get around to reviewing this properly or merging it until during/after the sprint. The first priority is building the flatpak authenticator, the backend infrastructure for building flatpaks into the new elementary repo and maybe some flatpak portals. But, thank you for all your hard work! It'll certainly save us some research ❤️

@meisenzahl
Copy link
Member Author

Many thanks for the detailed feedback!

To make the effects of permissions clearer to users, they may need to be grouped by topic. However, this adds a whole new level of complexity because, for example, graphical output is allowed for an app using X11, but not using Wayland. Mapping this to the flatpak permissions is a challenge.

I think developing a library for handling the Flatpak permissions is a good idea. The logic for handling the permissions is already implemented. Developing a reusable GUI for it is still pending. I trust in the capabilities of the elementary project 😉️

If I can help in any way, please let me know.

@meisenzahl
Copy link
Member Author

It would be fine to add Gee as a dependency, we use this nearly everywhere so one more place won't hurt

Apparently there is no Map in Gee that keeps the order of the keys. So I leave my implementation with 2 internal lists like this.

@vjr
Copy link
Member

vjr commented Feb 21, 2020

It would be fine to add Gee as a dependency, we use this nearly everywhere so one more place won't hurt

Apparently there is no Map in Gee that keeps the order of the keys. So I leave my implementation with 2 internal lists like this.

Won't the TreeMap work here? See https://valadoc.org/gee-0.8/Gee.TreeMap.html

@vjr
Copy link
Member

vjr commented Feb 21, 2020

It would be fine to add Gee as a dependency, we use this nearly everywhere so one more place won't hurt

Apparently there is no Map in Gee that keeps the order of the keys. So I leave my implementation with 2 internal lists like this.

Won't the TreeMap work here? See https://valadoc.org/gee-0.8/Gee.TreeMap.html

My bad, probably not.

Copy link
Member

@danirabbit danirabbit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey thank you for proposing this branch! I think this is a really great start :)

I left a couple of small inline comments about UI stuff.

With regards to display server settings, my feeling is that we probably shouldn't expose them. Pantheon doesn't currently run on Wayland, so all apps will need access to X11.

I also second what David said that some of these descriptions are very technically accurate but probably don't convey meaning to the average person. For example "Access PulseAudio sound server" should probably be something more like "Play sounds"

The names of the actual sockets are probably helpful to some people, but I think it would probably be reasonable to have that information in a tooltip instead of prominently displayed in the UI. This is what we do with the device address for Bluetooth devices for example.

Instead of listing the app ID as description text, it would probably make the UI more glanceable to list the permissions that are allowed. This is what we do in Notification settings for example.

Thanks again for your work on this! I'm excited to have these detailed app permissions available

@meisenzahl
Copy link
Member Author

meisenzahl commented Feb 22, 2020

@danrabbit many thanks for your detailed feedback and constructive suggestions!

As suggested, I have removed the options for X11 fallback and Wayland, as they are not needed.

I implemented your UI suggestions and added icons and descriptions to the permissions view. In my opinion the UI is now more appealing. But I think you probably have more good ideas to make it even more user friendly 😉️

Screenshot von 2020-02-22 09 19 06

Screenshot von 2020-02-22 09 20 02

Screenshot von 2020-02-22 09 20 06

Instead of listing the app ID as description text, I now list the permissions that are allowed.

Screenshot von 2020-02-22 09 19 43

I look forward to your feedback!

@danirabbit danirabbit added this to In progress in Sandboxing & Portals via automation Feb 24, 2020
@danirabbit danirabbit added this to In progress in AppCenter For Everyone Sprint via automation Mar 10, 2020
@danirabbit danirabbit moved this from In progress to Design Track in AppCenter For Everyone Sprint Mar 10, 2020
Copy link
Member

@danirabbit danirabbit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think at this point in the release cycle I'm inclined to merge this and we can iterate on the design and copy.

Thank you very much for you work on this!

@danirabbit danirabbit merged commit dc42e83 into elementary:master Jun 4, 2020
Sandboxing & Portals automation moved this from In progress to Done Jun 4, 2020
AppCenter For Everyone Sprint automation moved this from Design Track to Done Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants