Make suggestion for browser connection names

[ratijas]

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

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

[droidmonkey]

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]

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.

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]

We welcome your PR

[ratijas]

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. 🙂

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]

Code is here:

keepassxc/src/browser/BrowserService.cpp

Lines 327 to 329 in c6bd22a

	keyDialog.setLabelText(tr("You have received an association request for the following database:\n%1\n\n"
	"Give the connection a unique name or ID, for example:\nchrome-laptop.")
	.arg(db->metadata()->name().toHtmlEscaped()));

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]

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

[droidmonkey]

Nope, that is totally unrelated.

[ratijas]

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]

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.

[droidmonkey]

We should be showing the root group name instead.

[ratijas]

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]

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

[ratijas]

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]

We like to "keep it simple"

[l0b0]

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]

@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]

@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]

@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.