Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Add support for bitwarden password manager - resolves #4776 #6918

Merged
merged 1 commit into from
Mar 5, 2017

Conversation

kspearrin
Copy link
Contributor

@kspearrin kspearrin commented Jan 30, 2017

This change adds support for bitwarden password manager using the compatible Chrome extension:

https://chrome.google.com/webstore/detail/bitwarden-free-password-m/nngceckbapebfimnlniiiahkandclblb?hl=en

bitwarden does not require any external desktop application to be installed. It is similar to LastPass.

resolves #4776


  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).

Test Plan:

  • New tests added following the same pattern from all other password managers
  • Tests seem to pass properly when running, though overall build is broken right now for unrelated reasons.

@luixxiul
Copy link
Contributor

CC @jonathansampson for review 🙏

@bsclifton
Copy link
Member

bsclifton commented Jan 30, 2017

This is awesome, thanks @kspearrin! 😄

Heads up @bbondy, @diracdeltas (for the extension aspect, since we maintain our own store)

@kspearrin
Copy link
Contributor Author

kspearrin commented Jan 30, 2017

@bsclifton How does the store work? Any way for me to update/submit to the store or does it just feed from the Chrome web store? How are updates to the extension propagated? I'd be happy to update that aspect as well if necessary.

@bsclifton
Copy link
Member

@kspearrin good question... @bbondy would be able to answer in more detail 😄

I'd be more than happy to create a wiki page with the info, if you had a min to explain @bbondy?

@jonathansampson
Copy link
Collaborator

@kspearrin Thank you for the contribution! Would you mind removing all changes to locale files other than app/extensions/brave/locales/en-US/preferences.properties?

@kspearrin
Copy link
Contributor Author

@jonathansampson Done.

@kspearrin
Copy link
Contributor Author

Resolved conflicts with 29a8223

@luixxiul
Copy link
Contributor

luixxiul commented Feb 2, 2017

Does Muon support this? cf: #6991

@kspearrin
Copy link
Contributor Author

@luixxiul @bsclifton @bbondy muon support added to complete this addition here: brave/muon#153

@kspearrin
Copy link
Contributor Author

Conflicts resolved again.

@kspearrin
Copy link
Contributor Author

Can we expect this to be merged at any point in the near future? We have a lot of brave users that are asking about this.

@jonathansampson
Copy link
Collaborator

@kspearrin My apologies for not conducting an earlier review. Looking into this now for expedited consideration. Thank you for the great investment!

@jonathansampson
Copy link
Collaborator

jonathansampson commented Feb 27, 2017

@kspearrin Everything looks really great (thank you again for the incredible investment!). As a final ask, could you run a rebase and squash to a single commit?

docs/state.md Outdated
@@ -201,6 +201,7 @@ AppStore
'security.passwords.last-pass-enabled': boolean, // true if the Last password extension should be enabled
'security.passwords.manager-enabled': boolean, // whether to use default password manager
'security.passwords.one-password-enabled': boolean, // true if the 1Password extension should be enabled
'security.passwords.bitwarden-enabled': boolean, // true if the bitwarden extension should be enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please move this line after the line 'security.passwords.active-password-manager', so that properties are ordered alphabetically

@kspearrin
Copy link
Contributor Author

@jonathansampson I'm having a terrible time trying to squash this manually. Isn't it possible just to run the squash as part of the GitHub tooling when merging this PR?

@kspearrin
Copy link
Contributor Author

kspearrin commented Feb 27, 2017

It appears that since this PR has been open so long that there are a ton of other commits conflicting with the squash commands I am trying to run (from http://stackoverflow.com/questions/5189560/squash-my-last-x-commits-together-using-git).

I would hope we can rely on whatever magic GitHub's Squash & Merge does.

@NejcZdovc
Copy link
Contributor

NejcZdovc commented Feb 27, 2017

@kspearrin I managed to rebase it, so if you can please check Allow edits from maintainers, I can fix it. Thank you

https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/

@kspearrin
Copy link
Contributor Author

@NejcZdovc invite sent.

@kspearrin
Copy link
Contributor Author

Whoops. Misread your original request. Allow edits is on for the PR (it was already checked)

@NejcZdovc
Copy link
Contributor

@kspearrin Done. Can you please check if everything is ok and still working correctly?

@kspearrin
Copy link
Contributor Author

@NejcZdovc Looks good, though it's still showing 2 commits (the state.md file commit that you requested I change this morning).

@NejcZdovc
Copy link
Contributor

NejcZdovc commented Feb 27, 2017

Yeah I wanted to do it in two phases and check every phase with you 😃 First one was a simple rebase with a master and then squash, which I will do now.

@NejcZdovc
Copy link
Contributor

@kspearrin Now it's squashed as well.

@kspearrin
Copy link
Contributor Author

kspearrin commented Feb 27, 2017

@jonathansampson This is all squashed now. Please let me know if there is anything else that I need to do here.

@bsclifton bsclifton added this to the 0.13.6 milestone Feb 28, 2017
@bsclifton
Copy link
Member

@kspearrin thanks for all the changes (and huge thanks to @jonathansampson for the testing!)

We'll be able to merge this for our next release (0.13.6). I assigned the milestone officially and will make sure it makes it in there 😄

@Jacalz
Copy link
Contributor

Jacalz commented Mar 5, 2017

Have any one contacted bitwarden and said that it soon will be integrated in to Brave? I can do it if you think that it is a good ide 😄

@kspearrin
Copy link
Contributor Author

@Jacalz I am the lead developer of bitwarden. We're excited about bringing bitwarden to brave. Brave shares many of the same values that we do around open source so it's great that we've been able to work together to get this done.

If there's anything else we can work with the brave team team on to create a better experience between the two products please let me know and we'll make it a priority.

I may reach out soon to your iOS team about supporting our app extension for auto fill.

@Jacalz
Copy link
Contributor

Jacalz commented Mar 5, 2017

Oh, I never looked at your profile 😆 🤦‍♂️ @kspearrin Note to my self: Dont be stupid 👍

@ChillyHellion
Copy link

I'm just wondering if there's a status update for this feature. I'm a user of both Brave and Bitwarden and would love to see built-in support.

@bsclifton
Copy link
Member

@kspearrin @ChillyHellion support for this should be in our next version (0.17.x). We have some preview releases up and I'd guestimate the release being 1 or 2 weeks away 😄

@ChillyHellion
Copy link

Excellent, thank you very much!

@ChillyHellion
Copy link

Did this feature ever hit stable? I was trying it in preview for a while but couldn't get it working. I cleared Brave's AppData and reinstalled the stable build and see that Bitwarden is still not included. I appreciate that bugs are still being worked on; sounds like it's been a heck of a process.

@bsclifton
Copy link
Member

@ChillyHellion no it hasn't yet... I believe all of the major issues have been ironed out and there's a fix waiting to be accepted for the remaining issue 😄

We should be able to ship this with our next minor update, 0.19 (we're currently on 0.18.23 and have one more 0.18 update coming). Hopefully this can ship in the next 2 - 3 weeks 😄

@ChillyHellion
Copy link

Excellent, thank you for the update! Bitwarden and Brave are my two favorite open-source, privacy-aware projects, so I'm happy they'll soon be supporting each other as well.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Bitwarden Password Manager
7 participants