filebrowser: 2.23.0 -> 2.28.0, add service module #289750
Conversation
|
I haven't done rigorous testing, and I'm not experienced enough to provide full review of a module, but it is working well for me and seems good overall. Filebrowser's configuration has setting the host/port and socket under the same field, but that feels a bit weird IMO. Additionally, sockets typically (AFAIK) get put in Though, this directory would be useless if sockets are turned off. Maybe Also, I think |
Thanks for the review! I'll happily discuss how to make this module better.
Well, while writing this I pondered over this quite a bit myself, since filebrowser itself takes a separate option for the socket. But most services take either a IP-address or a socket path for the address, since the type can be determined quite reliably anyway. Additionally, sockets typically (AFAIK) get put in
Most modules AFAIK just put it under services.filebrowser.listenAddress = { address = "0.0.0.0"; port = 8080; }; # or
services.filebrowser.listenAddress = { socket = "/run/filebrowser.sock"; mode = "0666"; };
Seems sensible. 👍 Personally I don't have a strong opionon on this tho. |
|
That sounds like a good method, it still feels a bit weird but I'm not sure there's a better way. For the socket path, |
christoph-heiss
force-pushed
the
filebrowser
branch
from
716c44b
to
6e6f55a
Compare
christoph-heiss
changed the title
christoph-heiss
force-pushed
the
filebrowser
branch
from
6e6f55a
to
1472aeb
Compare
Signed-off-by: Christoph Heiss <[email protected]>
Signed-off-by: Christoph Heiss <[email protected]>
This package hasn't been updated in ~1.5 years and the maintainer does not seem to respond. Signed-off-by: Christoph Heiss <[email protected]>
christoph-heiss
force-pushed
the
filebrowser
branch
from
1472aeb
to
3e98a07
Compare
|
@fin444 Sorry for the long feedback loop! In any case, I have now simply removed the socket option, since it's quite a bit of work and needs a more thought-out plan as can be seen. I'd rather not get this PR stuck on this forever. |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/3816 |
|
Marking @nielsegberts here #290642 |
Signed-off-by: Christoph Heiss <[email protected]>
Signed-off-by: Christoph Heiss <[email protected]>
christoph-heiss
force-pushed
the
filebrowser
branch
from
3e98a07
to
7336e46
Compare
| ${pkgs.coreutils}/bin/mkdir -p ${toString cfg.rootDir} | ||
| ''; | ||
|
|
||
| ExecStart = "${cfg.package}/bin/filebrowser --config ${configFile}"; |
There was a problem hiding this comment.
| ExecStart = "${cfg.package}/bin/filebrowser --config ${configFile}"; | |
| ExecStart = "${lib.getExe cfg.package} --config ${configFile}"; |
| NoNewPrivileges = "yes"; | ||
| PrivateTmp = "yes"; | ||
| PrivateDevices = "yes"; | ||
| DevicePolicy = "closed"; | ||
| ProtectSystem = "strict"; | ||
| ProtectHome = "tmpfs"; | ||
| ProtectControlGroups = "yes"; | ||
| ProtectKernelModules = "yes"; | ||
| ProtectKernelTunables = "yes"; | ||
| RestrictAddressFamilies = "AF_UNIX AF_INET AF_INET6"; | ||
| RestrictNamespaces = "yes"; | ||
| RestrictRealtime = "yes"; | ||
| RestrictSUIDSGID = "yes"; | ||
| MemoryDenyWriteExecute = "yes"; | ||
| LockPersonality = "yes"; |
There was a problem hiding this comment.
In nixpkgs we mostly use true instead of "yes"
| type = types.str; | ||
| default = "filebrowser"; | ||
| description = '' | ||
| If `services.filebrowser.user` is set, this must be set as well. Must |
There was a problem hiding this comment.
Can't we create the default user?
| disableThumbnails = mkEnableOption "Disable image thumbnails."; | ||
| disablePreviewResize = mkEnableOption "Disable resize of image previews."; | ||
| disableCommandRunner = mkEnableOption "Disable the Command Runner feature."; | ||
| disableTypeDetectionByHeader = mkEnableOption | ||
| "Disable file type detection by reading file headers."; |
There was a problem hiding this comment.
The description of those will be prefixed with Wether to enable which I don't think the description considered.
|
|
||
| baseUrl = mkOption { | ||
| type = types.nullOr types.str; | ||
| default = null; |
There was a problem hiding this comment.
Shouldn't this default to / ?
| cert = cfg.tlsCertificate; | ||
| key = cfg.tlsCertificateKey; | ||
| }) | ||
| // (optionalAttrs (cfg.baseUrl != null) { baseurl = cfg.baseUrl; }) |
There was a problem hiding this comment.
optionalAttrs doesn't require sourounding brackets
| // (optionalAttrs (cfg.cacheDir != null) { cache-dir = cfg.cacheDir; }) | ||
| // (optionalAttrs cfg.disableThumbnails { disable-thumbnails = true; }) | ||
| // (optionalAttrs cfg.disablePreviewResize { disable-preview-resize = true; }) | ||
| // (optionalAttrs cfg.disableCommandRunner { disable-exec = true; }) | ||
| // (optionalAttrs cfg.disableTypeDetectionByHeader { disable-type-detection-by-header = true; }) |
There was a problem hiding this comment.
Why not use an rfc 42 freeform setting type for this?
| cfg.tlsCertificate != null; | ||
|
|
||
| configFile = pkgs.writeText "filebrowser-config.json" (generators.toJSON {} ({ | ||
| database = "/var/lib/filebrowser/filebrowser.db"; |
There was a problem hiding this comment.
We use 2 spaces to intend nix code
| database = "/var/lib/filebrowser/filebrowser.db"; | |
| database = "/var/lib/filebrowser/filebrowser.db"; |
| assert cfg.user != "filebrowser" -> cfg.group != "filebrowser"; | ||
| cfg.user == "filebrowser"; | ||
| enableTls = | ||
| assert cfg.tlsCertificate != null -> cfg.tlsCertificateKey != null; |
There was a problem hiding this comment.
This should be a module level assertion
| enableDynamicUser = | ||
| assert cfg.user != "filebrowser" -> cfg.group != "filebrowser"; | ||
| cfg.user == "filebrowser"; |
There was a problem hiding this comment.
I find it kinda strange and unexpected that DynamicUser gets disabled when the user and group settings are changed.
|
PING |
Description of changes
Updates the
filebrowserpackage from 2.23.0 to 2.28.0 and adds an appropriate service module for it.Changelog: filebrowser/filebrowser@v2.23.0...v2.28.0
cc @nielsegberts As this package has not been updated in nearly a year, are you still interested?
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.