search

keepassxreboot / keepassxc

KeePassXC is a cross-platform community-driven port of the Windows application “Keepass Password Safe”.
https://keepassxc.org/
Other
21.28k stars 1.48k forks source link

Make suggestion for browser connection names #5772

Open ratijas opened 3 years ago

ratijas commented 3 years ago

Summary

Suggest meaningful connection names upon receiving association requests from browsers.

Currently it seems to be hardcoded as "chrome-laptop". Which doesn't make sense for users who originated request e.g. from Firefox.

Examples

request from firefox

I am setting up browser integration with Firefox, but unfortunately KeePassXC lacks that context. I'd wish it could come up with a default name based on real contextual data. Also, would have to ensure the uniqueness of the made-up name, appending "-1", "-2" etc. as needed.

Context

Apart from being a convenient feature, the lack of it also raises security concern for unsuspecting users. When I saw that suggestion, my first thought was: "Am I really using Firefox right now? Is my browser extension messed up?" Well, in reality, it is just a hard-coded and translated string: https://github.com/keepassxreboot/keepassxc/search?q=chrome-laptop

It would require changed to browser integration protocol's handshake to adequately fix that, unless "clientID" from change-public-keys can be used for the purpose of program identification.

Related PR: #3638

ratijas commented 3 years ago

Needs pink label: "plugin: Browser integration".

droidmonkey commented 3 years ago

It says "for example", this is also a one time action for the vast majority of people. I don't think this is worth investing time into.

ratijas commented 3 years ago

Hi, @droidmonkey

I agree that this feature request is neither high-priority, nor a security issue. However, I can't find your points any valid.

It says "for example"

So what? It's still confusing. And at least on my system, the "for example" phrase ended up on the far right edge of the pop-up, while "chrome-laptop" is directly over the text field.

this is also a one time action for the vast majority of people.

So what? Is it allowed to lower the bar on UX just because something is a one-time action?

If the app is so opinionated that it suggests something, please, make sure that suggestion is actually useful.

Also, why not using text field hints? It could've been a hint easily. But instead, when text field is empty — it is completely empty. Screenshot_20201202_213316

Also [x2] let it actually suggest a derived name by inserting it into the text field, so all user would have to do is click on "Save..." button.

droidmonkey commented 3 years ago

We welcome your PR

ratijas commented 3 years ago

I'd be happy to help! But first lets make sure we agree in general and in details upfront, so my efforts would not die in vain. :slightly_smiling_face:

Oh, and just noticed: why there is nothing after colon at the first line? "request for the following database:" like which one??? Translation sources specify %1 there. I haven't found the code which uses it yet, so i don't know what's going on.

droidmonkey commented 3 years ago

Code is here: https://github.com/keepassxreboot/keepassxc/blob/c6bd22aa12c6b0ea1566a8337cee234aef4a59c1/src/browser/BrowserService.cpp#L327-L329

To implement your request as presented you will need to do the following:

  1. Update the browser-extension protocol for connection requests to include the browser name (eg. Firefox, Chrome, etc)
  2. Update the Browser Extension to actually send that information using the new protocol
  3. Update the Browser Service (KPXC side) to put the passed browser name with the computer name into the text field of this dialog.
ratijas commented 3 years ago

enable integration for these browsers

Code which is responsible for these parts is probably relevant too.

droidmonkey commented 3 years ago

Nope, that is totally unrelated.

ratijas commented 3 years ago

Code is here:

LOL. GitHub couldn't have found it by "chrome" keyword because it was preceded by \n, thus indexed differently.

OK. Thanks for advices, @droidmonkey! Gotta go for now. Ping me back if I stale this for more than two days.

ratijas commented 3 years ago

Oh, and just noticed: why there is nothing after colon at the first line? "request for the following database:" like which one??? Translation sources specify %1 there. I haven't found the code which uses it yet, so i don't know what's going on.

The hell with it. Seems like it's perfectly legal to have an empty database name. Why is it needed anyway? I mean, I didn't even know it's a thing until I noticed that weirdly truncated message. I think in that case KeePassXC codebase should be handling "unnamed" databases better.

Database Metadata - empty Database name

droidmonkey commented 3 years ago

We should be showing the root group name instead.

ratijas commented 3 years ago

Just bumping it up to remind myself, and to say that I'm gonna need 2-3 more days before I can get back to it.

Say, Jonathan, how would you estimate the time it will take to familiarize myself with project's architecture and code base?

droidmonkey commented 3 years ago

Depends on what you are doing and your skill level, probably a month.

ratijas commented 3 years ago

As they say, the slower you go — the further you'll get.

Well, for now I only intend to fix those UI/UX bugs I reported myself. Cryptography and security issues are probably out of scope. Can't complain about my skills either. So I hope that cuts it down to a week or two.

droidmonkey commented 3 years ago

We like to "keep it simple"

l0b0 commented 1 year ago

How about instead just "Allow"/"Do not allow" buttons? It seems the actual name has no meaning beyond giving a name to the permission relation.

varjolintu commented 1 year ago

@l0b0 In that case the extension's Connected Databases tab would only show some random generated names for the connections, and it's hard to see which databases are connected if there are multiple databases in use. The names are for easier management.

Plus you only give the name once, so the process is not seen very often after all.

l0b0 commented 1 year ago

@l0b0 In that case the extension's Connected Databases tab would only show some random generated names for the connections and it's hard to see which databases are connected if there are multiple databases in use. The names are for easier management.

Which use case does this solve? I've used KeePass* with multiple databases and browsers for years, and never wanted to know which databases were in use. It seems like this is a hack, maybe for testing purposes?

Plus you only give the name once, so the process is not seen very often after all.

The issue remains that the first (and, I'd argue, also subsequent) interactions with this dialogue are confusing. Why is this value necessary? What should it be? If KeePassXC can suggest a reasonable value, why ask me for input?

varjolintu commented 1 year ago

@l0b0 For example, I am using the same database for multiple browsers. When viewing database settings from KeePassXC I can clearly see with what name I have connected a browser to it, and I can revoke the access if I wish to. If the names were just generated, I would have to make a separate comparison for the generated name.

For suggesting a name based e.g. on the browser used, that would have to include the browser's name plus some random data after that. It's possible to do of course. But for automatic "Allow / Deny" option, it think it's not very handy for database management purposes. While it can make sense to some, many wish to set the names manually.

EDIT: Support for this can be added to the Protocol V2.