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.

Sign up for GitHub

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

Jump to bottom

transmission: Configurable download directory permissions #70542

Merged
merged 1 commit into from Dec 8, 2019
Merged

transmission: Configurable download directory permissions #70542

merged 1 commit into from Dec 8, 2019

Conversation

dsg22
Copy link
Contributor

@dsg22 dsg22 commented Oct 6, 2019

The ExecStartPre script previously ran a 'chmod 770' on all directories,
including complete and incomplete download directories. Change this
so it only ensures user and group rights, but don't touch world
permissions.

Motivation for this change

I would like to access my torrent complete folder (stored on NFS) from other sources. I have correct permissions set there, but on every start of the transmission service it resets the permissions to 770.

See also issue #55161

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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.
Notify maintainers

cc @astsmtl @vcunat @wizeman

@wizeman
Copy link
Member

wizeman commented Oct 7, 2019

Hmm, this probably merits some discussion.

I had a similar problem as you. In my case I wanted to make sure that both the download-dir and incomplete-dir were readable by others over NFS. However, when I was looking into this I have also locally modified the transmission service to set the homeDir and settingsDir permissions to 700 (not readable by the transmission group nor by others), as I didn't see a need for anything more permissive. The latter works perfectly fine for me.

If there's a need for different permissions on different machines, perhaps they should be configurable?
I'd be happy to get rid of my local modifications.

@dsg22
Copy link
Contributor Author

dsg22 commented Oct 7, 2019

Good points. Indeed, 700 should be enough. I also agree that making the permissions for download-dir and incomplete-dir configurable would be better.

I'll have another go at this and update the PR later.

@dsg22 dsg22 changed the title transmission: Don't trample directory permissions transmission: Configurable download directory permissions Oct 7, 2019
@dsg22
Copy link
Contributor Author

dsg22 commented Oct 7, 2019

I've reworked this to add a new option downloadDirPermissions which gets applied to download-dir and incomplete-dir. It defaults to 770 like the old code. I think that's a bad default, but I'd also like to avoid a package update changing people's torrent directory permissions. I'd like to hear others thoughts on this.

This patch also makes homeDir and settingsDir 700 (non-configurable). I don't expect this will affect anyone.

@dsg22
transmission: Configurable download directory permissions
58c0a05 
Allow the user to specify the permissions to apply to download folders
used by transmission. This is useful e.g. when they are stored on a
network share and accessed by other users.

This commit also makes the home and config directories 700, as there
is should be no need for wider permissions there.
Lassulus
Lassulus approved these changes Dec 8, 2019
View reviewed changes
Copy link
Member

@Lassulus Lassulus left a comment

Choose a reason for hiding this comment

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

LGTM, I have similiar issues currently with my transmission setup

@Lassulus Lassulus merged commit 077934e into NixOS:master Dec 8, 2019
@chreekat
Copy link
Contributor

fyi, this change made the transmission service unusable with default settings.

  1. The download dir is a subdir of the home dir
  2. The home dir is 0700
  3. Therefore, nobody except the owner can traverse to the download dir

I have to also echo #55161 - why do the directories get their permissions set on every load of the service?

@Lassulus
Copy link
Member

hmm, this is indeed not very practical, I will have to fix that, thanks for pointing that out.

I guess we could add a check if the directory already exists and chmod it only then or do it in an activationScript.

@dsg22
Copy link
Contributor Author

dsg22 commented Feb 24, 2020

There are already two pull requests open to fix the issue caused by this PR:
#76552
#78113

I agree though that setting perms on every start is suboptimal.

@bb2020
Copy link
Member

bb2020 commented Feb 26, 2020

Can't we just merge bb2020@3625af9 that I've mentioned in #76552.
It should fix all transmission related issues.

@Lassulus
Copy link
Member

@bb2020 ah, this looks good, is there a PR open for that?

@bb2020 bb2020 mentioned this pull request Feb 26, 2020
Merged
10 tasks
@bb2020
Copy link
Member

bb2020 commented Feb 26, 2020

Created one just now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lassulus Lassulus Lassulus approved these changes

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0 12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@dsg22 @wizeman @chreekat @Lassulus @bb2020 @Janik-Haag