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

Asynchronous Browser Access Request dialog #8273

Conversation

varjolintu
Copy link
Member

@varjolintu varjolintu commented Jul 16, 2022

Modifies the Access Control Dialog to work asynchronously, similar to the new Password Generator popup. The old/current implementation can break message sync between KeePassXC and the extension if another request for the dialog is retrieved. This can be only fixed when database is locked and reopened, which is not very user friendly.

This PR fixes the following issues:

  • When another dialog request is sent, an error is returned to the extension. The dialog must be closed before further intercation with browser is possible.
  • Fixes the message sync with browser extension when multiple access requests are retrieved.
  • Fixes the message sync if user switches tab to another page when the dialog is visible.
  • Fixes accessing the dialog (and message sync) when using multiple browsers and one of them requests the dialog.

Fixes #5765.

Testing strategy

Manually. Needs keepassxreboot/keepassxc-browser#1684 for testing.

Type of change

  • ✅ Bug fix (non-breaking change that fixes an issue)

@codecov-commenter
Copy link

codecov-commenter commented Jul 16, 2022

Codecov Report

Merging #8273 (e2899bd) into develop (4365557) will increase coverage by 0.01%.
The diff coverage is 41.93%.

@@             Coverage Diff             @@
##           develop    #8273      +/-   ##
===========================================
+ Coverage    64.43%   64.44%   +0.01%     
===========================================
  Files          339      339              
  Lines        43744    43953     +209     
===========================================
+ Hits         28184    28324     +140     
- Misses       15560    15629      +69     
Impacted Files Coverage Δ
src/autotype/AutoTypeSelectDialog.cpp 0.00% <0.00%> (ø)
src/browser/BrowserAccessControlDialog.cpp 0.00% <0.00%> (ø)
src/browser/BrowserAction.cpp 8.97% <0.00%> (-0.21%) ⬇️
src/browser/BrowserAction.h 100.00% <ø> (ø)
src/browser/BrowserService.h 100.00% <ø> (ø)
src/core/Database.h 100.00% <ø> (ø)
src/core/EntryAttributes.cpp 81.18% <ø> (ø)
src/gui/Application.cpp 31.56% <ø> (+0.28%) ⬆️
src/gui/entry/EntryView.cpp 70.78% <0.00%> (-1.17%) ⬇️
src/keys/drivers/YubiKeyInterfacePCSC.h 0.00% <ø> (ø)
... and 36 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@droidmonkey
Copy link
Member

What happens when another selection request is triggered with an existing selection dialog still visible? Do we show another one or reject the current and show a new? Could we cause a situation where you could spam access request dialogs being opened?

@varjolintu
Copy link
Member Author

What happens when another selection request is triggered with an existing selection dialog still visible? Do we show another one or reject the current and show a new? Could we cause a situation where you could spam access request dialogs being opened?

The new request is rejected. It's expected that user responds to the one that's already active.

Comment on lines +279 to +285
browserService()->findEntries(socket,
incrementedNonce,
m_clientPublicKey,
m_secretKey,
id,
hash,
requestId,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note: we might want to create a general "Request Struct" that has all this info baked into it and we can pass around to various functions that know what to do with the elements. Would reduce the variable jumble.

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.

Denied access not remembered
3 participants