serenity icon indicating copy to clipboard operation
serenity copied to clipboard

Base+Taskbar: Support launching apps that require root like PartitionEditor

Open SamuelBowman opened this issue 2 years ago • 4 comments

Thanks to @ne0ndrag0n, Settings supports launching apps like NetworkSettings that require root via Escalator. This PR simply extends this functionality to Taskbar so apps like PartitionEditor can benefit too.

The only difference is that in the case that Taskbar launches an root requiring executable in Terminal, we use pls instead of Escalator. No apps currently take advantage of both RunInTerminal and RequiresRoot like this, but it does work if you create such an app file. I tested this by installing the Python port and adding RequiresRoot=true to its app file.

SamuelBowman avatar Oct 15 '22 14:10 SamuelBowman

I think in general I like this, but I wonder whether the Partition Editor's UI (and also network settings) needs to run as root - or whether a non-privileged UI that talks to a higher privileged service would make more sense as a general model.

gunnarbeutner avatar Oct 15 '22 16:10 gunnarbeutner

I think it'd ideally be preferable for UIs to run unprivileged and only ask for elevated privileges when they're actually needed (ie. when you click the OK / Apply button to commit your changes). It'd be a better user experience with less security risk, but I'm not sure the best way to architect such a thing. The privileged parts of NetworkSettings could probably be moved into a NetworkManager-like service. I'm less sure what would be good to do for PartitionEditor. Maybe a DiskManager service that is started on demand? Or maybe just a privileged FileSystemAccessServer might be enough to get fds for the raw block devices.

SamuelBowman avatar Oct 15 '22 18:10 SamuelBowman

Maybe something like this:

  • LibNetworkSettings / LibPartitionEditor which contains both non-privileged and privileged APIs. Other apps can then use those instead of dealing with JSON files directly (e.g. SystemMonitor).
  • Like you mentioned: NetworkSettingsServer (or just part of NetworkServer?) / DiskManagerServer (not a huge fan of those names just yet) provide the privileged APIs for those libraries.

gunnarbeutner avatar Oct 16 '22 05:10 gunnarbeutner

If you go the service route, I have two commits that may be helpful. They separate Escalator's GUI from the part that execs the command, and moves EscalatorWindow into a library where it can be reused by services to validate one's password in a popup before continuing - the model you would use here is how FileSystemAccessServer will pause on a dialogue when the user drags-and-drops files into applications. This is currently slated to be a part of Users and Groups, a new settings dialogue which will run as anon and allow IPC operations into a server that will prompt before performing the root-privilege activity. Let me know if you would like me to split the Escalator commits out into a separate, earlier PR.

https://github.com/SerenityOS/serenity/commit/8b61a87b47131657ebf692856c9470c4e8f20bff https://github.com/SerenityOS/serenity/commit/a2e885ff61d0f7e933f57cdd5355f1ade7ea186b

ne0ndrag0n avatar Oct 18 '22 04:10 ne0ndrag0n

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

stale[bot] avatar Nov 08 '22 05:11 stale[bot]