Add ability to generate/add passwords

[emersion]

It would be nice to be able to generate and add passwords directly from the extension.

[adtac]

I just want to add: can the implementation make the password generation method configurable? I wrote a small patch to add XKCD style passwords (see mailing list), and I'd like to have the option to generate these. I think we'll need such an option in the settings any way to support flags like -n to generate passwords without symbols? I propose three options in the add-on settings:

• Default password
• No symbols (-n)
• XKCD/Diceware passphrases

[maximbaz]

Length and characters set will definitely be configurable, but probably not XKCD passphrases, not convinced it is worth the effort in the context of a password manager :) Seems like some folks from that thread think similarly...

[adtac]

I'm happy with that. As long as something is configurable, it'll be pretty easy for me to maintain a fork with the extra code (which is what I'm doing with the shell script right now).

[maximbaz]

Hi guys!

It's been a long time since I wanted to implement editing feature, but it seems I won't be able to finish it in timely fashion 😞

I would therefore like to ask if anyone is interested to pick it up, finish the work and contribute PRs?

The necessary changes would include:

• Updating the popup UI
• Extending browserpass-native with new methods to create/edit/delete password entries

Because it's a significant change, I believe it would make sense to split the work in several PRs, so that it's easier to review and address feedback, to make sure we are moving in the right direction and not implementing unnecessary functionality.

For example, I would start with creating an interface, focusing on bigger picture and simply disabling buttons like "Save", "Delete entry" and "Generate password", just to focus on the UI first. Filling the edit form with existing credentials would not require extending browserpass-native, it already contains all the necessary APIs. Once that is done, saving the edits, adding new entries, deleting existing entries, extending options screen with e.g. password generation configurations, can all come as separate PRs.

I have started the design implementation some time ago in this branch, I think some parts of the code could still be reused.

Here's how I see the interface looking:

@ozeidan has also contributed an interesting idea for automatically adding credentials to password store in #143 (which is very much appreciated!), but I would like to start with manual add/edit/delete first and later see if it makes sense to extend it with automatic credentials detection.

Please let me know if anyone is interested, and we can discuss in more details 🙂

[rgeorgiev583]

Hi @maximbaz, This is a really essential feature to me, so I'd like to work on it. I have to admit that I'm not familiar with the code at all (and I have very little experience with extension development), so I'd have to take some time to study it.

P. S. Should I create a related issue in https://github.com/browserpass/browserpass-native, too?

[maximbaz]

That sounds awesome!

I think we should maybe start from picking up #233 to avoid duplicating the work, and then adding a new password would be a natural step forward after having the edit functionality.

What do you think @erayd, would you like to finish that off, or would you like for @rgeorgiev583 to take that over? Any other thoughts in general on how to organize this?

One request from me is this: lets work in multiple small PRs, this is a very complex feature, it would be much easier to align on implementation and design this way, rather than getting one huge PR only to realize that we have different views on how this should be implemented.

We dont need another ticket to track this work, this one is enough.

[erayd]

@maximbaz Do you have some time later this coming week for us to go over the existing work in this area? I'd like to ensure that all the partial stuff is either merged, or sitting in an existing PR so that @rgeorgiev583 can see it.

Overall though, I'm very happy for @rgeorgiev583 to work on implementing this. I've not had as much free time as I'd like lately, so it would be useful to have someone else help out.

Lets work in multiple small PRs, this is a very complex feature...

I agree. We worked out the key functionality a while back, and I have an implementation for whole-file editing, which should be a useful starting point.

[rgeorgiev583]

@erayd I'm planning to start working on the host app first, if that's OK.

[erayd]

@rgeorgiev583 We already have a host implementation for writing files. What more do you think is necessary on that end of things?

[maximbaz]

For reference: it's in a PR, but @erayd and I will finalize all our unmerged work so that you get a clear and clean state to pick this up @rgeorgiev583 👍

https://github.com/browserpass/browserpass-native/pull/99

[rgeorgiev583]

@maximbaz I checked out the implement-tree-save-delete branch in the extension host repo and built the host. Following the documentation in PROTOCOL.md, I managed to run the echo and configure commands successfully. However, I couldn't understand how to invoke the other commands: I always seem to get empty results when I run them, even though my default password store is at the canonical location.

[rgeorgiev583]

I also noticed something very strange: after I checked out the add-edit branch in this repo and tried to build the extension, I got the following error when I ran make firefox:

make: *** No rule to make target 'src/css/popup.dist.css', needed by 'firefox/css/popup.dist.css'.  Stop.
make: *** Waiting for unfinished jobs....

It seems that the build failed after this because there was no manifest.json in the firefox directory. However, after I ran make firefox again, the build succeeded (I suppose) and the manifest file appeared.

[maximbaz]

It takes attention to work with native host to carefully construct the request, see also native.pl helper, that little thing helped me a lot. Extension works, so all the requests are functional, and I hope PROTOCOL.md does not contain any bugs, but if you spot any - do let me know!

Not sure about that make error, but glad to hear that it worked for you in the end! For me in a clean repo running make in the root seems to work fine.

[rgeorgiev583]

I implemented addition, editing and deletion of credentials. I tested all three of them and they seem to work. However, there is a small UI issue: I have to click the confirmation button in the edit dialog twice.

[patgmiller]

@maximbaz or @erayd I'm curious on what's keeping this feature from progressing?

[erayd]

On my end, it's simply a lack of available time to work on it. I've got half a PR, but it's been a while since I've made forward progress on it.

Was intending to spend a good week on this a while back, but then C19 threw my work schedule into chaos again and ruined that plan 😞.

It'll still happen, but I don't want to commit to a timeline at the moment.

[patgmiller]

I may be able to contribute if you have some direction you could provide. However, I understand if it's not easily conveyed.

[oktayacikalin]

Guys, please resume working on this. You already got some work done. Thanks a lot!

[erayd]

I have some time week after next - will pick this up again then.

[timsofteng]

Hello guys. Any updates about this issue?

[erayd]

Any updates about this issue?

Progress is happening, albeit slowly. My time remains a bottleneck, but we are at least moving forward.

@patgmiller has been making substantial progress with his work on the way our UI is rendered, including the editing portion of that. I think I am currently holding him up somewhat pending feedback. If you'd like to check out where things are at, you can find his branch here.

[patgmiller]

The PR for this is getting a little unmanageable for me track things so I'm going to compile my checklist of things to do here.

• [x] Add "loading data" please wait
• [x] Fix broken link > to view details after hitting < back button, and/or after using Search to filter
• [x] Missing tool tips
• [x] Keep secret highlighting in for "Add/Edit" view, and only allow input in the textarea
• [x] Alternate message handling, ie don't drop dom.
• [x] Replace js confirm with html <dialog>
• [x] File symlinks
• [x] Maxims overview
  • [x] 1. Allow secret to be empty during edit, but fail validation on save
  • [x] 2. secret handling
    • [x] Allow secret prefix
    • [x] Secret can be in any line of the textarea
  • [x] 3. Diff color for mutable / immutable input fields
  • [x] 4. Save + Delete buttons, try both at bottom
  • [x] 5. Empty forms for slower response ... see Add "loading data" ... above
  • [x] 6. Multiple decryption attempts, options: 1. Do not decrypt a second time. or 2. Create issue for tracking
  • [x] 7. Secret + Login fields ... need clarification
  • [x] 8. Length should have a floor at 1, no negatives.
• [x] Add auto suggest directory / tree
  • [x] Add basic Tree model with tree request to host app
  • [x] Build tree structure given list of dirs for a storeId
  • [x] Search tree given path string
  • [x] Add remaining color themed styles
• [x] Should native save distinguish between (adding) new encrypted file and an existing or not? Maybe have to specify diff action add and save? Or just boolean on same request.
• [x] bugfix: can no longer arrow down through list view
• [x] Set version number

[patgmiller]

@maximbaz i realized recently that the save on the native host side makes no distinction between saving a new file and updating an existing file. My question to you is, should it know the difference?

If yes, there are a couple ways to handle it, but essentially it returns an error or warning. At which point we can either provide a dialogue to confirm it should be overwritten, or leave at the error. If the latter the user can simply browse back to list then search and edit. Thoughts?

[maximbaz]

The less functionality we have in native host the better 😄 We probably can distinguish on extension side whether we are doing a "save create" or "save update" operation, and then just render error differently based on that?

[patgmiller]

There is no error, native doesn't care or know if it's new or not, it just saves (writes) the encrypted contents to the file.

[patgmiller]

though I suppose the add / edit view of the extension can check the existing list of files and if doing an add warn / error that the file already exists. So I can look at at since you'd prefer to keep the native host functionality reduced. I will look at it.

[maximbaz]

Yes this sounds best, thanks!

[maximbaz]

Hello everyone, we published a release candidate implementing add/edit/delete functionality - testing is highly appreciated, please leave your feedback in https://github.com/browserpass/browserpass-extension/issues/331

[maximbaz]

3.8.0 has been released :rocket:

[timsofteng]

@maximbaz any chance it will work on iOS and Android?