Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Auto-fill will be applied if related credentials are available #2409

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

wolfsilver
Copy link

If 'Auto-fill single credential entry' is selected, when multiple credentials are returned, it will also auto-fill the related credentials when available.

feat: support multi auto fill
@varjolintu
Copy link
Member

varjolintu commented Dec 15, 2024

Could you describe in more details what this fix actually does? What is the error that happens, and what is that is expected to happen?
The option is clearly described for filling a single credential. Your change fills a related credential even if there's multiple available? That's not really recommended because user cannot choose any of the others.

@wolfsilver
Copy link
Author

Your change fills a related credential even if there's multiple available? That's not really recommended because user cannot choose any of the others.

Yes. It will auto fill with related credential, but won't submit. User still can select others.

@varjolintu
Copy link
Member

This looks at the previously submitted login id, so is this valid only for passwords? That should be already covered.

@wolfsilver
Copy link
Author

It's valid for username and passwords.

Use the following method to get the relevant credentials

// Multiple entries found for the page, try to find out which one might have been used
const pageUuid = await sendMessage('page_get_login_id');
if (pageUuid) {
const credential = kpxc.credentials.find(c => c.uuid === pageUuid);
if (credential) {
return credential.login;
}
}

@varjolintu
Copy link
Member

What's still unclear:

  • Why is this under the same option that describes a different feature?
  • Where is the pageUuid set originally?
  • When does this fill actually takes place?

@wolfsilver
Copy link
Author

Why is this under the same option that describes a different feature?

Configuration description needs to be updated, or set a new configuration ?
KeePassXC-Browser will automatically fill the credentials when only one entry is received, or when relevant credentials are fetched.

Where is the pageUuid set originally?

pageUuid is updated when relevant credentials are selected, previously available functionality

When does this fill actually takes place?

same with filling a single credential current now.

@varjolintu
Copy link
Member

If you just go to any page, this feature will not work even if there are multiple credentials, because none have been selected before, right? So there must be some kind of situation where this works, and for some it does not work.

@wolfsilver
Copy link
Author

Yes, because the pageUuid is cleared when the browser is closed. Most of the time, the first credential is not the relevant one, so it will not automatically fill the first option.

@varjolintu
Copy link
Member

So my question is: if in that case none of the credentials are selected, in what scenario this added feature works as it should and fills the first "relevant" credential?

@wolfsilver
Copy link
Author

image

This pr depends on this function

@varjolintu
Copy link
Member

Then there should be a checkbox under that option that allows filling the relevant credential automatically. It shouldn't affect to the "fill single credential" option.

@wolfsilver
Copy link
Author

Then there should be a checkbox under that option that allows filling the relevant credential automatically. It shouldn't affect to the "fill single credential" option.

Add new option to control the filling of relevant credential.

Use AI to translate into other languages. Needs to be recalibrated.

image

@varjolintu
Copy link
Member

varjolintu commented Dec 15, 2024

Then there should be a checkbox under that option that allows filling the relevant credential automatically. It shouldn't affect to the "fill single credential" option.

Add new option to control the filling of relevant credential.

Use AI to translate into other languages. Needs to be recalibrated.

As I said in my previous message, that option is related to the combobox selection in another section. Its place is not here.

Use AI to translate..? What? Do not include any translations to PR's. Those are handled via Transifex.

@wolfsilver
Copy link
Author

Then there should be a checkbox under that option that allows filling the relevant credential automatically. It shouldn't affect to the "fill single credential" option.

Add new option to control the filling of relevant credential.
Use AI to translate into other languages. Needs to be recalibrated.

As I said in my previous message, that option is related to the combobox selection in another section. Its place is not here.

Use AI to translate..? What? Do not include any translations to PR's. Those are handled via Transifex.

Do you mean to move under "Sort credentials after fill by" ?
And translations reverted.

image

@varjolintu
Copy link
Member

Do you mean to move under "Sort credentials after fill by" ? And translations reverted.

Yes, but the select element's help text should be left under the selection. The new option should be below as its own.

@wolfsilver
Copy link
Author

Do you mean to move under "Sort credentials after fill by" ? And translations reverted.

Yes, but the select element's help text should be left under the selection. The new option should be below as its own.

OK now?
image

@varjolintu
Copy link
Member

varjolintu commented Dec 15, 2024

My question still is that where does this feature works if I enable the option? The internal login ID is only set after filling the credentials, and this option assumes the most relevant credential is automatically filled. If it hasn't been filled before, the sorting hasn't been even done at the first place.

I would understand this if it's only limited to passwords after username has been filled.

@wolfsilver
Copy link
Author

The sole purpose is to avoid re-selecting from the credential list if the previously used credential is chosen. When there are multiple credentials, users previously needed to select one. Now, if the last-used credential is being used, no selection is needed. However, if a different credential is required, users will still need to make a selection.

@wolfsilver
Copy link
Author

@varjolintu Should I submit translations in en/messages.json?

image

@varjolintu
Copy link
Member

@varjolintu Should I submit translations in en/messages.json?

Yes. That's the only place where new strings/messages are needed.

@varjolintu
Copy link
Member

Is there some site I can reliably test this feature?

@wolfsilver
Copy link
Author

Is there some site I can reliably test this feature?

google.com?
You can add more than one test data, then refresh the page, select "xxx", then refresh the page again and "xxx" will be automatically filled.

@varjolintu varjolintu added this to the 1.9.7 milestone Dec 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants