Prefill host when creating new password #383
Conversation
|
Thanks! It doesn't work in Chromium ( However I'd also think that popup already knows the hostname - after all, when you open popup, the search bar is already pre-filled with the hostname. Maybe we can get that domain, without querying again? I think the top part of |
|
Right, thats easier. Should work in Chromium now. On pages like |
|
Btw would you accept a PR to prefill the |
| const hostname = new URL(settings.origin).hostname; | ||
| loginObj.login = hostname; | ||
| } | ||
| } |
There was a problem hiding this comment.
cc @patgmiller if you have any feedback on this PR, and also what do you think about folder autocomplete, with this PR merged the autocomplete would basically never show up, people wouldn't know that this feature even exists - I'm currently thinking if we merge this PR we need to change auto complete so that it shows folder suggestions only based on what's to the left side of the cursor, not based on the entire input of the field - so that even when the value is prefilled, user can move cursor to the left and have folders auto suggested.
There was a problem hiding this comment.
I took a stab at it myself, in #386 - that + this PR together play very nicely in my mind. Feedback welcome, from anyone!
There was a problem hiding this comment.
@maximbaz I see what you mean. Personally I love automation, especially when it blends with flexibility for me to adjust it. I'll give them a test run tomorrow and then I should have some thing more concrete to feedback.
Regardless, if you already feel good about it don't let me stop progress. I am sure we can find a way to make them work together.
There was a problem hiding this comment.
@maximbaz I checked them both out. I initially experienced what I thought was a bug with this PR, but I think it was just a side affect of the auto pre-filling of the host and my own perception. At first it looked like I couldn't edit the path after it was pre-filled.
I love the end result of both PRs together and in fact the changes with maxim's lhs folder autocomplete fixes any functional issues I felt were present with using the host autocompletion.
However I do have two question to pose.
- For the addEditInterface, should we we auto focus the path with the cursor positioned at the start of the path? This could be done afterwards in a separate PR and I think if we did do it, that functionality might need to be configurable (on/off).
- Should the host autocomplete be also an optional configuration?
Essentially I am wondering if any auto populating values should be configured as apposed to the optional autocompletion. Does that make sense? I don't personally have any issues with it currently as it is, but I can see some people not wanting forced input changes if they don't match their personal workflow preferences.
There was a problem hiding this comment.
Thank you, very helpful! 🙏
Here are some arguments against adding new options:
- We are very minimalistic and conservative in this project when it comes to adding configuration options - I still like this approach, as it continues to force us to think more on how to provide a great UX out of the box.
- Our selling point is that we help fighting phishing attacks by prefilling popup with the current domain - in this sense, prefilling hostname also during entry creation feels very complementary to this idea.
- In the Organizing password store we define a convention that this project expects, saying that hostname must be somewhere in the path. If we know it must be there, then prefilling it guarantees to save time typing it, while still letting users edit the path to their preferences.
Having said that, I'm still open to consider adding e.g. file path template as an option, in light of #379 - but I suggest we take that separately. Auto-focus is an interesting idea, but if we do it I don't think we should do it configurable.
|
The idea with username sounds curious, a challenge could be to make sure that we aren't prefilling We can try to see how that feels, if you want to give it a go? As long as it's generic enough, doesn't give false positives and doesn't make other people unhappy, I don't think it would be a problem adding such feature - of course also dependent if @patgmiller or @erayd have any opinions on this. |
There was a problem hiding this comment.
Two minor suggestions and otherwise LGTM! Thank you for sticking with me on the review iterations!
I do think we need to do something about the folder autocomplete, but it feels unfair to block your PR because of it, so will give other maintainers up to a couple of days to comment and then will merge. And in case you feel like looking at folder autocomplete - you did mention that you use yourself www subfolder - that is of course very welcome :)
|
Sorry @maximbaz for the delay. I've had my head down on some things. I'll take a look at it now. |
So @maximbaz @ayykamp I am curious what pre-filling the user/username in the secret would look like? If we're talking about auto populating the "add new secret" content when first creating it with the "default username` from the settings, conditionally if it is set, I think that is fine. However, if we're talking about a use case where I have just opened a new site tab, filled out the create new user form, and then open the browserpass-extension to create/add a new secret and have it attempt to auto read/guess credentials from the existing tab content, I would have strong reservations against this case. I think it would be fraught with implementation and security issues. As @maximbaz points out just the task to correctly determine which form input to use is challenging enough, but add to that the potential for XSS attacks and things go downhill very fast. All of the pass community tools, to my knowledge, are based on a secure input source. But if we're reading from an unknown source I don't think that would be good. Please do correct me if I am miss understand anything about the idea in question, but those are my 2 cents. |
|
@maximbaz looks like these changes are ready, unless you are aware of other necessary modifications. |
3 participants
Fixes #346