Merge #846(kitsune): AccountSettings: escape forward- and backslashes…

… in MXIDs

Quotient/settings.cpp

@@ -3,6 +3,8 @@
 
 #include "settings.h"
 
+#include "ranges_extras.h"
+
 #include <QtCore/QUrl>
 
 using namespace Quotient;
@@ -17,6 +19,20 @@ void Settings::setLegacyNames(const QString& organizationName,
     legacyApplicationName = applicationName;
 }
 
+QString Settings::escapedForSettings(QString key)
+{
+    key.replace(u'/', u"%2F"_s);
+    key.replace(u'\\', u"%5C"_s);
+    return key;
+}
+
+QString Settings::unescapedFromSettings(QString key)
+{
+    key.replace(u"%2F"_s, u"/"_s);
+    key.replace(u"%5C"_s, u"\\"_s);
+    return key;
+}
+
 Settings::Settings(QObject* parent) : QSettings(parent)
 {}
 
@@ -57,6 +73,8 @@ QStringList Settings::childGroups() const
     for (const auto& g: legacyGroups)
         if (!groups.contains(g))
             groups.push_back(g);
+    if (group() == u"Accounts")
+        std::ranges::for_each(groups, [](QString& g) { g = unescapedFromSettings(g); }); // See #842
     return groups;
 }
 
@@ -78,7 +96,7 @@ QStringList SettingsGroup::childGroups() const
 {
     const_cast<SettingsGroup*>(this)->beginGroup(groupPath);
     const_cast<QSettings&>(legacySettings).beginGroup(groupPath);
-    QStringList l = Settings::childGroups();
+    auto l = Settings::childGroups();
     const_cast<SettingsGroup*>(this)->endGroup();
     const_cast<QSettings&>(legacySettings).endGroup();
     return l;
@@ -88,7 +106,7 @@ void SettingsGroup::remove(const QString& key)
 {
     QString fullKey { groupPath };
     if (!key.isEmpty())
-        fullKey += u'/' + key;
+        fullKey += u'/' + escapedForSettings(key);
     Settings::remove(fullKey);
 }
 
@@ -114,7 +132,7 @@ void AccountSettings::setHomeserver(const QUrl& url)
     setValue(HomeserverKey, url.toString());
 }
 
-QString AccountSettings::userId() const { return group().section(u'/', -1); }
+QString AccountSettings::userId() const { return unescapedFromSettings(group().section(u'/', -1)); }
 
 QByteArray AccountSettings::encryptionAccountPickle()
 {

Quotient/settings.h

@@ -67,8 +67,17 @@ class QUOTIENT_API Settings : public QSettings {
     }
 
     Q_INVOKABLE bool contains(const QString& key) const;
+    //! \brief Obtain the list of child groups from the current or, if missing, legacy settings
+    //! \note Group names under `Accounts` group will be automatically unescaped
+    //! \sa AccountSettings
     Q_INVOKABLE QStringList childGroups() const;
 
+    //! Escape forward- and backslashes in keys because QSettings doesn't (see #842)
+    static QString escapedForSettings(QString key);
+
+    //! Unescape `\` and `/` in keys stored with escapedForSettings()
+    static QString unescapedFromSettings(QString key);
+
 private:
     static QString legacyOrganizationName;
     static QString legacyApplicationName;
@@ -94,6 +103,9 @@ class QUOTIENT_API SettingsGroup : public Settings {
         return qv.isValid() && qv.canConvert<T>() ? qv.value<T>() : defaultValue;
     }
 
+    //! \brief Get the path for this settings group
+    //! \note Unlike Settings::childGroups(), this function will not unescape group names under
+    //!       `Accounts`
     Q_INVOKABLE QString group() const;
     Q_INVOKABLE QStringList childGroups() const;
     Q_INVOKABLE void setValue(const QString& key, const QVariant& value);
@@ -118,6 +130,12 @@ public:                                                  \
     type classname::propname() const { return get<type>(qsettingname##_L1, defaultValue); } \
     void classname::setter(type newValue) { setValue(qsettingname##_L1, std::move(newValue)); }
 
+//! \brief A group of settings for one Matrix account
+//!
+//! This class provides typesafe accessors to common account settings such as user and device id.
+//! User id (aka MXID) is stored as a group name. Although QSettings does not protect forward- and
+//! backslashes inside group names, AccountSettings covers for that, percent-encoding the user id
+//! before passing it to QSettings.
 class QUOTIENT_API AccountSettings : public SettingsGroup {
     Q_OBJECT
     Q_PROPERTY(QString userId READ userId CONSTANT)
@@ -128,7 +146,7 @@ class QUOTIENT_API AccountSettings : public SettingsGroup {
                    WRITE setEncryptionAccountPickle)
 public:
     explicit AccountSettings(const QString& accountId, QObject* parent = nullptr)
-        : SettingsGroup("Accounts/"_L1 + accountId, parent)
+        : SettingsGroup("Accounts/"_L1 + escapedForSettings(accountId), parent)
     {}
 
     QString userId() const;

autotests/CMakeLists.txt

@@ -29,4 +29,5 @@ quotient_add_test(NAME testcryptoutils)
 quotient_add_test(NAME testkeyverification)
 quotient_add_test(NAME testcrosssigning)
 quotient_add_test(NAME testkeyimport)
+quotient_add_test(NAME testsettings)
 quotient_add_test(NAME testthread)

autotests/testsettings.cpp

@@ -0,0 +1,82 @@
+// SPDX-FileCopyrightText: The Quotient Project Contributors
+// SPDX-License-Identifier: LGPL-3.0-or-later
+
+#include <QTest>
+
+#include <Quotient/settings.h>
+
+using namespace Qt::Literals;
+using Quotient::Settings, Quotient::SettingsGroup, Quotient::AccountSettings;
+
+class TestSettings : public QObject {
+    Q_OBJECT
+
+public:
+    static void initMain();
+
+private slots:
+    void initTestCase();
+    void accountSettings_data() const;
+    void accountSettings();
+
+private:
+    static inline const auto AccountsGroupName = u"Accounts"_s;
+    QSettings qSettings{};
+};
+
+void TestSettings::initMain() { QCoreApplication::setOrganizationName(u"Quotient"_s); }
+
+void TestSettings::initTestCase()
+{
+    qSettings.remove(AccountsGroupName);
+}
+
+void TestSettings::accountSettings_data() const
+{
+    QTest::addColumn<QString>("mxId");
+
+    QTest::newRow("normal MXID") << u"@user:example.org"_s;
+    QTest::newRow("MXID with slashes") << u"@user/with\\slashes:example.org"_s; // Test #842
+}
+
+void TestSettings::accountSettings()
+{
+    static const auto homeserverUrl = QUrl(u"https://example.org"_s);
+    static const auto deviceName = u"SomeDevice"_s;
+    QFETCH(QString, mxId);
+    const auto escapedMxId = Settings::escapedForSettings(mxId);
+
+    QVERIFY(SettingsGroup(AccountsGroupName).childGroups().empty()); // Pre-requisite
+    qSettings.beginGroup(AccountsGroupName);
+    { // Test writing to account settings
+        AccountSettings accSettings(mxId);
+        QCOMPARE(accSettings.userId(), mxId);
+        QCOMPARE(accSettings.group(), AccountsGroupName % u'/' % escapedMxId);
+        accSettings.setDeviceName(deviceName);
+        QCOMPARE(accSettings.deviceName(), deviceName);
+        accSettings.setHomeserver(homeserverUrl);
+        QCOMPARE(accSettings.homeserver(), homeserverUrl);
+    }
+
+    qSettings.sync();
+    // NB: QSettings::contains() doesn't work on groups, only on leaf keys; hence childGroups below
+    auto childGroups = qSettings.childGroups();
+    QVERIFY(childGroups.contains(escapedMxId));
+    QVERIFY(SettingsGroup(AccountsGroupName).childGroups().contains(mxId));
+    { // Test reading what was previously written
+        SettingsGroup allAccountNames(AccountsGroupName);
+        QCOMPARE(allAccountNames.childGroups().size(), 1);
+        AccountSettings accSettings(allAccountNames.childGroups().back());
+        QCOMPARE(accSettings.userId(), mxId);
+        QCOMPARE(accSettings.deviceName(), deviceName);
+        QCOMPARE(accSettings.homeserver(), homeserverUrl);
+    }
+    SettingsGroup(AccountsGroupName).remove(mxId); // Finally, test removal
+    qSettings.sync();
+    childGroups = qSettings.childGroups();
+    QVERIFY(!childGroups.contains(escapedMxId));
+    qSettings.endGroup();
+}
+
+QTEST_GUILESS_MAIN(TestSettings)
+#include "testsettings.moc"

quotest/quotest.cpp

@@ -3,6 +3,7 @@
 
 #include <Quotient/connection.h>
 #include <Quotient/room.h>
+#include <Quotient/settings.h>
 #include <Quotient/thread.h>
 #include <Quotient/user.h>
 #include <Quotient/uriresolver.h>