flathub / org.gnome.Lollypop

https://flathub.org/apps/details/org.gnome.Lollypop
5 stars 9 forks source link

Change filesystem access to xdg-music only #108

Closed Eonfge closed 3 years ago

Eonfge commented 3 years ago

Not functional at the moment, requires some upstream attention

flathubbot commented 3 years ago

Started test build 30130

Eonfge commented 3 years ago

@bellegarde-c In line with Flatpak specs, I made a special Lollypop branch that has the sandbox more restrictive. I hope you can take a look at it, because at the moment there are two issues:

I have no intention of releasing this in its current state, but I do want you and others to be able to test it.

flathubbot commented 3 years ago

Build 30130 successful To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/29016/org.gnome.Lollypop.flatpakref
bellegarde-c commented 3 years ago

flatpak install --user https://dl.flathub.org/build-repo/29016/org.gnome.Lollypop.flatpakref

Done this but looks like Lollypop is not sandboxed here.

bellegarde-c commented 3 years ago

sandbox

Eonfge commented 3 years ago

Weird. I previously had issues opening the portal browser. Well, I'll run it through a few more tests in that case.

The primary problem right now... is that this change is not backwards compatible. Users will have to re-select their music folders (other then dxg-music) to reimport their libraries. Is this worth the hassle?

Do you foresee any issues when we tighten up the sandbox permissions?

@bilelmoussaoui do you have any comments concerning the sandboxing of Lollypop?

flathubbot commented 3 years ago

Started test build 30498

flathubbot commented 3 years ago

Build 30498 successful To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/29378/org.gnome.Lollypop.flatpakref
bilelmoussaoui commented 3 years ago

As lollypop allows the user to select their music folder/add new ones, the change in the permissions won't work for everyone. Although the portal does support selecting folders now, it won't work for users with older version :/ So I would prefer to wait a bit before merging this.

Eonfge commented 3 years ago

I actually ran into that myself. My version of Lollypop had a direct link to a non-xdg location. So over time, people will have to re-configure lollypop for the paths to properly migrate? I'll test that a few times, and we might have to hold off on it for a year or so, making sure that the fallout is not to big.

flathubbot commented 3 years ago

Started test build 30912

flathubbot commented 3 years ago

Build 30912 successful To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/29778/org.gnome.Lollypop.flatpakref
flathubbot commented 3 years ago

Started test build 30979

flathubbot commented 3 years ago

Build 30979 successful To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/29844/org.gnome.Lollypop.flatpakref
Eonfge commented 3 years ago

@bilelmoussaoui I've done some testing. As of right now, it will cause issues when we deploy this update, even for users who only downloaded Lollypop yesterday.

What I've tested:

I had to re-select my music library and I had to reimport it. The current version of Lollypop and GtkFileChooserNetive, don't use a a /var/run/ mount when selecting files, if they don't have to. As such, when updating lollypop it will be unable to access the files in ~/CustomMusicFolder/

So, what's the plan? I do want to improve the security in the future

bellegarde-c commented 3 years ago

@Eonfge Can I do something from Lollypop POV ?

bilelmoussaoui commented 3 years ago

Nothing can be done regarding the migration as the files/directories you grant an application the permission to open/read/write are stored in the flatpak documents store and are added once you open a file using the portal (the native file dialog). The directories has to be re-added so that Lollypop has actually the permission granted to open/read those files instead of "having permissions to everything"

bilelmoussaoui commented 3 years ago

@Eonfge Can I do something from Lollypop POV ?

what would be nice is to migrate all file choosers to native ones as a first step

Eonfge commented 3 years ago

@bellegarde-c I was thinking about that. Would it be possible to add a -DFlatpak=true flag to meson?

You can then show a message that informs users about their folders and permissions when they start Lollypop and a folder is missing. If a folder is missing, you can tell users like like "Sandboxing good. Sandboxing WIP. Please re-import".

The comment of @bilelmoussaoui is certainly true, making sure all file choosers are up-to-date is important, but I think we can't escape the realization that enforcing sandboxing does require a minor action from the user.

Eonfge commented 3 years ago

@bellegarde-c Would it be possible to provide some user-notification when they start the Flatpak version first?

If you could include such a message, I can combine it with this change. Then, when users update Lollypop to a more sandboxed version, they'll be informed about the change. The flag and notification could be temporary, because as time progresses all people will migrate. After a few months, we could remove it.

flathubbot commented 3 years ago

Started test build 32608

flathubbot commented 3 years ago

Build 32608 successful To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/31417/org.gnome.Lollypop.flatpakref
flathubbot commented 3 years ago

Started test build 32676

flathubbot commented 3 years ago

Build 32676 successful To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/31475/org.gnome.Lollypop.flatpakref
flathubbot commented 3 years ago

Started test build 32822

flathubbot commented 3 years ago

Build 32822 successful To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/31620/org.gnome.Lollypop.flatpakref
flathubbot commented 3 years ago

Started test build 33272

flathubbot commented 3 years ago

Build 33272 successful To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/32046/org.gnome.Lollypop.flatpakref
flathubbot commented 3 years ago

Started test build 33889

flathubbot commented 3 years ago

Build 33889 successful To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/32647/org.gnome.Lollypop.flatpakref
flathubbot commented 3 years ago

Started test build 34699

flathubbot commented 3 years ago

Build 34699 successful To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/33410/org.gnome.Lollypop.flatpakref
Eonfge commented 3 years ago

Screenshot from 2020-12-20 13-26-48

@bellegarde-c Would such a one-time notice be possible? There is not really a way to migrate users, but I do want to remove the host-access from this Flatpak.

It should only show on startup when the project is build with '-DFlatpak=true' and it should be a one-time thing. After a few months, we can remove it again since then all users would have migrated. In the backend, we can use a simple g-schema bool to check if the user has ever clicked on continue, which would prevent it from showing ever again.

flathubbot commented 3 years ago

Started test build 35009

flathubbot commented 3 years ago

Build 35009 successful To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/33708/org.gnome.Lollypop.flatpakref
bellegarde-c commented 3 years ago

Here we go: https://gitlab.gnome.org/World/lollypop/-/commit/d7bc69b67ffb045bde51dd3c57163113fcd23001

Don't want technical information. Sandboxing is not a user word :-)

migration1

migration2

It's a one time migration, so we need to test this before releasing on flathub ;)

To test again, in dconf-editor:

dconf

flathubbot commented 3 years ago

Started test build 35309

flathubbot commented 3 years ago

Build 35309 successful To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/33982/org.gnome.Lollypop.flatpakref
Eonfge commented 3 years ago

Don't want technical information. Sandboxing is not a user word :-)

And that's why you make a kickass music player, and I bother packaging it :innocent:

I'll be doing some testing using this version and if all goes according to plan, then I'll change the sandboxing settings with the next release.

Eonfge commented 3 years ago

Well, first tests positive, works both with my existing player and in a VM.

Do have a few nit-picks though:

Unrelated:

bellegarde-c commented 3 years ago

The dialogue is not necessary for users who install Lollypop fresh

Does it mean that XDG music dir is available by default ?

Eonfge commented 3 years ago

The dialogue is not necessary for users who install Lollypop fresh

Does it mean that XDG music dir is available by default ?

That seems completely reasonable for a music player, so I've enabled it by default. Lollypop can also access GVFS in case that people host their music on a network.

bellegarde-c commented 3 years ago

Do you think there is a simple way to test wizard is needed ?

Eonfge commented 3 years ago

Do you think there is a simple way to test wizard is needed ?

Not sure... @bilelmoussaoui Do you know if it's possible to detect a dxg-music folder from the inside?

There is of cause a simple workaround: Only show the message if there is already music imported.

bellegarde-c commented 3 years ago

@bilelmoussaoui What I want is detecting configured folders in Lollypop with access denied by sandbox.

bellegarde-c commented 3 years ago
  • The pop-up about 'Automatically download metadata?' doesn't actually enable network access, thus being non-functional

This one is working here. Do you mean that lollypop is not downloading artwork or that the setting is not updated ?

bilelmoussaoui commented 3 years ago

@bilelmoussaoui What I want is detecting configured folders in Lollypop with access denied by sandbox.

You can query the documents portal and see which permissions your app has for a specific folder/file

Eonfge commented 3 years ago
  • The pop-up about 'Automatically download metadata?' doesn't actually enable network access, thus being non-functional

This one is working here. Do you mean that lollypop is not downloading artwork or that the setting is not updated ?

It would be the setting then. This is what I see after I click agree: Screenshot from 2020-12-27 14-28-35

bellegarde-c commented 3 years ago

It modifies the background data, it's another issue :D

bellegarde-c commented 3 years ago

Updated Lollypop to handle migration only if needed.