browserpass / browserpass-legacy

Legacy Browserpass repo, development is now happening at:
https://github.com/browserpass/browserpass-extension
MIT License
999 stars 80 forks source link

Add option to filter all entries #253

Closed erayd closed 6 years ago

erayd commented 6 years ago

A few people have asked about this, so here's a PR that implements it. The feature needs to be explicitly enabled, and defaults to off.

Note that 'filter everything' mode is not triggered when all results from a search are filtered out, in order to prevent accidental submission of non-domain credentials after an automatic domain search.

If the 'filter everything' option is enabled, then will enter 'filtering everything' mode:

If in 'filtering everything' mode:

maximbaz commented 6 years ago

I'm wondering if we should have used this approach from the beginning instead of implementing fuzzy search on the host app side...

I think the bottleneck of communication between browser and host app is the request-response part, not the amount of data that is being sent. With this approach, once you hit Backspace you do one request, get all the data and then do a quick and realtime filtering on the spot, you never need to make a second request at all.

Hmm, I'm thinking what would be the downsides of having this enabled by default, possibly even without an option to disable this...

erayd commented 6 years ago

Hmm, I'm thinking what would be the downsides of having this enabled by default, possibly even without an option to disable this...

There's a noticeable lag when loading everything from the host app, at least on my system (late 2013 Macbook Pro). It's considerably faster searching for a term that returns only a few results.

If you want to have it enabled by default though, I have no objection. I'll just turn it off ;-).

erayd commented 6 years ago

To clarify - the lag is less than half a second. It's noticeable, but not a show-stopper.

maximbaz commented 6 years ago

I see, so your experience is that it does matter how many results are returned...

I will test this on Windows - last time I checked, on Windows even the domain search that is performed when popup is being opened has a lag of 1-2 seconds. I wonder what it will be when the entire password store is being returned.

erayd commented 6 years ago

Might be worth testing on OSX as well. I run Gentoo on this system.

maximbaz commented 6 years ago

Yeah, but I don't have it 😃

On a different note, do you think it makes sense to add scrollbar for browserpass? Just as an indication that there are more entries in the list that is currently visible.

erayd commented 6 years ago

On a different note, do you think it makes sense to add scrollbar for browserpass?

Yes (vertical only though). Would you like me to add it, or will you?

maximbaz commented 6 years ago

Please add 🙂

erayd commented 6 years ago

Yeah, but I don't have it.

I have it, but I never use it (it hides on a tiny partition that only gets used for testing things in Safari) - so none of the GPG stuff is set up there, and last time I looked into it, the smartcard stuff was a mission on OSX (I use a yubikey for everything) :disappointed:.

maximbaz commented 6 years ago

I think it's pretty safe to leave OSX out of testing, I haven't heard anyone complaining about the performance on OSX - but I did hear and personally see how browserpass is super slow on Windows...

erayd commented 6 years ago

Please add

Done.

maximbaz commented 6 years ago

The logic becomes more and more complicated 🙂 I wish there was a cheap way to have scenario tests in the code, instead of testing everything manually... But I have a feeling this is not testable.

I tested on Windows, apparently things are much faster these days than what I remember. The lag is less than a second, regardless of the amount of password entries.


My test results:

Note that 'filter everything' mode is not triggered when all results from a search are filtered out, in order to prevent accidental submission of non-domain credentials after an automatic domain search.

I like this decision 👍

If the 'filter everything' option is enabled, then will enter 'filtering everything' mode:

  • If opening from a tab that does not match a domain.

This happens even if "filter everything" option is disabled...

If in 'filtering everything' mode:

  • Pressing backspace will exit to normal search mode.

Yes, however if I search something in this search mode, make a typo, hit Backspace to fix it, my search term will be replaced with *.

maximbaz commented 6 years ago

Actually, about this:

If in 'filtering everything' mode:

  • Pressing backspace will exit to normal search mode.

Do we even need a normal search mode? There's nothing you can find with normal search mode that you cannot with "filter everything" mode, so maybe with this option on we should hide the "search" mode completely, and keep only filter mode?

erayd commented 6 years ago

This happens even if "filter everything" option is disabled...

Fixed.

Do we even need a normal search mode? There's nothing you can find with normal search mode that you cannot with "filter everything" mode, so maybe with this option on we should hide the "search" mode completely, and keep only filter mode?

I like this idea. Stand by.

erayd commented 6 years ago

This may take a couple of days to get done - doing multiple things at once, and what I want to change is slightly complicated. Maybe hold fire on this for now while I rework it.

maximbaz commented 6 years ago

Sure 😉

erayd commented 6 years ago

So I added you to a project - have been playing around with ideas for improving the native component. What are your thoughts on this kind of approach?

I've done this in bash to get something to quickly demonstrate what I'm thinking, but I figure you'll likely want to stick with something a bit more windows-compatible in the long term.

maximbaz commented 6 years ago

Very interesting!

I've been thinking myself recently that if we were to design the native component from scratch, we would do a lot of things differently today - have standardized messages, with statuses, status codes and proper descriptions; have proper support for multiple password stores without hacks like splitting string by semicolon; or have a new action list instead of workarounds like "search by *". That's just a few things that I picked up from your prototype. Your idea to pass fields object is also interesting.

I also feel that as new features are getting implemented and we are better understanding what people expect and want, some parts of the code should be considered for cleansing - for example, with this PR I now see little sense in having a manual search functionality supported by the host app (both glob and fuzzy). This new real-time filtering approach works so well, and it just feels weird that there is another approach where I need to blindly guess and search for some term, then wait to results, then potentially use filter to reduce the number of matches, then realize that I didn't find something that I wanted and I must get out of filter mode and do a blind search again...

Regarding bash and windows compatibility in general, I've been actively playing with aurutils for the past few months, and I got an understanding at what it takes to maintain a codebase in bash of relatively medium complexity. It is painful 🙂 I cannot say that I love golang very much, but I think this was a good decision to implement the host app in golang for the following 3 reasons:

What I think is the biggest issue here is the distribution of the native component, Arch users are lucky that there is AUR, the rest of people are not getting automatic updates, and I know majority is not updating unless they absolutely have to (they don't even know that a new version is available). The fact that I cannot enforce everyone to update the host app (and thus to often make breaking changes) like I can with browser extensions is very annoying 🙂 But I don't see a solution for this, and the issue remains regardless of the chosen language.

erayd commented 6 years ago

I've been thinking myself recently that if we were to design the native component from scratch, we would do a lot of things differently today...

My thought also. Working on this PR has motivated me to sink a bunch of time into redoing the host app, and completely changing the way search works. But I don't want to do things that people don't want, so I figured I'd bounce some ideas off you first before doing anything serious.

I also feel that as new features are getting implemented and we are better understanding what people expect and want, some parts of the code should be considered for cleansing...

To be perfectly honest, I'm getting to the point where I want to do a ground-up rewrite of the project, with the intention of producing something that has feature parity, but is technically a lot nicer (plus nicer to work on and maintain). If I were to do a major, break-all-the-things rewrite PR, would you be open to that?

If you'd rather that not happen, then I can make smaller changes within the current codebase. But my preference is now to rewrite it.

Regarding language for the host app - I agree that Go is an excellent choice for it. I'm not terribly familiar with the language (this project gives me a good excuse to learn), but I like much of what I've seen, and the distribution aspect is definitely a significant upside. I also agree that bash is undesirable, for all the reasons you mentioned - I just used it for my example script because it was the fastest way to get some of my ideas out of my head and in a form that I could start playing around with.

Regarding the problem of users not being able to / not desiring to update the host app, what if we went with a different approach entirely, and went with a really simple host app that does only two things:

Everything else becomes part of the browser extension, which we can easily update at will.

maximbaz commented 6 years ago

To be perfectly honest, I'm getting to the point where I want to do a ground-up rewrite of the project, with the intention of producing something that has feature parity, but is technically a lot nicer (plus nicer to work on and maintain). If I were to do a major, break-all-the-things rewrite PR, would you be open to that?

I'm up for a rewrite, instead of a PR I'd like to create a github organization and a new project in there. The thing is, I don't have ownership rights in this project, I only have collaborator rights, and I cannot do even basic things like to give you access. I'd also like to have browser extension and host app in different repositories.

What do you think?

Would be cool to re-use the browserpass name, but I'm not sure that MIT license allows that (probably not).

what if we went with a different approach entirely, and went with a really simple host app that does only two things:

Yes! Ideally we decrease the need to update the host app to a minimum. I have a pretty good memory of what people requested of the host app (e.g. problems with $PATH on macOS, support file/directory symlinks, declare OpenBSD pledge, etc. - will explain later in details), I think it is totally possible to ensure that by the time we release v1.0.0 all of the important features of current browserpass are implemented and nothing is forgotten.

maximbaz commented 6 years ago

P.S. Do you happen to use https://wire.com maybe, or want to join? I know you use keybase chat, I can install it too - but right now Wire is my only chat app and I'm often online there 😉

erayd commented 6 years ago

I'm up for a rewrite, instead of a PR I'd like to create a github organization and a new project in there... I'd also like to have browser extension and host app in different repositories.

What do you think?

I think yes, to both comments. Organisations are really good, because they reduce the bus factor significantly. If we go down that road it should have multiple owners who are engaged with the project, and when org owners move on, they should be replaced.

erayd commented 6 years ago

P.S. Do you happen to use https://wire.com maybe, or want to join? I know you use keybase chat, I can install it too - but right now Wire is my only chat app and I'm often online there.

I don't, but I'm happy to join it. I'll look into it more tomorrow - it's after midnight here :-).

maximbaz commented 6 years ago

I think yes, to both comments. Organisations are really good, because they reduce the bus factor significantly. If we go down that road it should have multiple owners who are engaged with the project, and when org owners move on, they should be replaced.

Exactly. Ping me tomorrow on wire, and we can think of a name and create an org with two of us as owners. Heh, it looks like we have 10 hours time difference 🙂

erayd commented 6 years ago

Closing this PR, because we are rewriting it anyway.