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

Allow configuring multiple custom password stores #237

Merged
merged 38 commits into from
Mar 28, 2018

Conversation

maximbaz
Copy link
Member

@maximbaz maximbaz commented Mar 24, 2018

I had a few hours free, so I decided to play with implementing this feature.

I suggest reviewing the diff without whitespace changes.

The idea is to allow configuring multiple password stores in options, and even toggle them when needed:


image

image


image

image


A folder can be a path to a custom password store (#98), a path to a subdirectory of a password store (#34) or a path to gopass mount (#77).

Known limitations

Nothing left here 🙂

Fixed 1. It has to be an absolute path, no expansion of `~` or environment variables.
Fixed by @erayd in #239
  1. I didn't have time (and desire) to dynamically add/remove more lines for directories, so I pre-created 5 empty input lines. Because 5 should be enough for everyone 😄 I'm joking, but if you want to implement something fancier, please submit a new PR.

  2. Current solution doesn't support having files with identical names. In other words, if you enable two password stores in the options:

/home/user/.password-store-1/
/home/user/.password-store-2/

And you have identically named files in both password stores, like this:

/home/user/.password-store-1/git.luolix.top.gpg
/home/user/.password-store-2/git.luolix.top.gpg

Only the first git.luolix.top.gpg will be seen by browserpass. Use checkbox in the extension options to temporarily disable password store that you don't need.

However if the files are in different subfolders (like below), then it's fine:

/home/user/.password-store-1/personal/git.luolix.top.gpg
/home/user/.password-store-2/work/git.luolix.top.gpg

Fixes #98
Fixes #34
Fixes #77

@erayd
Copy link
Contributor

erayd commented Mar 25, 2018

Current solution doesn't support having files with identical names.

What's the limitation causing this? Would adding a temporary prefix be a suitable workaround?

pass/disk.go Outdated

for i, match := range allMatches {
// TODO this does not handle identical file names in multiple s.paths
item, err := filepath.Rel(path, match)
Copy link
Member Author

Choose a reason for hiding this comment

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

@erayd this is the cause of the limitation, it generates the entry name by cutting off the configured prefix. A better solution would need to account for multiple configured paths to make sure it cuts off as much as possible, but still keeping the resulting string unique among all paths.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mind if I open a PR against this branch with a proposed workaround?

Copy link
Member Author

Choose a reason for hiding this comment

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

Please go ahead 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Done - see #239.

pass/disk.go Outdated
continue
}
// TODO this does not handle identical file names in multiple s.paths
return f, err
Copy link
Member Author

Choose a reason for hiding this comment

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

And then here, when we receive an entry "github.com", the code is trying to guess, which password store it belongs to. Right now the code is simple, the first password store that contains "git.luolix.top.gpg" is the one 🙂

Copy link
Contributor

@erayd erayd left a comment

Choose a reason for hiding this comment

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

Looks like this picked up a lot that prettier did not notice when I ran it via docker. Is the makefile config for running prettier via docker correct?

@maximbaz
Copy link
Member Author

Description updated, all limitations fixed.

@maximbaz
Copy link
Member Author

Looks like this picked up a lot that prettier did not notice when I ran it via docker. Is the makefile config for running prettier via docker correct?

Mmm, the makefile is the same and I just ran make that should be run by docker. I haven't been using this docker image for a while though... When you use the docker image, does it fix any kind of style issues at all? No errors printed? Maybe try to rebuild the docker container?

This is the output when I run make by the way, I think when you use docker image it should print exactly the same output.

image

@erayd
Copy link
Contributor

erayd commented Mar 27, 2018

That looks quite different - I just get this:
scr-20180327-223340

var fileNames []string
for _, entry := range entries {
fileNames = append(fileNames, strings.SplitN(entry, ":", 2)[1])
}
Copy link
Member Author

Choose a reason for hiding this comment

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

@erayd you'll love this 🙂 Caught by a unit test, that suddenly started finding 10 entries instead of 1 for a search term "def" (which started to match store name "default")

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch :-).

I actually use the store name as part of my search fairly commonly, to distinguish between files in different stores that have otherwise identical names. However, I don't actually need that capability in the host app - having it in the "refine search" filter (which also includes the store name in searches) is enough for my use-case :-).

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, until now I didn't even think to test how "refine search" behaves with store names, but I agree this is a very common use-case to refine by the store name. In fact, I'm used to that too, because I have both "work" and "personal" in the same password store, but now I can configure different browser profiles to use different subfolders and this reduces 99% of cases where I needed to refine - now I just press Enter 🙂

@maximbaz maximbaz merged commit 140b11c into master Mar 28, 2018
@maximbaz maximbaz deleted the multiple-configurable-password-stores branch April 3, 2018 15:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants