browserpass / browserpass-extension

Browserpass web extension
ISC License
825 stars 50 forks source link

Configure Default Options for Add/Edit Credentials Page #307

Open patgmiller opened 1 year ago

patgmiller commented 1 year ago
    Hi @maximbaz I've addressed all but `#1` for which I need clarification before proceeding.

Hey guys, once again - super cool job, thanks for all the work here, and I am sorry I am giving this PR less attention than it definitely deserves!

I just have some additional generic feedback from a user perspective - we can always choose to exclude this from the scope of this PR if need be +1 Sorry in advance if you already discussed those and I missed the thread...

  1. First of all, it's a lot more polished right now, feels.... complete, and behaves in exactly the way I expect it to - really cool feeling! rocket

Thanks!

  1. On the "Add credentials" page, it would be nice to remember the last selected values of password Symbols and Length - it feels intuitive that if I changed length to 20, unticked the Symbols checkbox and left the popup, next time I use "Add credentials" it would default to 20 and no symbols.

Are you saying this should persist between open / close of the browserpass popup, or just for the current open session? I imagine it will be requested at some point to have persistent defaults which can be set.

  1. Also on the "Add credentials" page: when I click on the "filename", the dropdown appears with folders - I feel like they should all have a trailing / symbol, to indicate that this is indeed a folder - when we insert the value, we do insert the trailing slash after all.

Done

  1. The above dropdown can only be completed with Enter, I actually first tried to click on an entry with a mouse, and that did nothing, so I thought the dropdown is not implemented fully yet, until I decided to try Enter... Would it be difficult to also support click to select?

Was there originally, it's fixed now.

  1. On the "Edit credentials", I somehow expect that the buttons "Save" and "Delete" should be swapped, can't really explain it, just a feeling that the "Good" action must be on the right side...? sweat_smile (also on "Add credentials").

Updated

Originally posted by @patgmiller in https://github.com/browserpass/browserpass-extension/issues/290#issuecomment-1274464183

patgmiller commented 1 year ago

We essentially want to the user to either remember the last used credential options (Symbols and Length) when Adding a new credential, or provide defaults which are configurable to the users desired values.

@maximbaz Please correct/clarify anything in the description which doesn't meet with your original request. Thanks

maximbaz commented 1 year ago

Exactly right 👍 If I leave the "Add new credentials" page for example with length set to 40 (doesn't matter if at that time credentials were actually added or not), next time I open "Add new credentials", I expect to see the setup exactly as I left it last time (i.e. with length defaulted to 40 now).

patgmiller commented 1 year ago

Exactly right 👍 If I leave the "Add new credentials" page for example with length set to 40 (doesn't matter if at that time credentials were actually added or not), next time I open "Add new credentials", I expect to see the setup exactly as I left it last time (i.e. with length defaulted to 40 now).

Do you mean even if you don't actually add any credentials? What if the user cancels and doesn't add new credentials? I was thinking only when they actually add new credentials, or to set the default length in the browser pass extension options page.

maximbaz commented 1 year ago

Yeah let's not over complicate things 😊

I propose we don't add "length" and "symbols" to browserpass options, and just have one place where user can change generated password options instead of two (directly on the "add" screen).

And yes, if I changed password length and for one or another reason left the screen without adding password, personally I would be annoyed if my changes were not saved and I'd have to do it again!

patgmiller commented 1 year ago

Yeah let's not over complicate things blush

I propose we don't add "length" and "symbols" to browserpass options, and just have one place where user can change generated password options instead of two (directly on the "add" screen).

And yes, if I changed password length and for one or another reason left the screen without adding password, personally I would be annoyed if my changes were not saved and I'd have to do it again!

okay, I will look at it and let you know if I have questions.

patgmiller commented 1 year ago

@maximbaz it looks like i can safely include the length and symbols with each of the individual store settings in the local storage under the current stores key. The host app golang should ignore any json details not defined in the structs and it will avoid any merging. OR would you rather have one single key value pair for last saved length=saved_value and symbols=boolean? Basically

Option 1: Single last saved values (length, symbols) to be used for default on all new secrets. Global, not specific to any store. Option 2: Each password-store gets it's own combination of length, symbols).

I can see value for both. Some users may want separate default values for each. I think the single global option would be less potentially confusing, but the downside is you have to change it every time if another store need a diff default. Not sure how often that will be for some, might not be an issue at all?

I think I am in favor of Option 2 because IMO I might not want my personal store to be as long as my work related one. What are your thoughts on it?

maximbaz commented 1 year ago

I think the use-case you provided is a valid one, so I agree on option 2 👍

Idea, curious to hear your opinions, guys: remembering the last choice for length and symbols on the "add" screen is an easy productivity win if you haven't stated your desirable password length or symbols elsewhere... While I still think we shouldn't add these to the options page, what about respecting $PASSWORD_STORE_GENERATED_LENGTH and $PASSWORD_STORE_CHARACTER_SET, and maybe also having these configs in .browserpass.json file? If none of those exist, we could go with "Option 2" as per above, however if either of those is defined, then we shouldn't remember the last values for length and symbols on the "add" screen, and instead use .browserpass.json (probably should take priority, as it's per-store) or $PASSWORD_STORE_* every time 🤔 Can't decide if it's a good or a bad idea... 🤔 You guys @patgmiller and @erayd definitely have a way more complex password store structures than I do, I'm curious to hear what behavior you'd find more intuitive and useful, what would feel pleasant out of the box, without configuring Browserpass too much?

patgmiller commented 1 year ago

I think I will almost always be in favor of supporting pass functionality, so I'm in favor of @maximbaz suggestions. However, I don't know if the existence of the env vars or browserpass json config should stop us from saving last added length / symbols. Granted that's just a nuance of how order of precedence is handled, and will mostly be determined by how it is currently handled. I haven't looked closely at this yet.