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

Secret Service: cleanup and fix crash #5660

Merged
merged 6 commits into from
Nov 16, 2020

Conversation

Aetf
Copy link
Contributor

@Aetf Aetf commented Nov 4, 2020

Fixes #5279. Aside from typos and formatting fixing, this does the following

  • Correctly propagate error message to the UI rather than ignoring them
  • Simplify the internal states of Collection, so it's less likely to be left in an inconsistent state
  • Handle corner cases when registering Collection on DBus
    • Previously QFileInfo::baseName was used to derive the name, but this can be empty if there are leading dots in the filename, causing Secret service crash when searching with null baseGroup #5279. QFileInfo::completeBaseName is used instead.
    • Although unlikely, if two open databases have the same filename (but on different paths), the second database will fail to register. New logic is added to address this.

Testing strategy

New test cases added covering the corner cases mentioned above.

Type of change

  • ✅ Bug fix (non-breaking change that fixes an issue)
  • ✅ Refactor (significant modification to existing code)

@Aetf Aetf requested a review from droidmonkey November 4, 2020 00:29
@Aetf Aetf force-pushed the fix/fdosecrets-5279 branch from ff3e518 to a2557c3 Compare November 4, 2020 00:31
@droidmonkey droidmonkey added this to the v2.7.0 milestone Nov 4, 2020
tests/gui/TestGuiFdoSecrets.cpp Outdated Show resolved Hide resolved
src/fdosecrets/FdoSecretsPlugin.cpp Outdated Show resolved Hide resolved
src/fdosecrets/FdoSecretsPlugin.cpp Show resolved Hide resolved
src/fdosecrets/FdoSecretsPlugin.h Outdated Show resolved Hide resolved
src/fdosecrets/objects/Collection.cpp Outdated Show resolved Hide resolved
src/fdosecrets/objects/Collection.cpp Outdated Show resolved Hide resolved
@Aetf Aetf force-pushed the fix/fdosecrets-5279 branch 3 times, most recently from 892143b to b16a318 Compare November 4, 2020 16:52
@Aetf Aetf requested a review from droidmonkey November 4, 2020 19:20
@Aetf
Copy link
Contributor Author

Aetf commented Nov 4, 2020

The CI failure on Windows seems unrelated

@droidmonkey
Copy link
Member

@Aetf Is this ready to go?

@Aetf Aetf force-pushed the fix/fdosecrets-5279 branch from b16a318 to d7ac18c Compare November 12, 2020 00:09
@Aetf
Copy link
Contributor Author

Aetf commented Nov 12, 2020

@droidmonkey Yes, it's ready. I just rebased.

Aetf added 5 commits November 13, 2020 17:16
This gets rid of the m_registered state, so whenever there is a valid m_backend, it is guaranteed to be registered already.

While at it, this commit also improves DBusObject::registerWithPath a little bit by allowing properly registering multiple paths using the same adaptor, mostly for supporting Collection aliases.

Now when DBus registration fails, the code does not go into an inconsistent state or crash.
…xreboot#5279

- Use completeBaseName rather than baseName to ensure nonempty name
- Handle two databases have the same name
- Cleanup Service::onDatabaseTabOpened logic
@Aetf Aetf force-pushed the fix/fdosecrets-5279 branch from d7ac18c to 668a918 Compare November 13, 2020 22:18
@Aetf Aetf force-pushed the fix/fdosecrets-5279 branch from 668a918 to 9f41189 Compare November 13, 2020 22:20
@Aetf
Copy link
Contributor Author

Aetf commented Nov 13, 2020

@droidmonkey ping. I also fixed a signal connection problem that was introduced during the refactoring.

@Aetf Aetf merged commit 3d10f31 into keepassxreboot:develop Nov 16, 2020
@Aetf Aetf deleted the fix/fdosecrets-5279 branch November 16, 2020 03:34
@droidmonkey
Copy link
Member

Excellent! Can we merge the prompt for access one soon?

@Aetf
Copy link
Contributor Author

Aetf commented Nov 16, 2020

I'm working on that one now. It still needs some work and doesn't apply cleanly with this merged. I'll update very soon 😄

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.

Secret service crash when searching with null baseGroup
3 participants