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

keepassxc: 2.2.4 -> 2.3.0 #36281

Merged
merged 2 commits into from Mar 4, 2018
Merged

keepassxc: 2.2.4 -> 2.3.0 #36281

merged 2 commits into from Mar 4, 2018

Conversation

jonafato
Copy link
Contributor

@jonafato jonafato commented Mar 4, 2018

Motivation for this change

See the changelog.

Changes to options:

  • withKeePassBrowser: New in version 2.3.0 and replaces the functionality provided by the KeePassHTTP plugin. Users should install the KeePassXC-Browser extension. Defaults to true to remain consistent with the old default for withKeePassHTTP.

  • withKeePassHTTP: This feature is deprecated and disabled by default in the source repository. Default changed from true to false.

  • withKeePassNetworking: Controls all network access code within KeePassXC. Defaults to false to match the source repository.

  • withKeePassSSHAgent: Enables the SSH Agent plugin for use with [KeeAgent](https://lechnology.com/software

  • Add darwin to libargon2 platforms for MacOS support.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-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 nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@jonafato
libargon2: add darwin to platforms
29f70db
@jonafato
keepassxc: 2.2.4 -> 2.3.0
e0ba76c 
See the [changelog](https://github.com/keepassxreboot/keepassxc/blob/a06e85f6ceac39851ca694464f74c927dd953f1e/CHANGELOG#L1-L51).

Changes to options:

- `withKeePassBrowser`: New in version 2.3.0 and replaces the
  functionality provided by the KeePassHTTP plugin. Users should
  install the
  [KeePassXC-Browser extension](https://github.com/keepassxreboot/keepassxc-browser).
  Defaults to `true` to remain consistent with the old default for
  `withKeePassHTTP`.

- `withKeePassHTTP`: This feature is deprecated and disabled by default
  in the source repository. Default changed from `true` to `false`.

- `withKeePassNetworking`: Controls all network access code within
  KeePassXC. Defaults to `false` to match the source repository.

- `withKeePassSSHAgent`: Enables the SSH Agent plugin for use with
  [KeeAgent](https://lechnology.com/software/keeagent/). Defaults to
  `false` to match the source repository.
@jonafato jonafato changed the title Keepassxc 2.3.0 keepassxc: 2.2.4 -> 2.3.0 Mar 4, 2018
@jonafato jonafato mentioned this pull request Mar 4, 2018
Closed
8 tasks
Mic92
Mic92 reviewed Mar 4, 2018
View reviewed changes
pkgs/applications/misc/keepassx/community.nix
, withKeePassBrowser ? true
, withKeePassHTTP ? false
, withKeePassNetworking ? false
, withKeePassSSHAgent ? false
Copy link
Member

Choose a reason for hiding this comment

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

You would not enable this?

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 don't use the feature, so I don't have much of an opinion on this. I went with the default, but I'm happy to change it if it's generally useful to others.

Copy link
Member

Choose a reason for hiding this comment

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

The default is not every useful. It also disable WITH_XC_BROWSER.

@drdaeman
Copy link
Contributor

drdaeman commented Mar 4, 2018

Thanks for this PR!

JFYI: I've had to manually specify keepassxc-proxy location in the settings to get KeePassXC-Browser detect the app. I'm really not sure, but it could be that something out there needs some manual tweaking to Nix/NixOS filesystem layout.

@Mic92 Mic92 merged commit bcb5271 into NixOS:master Mar 4, 2018
@Mic92
Copy link
Member

Mic92 commented Mar 4, 2018

Let's address the browser location in a different pull request.

@Mic92
Copy link
Member

Mic92 commented Mar 4, 2018 •
edited

I you don't specify a path it defaults to keepassxc-proxy:

$ cat /home/joerg/.mozilla/native-messaging-hosts/org.keepassxc.keepassxc_browser.json
{
    "allowed_extensions": [
        "keepassxc-browser@keepassxc.org"
    ],
    "description": "KeePassXC integration with native messaging support",
    "name": "org.keepassxc.keepassxc_browser",
    "path": "/nix/store/vrjh4h32mw8ffnjx5kgy6w3k08gwg1gr-keepassxc-2.3.0/bin/keepassxc-proxy",
    "type": "stdio"
}

However this path might be garbage collected eventually.
Therefore a patch to replace it by the output of which keepassxc-proxy might be sensible.
Anyone who is more experience with C++/QT?

@Mic92
Copy link
Member

Mic92 commented Mar 4, 2018

The string that needs to replaced here:
https://github.com/keepassxreboot/keepassxc/blob/develop/src/browser/HostInstaller.cpp#L178

@drdaeman
Copy link
Contributor

drdaeman commented Mar 4, 2018 •
edited

Never mind my comment above - I was wrong, it also works without specifying the path. Not sure why KeePassXC sometimes does not get detected (at least, for me) unless I open the settings dialog, but don't think it's a packaging issue.

BTW, that C++ code looks correct to me - it takes keepassxc's binary path (QFileInfo(QCoreApplication::applicationFilePath()).absolutePath()) and deduces keepass-proxy path from it assuming both are in the same directory. If "update manifest files" is enabled, package upgrades should work without issues.

@jonafato jonafato deleted the keepassxc-2.3.0 branch March 4, 2018 17:52
@Mic92
Copy link
Member

Mic92 commented Mar 4, 2018

I wonder why updating the manifest is not set by default.

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

@Mic92 Mic92 Mic92 left review comments

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jonafato @drdaeman @Mic92 @GrahamcOfBorg