Added option for connection timeout used when browser extension is trying to connect to KeePassXC #2752
Conversation
…ying to connect to KeePassXC
|
I recommend a slide bar that varies between 1.5 seconds and 60 seconds vice a text input. |
…r and handle the conversion between seconds and milliseconds during options processing
- decide if settings value should be used based on parameter count
|
|
||
| keepass.reconnect = async function(tab = null, connectionTimeout = page.settings.connectionTimeout) { | ||
| keepass.reconnect = async function(tab = null, connectionTimeout = CONNECTION_TIMEOUT) { | ||
| if (arguments.length < 2) { |
There was a problem hiding this comment.
This code block is not needed.
There was a problem hiding this comment.
From my point of view it is the most important part of the change as it makes sure to use the user configured timeout if no argument is provided. Am I missing something?
There was a problem hiding this comment.
The default value CONNECTION_TIMEOUT in the function is already setting that to the value if no argument is provided.
There was a problem hiding this comment.
CONNECTION_TIMEOUT is set to a hard value of 1500 in global.js. The user defined value in the settings is not the same value. We need to make sure that if a developer has not provided any value to the second argument of "reconnect" we are considering the settings value.
There was a problem hiding this comment.
By definition, by allowing a connectionTimeout parameter at all we can ignore the setting value. You can set it to whatever you want with a direct call. If we don't want that behavior then you need to remove the parameter altogether and always use the settings value (defaulting to CONNECTION_TIMEOUT if invalid or unset).
There was a problem hiding this comment.
The idea behind the implementation was to keep the backward compatibility of the "reconnect" method calls to not require the second parameter and still use the user setting value. I am aware that passing the settings value with every call would be possible too, but that means that in the future a developer needs to remember to pass the settings value when a new call to "reconnect" is made. To avoid this dependency I decided to handle it in "reconnect" directly. Do you think passing the settings value with every "reconnect" call is a better solution?
There was a problem hiding this comment.
My preference is to drop the parameter altogether and use the settings value directly in this function, falling back to the default value if unset. This is an internal function call and there is no reason to retain any compatibility.
There was a problem hiding this comment.
I don't think dropping this parameter is an option. Looking at background/init.js there we are using this exact parameter to pass a different timeout. It is the only place I found, but I assume this was changed for a reason :)
There was a problem hiding this comment.
The easiest solution for this is to:
- Keep the default parameter value
connectionTimeout = CONNECTION_TIMEOUTin the function. It doesn't hurt to keep the default here. - From
init.jsthe parameter is overridden with5000(we should make this a const too). - The function is called from
kpxcEvent.onReconnect()where thepage.settings.connectionTimeoutmust be used as a parameter. - The function is called also from
keepass.enableAutomaticReconnect(). This function is not used at the point, but it should also includepage.settings.connectionTimeoutas the parameter.
| page.attributeMenuItems = []; | ||
| page.blockedTabs = []; | ||
| page.clearCredentialsTimeout = null; | ||
| page.connectionTimeout = null; |
There was a problem hiding this comment.
This variable is not used, and not needed.
| $('#tab-general-settings input#defaultGroup').value = options.settings['defaultGroup']; | ||
| $('#tab-general-settings input#defaultPasskeyGroup').value = options.settings['defaultPasskeyGroup']; | ||
| $('#tab-general-settings input#clearCredentialTimeout').value = options.settings['clearCredentialsTimeout']; | ||
| const connectionTimeout = (options.settings['connectionTimeout']/1000); |
There was a problem hiding this comment.
Don't do this. Just retrieve the value from settings and do any modifications to it when inserting it as element value.
3 participants
This small feature adds a connection timeout setting which is used when keepassxc-browser extension is trying to connect to KeePassXC instance.
Fixes #2751
Screenshots or videos
Testing strategy
I tested this feature by making sure that:
Type of change