nextcloud-occ: replace sudo with su

[eyJhb]

Motivation for this change

Systems that have 100% replaced sudo with doas would not work with the script as is.
This script just needs to be run as root, and then it would work just fine.

@adisbladis for the patch, just posting this here.

Things done

• Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
• Built on platform(s)
  • NixOS
  • macOS
  • other Linux distributions
• Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
• Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
• Tested execution of all binary files (usually in ./result/bin/)
• Determined the impact on package closure size (by running nix path-info -S before and after)
• Ensured that relevant documentation is up to date
• Fits CONTRIBUTING.md.

[aanderse]

This script just needs to be run as root, and then it would work just fine.

That statement doesn't sound true to me at all... but I haven't actually checked. It seems like there would be a ton of implications if you run this as another user than currently intended...

[eyJhb]

The change is in not using sudo to change user, but rather use su. It does not run it as root, the actual things that should be executed, but rather you run the script as root, so it can do the 'su' part. The difference is here, not to depend on sudo to change user, as it is not required and causes issues for people without sudo installed.

…

On 3 March 2021 13.05.52 CET, Aaron Andersen ***@***.***> wrote: > This script just needs to be run as root, and then it would work just fine. That statement doesn't sound true to me at all... but I haven't actually checked. It seems like there would be a ton of implications if you run this as another user than currently intended... -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #114966 (comment)

[aanderse]

@eyJhb Sorry I misread that! LGTM 👍

[adisbladis]

Reading the expression again I finally realised what this is for:
occ is added to environment.systemPackages and this is mean for being able to run occ from the shell while running it as the correct user.

Hard-coding sudo as the privilege dropping method isn't very friendly and whether this is even going to work or not highly depends on your sudo setup.
sudo isn't even the only privilege escalation/dropping method, there is also doas for example and this module implicitly depends on sudo being present which isn't always the case.

The current sudo-based approach will implicitly require root access for a majority of setups while changing the privilege dropping would make that explicit and require a user to call sudo occ.

I think changing this to su makes more sense as it makes less assumptions about the surrounding system, but it should have a changelog entry.

[lukegb]

This probably needs a different approach: su doesn't take --preserve-env, so this would need to be --preserve-environment but you might not want the entire environment.

[eyJhb]

I have changed this, and currently su will perserve the env.
Seeing as this is being run by systemd services, etc. most of the time it should be OK.

I don't think there should be any issues with doing this.
I will check later/tomorrow and see how it works.

Are there any objections to this?

[Ma27]

I agree with @lukegb here.

[eyJhb]

Closing, might pick up in the future, but not needed as of now.