Rewrite to allow matches from multiple stores #239

erayd · 2018-03-27T02:08:37Z

Rewrite the multiple-store feature to allow files with the same name in different stores
Add dynamic number of stores
Rewrite options screen
Add styling to login entries when multiple stores are enabled
Various other minor improvements.

erayd · 2018-03-27T02:17:10Z

@maximbaz Worth clarifying, because this may not be obvious - the browser extension in this branch is backwards-compatible with the current release version of the host app. There are no breaking changes.

erayd · 2018-03-27T02:27:13Z

There's also something in this PR which is causing Firefox to occasionally display a horizontal scrollbar. It's not the min-width of the popup. Will update when I've found the cause.

erayd · 2018-03-27T04:55:03Z

Firefox scroll issues sorted.

maximbaz · 2018-03-27T07:37:06Z

This looks really nice! I particularly like that the label is not displayed when only one password store is configured.

maximbaz · 2018-03-27T07:53:38Z

chrome/options.browserify.js

+  },
+  customStore: {
+    title: "Custom password store locations",
+    value: [{name: "one", enabled: true, name: "fish", path: "/home/fish"}],


This default entry "fish" is confusing, the first time I saw it I toggled the checkbox twice, closed, and browserpass started showing me error Error: lstat /home/fish: no such file or directory. I will change this line to this if you don't have objections - the placeholders explain well what needs to be entered where:

value: [{enabled: true, name: "", path: ""}]

Oops - that was a leftover test value. I've just nuked it. Thanks for picking it up :-).

(To clarify - the default is supposed to be an empty array; it doesn't need to contain a value).

I tried a few options before writing that comment, and actually I found it quite pleasant for the eye to have one empty line pre-created, only without any hardcoded values 😉

Makes sense - I've added your suggested default in f686716.

maximbaz · 2018-03-27T08:03:49Z

pass/disk.go

@@ -109,11 +114,11 @@ func (s *diskStore) GlobSearch(query string) ([]string, error) {

 		for i, match := range allMatches {
 			// TODO this does not handle identical file names in multiple s.paths


Now you can remove the TODO, here and below in Open 🙂

maximbaz · 2018-03-27T08:05:18Z

pass/disk.go

@@ -14,46 +14,51 @@ import (
 	sfuzzy "github.com/sahilm/fuzzy"
 )

+// StoreDefinition defines a password store object
+type StoreDefinition struct {


Could you please also adjust pass/disk_test.go for these changes?

Yes. I just need to figure out what needs doing - I don't work much with Go, so I'm a bit slow with it.

I can fix the tests myself after merging too, that's not a big deal 🙂

That works too :-). If you're happy to do that, I would definitely prefer it - you are likely to pick up things in testing that I may miss due to my lack of familarity.

maximbaz · 2018-03-27T08:05:58Z

browserpass.go

-	UseFuzzy bool     `json:"use_fuzzy_search"`
-	Paths    []string `json:"paths"`
+	UseFuzzy    bool                   `json:"use_fuzzy_search"`
+	CustomStore []pass.StoreDefinition `json:"customStore"`


should we maybe call it in plural?

Could you clarify what you mean by "call it in plural" please? I'm not sure what you are asking me to do.

sorry, I meant name it "CustomStores" 🙂

Thanks for clarifying - done :-).

maximbaz · 2018-03-27T08:29:24Z

browserpass.go

@@ -47,6 +49,18 @@ type msg struct {
 	Entry    string `json:"entry"`
 }

+func SendError(err error, stdout io.Writer) error {


My special THANK YOU for this!

I'm going to separately test this more, but what is your opinion on this as a fix for #164? I personally want to see errors logged and reported back to the extension, this code already sends the message back and logs it to stdout, I would only add logging to a file too (not necessarily in this PR). Anything you would additionally want for better error reporting?

My special THANK YOU for this!

You're most welcome :-).

I think this goes partway to addressing #164, but I don't consider it a full fix.

I added it because I needed a way to get error info out of the host app while I was debugging it, and figured I'd do so in a way that was more generally useful. So it helps - but I don't think that #164 should be closed because of it. I suspect there's still a fair bit of error information that may not be caught.

Besides, my way of doing it here is a little messy ;-). Anything that's returned as a raw string is considered an error. In the longer term, I'd like to see the return message standardised properly, and include a status code. But I couldn't do that without breaking backwards-compatibility, hence the string thing.

I had that idea too, but then I saw your approach with strings and I thought that perhaps this is actually fine, we can log more details in the log file (detailed messages, error code, etc), and have the extension receive just a string error. That way we don't break compatibility, and it's easy to add to ISSUE_TEMPLATE an ask to print the last 100 lines from /var/log/browserpass.log 🙂

It's not really fine in the longer term though, because at the moment we can't change much functionality-wise without breaking stuff (and besides, co-opting the entire string datatype for errors isn't very nice, because nobody can use it for anything else). Switching to a consistent and extensible message format will allow much more scope to add functionality without requiring a host app update every time we do something significant.

I don't think this PR is the right place for such a change, but IMO it definitely needs to happen at some point.

Agreed, and I will update the ticket with your comment :)

erayd added 8 commits March 27, 2018 09:36

Switch options.js to browserify generation

f4d18df

Rewrite options screen

34af039

Properly declare local variables as local

16b261d

Add some spacing

a665883

Use getSettings() instead of directly reading from local storage

38799f5

Define getSettings in a more general manner

6259c7c

Use a list of named password stores

6d6508a

Multi-store display mode

049494d

erayd mentioned this pull request Mar 27, 2018

Allow configuring multiple custom password stores #237

Merged

Don't display the store badge if the store name isn't present

d12a948

Increase the minimum width of the popup by 70px

1b6df1c

erayd added 2 commits March 27, 2018 17:16

Firefox UI improvements

ca7e084

Return some error messages from the native host app

6ac632f

erayd added 6 commits March 27, 2018 18:04

Don't search empty or ignored domains

86a7ded

Use settings object from search

696ff4a

Don't show badges if there is only one custom store configured

c85ad59

Remove unused variable

173e554

Remove prefix when detecting the URL from the path

8c8ca8f

Fix hints

abe6a7c

maximbaz reviewed Mar 27, 2018

View reviewed changes

erayd added 3 commits March 27, 2018 21:19

Remove the fish

af8df7e

Remove TODO comment

c1d750a

Remove TODO comment

6e9e96f

maximbaz reviewed Mar 27, 2018

View reviewed changes

erayd added 2 commits March 27, 2018 21:34

Use plural name for variable

d1178ed

Add default empty entry

f686716

maximbaz merged commit a9aa62c into browserpass:multiple-configurable-password-stores Mar 27, 2018

erayd deleted the multiple-configurable-password-stores branch March 27, 2018 08:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite to allow matches from multiple stores #239

Rewrite to allow matches from multiple stores #239

erayd commented Mar 27, 2018 •

edited

Loading

erayd commented Mar 27, 2018

erayd commented Mar 27, 2018

erayd commented Mar 27, 2018

maximbaz commented Mar 27, 2018

maximbaz Mar 27, 2018 •

edited

Loading

erayd Mar 27, 2018

erayd Mar 27, 2018

maximbaz Mar 27, 2018 •

edited

Loading

erayd Mar 27, 2018

maximbaz Mar 27, 2018

erayd Mar 27, 2018

maximbaz Mar 27, 2018

erayd Mar 27, 2018

maximbaz Mar 27, 2018

erayd Mar 27, 2018

maximbaz Mar 27, 2018

erayd Mar 27, 2018

maximbaz Mar 27, 2018

erayd Mar 27, 2018

maximbaz Mar 27, 2018

erayd Mar 27, 2018

erayd Mar 27, 2018

maximbaz Mar 27, 2018

erayd Mar 27, 2018

maximbaz Mar 27, 2018

		@@ -109,11 +114,11 @@ func (s *diskStore) GlobSearch(query string) ([]string, error) {

		for i, match := range allMatches {
		// TODO this does not handle identical file names in multiple s.paths

Rewrite to allow matches from multiple stores #239

Rewrite to allow matches from multiple stores #239

Conversation

erayd commented Mar 27, 2018 • edited Loading

erayd commented Mar 27, 2018

erayd commented Mar 27, 2018

erayd commented Mar 27, 2018

maximbaz commented Mar 27, 2018

maximbaz Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximbaz Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erayd commented Mar 27, 2018 •

edited

Loading

maximbaz Mar 27, 2018 •

edited

Loading

maximbaz Mar 27, 2018 •

edited

Loading