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

FdoSecrets: implement unlock before search #6943

Merged

Conversation

Aetf
Copy link
Contributor

@Aetf Aetf commented Sep 24, 2021

Resolves #4443. While at it, also fixes #6942.

Right now this is based on #6915. But I'll rebase once that is merged.

Screenshots

image

Testing strategy

Added unit test also tested manually

Type of change

  • ✅ Bug fix (non-breaking change that fixes an issue)
  • ✅ New feature (change that adds functionality)

@Aetf Aetf changed the title FdoSecrets: implement unlock before search [WIP] FdoSecrets: implement unlock before search Sep 24, 2021
@Aetf
Copy link
Contributor Author

Aetf commented Sep 24, 2021

Update: added guards for prompts to run concurrently.


While haven't tested it yet, but I realize using a nested event loop for the blocking call isn't the best approach. What if another dbus call happens while KPXC sits in the nested loop? If the other call is for unrelated parts, it is probably fine because there won't be any other nesting.
But what if the other call also results in unlocking the same database?

Actually, that's a problem in general: what will happen while one database is being unlocked asynchronously, and another client requests to unlock the same database.

I need to think more about this. The solution is probably to remember database unlocking is in progress, and for any new requests, submit to the old prompt signal rather than create a new unlock dialog.

Changing to WIP for now.

@Aetf Aetf changed the title [WIP] FdoSecrets: implement unlock before search FdoSecrets: implement unlock before search Sep 25, 2021
@Aetf
Copy link
Contributor Author

Aetf commented Sep 25, 2021

Ready for review (or probably wait after #6915 so the history is cleaner) @droidmonkey

@michaelk83
Copy link

So you're going with just an internal blocking prompt, without all the IsLocked / InteractiveAuthorizationRequired stuff?

@Aetf
Copy link
Contributor Author

Aetf commented Sep 25, 2021

So you're going with just an internal blocking prompt, without all the IsLocked / InteractiveAuthorizationRequired stuff?

Yes. I view this as a workaround to increase compatibility.

The proper fix should be done in the spec, either returning InteractiveAuthorizationRequired error, or returning a prompt object just like other methods. IMHO returning a prompt object is better and consistent with the rest of the spec, though.

@droidmonkey
Copy link
Member

Rebased and squashed. Ready for review @Aetf ?

@Aetf
Copy link
Contributor Author

Aetf commented Oct 1, 2021

Somehow I can't reproduce the test failure TestGuiFdoSecrets::testServiceUnlockItems locally. But let me try to reproduce in the CI docker environment.

@Aetf Aetf force-pushed the feature/fdo-blocking-search branch from e7d84ef to 485592e Compare October 2, 2021 02:30
@Aetf
Copy link
Contributor Author

Aetf commented Oct 2, 2021

Found the root cause of test failure. Ready to review. @droidmonkey

@Aetf Aetf force-pushed the feature/fdo-blocking-search branch from 485592e to ac77491 Compare October 2, 2021 02:33
@codecov-commenter
Copy link

codecov-commenter commented Oct 2, 2021

Codecov Report

Merging #6943 (f09e97e) into develop (b6716bd) will increase coverage by 0.15%.
The diff coverage is 85.71%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #6943      +/-   ##
===========================================
+ Coverage    63.61%   63.76%   +0.15%     
===========================================
  Files          330      330              
  Lines        41807    41860      +53     
===========================================
+ Hits         26595    26692      +97     
+ Misses       15212    15168      -44     
Impacted Files Coverage Δ
src/core/Config.cpp 86.50% <ø> (ø)
src/core/Config.h 100.00% <ø> (ø)
src/fdosecrets/FdoSecretsSettings.h 100.00% <ø> (ø)
src/fdosecrets/dbus/DBusMgr.cpp 52.20% <ø> (+0.30%) ⬆️
src/fdosecrets/dbus/DBusObject.h 92.31% <ø> (ø)
src/fdosecrets/objects/Collection.h 0.00% <ø> (ø)
src/fdosecrets/objects/Service.h 100.00% <ø> (ø)
src/gui/entry/EditEntryWidget.cpp 68.69% <ø> (ø)
...rc/fdosecrets/widgets/SettingsWidgetFdoSecrets.cpp 58.92% <50.00%> (-0.10%) ⬇️
src/gui/GuiTools.cpp 50.65% <50.00%> (+0.65%) ⬆️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b6716bd...f09e97e. Read the comment docs.

src/gui/DatabaseWidget.h Outdated Show resolved Hide resolved
src/gui/entry/EditEntryWidget.cpp Show resolved Hide resolved
@droidmonkey droidmonkey added this to the v2.7.0 milestone Oct 9, 2021
Fixes keepassxreboot#6942 and fixes keepassxreboot#4443

- Return number of deleted entries
- Fix minor memory leak
- FdoSecrets: make all prompt truly async per spec and update tests
    * the waited signal may already be emitted before calling spy.wait(),
      causing the test to fail. This commit checks the count before waiting.
    * check unlock result after waiting for signal
- FdoSecrets: implement unlockBeforeSearch option
- FdoSecrets: make search always work regardless of entry group searching settings, fixes keepassxreboot#6942
- FdoSecrets: cleanup gracefully even if some test failed
- FdoSecrets: make it safe to call prompts concurrently
- FdoSecrets: make sure in unit test we click on the correct dialog

Note on the unit tests: objects are not deleted (due to deleteLater event not handled).
So there may be multiple AccessControlDialog. But only one of
it is visible and is the correctly one to click on.

Before this change, a random one may be clicked on, causing the
completed signal never be sent.
@Aetf
Copy link
Contributor Author

Aetf commented Oct 12, 2021

@droidmonkey updated this per comments. I also created #7041 to clean up deleteEntries.

StayPirate added a commit to StayPirate/dotfiles that referenced this pull request Jun 13, 2022
Thanks to the merge of PR#6943 [0] whenever a process runs a search
while the keyring is locked the user will be prompted to unlock it.

[0] keepassxreboot/keepassxc#6943
StayPirate added a commit to StayPirate/sieve-susede that referenced this pull request Jun 13, 2022
Thanks to the merge of PR#6943 [0] whenever a process runs a search
while the keyring is locked the user will be prompted to unlock it.

[0] keepassxreboot/keepassxc#6943
StayPirate added a commit to StayPirate/dotfiles that referenced this pull request Jun 13, 2022
Thanks to the merge of PR#6943 [0] whenever a process runs a search
while the keyring is locked the user will be prompted to unlock it.

[0] keepassxreboot/keepassxc#6943
StayPirate added a commit to StayPirate/dotfiles that referenced this pull request Jun 14, 2022
Thanks to the merge of PR#6943 [0] whenever a process runs a search
while the keyring is locked the user will be prompted to unlock it.

[0] keepassxreboot/keepassxc#6943
StayPirate added a commit to StayPirate/dotfiles that referenced this pull request Jun 16, 2022
Thanks to the merge of PR#6943 [0] whenever a process runs a search
while the keyring is locked the user will be prompted to unlock it.

[0] keepassxreboot/keepassxc#6943
StayPirate added a commit to StayPirate/dotfiles that referenced this pull request Jun 16, 2022
Thanks to the merge of PR#6943 [0] whenever a process runs a search
while the keyring is locked the user will be prompted to unlock it.

[0] keepassxreboot/keepassxc#6943
StayPirate added a commit to StayPirate/dotfiles that referenced this pull request Jun 17, 2022
Thanks to the merge of PR#6943 [0] whenever a process runs a search
while the keyring is locked the user will be prompted to unlock it.

[0] keepassxreboot/keepassxc#6943
StayPirate added a commit to StayPirate/dotfiles that referenced this pull request Jun 21, 2022
Thanks to the merge of PR#6943 [0] whenever a process runs a search
while the keyring is locked the user will be prompted to unlock it.

[0] keepassxreboot/keepassxc#6943
StayPirate added a commit to StayPirate/dotfiles that referenced this pull request Jun 24, 2022
Thanks to the merge of PR#6943 [0] whenever a process runs a search
while the keyring is locked the user will be prompted to unlock it.

[0] keepassxreboot/keepassxc#6943
StayPirate added a commit to StayPirate/dotfiles that referenced this pull request Jul 6, 2022
Thanks to the merge of PR#6943 [0] whenever a process runs a search
while the keyring is locked the user will be prompted to unlock it.

[0] keepassxreboot/keepassxc#6943
StayPirate added a commit to StayPirate/dotfiles that referenced this pull request Jul 29, 2022
Thanks to the merge of PR#6943 [0] whenever a process runs a search
while the keyring is locked the user will be prompted to unlock it.

[0] keepassxreboot/keepassxc#6943
@phoerious phoerious added pr: new feature Pull request that adds a new feature and removed new feature labels Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: Secret Service pr: new feature Pull request that adds a new feature
Projects
None yet
5 participants