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

Prefer kwallet over libsecret for KDE #111

Merged
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 20 additions & 7 deletions keychain_unix.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -78,11 +78,19 @@ static DesktopEnvironment detectDesktopEnvironment() {

static KeyringBackend detectKeyringBackend()
{
/* Libsecret unifies access to KDE and GNOME
* password services. */
if (LibSecretKeyring::isAvailable()) {
return Backend_LibSecretKeyring;
}
/* The secret service dbus api, accessible through libsecret, is supposed
* to unify password services.
*
* Unfortunately at the time of Kubuntu 18.04 the secret service backend
* in KDE is gnome-keyring-daemon - using it has several complications:
* - the default collection isn't opened on session start, so users need
* to manually unlock it when the first application uses it
* - it's separate from the kwallet5 keyring, so switching to it means the
* existing keyring data can't be accessed anymore
*
* Thus we still prefer kwallet backends on KDE even if libsecret is
* available.
*/

switch (detectDesktopEnvironment()) {
case DesktopEnv_Kde4:
Expand All @@ -91,13 +99,18 @@ static KeyringBackend detectKeyringBackend()
case DesktopEnv_Plasma5:
return Backend_Kwallet5;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we check somehow if kwallet5 is running. And if it is not, then we'd fallback to libsecret?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will check.

break;
// fall through

// fall through
Copy link
Contributor

Choose a reason for hiding this comment

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

(nitpick: fall through what? there is already one return AND a redundent break :-) )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I didn't add it.

case DesktopEnv_Gnome:
case DesktopEnv_Unity:
case DesktopEnv_Xfce:
case DesktopEnv_Other:
default:
if ( GnomeKeyring::isAvailable() ) {
if (LibSecretKeyring::isAvailable()) {
return Backend_LibSecretKeyring;
}

if ( GnomeKeyring::isAvailable() ) {
return Backend_GnomeKeyring;
} else {
return Backend_Kwallet4;
Copy link
Contributor

@ogoffart ogoffart Jun 15, 2018

Choose a reason for hiding this comment

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

Please change to Backend_Kwallet5

So this would cover the use case that we are running Plasma 5, but neither isKwallet5Available() nor LibSecretKeyring::isAvailable() (because the application just started at startup before kwallet5d is running)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, that might happen during startup. So:

Plasma: Try kwallet5, try libsecret, try gnome-keyring, fallback kwallet5
Gnome and others: Try libsecret, try gnome-keyring, try kwallet5, fallback gnome-keyring

?

Expand Down