Skip to content

Added config file to allow backend selection (linux) #103

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.

Sign up for GitHub

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

Open
akallabeth wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Added config file to allow backend selection (linux) #103

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
65 changes: 51 additions & 14 deletions keychain_unix.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "plaintextstore_p.h"

#include <QScopedPointer>
#include <QSettings>

using namespace QKeychain;

Expand All @@ -31,6 +32,11 @@ enum DesktopEnvironment {
DesktopEnv_Other
};

static const QString backendLibSecret = "libsecret";
static const QString backendKwallet4 = "kwallet4";
static const QString backendKwallet5 = "kwallet5";
static const QString backendGnomeKeyring = "gnome-keyring";
Copy link
Contributor

@ogoffart ogoffart Jul 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Also, non-trivial static global object is to be avoided in libraries.

Edit: I'd use plain const char [], or not have them there at all and just use the plain text in the function.


// the following detection algorithm is derived from chromium,
// licensed under BSD, see base/nix/xdg_util.cc

Expand Down Expand Up @@ -76,37 +82,68 @@ static DesktopEnvironment detectDesktopEnvironment() {
return DesktopEnv_Other;
}

static KeyringBackend detectKeyringBackend()
static KeyringBackend detectKeyringBackend(void)
Copy link
Contributor

@ogoffart ogoffart Jul 10, 2018

Choose a reason for hiding this comment

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

another useless "void"

Copy link
Contributor Author

@akallabeth akallabeth Jul 11, 2018

Choose a reason for hiding this comment

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

ah, old C habits, always forget that the definition in C++ differs ^^

{
#if QT_VERSION >= 0x050000
const QString prefix = "qt5-";
#else
const QString prefix = "qt4-";
Copy link
Contributor

@ogoffart ogoffart Jul 10, 2018

Choose a reason for hiding this comment

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

I don't think we need this prefix.
The settings should apply to both version equally.

Also, there should not be so many apps using Qt4 at this point anywyay.

#endif
const QSettings settings(QSettings::SystemScope, prefix + "keychain");
Copy link
Contributor

@ogoffart ogoffart Jul 10, 2018

Choose a reason for hiding this comment

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

Should not be system scope.

This is something the user would configure.

const QString backend = settings.value("backend", "").toString();
const QStringList disabled = settings.value("disabled-backends").toStringList();

if (backend.compare(backendLibSecret, Qt::CaseInsensitive) == 0) {
return Backend_LibSecretKeyring;
}
if (backend.compare(backendKwallet4, Qt::CaseInsensitive) == 0) {
return Backend_Kwallet4;
}
if (backend.compare(backendKwallet5, Qt::CaseInsensitive) == 0) {
return Backend_Kwallet5;
}
if (backend.compare(backendGnomeKeyring, Qt::CaseInsensitive) == 0) {
return Backend_GnomeKeyring;
}

/* Libsecret unifies access to KDE and GNOME
* password services. */
if (LibSecretKeyring::isAvailable()) {
return Backend_LibSecretKeyring;
}
if (!disabled.contains(backendLibSecret)) {
if (LibSecretKeyring::isAvailable()) {
return Backend_LibSecretKeyring;
}
}

switch (detectDesktopEnvironment()) {
case DesktopEnv_Kde4:
return Backend_Kwallet4;
break;
if (!disabled.contains(backendKwallet4, Qt::CaseInsensitive)) {
return Backend_Kwallet4;
}
case DesktopEnv_Plasma5:
return Backend_Kwallet5;
break;
if (!disabled.contains(backendKwallet5, Qt::CaseInsensitive)) {
return Backend_Kwallet5;
}
// fall through
case DesktopEnv_Gnome:
case DesktopEnv_Unity:
case DesktopEnv_Xfce:
case DesktopEnv_Other:
default:
if ( GnomeKeyring::isAvailable() ) {
return Backend_GnomeKeyring;
} else {
return Backend_Kwallet4;
}
if (!disabled.contains(backendGnomeKeyring, Qt::CaseInsensitive)) {
if ( GnomeKeyring::isAvailable() ) {
Copy link
Owner

@frankosterfeld frankosterfeld Jun 14, 2018

Choose a reason for hiding this comment

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

Code looks good, but please cleanup the indentation (4 spaces, no tabs)

Copy link
Owner

@frankosterfeld frankosterfeld Jul 6, 2018

Choose a reason for hiding this comment

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

(Had this review unsubmitted here..)

return Backend_GnomeKeyring;
}
}
if (!disabled.contains(backendKwallet4, Qt::CaseInsensitive)) {
return Backend_Kwallet4;
}

return Backend_Kwallet4;
}

}

static KeyringBackend getKeyringBackend()
static KeyringBackend getKeyringBackend(void)
Copy link
Owner

@frankosterfeld frankosterfeld Jun 14, 2018

Choose a reason for hiding this comment

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

Please remove this change

{
static KeyringBackend backend = detectKeyringBackend();
return backend;
Expand Down