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

Add integration with Brave browser (for Linux, OSX, and Windows) #2933

Merged
merged 1 commit into from
Apr 8, 2019
Merged

Add integration with Brave browser (for Linux, OSX, and Windows) #2933

merged 1 commit into from
Apr 8, 2019

Conversation

mjgiarlo
Copy link

@mjgiarlo mjgiarlo commented Apr 3, 2019 •
edited
Loading

Type of change

  • ✅ New feature (non-breaking change which adds functionality)

Description and Context

Fixes #2414

Screenshots

Screenshot from 2019-04-05 14-21-06

Testing strategy

  • Ran unit tests and all 35 passed
  • Tested with binary built from this branch:
    • Manually uninstalled KPXC config JSON from Brave .config dir
    • Started binary
    • Checked the new "Brave" box in KPXC settings (see screenshot above)
    • Verified the config JSON was in Brave's .config/NativeMessagingHosts directory
    • Verified password auto-fill works in Brave

Checklist:

  • ✅ I have read the CONTRIBUTING document. [REQUIRED]
  • ✅ My code follows the code style of this project. [REQUIRED]
  • ✅ My change requires a change to the documentation, and I have updated it accordingly.
  • ✅ All new and existing tests passed. [REQUIRED]
  • ✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]

@mjgiarlo mjgiarlo mentioned this pull request Apr 3, 2019
Closed
mjgiarlo
mjgiarlo commented Apr 3, 2019
View reviewed changes
src/browser/HostInstaller.h Outdated Show resolved Hide resolved
@mjgiarlo mjgiarlo force-pushed the brave_integration_2414 branch 4 times, most recently from c353dff to 0916840 Compare April 4, 2019 16:38
@mjgiarlo
Copy link
Author

mjgiarlo commented Apr 5, 2019 •
edited
Loading

@varjolintu @droidmonkey @phoerious See updated description above. I believe this is now ready for another look, and has been fully tested w/ documentation added.

@droidmonkey droidmonkey self-requested a review April 6, 2019 00:48
@droidmonkey droidmonkey changed the base branch from develop to release/2.4.1 April 6, 2019 01:05
@droidmonkey droidmonkey added this to the v2.4.1 milestone Apr 6, 2019
@mjgiarlo
Copy link
Author

mjgiarlo commented Apr 6, 2019

Thx, @droidmonkey

@varjolintu
Copy link
Member

I'll review this tomorrow or the day after.

@mjgiarlo
Copy link
Author

mjgiarlo commented Apr 6, 2019 •
edited
Loading

The Windows build was passing before the force-push of 7a60055. Not sure why it is failing now. From TeamCity:

[01:17:27][Step 4/5] FAIL!  : TestCli::testClip() QTestLib: This test case check ("(((clipboard->text()) == (QString(""))))") failed because the requested timeout (1500 ms) was too short, 1800 ms would have been sufficient this time.
[01:17:27][Step 4/5] K:/c401303cba1b4098/tests/TestCli.cpp(303) : failure location

@droidmonkey any thoughts? Maybe a flappy build that needs to be restarted?

@droidmonkey
Copy link
Member

I tried a couple times, the tests pass on my local windows machine. Might need to reboot the ci.

@droidmonkey droidmonkey merged commit 1493943 into keepassxreboot:release/2.4.1 Apr 8, 2019
@mjgiarlo mjgiarlo deleted the brave_integration_2414 branch April 8, 2019 14:44
droidmonkey added a commit that referenced this pull request Apr 12, 2019
@droidmonkey
Release 2.4.1
7bafe65 
- Fix database deletion when using unsafe saves to a different file system [#2889]
- Fix opening databases with legacy key files that contain '/' [#2872]
- Fix opening database files from the command line [#2919]
- Fix crash when editing master key [#2836]
- Fix multiple issues with apply button behavior [#2947]
- Fix issues on application startup (tab order, --pw-stdin, etc.) [#2830]
- Fix building without WITH_XC_KEESHARE
- Fix reference entry coloring on macOS dark mode [#2984]
- Hide window when performing entry auto-type on macOS [#2969]
- Improve UX of update checker; reduce checks to every 7 days [#2968]
- KeeShare improvements [#2946, #2978, #2824]
- Re-enable Ctrl+C to copy password from search box [#2947]
- Add KeePassXC-Browser integration for Brave browser [#2933]
- SSH Agent: Re-Add keys on database unlock [#2982]
- SSH Agent: Only remove keys on app exit if they are removed on lock [#2985]
- CLI: Add --no-password option [#2708]
- CLI: Improve database extraction to XML [#2698]
- CLI: Don't call mandb on build [#2774]
- CLI: Add debug info [#2714]
- Improve support for Snap theming [#2832]
- Add support for building on Haiku OS [#2859]
- Ctrl+PgDn now goes to the next tab and Ctrl+PgUp to the previous
- Fix compiling on GCC 5 / Xenial [#2990]
- Add .gitrev output to tarball for third-party builds [#2970]
- Add WITH_XC_UPDATECHECK compile flag to toggle the update checker [#2968]
ahaczewski added a commit to ahaczewski/org.keepassxc.KeePassXC that referenced this pull request Apr 18, 2019
@ahaczewski
keepassxc: Grant access to Brave browser integration
0abcda2 
KeepassXC 2.4.1 introduced integration with Brave browser.
keepassxreboot/keepassxc#2933
AsavarTzeth pushed a commit to flathub/org.keepassxc.KeePassXC that referenced this pull request Apr 20, 2019
@ahaczewski @AsavarTzeth
keepassxc: Grant access to Brave browser integration
57ca2ce 
KeepassXC 2.4.1 introduced integration with Brave browser.
keepassxreboot/keepassxc#2933
@acesabe
Copy link

acesabe commented May 12, 2019

I've already posted to the snapcraft.io forums that the section for Browser Integration as seen in the above screenshot is missing in the Snap versions of KeepassXC (2.4.1-2.5.0)
e652d8dc050090f2a9e7388c91f702112ce97181

I'm hoping it's a Snap specific thing however I'm presuming that the browser select checkbox section does actually exist in the latest versions of KeepassXC?

@droidmonkey
Copy link
Member

@acesabe there is no issue here, this is by design.

@acesabe
Copy link

acesabe commented May 12, 2019

Thanks, so the issue is the link in the app to https://keepassxc.org/download/#linux links to an old ver of the helper script that doesn't include Brave.
The latest version
https://github.com/keepassxreboot/keepassxc/blob/2.4.1/utils/keepassxc-snap-helper.sh
Resolves the issue (y)

@droidmonkey
Copy link
Member

Oh wow I didn't realize that was outdated!

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

@varjolintu varjolintu varjolintu left review comments

@droidmonkey droidmonkey droidmonkey approved these changes

Assignees
No one assigned
Labels
feature: Browser
Projects
None yet
Milestone
v2.4.1
Development

Successfully merging this pull request may close these issues.
4 participants
@mjgiarlo @varjolintu @droidmonkey @acesabe