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

privatebin: init at 1.5.2; nixos/privatebin: init, nixosTests.privatebin: init, pbincli: init at 0.3.5 #234287

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

e1mo
Copy link
Member

@e1mo e1mo commented May 26, 2023

Description of changes

PrivateBin is an end to end encrypted, zero knoweledge, pastebin service. This PR also adds a module and tests. PBinCLI is a third party client to work with PrivateBin, which is used in tests (I am not motivated to implement the clientside crypto by hand in the tests).

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` labels May 26, 2023
@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels May 26, 2023
@e1mo e1mo requested review from illustris and removed request for illustris May 26, 2023 20:14
@e1mo e1mo requested a review from onny May 27, 2023 08:28
@onny
Copy link
Contributor

onny commented May 27, 2023

Result of nixpkgs-review pr 234287 run on x86_64-linux 1

3 packages marked as broken and skipped:
  • tests.testers.nixosTest-example
  • tests.testers.runNixOSTest-example
  • tests.trivial-builders.references
2 packages blacklisted:
  • nixos-install-tools
  • tests.nixos-functions.nixos-test
3 packages built:
  • pbincli
  • pbincli.dist
  • privatebin

@Minion3665
Copy link
Member

As this is a new module, my understanding is that it is backportable. May we do so when this is merged?

Copy link
Member

@Minion3665 Minion3665 left a comment

Choose a reason for hiding this comment

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

Just set-up privatebin from this patch and a couple things bugged me. Overall, though, this is working fine- thank you and very well done!

nixos/modules/services/web-apps/privatebin.nix Outdated Show resolved Hide resolved
@e1mo e1mo requested a review from Minion3665 August 6, 2023 11:51
Copy link
Member

@Minion3665 Minion3665 left a comment

Choose a reason for hiding this comment

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

Very happy with the solution of sorting by the stated time, it might be worth talking to upstream privatebin if we still want to be able to sort differently but I'm not sure that we need to.

Just this last nit before I'm completely good with this PR :)

nixos/modules/services/web-apps/privatebin.nix Outdated Show resolved Hide resolved
@e1mo e1mo requested a review from Minion3665 August 6, 2023 14:59
Copy link
Member

@Minion3665 Minion3665 left a comment

Choose a reason for hiding this comment

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

Thank you very much for your hard work, this seems good to me!

@Minion3665 Minion3665 added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Aug 6, 2023
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 6, 2023
@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Sep 7, 2023
@h7x4 h7x4 added 8.has: module (new) This PR adds a module in `nixos/` 8.has: tests This PR has tests and removed 8.has: module (update) This PR changes an existing module in `nixos/` labels Oct 16, 2023
@SuperSandro2000
Copy link
Member

Can you rebase to fix the merge conflict?

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

Thank you so much for this. I hope I can leverage and test this soon!

Comment on lines +28 to +29
binaryWorks = runCommand "${pname}-binary-test" {} ''
export PATH=${pbincli}/bin:$PATH
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
binaryWorks = runCommand "${pname}-binary-test" {} ''
export PATH=${pbincli}/bin:$PATH
binaryWorks = runCommand "${pname}-binary-test" {
nativeBuildInputs = [ pbincli ];
} ''

Comment on lines +9 to +14
pname = "PBinCLI";
version = "0.3.5";
format = "setuptools";

src = fetchPypi {
inherit pname version;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pname = "PBinCLI";
version = "0.3.5";
format = "setuptools";
src = fetchPypi {
inherit pname version;
pname = "pbincli";
version = "0.3.5";
format = "setuptools";
src = fetchPypi {
pname = "PBinCLI";
inherit version;

see python language docs

virtualHosts.privatebin = mkMerge [
cfg.nginx
{
root = mkForce "${pkgs.privatebin}/share/privatebin";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
root = mkForce "${pkgs.privatebin}/share/privatebin";
root = mkForce pkgs.privatebin;

we could also just place it here like other php web things

cfg.nginx
{
root = mkForce "${pkgs.privatebin}/share/privatebin";
extraConfig = optionalString (cfg.nginx.addSSL || cfg.nginx.forceSSL || cfg.nginx.onlySSL || cfg.nginx.enableACME) "fastcgi_param HTTPS on;";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
extraConfig = optionalString (cfg.nginx.addSSL || cfg.nginx.forceSSL || cfg.nginx.onlySSL || cfg.nginx.enableACME) "fastcgi_param HTTPS on;";
extraConfig = optionalString (cfg.nginx.addSSL || cfg.nginx.forceSSL || cfg.nginx.onlySSL) "fastcgi_param HTTPS on;";

acme is not required for https

Comment on lines +260 to +262
"/" = {
index = "index.php";
};
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"/" = {
index = "index.php";
};
"/".index = "index.php";


phpPackage = mkOption {
type = types.package;
relatedPackages = [ "php80" "php81" "php82" ];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
relatedPackages = [ "php80" "php81" "php82" ];
relatedPackages = [ "php81" "php82" ];

that's EOL


databaseSetup = {
enable = mkEnableOption (lib.mdDoc "Automatic database setup and configuration");
kind = mkOption {
Copy link
Member

Choose a reason for hiding this comment

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

How about we default this to sqlite so that things just work?

info = "This instance of PrivateBin is hosted on NixOS!";
languageselection = true;
icon = "none";
cspheader = "default-src 'none'; base-uri 'self'; form-action 'none'; manifest-src 'self'; connect-src * blob:; script-src 'self' 'unsafe-eval'; style-src 'self'; font-src 'self'; frame-ancestors 'none'; img-src 'self' data: blob:; media-src blob:; object-src blob:; sandbox allow-same-origin allow-scripts allow-forms allow-popups allow-modals allow-downloads";
Copy link
Member

Choose a reason for hiding this comment

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

There is no upstream value we can just copy and enable?

} else if autoDb == "sqlite" then {
model.class = "Database";
model_options.dsn = "sqlite:/var/lib/privatebin/data/db.sqlite3";
} else {}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
} else {}
};

there is no other option

isEnv = v: isAttrs v && v ? _env && isString v._env;
format =
let
iniAtom = (pkgs.formats.ini {}).type/*attrsOf*/.functor.wrapped/*attrsOf*/.functor.wrapped;
Copy link
Member

Choose a reason for hiding this comment

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

What are the comments here supposed to mean?

, config
, ...
}:
with lib;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
with lib;

to address #208242 for this module, uses of with such as this should be avoided.
consider inherits where you find yourself using lib.foo excessively.
this nix.dev page on best practices with with may be helpful

meta.maintainers = with lib.maintainers; [ e1mo ];

options.services.privatebin = {
enable = mkEnableOption (mdDoc "PrivateBin web application");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
enable = mkEnableOption (mdDoc "PrivateBin web application");
enable = mkEnableOption "PrivateBin web application";

lib.mdDoc is now just an alias and can be safely removed everywhere.
see d36f950 and #237557

@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 8.has: module (new) This PR adds a module in `nixos/` 8.has: package (new) This PR adds a new package 8.has: tests This PR has tests 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants