Added config file to allow backend selection (linux) by akallabeth · Pull Request #103 · frankosterfeld/qtkeychain

akallabeth · 2017-06-12T14:53:31Z

Checking for an existing configuration file /etc/xdg/qt5-keychain.conf or /etc/xdg/qt4-keychain.conf depending on Qt version compiled.
With backend=[libsecret|kwallet4|kwallet5|gnome-keyring] a specific backend can be forced and autodetection disabled.
With disable-backend=[libsecret|kwallet4|kwallet5|gnome-keyring],[[[libsecret|kwallet4|kwallet5|gnome-keyring]]? some / all backends can be skipped during auto detection

ckamm · 2018-06-14T08:23:10Z

It would be great to get this PR merged: the ownCloud client now ships qtkeychain with the libsecret backend enabled and there will likely be people who'd prefer to continue using the kwallet backend instead (see #99). With the config file setting there'd be an option for that.

Alternatively there could be an API for controling backend choice.

I'd be up for helping to get this to work.

guruz · 2018-06-14T08:27:06Z

@ckamm did you review+test this PR?
Maybe @frankosterfeld wants to merge it then..

@frankosterfeld BTW, i think we (ownCloud client guys @ogoffart @ckamm @guruz @dschmidt ) are interested in taking over more power+responsibility here if needed.

frankosterfeld · 2018-06-14T08:32:24Z

I can merge this, although having the user to do this choice seems like the very last resort to me.

I definitely could help, I have no time for this project right now, especially those issues where I would have to test on a handful of different test setups. Especially on Linux, I'm out of the loop regarding the status of the various desktops and distributions.

ckamm · 2018-06-14T08:36:09Z

@frankosterfeld Do you know if libsecret is going to superseede kwallet on KDE? If not, maybe the detection code should prefer kwallet backends for KDE sessions, instead of always prefering libsecret?

frankosterfeld · 2018-06-14T08:38:54Z

@ckamm yeah, i'm wondering if going back to kwallet by default would make sense, if it is running. But that's the part where I'm out of loop, I don't know what the status of kwallet vs gnome-keyring vs. libsecret is these days, both in deployed systems and in current development.

ckamm · 2018-06-14T08:41:08Z

@frankosterfeld At least for gnome sessions it looks like libsecret is preferred now - libgnome-keyring will actually be removed from some distros soon.

For kde I don't know either unfortunately. It looks like mid 2016 there was talk of going to ksecretservice or libsecret: https://mail.kde.org/pipermail/plasma-devel/2016-July/055668.html

guruz · 2018-06-14T08:43:07Z

@frankosterfeld I meant it the other way: We can help reviewing+merging stuff if you want.

frankosterfeld · 2018-06-14T08:46:33Z

@guruz that would probably best, moving this out of my personal space and give you guys the correct permissions.

ckamm · 2018-06-14T09:02:37Z

There was a similar discussion here: jaraco/keyring#273 that ended up with them prefering the kwallet backend over the secret service dbus api.

ckamm · 2018-06-14T09:09:30Z

It looks like the secret service dbus api on kubuntu 18.04 is provided by gnome-keyring-daemon. Switching to libsecret will thus talk to a different keyring backend than before.

I'll make a PR for prefering kwallet on kde sessions.

frankosterfeld · 2018-06-14T08:33:11Z

 }

-static KeyringBackend getKeyringBackend()
+static KeyringBackend getKeyringBackend(void)


Please remove this change

frankosterfeld · 2018-06-14T08:34:25Z

-            return Backend_Kwallet4;
-        }
+		if (!disabled.contains(backendGnomeKeyring, Qt::CaseInsensitive)) {
+			if ( GnomeKeyring::isAvailable() ) {


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

(Had this review unsubmitted here..)

akallabeth · 2018-07-09T07:55:32Z

@frankosterfeld will have a look at it, need to rebase though.
Quick question: Would it be possible to introduce a Backend_None if all of the backends are disabled?

frankosterfeld · 2018-07-09T11:25:03Z

@akallabeth would make sense to me

guruz · 2018-07-09T11:57:18Z

@akallabeth Why do you want to use QSettings::SystemScope?
From the documentation:

If scope is QSettings::UserScope, the QSettings object searches user-specific settings first, before it searches system-wide settings as a fallback. If scope is QSettings::SystemScope, the QSettings object ignores user-specific settings and provides access to system-wide settings.

http://doc.qt.io/qt-5/qsettings.html#QSettings-1

akallabeth · 2018-07-09T12:00:34Z

@guruz thought it would make most sense to have such a setting per installation and not per user.
Additionally I didn't want to think about implications with user level settings (from a security perspective)

guruz · 2018-07-09T12:06:37Z

@akallabeth I'd think user is better (it also falls back to system).
Your KDE and Gnome are anyway running as user?

ogoffart · 2018-07-10T21:22:27Z

+#if QT_VERSION >= 0x050000
+	const QString prefix = "qt5-";
+#else
+	const QString prefix = "qt4-";


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.

ogoffart · 2018-07-10T21:22:50Z

+#else
+	const QString prefix = "qt4-";
+#endif
+	const QSettings settings(QSettings::SystemScope, prefix + "keychain");


Should not be system scope.

This is something the user would configure.

ogoffart · 2018-07-10T21:23:19Z

 }

-static KeyringBackend detectKeyringBackend()
+static KeyringBackend detectKeyringBackend(void)


another useless "void"

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

ogoffart · 2018-07-11T07:59:57Z

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


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.

guruz · 2018-07-13T07:44:45Z

@akallabeth Any news on when you can update this? :) Otherwise it can't go into 0.9 (which is fine)
(Ref: #111 (comment) )

michaelk83 · 2025-04-05T09:22:08Z

Alternative solution has been merged in #264. This can probably be closed too (looks abandoned, anyway).

Added config file to allow backend selection

a11152e

ckamm mentioned this pull request Jun 12, 2018

Reading from keychain failed with error: No keychain service available Ubuntu 18.04 owncloud/client#6455

Closed

ckamm added a commit to ckamm/qtkeychain that referenced this pull request Jun 14, 2018

Prefer kwallet over libsecret for KDE frankosterfeld#103

ed2ff1b

ckamm mentioned this pull request Jun 14, 2018

Prefer kwallet over libsecret for KDE #111

Merged

frankosterfeld reviewed Jul 6, 2018

View reviewed changes

ogoffart reviewed Jul 10, 2018

View reviewed changes

ogoffart reviewed Jul 11, 2018

View reviewed changes

michaelk83 mentioned this pull request Aug 29, 2022

Feature request: Add ability to choose the backend from an environment variable. #171

Open

Conversation

akallabeth commented Jun 12, 2017

Uh oh!

ckamm commented Jun 14, 2018

Uh oh!

guruz commented Jun 14, 2018

Uh oh!

frankosterfeld commented Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ckamm commented Jun 14, 2018

Uh oh!

frankosterfeld commented Jun 14, 2018

Uh oh!

ckamm commented Jun 14, 2018

Uh oh!

guruz commented Jun 14, 2018

Uh oh!

frankosterfeld commented Jun 14, 2018

Uh oh!

ckamm commented Jun 14, 2018

Uh oh!

ckamm commented Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

frankosterfeld Jun 14, 2018

Choose a reason for hiding this comment

Uh oh!

frankosterfeld Jun 14, 2018

Choose a reason for hiding this comment

Uh oh!

frankosterfeld Jul 6, 2018

Choose a reason for hiding this comment

Uh oh!

akallabeth commented Jul 9, 2018

Uh oh!

frankosterfeld commented Jul 9, 2018

Uh oh!

guruz commented Jul 9, 2018

Uh oh!

akallabeth commented Jul 9, 2018

Uh oh!

guruz commented Jul 9, 2018

Uh oh!

ogoffart Jul 10, 2018

Choose a reason for hiding this comment

Uh oh!

ogoffart Jul 10, 2018

Choose a reason for hiding this comment

Uh oh!

ogoffart Jul 10, 2018

Choose a reason for hiding this comment

Uh oh!

akallabeth Jul 11, 2018

Choose a reason for hiding this comment

Uh oh!

ogoffart Jul 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guruz commented Jul 13, 2018

Uh oh!

michaelk83 commented Apr 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

frankosterfeld commented Jun 14, 2018 •

edited

Loading

ckamm commented Jun 14, 2018 •

edited

Loading

ogoffart Jul 11, 2018 •

edited

Loading