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

linux: Fix saving file with root ownership #22045

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

0xtimsb
Copy link
Contributor

@0xtimsb 0xtimsb commented Dec 15, 2024

Closes #13585

Currently, saving files with root ownership or root as the group throws a Permission denied (os error 13). Please try again. error. This PR fixes the issue on Linux by prompting the user for a password and saving the file with elevated privileges.

It uses pkexec (Polkit), which is by default available on GNOME, KDE, and most Linux systems. I haven't implemented this for macOS as I don't have a device to test it on.

This implementation is similar to how Vscode handles it. Except, they don't show custom message.

Working:

When file saving fails due to a PermissionDenied error, we create a temporary file in the same directory as the target file and writes the data to this temporary file. After, the contents of this file are copied to the original file using the tee command instead of cp or mv. This ensures that the ownership and permissions of the original file are preserved. This command is executed using pkexec which will prompt user for their password.

Custom Message:

The message displayed to the user in the prompt is automatically retrieved from the org.zed.app.policy file, which is located at /usr/share/polkit-1/actions/. This file should be installed during the setup process. While the policy file is optional, omitting it will cause the user to see the underlying command being executed rather than a user-friendly message. Currently, VSCode does not display the user-friendly message.

The policy file must specify a unique binary, ensuring that only that binary can use the policy file. It cannot be as generic as a /bin/bash, as any software using bash to prompt will end up showing Zed’s custom message. To address this, we will create a custom bash script, as simple as the following, placed in /usr/bin/zed/elevate.sh. The script should have root ownership and should not reside in the home directory, since the policy file cannot resolve $HOME.

#!/bin/bash
eval "$@"

IMPORTANT NOTE

Since copying the policy file and our script requires sudo privileges, the installation script will now prompt for the password at very end. Only on Linux, if pexec is installed.

Screenshots:

KDE with policy file:
Screenshot from 2024-12-15 22-13-06

Gnome with policy file:
Screenshot from 2024-12-15 22-21-48

Gnome without policy file:
image

VSCode:
image

User declines the permission request:
image

Release Notes:

  • Fixed file saving with root ownership on Linux.

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Dec 15, 2024
script/install.sh Outdated Show resolved Hide resolved
@0xtimsb
Copy link
Contributor Author

0xtimsb commented Dec 16, 2024

@mikayla-maki moved it to a separate optional script, as you asked. Also added docs for it in linux.md.

@0xtimsb 0xtimsb requested a review from mikayla-maki December 16, 2024 19:58
@mikayla-maki
Copy link
Contributor

mikayla-maki commented Dec 16, 2024

@0xtimsb thanks for making that change

That said, I think my suggestion has terrible UX… could we do something like detect if the user has Polkit installed, and then prompt them if they want to install our policy, and only then escalate the script’s permissions to su so we can do the installation?

@0xtimsb
Copy link
Contributor Author

0xtimsb commented Dec 16, 2024

Yes, it's weird to ask them to run script to get pretty message :P. Updated it to do it in install.sh itself.

Now, in case Polkit is detected, we prompt the user to ask if we should configure Polkit. Only when the user accepts (y/Y/ enter) will it ask for the user's password.

yes

If the user rejects the installation will proceed normally.

no

If Polkit is not detected, we simply add a warning and a docs link to install it. This should be very rare.

no

script/install.sh Outdated Show resolved Hide resolved
script/install.sh Show resolved Hide resolved
Comment on lines 145 to 146
echo "Zed needs sudo access to improve root file editing experience."
printf "Configure polkit? [Y/n] "
Copy link
Contributor

Choose a reason for hiding this comment

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

This could potentially be more beginner-friendly if worded as something like "Configure polkit to allow Zed to request elevated permissions when editing system files?"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, not super accurate (since it’d still work even after saying no), but let’s just write it this way for simplicity's sake.

script/install.sh Outdated Show resolved Hide resolved
crates/fs/src/fs.rs Outdated Show resolved Hide resolved
let mut cmd = Command::new(pkexec_path);
cmd.arg("--disable-internal-agent");

let script_path = PathBuf::from("/usr/libexec/zed/elevate.sh");
Copy link
Contributor

Choose a reason for hiding this comment

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

This is also not going to work on NixOS. Would be nice to have a way to specify this as a command line flag or env variable while building so we don't have to resort to patching the code in the NixOS package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ve added the env variable which can be used in Nix installation. But, I'm not picking that in this PR as I'm not familiar with Nix yet. It should still work on NixOS as long as pkexec is in the PATH without custom message. I will create another PR after setting up NixOS VM and testing there.

crates/fs/src/fs.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFE/Linux: polkit integration
3 participants