From d3135898fd195915fb7ec832d895defc8387b5d4 Mon Sep 17 00:00:00 2001
From: Adriaan de Groot <groot@kde.org>
Date: Tue, 13 Oct 2020 17:35:07 +0200
Subject: [PATCH 01/30] [users] More fine-grained group descriptions

Describe groups with more detail:
 - groups can be system groups (low GID) or not
 - groups may be pre-configured (e.g. come from the unpackfs stage)
---
 src/modules/users/Config.cpp | 16 ++++++++++---
 src/modules/users/Config.h   | 44 ++++++++++++++++++++++++++++++++++--
 2 files changed, 55 insertions(+), 5 deletions(-)

diff --git a/src/modules/users/Config.cpp b/src/modules/users/Config.cpp
index d0f573286..e190b30dc 100644
--- a/src/modules/users/Config.cpp
+++ b/src/modules/users/Config.cpp
@@ -590,8 +590,12 @@ Config::checkReady()
 
 
 STATICTEST void
-setConfigurationDefaultGroups( const QVariantMap& map, QStringList& defaultGroups )
+setConfigurationDefaultGroups( const QVariantMap& map, QList< GroupDescription >& defaultGroups )
 {
+    defaultGroups.clear();
+    auto groupsFromConfig = map.value( "defaultGroups" ).toList();
+    cDebug() << groupsFromConfig;
+#if 0
     // '#' is not a valid group name; use that to distinguish an empty-list
     // in the configuration (which is a legitimate, if unusual, choice)
     // from a bad or missing configuration value.
@@ -601,6 +605,7 @@ setConfigurationDefaultGroups( const QVariantMap& map, QStringList& defaultGroup
         cWarning() << "Using fallback groups. Please check *defaultGroups* in users.conf";
         defaultGroups = QStringList { "lp", "video", "network", "storage", "wheel", "audio" };
     }
+#endif
 }
 
 STATICTEST HostNameActions
@@ -737,8 +742,13 @@ Config::createJobs() const
 
     Calamares::Job* j;
 
-    j = new CreateUserJob(
-        loginName(), fullName().isEmpty() ? loginName() : fullName(), doAutoLogin(), defaultGroups() );
+    QStringList groupNames = std::accumulate(
+        m_defaultGroups.begin(),
+        m_defaultGroups.end(),
+        QStringList(),
+        []( const QStringList& l, const GroupDescription& g ) { return QStringList( l ) << g.name(); } );
+
+    j = new CreateUserJob( loginName(), fullName().isEmpty() ? loginName() : fullName(), doAutoLogin(), groupNames );
     jobs.append( Calamares::job_ptr( j ) );
 
     j = new SetPasswordJob( loginName(), userPassword() );
diff --git a/src/modules/users/Config.h b/src/modules/users/Config.h
index e4057941c..384575ed4 100644
--- a/src/modules/users/Config.h
+++ b/src/modules/users/Config.h
@@ -15,6 +15,7 @@
 #include "Job.h"
 #include "utils/NamedEnum.h"
 
+#include <QList>
 #include <QObject>
 #include <QVariantMap>
 
@@ -30,6 +31,45 @@ Q_DECLARE_OPERATORS_FOR_FLAGS( HostNameActions )
 
 const NamedEnumTable< HostNameAction >& hostNameActionNames();
 
+/** @brief Settings for a single group
+ *
+ * The list of defaultgroups from the configuration can be
+ * set up in a fine-grained way, with both user- and system-
+ * level groups; this class stores a configuration for each.
+ */
+class GroupDescription
+{
+public:
+    ///@brief An invalid, empty group
+    GroupDescription() {}
+
+    ///@brief A group with full details
+    GroupDescription( const QString& name, bool mustExistAlready = false, bool isSystem = false )
+        : m_name( name )
+        , m_isValid( !name.isEmpty() )
+        , m_mustAlreadyExist( mustExistAlready )
+        , m_isSystem( isSystem )
+    {
+    }
+
+    bool isValid() const { return m_isValid; }
+    bool isSystemGroup() const { return m_isSystem; }
+    QString name() const { return m_name; }
+
+    ///@brief Equality of groups depends only on name and kind
+    bool operator==( const GroupDescription& rhs ) const
+    {
+        return rhs.name() == name() && rhs.isSystemGroup() == isSystemGroup();
+    }
+
+private:
+    QString m_name;
+    bool m_isValid = false;
+    bool m_mustAlreadyExist = false;
+    bool m_isSystem = false;
+};
+
+
 class Config : public QObject
 {
     Q_OBJECT
@@ -158,7 +198,7 @@ public:
     /// Current setting for "require strong password"?
     bool requireStrongPasswords() const { return m_requireStrongPasswords; }
 
-    const QStringList& defaultGroups() const { return m_defaultGroups; }
+    const QList< GroupDescription >& defaultGroups() const { return m_defaultGroups; }
 
     // The user enters a password (and again in a separate UI element)
     QString userPassword() const { return m_userPassword; }
@@ -242,7 +282,7 @@ private:
     PasswordStatus passwordStatus( const QString&, const QString& ) const;
     void checkReady();
 
-    QStringList m_defaultGroups;
+    QList< GroupDescription > m_defaultGroups;
     QString m_userShell;
     QString m_autologinGroup;
     QString m_sudoersGroup;

From e46d9f735d8c6fd2ebc0e72124d7360d1e0d1b82 Mon Sep 17 00:00:00 2001
From: Adriaan de Groot <groot@kde.org>
Date: Tue, 13 Oct 2020 17:36:24 +0200
Subject: [PATCH 02/30] [users] Adjust tests to changed groups-list

---
 src/modules/users/Tests.cpp | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/src/modules/users/Tests.cpp b/src/modules/users/Tests.cpp
index 9ed7718c7..ac3a03446 100644
--- a/src/modules/users/Tests.cpp
+++ b/src/modules/users/Tests.cpp
@@ -16,7 +16,7 @@
 #include <QtTest/QtTest>
 
 // Implementation details
-extern void setConfigurationDefaultGroups( const QVariantMap& map, QStringList& defaultGroups );
+extern void setConfigurationDefaultGroups( const QVariantMap& map, QList< GroupDescription >& defaultGroups );
 extern HostNameActions getHostNameActions( const QVariantMap& configurationMap );
 extern bool addPasswordCheck( const QString& key, const QVariant& value, PasswordCheckList& passwordChecks );
 
@@ -56,29 +56,32 @@ void
 UserTests::testDefaultGroups()
 {
     {
-        QStringList groups;
+        QList< GroupDescription > groups;
         QVariantMap hweelGroup;
         QVERIFY( groups.isEmpty() );
         hweelGroup.insert( "defaultGroups", QStringList { "hweel" } );
         setConfigurationDefaultGroups( hweelGroup, groups );
         QCOMPARE( groups.count(), 1 );
-        QVERIFY( groups.contains( "hweel" ) );
+        QVERIFY( groups.contains( GroupDescription( "hweel" ) ) );
     }
 
     {
         QStringList desired { "wheel", "root", "operator" };
-        QStringList groups;
+        QList< GroupDescription > groups;
         QVariantMap threeGroup;
         QVERIFY( groups.isEmpty() );
         threeGroup.insert( "defaultGroups", desired );
         setConfigurationDefaultGroups( threeGroup, groups );
         QCOMPARE( groups.count(), 3 );
-        QVERIFY( !groups.contains( "hweel" ) );
-        QCOMPARE( groups, desired );
+        QVERIFY( !groups.contains( GroupDescription( "hweel" ) ) );
+        for ( const auto& s : desired )
+        {
+            QVERIFY( groups.contains( GroupDescription( s ) ) );
+        }
     }
 
     {
-        QStringList groups;
+        QList< GroupDescription > groups;
         QVariantMap explicitEmpty;
         QVERIFY( groups.isEmpty() );
         explicitEmpty.insert( "defaultGroups", QStringList() );
@@ -87,22 +90,22 @@ UserTests::testDefaultGroups()
     }
 
     {
-        QStringList groups;
+        QList< GroupDescription > groups;
         QVariantMap missing;
         QVERIFY( groups.isEmpty() );
         setConfigurationDefaultGroups( missing, groups );
         QCOMPARE( groups.count(), 6 );  // because of fallback!
-        QVERIFY( groups.contains( "lp" ) );
+        QVERIFY( groups.contains( GroupDescription( "lp" ) ) );
     }
 
     {
-        QStringList groups;
+        QList< GroupDescription > groups;
         QVariantMap typeMismatch;
         QVERIFY( groups.isEmpty() );
         typeMismatch.insert( "defaultGroups", 1 );
         setConfigurationDefaultGroups( typeMismatch, groups );
         QCOMPARE( groups.count(), 6 );  // because of fallback!
-        QVERIFY( groups.contains( "lp" ) );
+        QVERIFY( groups.contains( GroupDescription( "lp" ) ) );
     }
 }
 

From ceeab7087cd0ad792390615eda0e1dc8fa9fb17d Mon Sep 17 00:00:00 2001
From: Adriaan de Groot <groot@kde.org>
Date: Tue, 13 Oct 2020 22:12:26 +0200
Subject: [PATCH 03/30] [users] Fix implementation so existing tests pass

---
 src/modules/users/Config.cpp | 42 ++++++++++++++++++++++++++----------
 1 file changed, 31 insertions(+), 11 deletions(-)

diff --git a/src/modules/users/Config.cpp b/src/modules/users/Config.cpp
index e190b30dc..73465edc2 100644
--- a/src/modules/users/Config.cpp
+++ b/src/modules/users/Config.cpp
@@ -593,19 +593,39 @@ STATICTEST void
 setConfigurationDefaultGroups( const QVariantMap& map, QList< GroupDescription >& defaultGroups )
 {
     defaultGroups.clear();
-    auto groupsFromConfig = map.value( "defaultGroups" ).toList();
-    cDebug() << groupsFromConfig;
-#if 0
-    // '#' is not a valid group name; use that to distinguish an empty-list
-    // in the configuration (which is a legitimate, if unusual, choice)
-    // from a bad or missing configuration value.
-    defaultGroups = CalamaresUtils::getStringList( map, QStringLiteral( "defaultGroups" ), QStringList { "#" } );
-    if ( defaultGroups.contains( QStringLiteral( "#" ) ) )
+
+    const QString key( "defaultGroups" );
+    auto groupsFromConfig = map.value( key ).toList();
+    if ( groupsFromConfig.isEmpty() )
     {
-        cWarning() << "Using fallback groups. Please check *defaultGroups* in users.conf";
-        defaultGroups = QStringList { "lp", "video", "network", "storage", "wheel", "audio" };
+        if ( map.contains( key ) && map.value( key ).isValid() && map.value( key ).canConvert( QVariant::List ) )
+        {
+            // Explicitly set, but empty: this is valid, but unusual.
+            cDebug() << key << "has explicit empty value.";
+        }
+        else
+        {
+            cWarning() << "Using fallback groups. Please check *defaultGroups* value in users.conf";
+            for ( const auto& s : { "lp", "video", "network", "storage", "wheel", "audio" } )
+            {
+                defaultGroups.append( GroupDescription( s ) );
+            }
+        }
+    }
+    else
+    {
+        for ( const auto& v : groupsFromConfig )
+        {
+            if ( v.type() == QVariant::String )
+            {
+                defaultGroups.append( GroupDescription( v.toString() ) );
+            }
+            else
+            {
+                cWarning() << "Unknown *defaultGroups* entry" << v;
+            }
+        }
     }
-#endif
 }
 
 STATICTEST HostNameActions

From b20c80a28c6221f921046aebfe7cba034fe5f490 Mon Sep 17 00:00:00 2001
From: Adriaan de Groot <groot@kde.org>
Date: Tue, 13 Oct 2020 22:58:02 +0200
Subject: [PATCH 04/30] [users] Introduce class-scoped aliases for true and
 false for the bools

This is somewhat experimental and weird; the idea is that bool
arguments are a lot easier to understand if there are proper
names attached, rather than "true" and "false".
---
 src/modules/users/Config.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/modules/users/Config.h b/src/modules/users/Config.h
index 384575ed4..853d65b5f 100644
--- a/src/modules/users/Config.h
+++ b/src/modules/users/Config.h
@@ -40,11 +40,17 @@ const NamedEnumTable< HostNameAction >& hostNameActionNames();
 class GroupDescription
 {
 public:
+    // TODO: still too-weakly typed, add a macro to define strongly-typed bools
+    class MustExist : public std::true_type {};
+    class CreateIfNeeded : public std::false_type {};
+    class SystemGroup : public std::true_type {};
+    class UserGroup : public std::false_type {};
+
     ///@brief An invalid, empty group
     GroupDescription() {}
 
     ///@brief A group with full details
-    GroupDescription( const QString& name, bool mustExistAlready = false, bool isSystem = false )
+    GroupDescription( const QString& name, bool mustExistAlready = CreateIfNeeded{}, bool isSystem = UserGroup{} )
         : m_name( name )
         , m_isValid( !name.isEmpty() )
         , m_mustAlreadyExist( mustExistAlready )

From ff9abdfc08f71386a4b48d3a89e3e01d77616fc2 Mon Sep 17 00:00:00 2001
From: Adriaan de Groot <groot@kde.org>
Date: Tue, 13 Oct 2020 22:58:30 +0200
Subject: [PATCH 05/30] [users] The fallback groups are all **system** groups

---
 src/modules/users/Config.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/modules/users/Config.cpp b/src/modules/users/Config.cpp
index 73465edc2..45b273a46 100644
--- a/src/modules/users/Config.cpp
+++ b/src/modules/users/Config.cpp
@@ -608,7 +608,7 @@ setConfigurationDefaultGroups( const QVariantMap& map, QList< GroupDescription >
             cWarning() << "Using fallback groups. Please check *defaultGroups* value in users.conf";
             for ( const auto& s : { "lp", "video", "network", "storage", "wheel", "audio" } )
             {
-                defaultGroups.append( GroupDescription( s ) );
+                defaultGroups.append( GroupDescription( s, GroupDescription::CreateIfNeeded{}, GroupDescription::SystemGroup{} ) );
             }
         }
     }

From 07027c24304ac7cb9ce8cb7563f4e050a9682baa Mon Sep 17 00:00:00 2001
From: Adriaan de Groot <groot@kde.org>
Date: Tue, 13 Oct 2020 23:00:12 +0200
Subject: [PATCH 06/30] [users] Test distinguishes system groups from user
 groups

---
 src/modules/users/Tests.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/modules/users/Tests.cpp b/src/modules/users/Tests.cpp
index ac3a03446..6df215e02 100644
--- a/src/modules/users/Tests.cpp
+++ b/src/modules/users/Tests.cpp
@@ -95,7 +95,7 @@ UserTests::testDefaultGroups()
         QVERIFY( groups.isEmpty() );
         setConfigurationDefaultGroups( missing, groups );
         QCOMPARE( groups.count(), 6 );  // because of fallback!
-        QVERIFY( groups.contains( GroupDescription( "lp" ) ) );
+        QVERIFY( groups.contains( GroupDescription( "lp", false, GroupDescription::SystemGroup{} ) ) );
     }
 
     {
@@ -105,7 +105,7 @@ UserTests::testDefaultGroups()
         typeMismatch.insert( "defaultGroups", 1 );
         setConfigurationDefaultGroups( typeMismatch, groups );
         QCOMPARE( groups.count(), 6 );  // because of fallback!
-        QVERIFY( groups.contains( GroupDescription( "lp" ) ) );
+        QVERIFY( groups.contains( GroupDescription( "lp", false, GroupDescription::SystemGroup{} ) ) );
     }
 }
 

From a86374386b18b617a7848dfcfe803678e7ea4e83 Mon Sep 17 00:00:00 2001
From: Adriaan de Groot <groot@kde.org>
Date: Tue, 13 Oct 2020 23:13:23 +0200
Subject: [PATCH 07/30] [users] Add test for new notation for
 groups-with-details

---
 src/modules/users/Tests.cpp               |  1 +
 src/modules/users/tests/5-issue-1523.conf | 14 ++++++++++++++
 2 files changed, 15 insertions(+)
 create mode 100644 src/modules/users/tests/5-issue-1523.conf

diff --git a/src/modules/users/Tests.cpp b/src/modules/users/Tests.cpp
index 6df215e02..1716034fc 100644
--- a/src/modules/users/Tests.cpp
+++ b/src/modules/users/Tests.cpp
@@ -119,6 +119,7 @@ UserTests::testDefaultGroupsYAML_data()
     QTest::newRow( "users.conf" ) << "users.conf" << 7 << "video";
     QTest::newRow( "dashed list" ) << "tests/4-audio.conf" << 4 << "audio";
     QTest::newRow( "blocked list" ) << "tests/3-wing.conf" << 3 << "wing";
+    QTest::newRow( "issue 1523" ) << "tests/5-issue-1523.conf" << 4 << "foobar";
 }
 
 void
diff --git a/src/modules/users/tests/5-issue-1523.conf b/src/modules/users/tests/5-issue-1523.conf
new file mode 100644
index 000000000..a0c5e49ba
--- /dev/null
+++ b/src/modules/users/tests/5-issue-1523.conf
@@ -0,0 +1,14 @@
+# SPDX-FileCopyrightText: no
+# SPDX-License-Identifier: CC0-1.0
+#
+---
+defaultGroups:
+  - adm
+  -   name: foo
+      must_exist: false
+      system: true
+  -   name: bar
+      must_exist: true
+  -   name: foobar
+      must_exist: false
+      system: false

From 02e9872a9994a3a79c18b05e98fd4f0f4eb6a34f Mon Sep 17 00:00:00 2001
From: Adriaan de Groot <groot@kde.org>
Date: Tue, 13 Oct 2020 23:30:28 +0200
Subject: [PATCH 08/30] [users] Handle detailed groups list

Groups can be specified with must_exist and/or system set,
so they fill in the groups list more carefully.
---
 src/modules/users/Config.cpp | 20 +++++++++++++++++++-
 src/modules/users/Config.h   | 18 +++++++++++++-----
 src/modules/users/Tests.cpp  |  4 ++--
 3 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/src/modules/users/Config.cpp b/src/modules/users/Config.cpp
index 45b273a46..d764df846 100644
--- a/src/modules/users/Config.cpp
+++ b/src/modules/users/Config.cpp
@@ -605,10 +605,13 @@ setConfigurationDefaultGroups( const QVariantMap& map, QList< GroupDescription >
         }
         else
         {
+            // By default give the user a handful of "traditional" groups, if
+            // none are specified at all. These are system (GID < 1000) groups.
             cWarning() << "Using fallback groups. Please check *defaultGroups* value in users.conf";
             for ( const auto& s : { "lp", "video", "network", "storage", "wheel", "audio" } )
             {
-                defaultGroups.append( GroupDescription( s, GroupDescription::CreateIfNeeded{}, GroupDescription::SystemGroup{} ) );
+                defaultGroups.append(
+                    GroupDescription( s, GroupDescription::CreateIfNeeded {}, GroupDescription::SystemGroup {} ) );
             }
         }
     }
@@ -620,6 +623,21 @@ setConfigurationDefaultGroups( const QVariantMap& map, QList< GroupDescription >
             {
                 defaultGroups.append( GroupDescription( v.toString() ) );
             }
+            else if ( v.type() == QVariant::Map )
+            {
+                const auto innermap = v.toMap();
+                QString name = CalamaresUtils::getString( innermap, "name" );
+                if ( !name.isEmpty() )
+                {
+                    defaultGroups.append( GroupDescription( name,
+                                                            CalamaresUtils::getBool( innermap, "must_exist", false ),
+                                                            CalamaresUtils::getBool( innermap, "system", false ) ) );
+                }
+                else
+                {
+                    cWarning() << "Ignoring *defaultGroups* entry without a name" << v;
+                }
+            }
             else
             {
                 cWarning() << "Unknown *defaultGroups* entry" << v;
diff --git a/src/modules/users/Config.h b/src/modules/users/Config.h
index 853d65b5f..04f37d231 100644
--- a/src/modules/users/Config.h
+++ b/src/modules/users/Config.h
@@ -41,16 +41,24 @@ class GroupDescription
 {
 public:
     // TODO: still too-weakly typed, add a macro to define strongly-typed bools
-    class MustExist : public std::true_type {};
-    class CreateIfNeeded : public std::false_type {};
-    class SystemGroup : public std::true_type {};
-    class UserGroup : public std::false_type {};
+    class MustExist : public std::true_type
+    {
+    };
+    class CreateIfNeeded : public std::false_type
+    {
+    };
+    class SystemGroup : public std::true_type
+    {
+    };
+    class UserGroup : public std::false_type
+    {
+    };
 
     ///@brief An invalid, empty group
     GroupDescription() {}
 
     ///@brief A group with full details
-    GroupDescription( const QString& name, bool mustExistAlready = CreateIfNeeded{}, bool isSystem = UserGroup{} )
+    GroupDescription( const QString& name, bool mustExistAlready = CreateIfNeeded {}, bool isSystem = UserGroup {} )
         : m_name( name )
         , m_isValid( !name.isEmpty() )
         , m_mustAlreadyExist( mustExistAlready )
diff --git a/src/modules/users/Tests.cpp b/src/modules/users/Tests.cpp
index 1716034fc..0ffeeae51 100644
--- a/src/modules/users/Tests.cpp
+++ b/src/modules/users/Tests.cpp
@@ -95,7 +95,7 @@ UserTests::testDefaultGroups()
         QVERIFY( groups.isEmpty() );
         setConfigurationDefaultGroups( missing, groups );
         QCOMPARE( groups.count(), 6 );  // because of fallback!
-        QVERIFY( groups.contains( GroupDescription( "lp", false, GroupDescription::SystemGroup{} ) ) );
+        QVERIFY( groups.contains( GroupDescription( "lp", false, GroupDescription::SystemGroup {} ) ) );
     }
 
     {
@@ -105,7 +105,7 @@ UserTests::testDefaultGroups()
         typeMismatch.insert( "defaultGroups", 1 );
         setConfigurationDefaultGroups( typeMismatch, groups );
         QCOMPARE( groups.count(), 6 );  // because of fallback!
-        QVERIFY( groups.contains( GroupDescription( "lp", false, GroupDescription::SystemGroup{} ) ) );
+        QVERIFY( groups.contains( GroupDescription( "lp", false, GroupDescription::SystemGroup {} ) ) );
     }
 }
 

From ef70b2c32e2692ecb7907067ca9d93c29e0e8a14 Mon Sep 17 00:00:00 2001
From: Adriaan de Groot <groot@kde.org>
Date: Wed, 14 Oct 2020 00:23:00 +0200
Subject: [PATCH 09/30] [users] Run CreateUserJob off of the Config object

- don't pass in copies or bits of the Config, hand over the whole Config
- don't pluck some parts of the Config from Global Storage
---
 src/modules/users/Config.cpp        |  8 +----
 src/modules/users/CreateUserJob.cpp | 56 ++++++++++++++++-------------
 src/modules/users/CreateUserJob.h   |  9 ++---
 3 files changed, 35 insertions(+), 38 deletions(-)

diff --git a/src/modules/users/Config.cpp b/src/modules/users/Config.cpp
index d764df846..b3d94666a 100644
--- a/src/modules/users/Config.cpp
+++ b/src/modules/users/Config.cpp
@@ -780,13 +780,7 @@ Config::createJobs() const
 
     Calamares::Job* j;
 
-    QStringList groupNames = std::accumulate(
-        m_defaultGroups.begin(),
-        m_defaultGroups.end(),
-        QStringList(),
-        []( const QStringList& l, const GroupDescription& g ) { return QStringList( l ) << g.name(); } );
-
-    j = new CreateUserJob( loginName(), fullName().isEmpty() ? loginName() : fullName(), doAutoLogin(), groupNames );
+    j = new CreateUserJob( this );
     jobs.append( Calamares::job_ptr( j ) );
 
     j = new SetPasswordJob( loginName(), userPassword() );
diff --git a/src/modules/users/CreateUserJob.cpp b/src/modules/users/CreateUserJob.cpp
index 24b0f36d1..2bdb6296a 100644
--- a/src/modules/users/CreateUserJob.cpp
+++ b/src/modules/users/CreateUserJob.cpp
@@ -7,6 +7,8 @@
 
 #include "CreateUserJob.h"
 
+#include "Config.h"
+
 #include "GlobalStorage.h"
 #include "JobQueue.h"
 #include "utils/CalamaresUtilsSystem.h"
@@ -21,15 +23,9 @@
 #include <QTextStream>
 
 
-CreateUserJob::CreateUserJob( const QString& userName,
-                              const QString& fullName,
-                              bool autologin,
-                              const QStringList& defaultGroups )
+CreateUserJob::CreateUserJob( const Config* config )
     : Calamares::Job()
-    , m_userName( userName )
-    , m_fullName( fullName )
-    , m_autologin( autologin )
-    , m_defaultGroups( defaultGroups )
+    , m_config( config )
 {
 }
 
@@ -37,21 +33,21 @@ CreateUserJob::CreateUserJob( const QString& userName,
 QString
 CreateUserJob::prettyName() const
 {
-    return tr( "Create user %1" ).arg( m_userName );
+    return tr( "Create user %1" ).arg( m_config->loginName() );
 }
 
 
 QString
 CreateUserJob::prettyDescription() const
 {
-    return tr( "Create user <strong>%1</strong>." ).arg( m_userName );
+    return tr( "Create user <strong>%1</strong>." ).arg( m_config->loginName() );
 }
 
 
 QString
 CreateUserJob::prettyStatusMessage() const
 {
-    return tr( "Creating user %1." ).arg( m_userName );
+    return tr( "Creating user %1." ).arg( m_config->loginName() );
 }
 
 STATICTEST QStringList
@@ -86,16 +82,16 @@ groupsInTargetSystem( const QDir& targetRoot )
 }
 
 static void
-ensureGroupsExistInTarget( const QStringList& wantedGroups, const QStringList& availableGroups )
+ensureGroupsExistInTarget( const QList< GroupDescription >& wantedGroups, const QStringList& availableGroups )
 {
-    for ( const QString& group : wantedGroups )
+    for ( const auto& group : wantedGroups )
     {
-        if ( !availableGroups.contains( group ) )
+        if ( group.isValid() && !availableGroups.contains( group.name() ) )
         {
 #ifdef __FreeBSD__
-            CalamaresUtils::System::instance()->targetEnvCall( { "pw", "groupadd", "-n", group } );
+            CalamaresUtils::System::instance()->targetEnvCall( { "pw", "groupadd", "-n", group.name() } );
 #else
-            CalamaresUtils::System::instance()->targetEnvCall( { "groupadd", group } );
+            CalamaresUtils::System::instance()->targetEnvCall( { "groupadd", group.name() } );
 #endif
         }
     }
@@ -189,19 +185,29 @@ CreateUserJob::exec()
 
     cDebug() << "[CREATEUSER]: preparing groups";
 
+    // loginName(), fullName().isEmpty() ? loginName() : fullName(), doAutoLogin(), groupNames );
+    const auto& defaultGroups = m_config->defaultGroups();
+    QStringList groupsForThisUser = std::accumulate(
+        defaultGroups.begin(),
+        defaultGroups.end(),
+        QStringList(),
+        []( const QStringList& l, const GroupDescription& g ) { return QStringList( l ) << g.name(); } );
+
     QStringList availableGroups = groupsInTargetSystem( destDir );
-    QStringList groupsForThisUser = m_defaultGroups;
-    if ( m_autologin && gs->contains( "autologinGroup" ) && !gs->value( "autologinGroup" ).toString().isEmpty() )
+    ensureGroupsExistInTarget( defaultGroups, availableGroups );
+
+    if ( m_config->doAutoLogin() && !m_config->autologinGroup().isEmpty() )
     {
-        groupsForThisUser << gs->value( "autologinGroup" ).toString();
+        const QString autologinGroup = m_config->autologinGroup();
+        groupsForThisUser << autologinGroup;
+        ensureGroupsExistInTarget( QList< GroupDescription >() << GroupDescription( autologinGroup ), availableGroups );
     }
-    ensureGroupsExistInTarget( groupsForThisUser, availableGroups );
 
     // If we're looking to reuse the contents of an existing /home.
     // This GS setting comes from the **partitioning** module.
     if ( gs->value( "reuseHome" ).toBool() )
     {
-        QString shellFriendlyHome = "/home/" + m_userName;
+        QString shellFriendlyHome = "/home/" + m_config->loginName();
         QDir existingHome( destDir.absolutePath() + shellFriendlyHome );
         if ( existingHome.exists() )
         {
@@ -216,20 +222,20 @@ CreateUserJob::exec()
 
     cDebug() << "[CREATEUSER]: creating user";
 
-    auto useraddResult = createUser( m_userName, m_fullName, gs->value( "userShell" ).toString() );
+    auto useraddResult = createUser( m_config->loginName(), m_config->fullName(), m_config->userShell() );
     if ( !useraddResult )
     {
         return useraddResult;
     }
 
-    auto usergroupsResult = setUserGroups( m_userName, groupsForThisUser );
+    auto usergroupsResult = setUserGroups( m_config->loginName(), groupsForThisUser );
     if ( !usergroupsResult )
     {
         return usergroupsResult;
     }
 
-    QString userGroup = QString( "%1:%2" ).arg( m_userName ).arg( m_userName );
-    QString homeDir = QString( "/home/%1" ).arg( m_userName );
+    QString userGroup = QString( "%1:%2" ).arg( m_config->loginName() ).arg( m_config->loginName() );
+    QString homeDir = QString( "/home/%1" ).arg( m_config->loginName() );
     auto commandResult = CalamaresUtils::System::instance()->targetEnvCommand( { "chown", "-R", userGroup, homeDir } );
     if ( commandResult.getExitCode() )
     {
diff --git a/src/modules/users/CreateUserJob.h b/src/modules/users/CreateUserJob.h
index 0a46198b9..0bc140bff 100644
--- a/src/modules/users/CreateUserJob.h
+++ b/src/modules/users/CreateUserJob.h
@@ -12,23 +12,20 @@
 
 #include "Job.h"
 
-#include <QStringList>
+class Config;
 
 class CreateUserJob : public Calamares::Job
 {
     Q_OBJECT
 public:
-    CreateUserJob( const QString& userName, const QString& fullName, bool autologin, const QStringList& defaultGroups );
+    CreateUserJob( const Config* config );
     QString prettyName() const override;
     QString prettyDescription() const override;
     QString prettyStatusMessage() const override;
     Calamares::JobResult exec() override;
 
 private:
-    QString m_userName;
-    QString m_fullName;
-    bool m_autologin;
-    QStringList m_defaultGroups;
+    const Config* m_config;
 };
 
 #endif /* CREATEUSERJOB_H */

From 58f10739e110c42a74670ea62b9262ce4588d3a3 Mon Sep 17 00:00:00 2001
From: Adriaan de Groot <groot@kde.org>
Date: Wed, 14 Oct 2020 00:55:00 +0200
Subject: [PATCH 10/30] [users] Show progress during the module

- add a status member so the different steps can show progress
  as the user is created and configured. The progress values
  are hard-coded guesses as to how much work is done for each step.
- while here, reduce the scope of the global storage variable
---
 src/modules/users/CreateUserJob.cpp | 30 +++++++++++++++++++++++------
 src/modules/users/CreateUserJob.h   |  1 +
 2 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/src/modules/users/CreateUserJob.cpp b/src/modules/users/CreateUserJob.cpp
index 2bdb6296a..c9830abd4 100644
--- a/src/modules/users/CreateUserJob.cpp
+++ b/src/modules/users/CreateUserJob.cpp
@@ -47,7 +47,7 @@ CreateUserJob::prettyDescription() const
 QString
 CreateUserJob::prettyStatusMessage() const
 {
-    return tr( "Creating user %1." ).arg( m_config->loginName() );
+    return m_status.isEmpty() ? tr( "Creating user %1." ).arg( m_config->loginName() ) : m_status;
 }
 
 STATICTEST QStringList
@@ -157,14 +157,22 @@ setUserGroups( const QString& loginName, const QStringList& groups )
 Calamares::JobResult
 CreateUserJob::exec()
 {
-    Calamares::GlobalStorage* gs = Calamares::JobQueue::instance()->globalStorage();
-    QDir destDir( gs->value( "rootMountPoint" ).toString() );
+    QDir destDir;
+    bool reuseHome = false;
 
-    if ( gs->contains( "sudoersGroup" ) && !gs->value( "sudoersGroup" ).toString().isEmpty() )
     {
+        Calamares::GlobalStorage* gs = Calamares::JobQueue::instance()->globalStorage();
+        destDir = QDir( gs->value( "rootMountPoint" ).toString() );
+        reuseHome = gs->value( "reuseHome" ).toBool();
+    }
+
+    if ( !m_config->sudoersGroup().isEmpty() )
+    {
+        m_status = tr( "Preparing sudo for user %1" ).arg( m_config->loginName() );
+        emit progress( 0.05 );
         cDebug() << "[CREATEUSER]: preparing sudoers";
 
-        QString sudoersLine = QString( "%%1 ALL=(ALL) ALL\n" ).arg( gs->value( "sudoersGroup" ).toString() );
+        QString sudoersLine = QString( "%%1 ALL=(ALL) ALL\n" ).arg( m_config->sudoersGroup() );
         auto fileResult
             = CalamaresUtils::System::instance()->createTargetFile( QStringLiteral( "/etc/sudoers.d/10-installer" ),
                                                                     sudoersLine.toUtf8().constData(),
@@ -185,6 +193,8 @@ CreateUserJob::exec()
 
     cDebug() << "[CREATEUSER]: preparing groups";
 
+    m_status = tr( "Preparing groups for user %1" ).arg( m_config->loginName() );
+    emit progress( 0.1 );
     // loginName(), fullName().isEmpty() ? loginName() : fullName(), doAutoLogin(), groupNames );
     const auto& defaultGroups = m_config->defaultGroups();
     QStringList groupsForThisUser = std::accumulate(
@@ -205,8 +215,10 @@ CreateUserJob::exec()
 
     // If we're looking to reuse the contents of an existing /home.
     // This GS setting comes from the **partitioning** module.
-    if ( gs->value( "reuseHome" ).toBool() )
+    if ( reuseHome )
     {
+        m_status = tr( "Preserving home directory" );
+        emit progress( 0.2 );
         QString shellFriendlyHome = "/home/" + m_config->loginName();
         QDir existingHome( destDir.absolutePath() + shellFriendlyHome );
         if ( existingHome.exists() )
@@ -222,18 +234,24 @@ CreateUserJob::exec()
 
     cDebug() << "[CREATEUSER]: creating user";
 
+    m_status = tr( "Creating user %1" ).arg( m_config->loginName() );
+    emit progress( 0.5 );
     auto useraddResult = createUser( m_config->loginName(), m_config->fullName(), m_config->userShell() );
     if ( !useraddResult )
     {
         return useraddResult;
     }
 
+    m_status = tr( "Configuring user %1" ).arg( m_config->loginName() );
+    emit progress( 0.8 );
     auto usergroupsResult = setUserGroups( m_config->loginName(), groupsForThisUser );
     if ( !usergroupsResult )
     {
         return usergroupsResult;
     }
 
+    m_status = tr( "Setting file permissions" );
+    emit progress( 0.9 );
     QString userGroup = QString( "%1:%2" ).arg( m_config->loginName() ).arg( m_config->loginName() );
     QString homeDir = QString( "/home/%1" ).arg( m_config->loginName() );
     auto commandResult = CalamaresUtils::System::instance()->targetEnvCommand( { "chown", "-R", userGroup, homeDir } );
diff --git a/src/modules/users/CreateUserJob.h b/src/modules/users/CreateUserJob.h
index 0bc140bff..28a48c886 100644
--- a/src/modules/users/CreateUserJob.h
+++ b/src/modules/users/CreateUserJob.h
@@ -26,6 +26,7 @@ public:
 
 private:
     const Config* m_config;
+    QString m_status;
 };
 
 #endif /* CREATEUSERJOB_H */

From 788a233319a1d5c143351098e9fb5709de0a8700 Mon Sep 17 00:00:00 2001
From: Adriaan de Groot <groot@kde.org>
Date: Wed, 14 Oct 2020 01:10:48 +0200
Subject: [PATCH 11/30] [users] Introduce a test for Config getters and setters

---
 src/modules/users/Tests.cpp | 57 +++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/src/modules/users/Tests.cpp b/src/modules/users/Tests.cpp
index 0ffeeae51..e973868db 100644
--- a/src/modules/users/Tests.cpp
+++ b/src/modules/users/Tests.cpp
@@ -33,6 +33,9 @@ public:
 private Q_SLOTS:
     void initTestCase();
 
+    // Derpy test for getting and setting regular values
+    void testGetSet();
+
     void testDefaultGroups();
     void testDefaultGroupsYAML_data();
     void testDefaultGroupsYAML();
@@ -52,6 +55,60 @@ UserTests::initTestCase()
     cDebug() << "Users test started.";
 }
 
+void
+UserTests::testGetSet()
+{
+    Config c;
+
+    {
+        const QString sh( "/bin/sh" );
+        QCOMPARE( c.userShell(), QString() );
+        c.setUserShell( sh );
+        QCOMPARE( c.userShell(), sh );
+        c.setUserShell( sh + sh );
+        QCOMPARE( c.userShell(), sh + sh );
+
+        const QString badsh( "bash" );  // Not absolute, that's bad
+        c.setUserShell( badsh );
+        QEXPECT_FAIL( "", "Shell Unchanged", Abort );
+        QCOMPARE( c.userShell(), badsh );
+        QCOMPARE( c.userShell(), sh + sh );  // what was set previously
+
+        // Explicit set to empty is ok
+        c.setUserShell( QString() );
+        QCOMPARE( c.userShell(), QString() );
+    }
+    {
+        const QString al( "autolg" );
+        QCOMPARE( c.autologinGroup(), QString() );
+        c.setAutologinGroup( al );
+        QCOMPARE( c.autologinGroup(), al );
+        QVERIFY( !c.doAutoLogin() );
+        c.setAutoLogin( true );
+        QVERIFY( c.doAutoLogin() );
+        QCOMPARE( c.autologinGroup(), al );
+    }
+    {
+        const QString su( "sudogrp" );
+        QCOMPARE( c.sudoersGroup(), QString() );
+        c.setSudoersGroup( su );
+        QCOMPARE( c.sudoersGroup(), su );
+    }
+    {
+        const QString ful( "Jan-Jaap Karel Kees" );
+        const QString lg( "jjkk" );
+        QCOMPARE( c.fullName(), QString() );
+        QCOMPARE( c.loginName(), QString() );
+        QVERIFY( !c.loginNameStatus().isEmpty() );  // login name is not ok
+        c.setLoginName( lg );
+        c.setFullName( ful );
+        QVERIFY( c.loginNameStatus().isEmpty() );  // now it's ok
+        QCOMPARE( c.loginName(), lg );
+        QCOMPARE( c.fullName(), ful );
+    }
+}
+
+
 void
 UserTests::testDefaultGroups()
 {

From 6b2d7f6a4259cf575a5736749bd5789d0bfc7db8 Mon Sep 17 00:00:00 2001
From: Adriaan de Groot <groot@kde.org>
Date: Wed, 14 Oct 2020 01:15:16 +0200
Subject: [PATCH 12/30] [users] Protect against JobQueue or GS being NULL

- Avoid SIGSEGV in tests, make sure JobQueue exists, GS optional
---
 src/modules/users/Config.cpp | 5 +++++
 src/modules/users/Tests.cpp  | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/src/modules/users/Config.cpp b/src/modules/users/Config.cpp
index b3d94666a..da57db0fc 100644
--- a/src/modules/users/Config.cpp
+++ b/src/modules/users/Config.cpp
@@ -34,6 +34,11 @@ static void
 updateGSAutoLogin( bool doAutoLogin, const QString& login )
 {
     Calamares::GlobalStorage* gs = Calamares::JobQueue::instance()->globalStorage();
+    if ( !gs )
+    {
+        cWarning() << "No Global Storage available";
+        return;
+    }
 
     if ( doAutoLogin && !login.isEmpty() )
     {
diff --git a/src/modules/users/Tests.cpp b/src/modules/users/Tests.cpp
index e973868db..3d4aa8694 100644
--- a/src/modules/users/Tests.cpp
+++ b/src/modules/users/Tests.cpp
@@ -53,6 +53,11 @@ UserTests::initTestCase()
 {
     Logger::setupLogLevel( Logger::LOGDEBUG );
     cDebug() << "Users test started.";
+
+    if ( !Calamares::JobQueue::instance() )
+    {
+        (void)new Calamares::JobQueue();
+    }
 }
 
 void

From f726634c2fe6ba668648e961181108e4e0b1c48c Mon Sep 17 00:00:00 2001
From: Adriaan de Groot <groot@kde.org>
Date: Wed, 14 Oct 2020 12:52:47 +0200
Subject: [PATCH 13/30] [users] Fix tests for setting shell

- The EXPECT_FAIL value "Abort" stops the test (I wanted 'if this
  unexpectedly passes, raise an error' -- should have read the
  documentation more closely).
- Set the shell in the config object, not just in GS.
---
 src/modules/users/Config.cpp | 13 +++++++++----
 src/modules/users/Tests.cpp  |  4 +---
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/src/modules/users/Config.cpp b/src/modules/users/Config.cpp
index da57db0fc..e8d0192ee 100644
--- a/src/modules/users/Config.cpp
+++ b/src/modules/users/Config.cpp
@@ -100,11 +100,16 @@ Config::setUserShell( const QString& shell )
         cWarning() << "User shell" << shell << "is not an absolute path.";
         return;
     }
-    // The shell is put into GS because the CreateUser job expects it there
-    auto* gs = Calamares::JobQueue::instance()->globalStorage();
-    if ( gs )
+    if ( shell != m_userShell )
     {
-        gs->insert( "userShell", shell );
+        m_userShell = shell;
+        emit userShellChanged(shell);
+        // The shell is put into GS as well.
+        auto* gs = Calamares::JobQueue::instance()->globalStorage();
+        if ( gs )
+        {
+            gs->insert( "userShell", shell );
+        }
     }
 }
 
diff --git a/src/modules/users/Tests.cpp b/src/modules/users/Tests.cpp
index 3d4aa8694..b1ec8ebbc 100644
--- a/src/modules/users/Tests.cpp
+++ b/src/modules/users/Tests.cpp
@@ -74,9 +74,7 @@ UserTests::testGetSet()
         QCOMPARE( c.userShell(), sh + sh );
 
         const QString badsh( "bash" );  // Not absolute, that's bad
-        c.setUserShell( badsh );
-        QEXPECT_FAIL( "", "Shell Unchanged", Abort );
-        QCOMPARE( c.userShell(), badsh );
+        c.setUserShell( badsh );  // .. so unchanged
         QCOMPARE( c.userShell(), sh + sh );  // what was set previously
 
         // Explicit set to empty is ok

From 89e279c96ad9af66b36a735d6de8d71bb8718321 Mon Sep 17 00:00:00 2001
From: Adriaan de Groot <groot@kde.org>
Date: Wed, 14 Oct 2020 15:04:37 +0200
Subject: [PATCH 14/30] [users] Set auto- and sudo-groups correctly

- Was updating GS only, not internals
- restructure all to update internals, then GS, then emit change signals
---
 src/modules/users/Config.cpp | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/src/modules/users/Config.cpp b/src/modules/users/Config.cpp
index e8d0192ee..024661d8d 100644
--- a/src/modules/users/Config.cpp
+++ b/src/modules/users/Config.cpp
@@ -127,15 +127,23 @@ insertInGlobalStorage( const QString& key, const QString& group )
 void
 Config::setAutologinGroup( const QString& group )
 {
-    insertInGlobalStorage( QStringLiteral( "autologinGroup" ), group );
-    emit autologinGroupChanged( group );
+    if ( group != m_autologinGroup )
+    {
+        m_autologinGroup = group;
+        insertInGlobalStorage( QStringLiteral( "autologinGroup" ), group );
+        emit autologinGroupChanged( group );
+    }
 }
 
 void
 Config::setSudoersGroup( const QString& group )
 {
-    insertInGlobalStorage( QStringLiteral( "sudoersGroup" ), group );
-    emit sudoersGroupChanged( group );
+    if ( group != m_sudoersGroup )
+    {
+        m_sudoersGroup = group;
+        insertInGlobalStorage( QStringLiteral( "sudoersGroup" ), group );
+        emit sudoersGroupChanged( group );
+    }
 }
 
 
@@ -144,10 +152,9 @@ Config::setLoginName( const QString& login )
 {
     if ( login != m_loginName )
     {
-        updateGSAutoLogin( doAutoLogin(), login );
-
         m_customLoginName = !login.isEmpty();
         m_loginName = login;
+        updateGSAutoLogin( doAutoLogin(), login );
         emit loginNameChanged( login );
         emit loginNameStatusChanged( loginNameStatus() );
     }
@@ -199,6 +206,8 @@ Config::setHostName( const QString& host )
 {
     if ( host != m_hostName )
     {
+        m_customHostName = !host.isEmpty();
+        m_hostName = host;
         Calamares::GlobalStorage* gs = Calamares::JobQueue::instance()->globalStorage();
         if ( host.isEmpty() )
         {
@@ -208,9 +217,6 @@ Config::setHostName( const QString& host )
         {
             gs->insert( "hostname", host );
         }
-
-        m_customHostName = !host.isEmpty();
-        m_hostName = host;
         emit hostNameChanged( host );
         emit hostNameStatusChanged( hostNameStatus() );
     }
@@ -379,8 +385,8 @@ Config::setAutoLogin( bool b )
 {
     if ( b != m_doAutoLogin )
     {
-        updateGSAutoLogin( b, loginName() );
         m_doAutoLogin = b;
+        updateGSAutoLogin( b, loginName() );
         emit autoLoginChanged( b );
     }
 }

From 2911c789f928d65307e315aaf2f58dd56d183ba5 Mon Sep 17 00:00:00 2001
From: Adriaan de Groot <groot@kde.org>
Date: Wed, 14 Oct 2020 15:15:47 +0200
Subject: [PATCH 15/30] [users] Fix up tests for login status

- an empty login name is "ok" even if it isn't -- there's no
  warning message in that case
---
 src/modules/users/Tests.cpp | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/modules/users/Tests.cpp b/src/modules/users/Tests.cpp
index b1ec8ebbc..5259fb8a8 100644
--- a/src/modules/users/Tests.cpp
+++ b/src/modules/users/Tests.cpp
@@ -102,12 +102,14 @@ UserTests::testGetSet()
         const QString lg( "jjkk" );
         QCOMPARE( c.fullName(), QString() );
         QCOMPARE( c.loginName(), QString() );
-        QVERIFY( !c.loginNameStatus().isEmpty() );  // login name is not ok
+        QVERIFY( c.loginNameStatus().isEmpty() );  // empty login name is ok
         c.setLoginName( lg );
         c.setFullName( ful );
-        QVERIFY( c.loginNameStatus().isEmpty() );  // now it's ok
+        QVERIFY( c.loginNameStatus().isEmpty() );  // now it's still ok
         QCOMPARE( c.loginName(), lg );
         QCOMPARE( c.fullName(), ful );
+        c.setLoginName( "root" );
+        QVERIFY( !c.loginNameStatus().isEmpty() );  // can't be root
     }
 }
 

From 328a5bbbfb693c16e11e203ac98445e2a449c2d8 Mon Sep 17 00:00:00 2001
From: Adriaan de Groot <groot@kde.org>
Date: Sat, 17 Oct 2020 15:48:12 +0200
Subject: [PATCH 16/30] [users] Don't allow continuing with an empty login name

The status for an empty login name is '' (empty), for ok -- this is
so that there is no complaint about it. But it's not ok to
continue with an empty name.
---
 src/modules/users/Config.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/modules/users/Config.cpp b/src/modules/users/Config.cpp
index 024661d8d..1216b22a2 100644
--- a/src/modules/users/Config.cpp
+++ b/src/modules/users/Config.cpp
@@ -582,7 +582,7 @@ Config::isReady() const
 {
     bool readyFullName = !fullName().isEmpty();  // Needs some text
     bool readyHostname = hostNameStatus().isEmpty();  // .. no warning message
-    bool readyUsername = loginNameStatus().isEmpty();  // .. no warning message
+    bool readyUsername = !loginName().isEmpty() && loginNameStatus().isEmpty();  // .. no warning message
     bool readyUserPassword = userPasswordValidity() != Config::PasswordValidity::Invalid;
     bool readyRootPassword = rootPasswordValidity() != Config::PasswordValidity::Invalid;
     return readyFullName && readyHostname && readyUsername && readyUserPassword && readyRootPassword;

From 29e6934672464fa84bca5faa7440919184c8da00 Mon Sep 17 00:00:00 2001
From: Adriaan de Groot <groot@kde.org>
Date: Wed, 21 Oct 2020 14:43:45 +0200
Subject: [PATCH 17/30] [users] Factor out Sudo creation into separate job

---
 src/modules/users/CMakeLists.txt    |  1 +
 src/modules/users/Config.cpp        |  9 ++++-
 src/modules/users/CreateUserJob.cpp | 25 -------------
 src/modules/users/MiscJobs.cpp      | 57 +++++++++++++++++++++++++++++
 src/modules/users/MiscJobs.h        | 34 +++++++++++++++++
 5 files changed, 100 insertions(+), 26 deletions(-)
 create mode 100644 src/modules/users/MiscJobs.cpp
 create mode 100644 src/modules/users/MiscJobs.h

diff --git a/src/modules/users/CMakeLists.txt b/src/modules/users/CMakeLists.txt
index 5752ae67a..d758e2bb9 100644
--- a/src/modules/users/CMakeLists.txt
+++ b/src/modules/users/CMakeLists.txt
@@ -25,6 +25,7 @@ include_directories( ${PROJECT_BINARY_DIR}/src/libcalamaresui )
 
 set( JOB_SRC
     CreateUserJob.cpp
+    MiscJobs.cpp
     SetPasswordJob.cpp
     SetHostNameJob.cpp
 )
diff --git a/src/modules/users/Config.cpp b/src/modules/users/Config.cpp
index 1216b22a2..dc50f42c5 100644
--- a/src/modules/users/Config.cpp
+++ b/src/modules/users/Config.cpp
@@ -10,6 +10,7 @@
 #include "Config.h"
 
 #include "CreateUserJob.h"
+#include "MiscJobs.h"
 #include "SetHostNameJob.h"
 #include "SetPasswordJob.h"
 
@@ -103,7 +104,7 @@ Config::setUserShell( const QString& shell )
     if ( shell != m_userShell )
     {
         m_userShell = shell;
-        emit userShellChanged(shell);
+        emit userShellChanged( shell );
         // The shell is put into GS as well.
         auto* gs = Calamares::JobQueue::instance()->globalStorage();
         if ( gs )
@@ -796,6 +797,12 @@ Config::createJobs() const
 
     Calamares::Job* j;
 
+    if ( m_sudoersGroup.isEmpty() )
+    {
+        j = new SetupSudoJob( m_sudoersGroup );
+        jobs.append( Calamares::job_ptr( j ) );
+    }
+
     j = new CreateUserJob( this );
     jobs.append( Calamares::job_ptr( j ) );
 
diff --git a/src/modules/users/CreateUserJob.cpp b/src/modules/users/CreateUserJob.cpp
index c9830abd4..efbc4d9e3 100644
--- a/src/modules/users/CreateUserJob.cpp
+++ b/src/modules/users/CreateUserJob.cpp
@@ -166,31 +166,6 @@ CreateUserJob::exec()
         reuseHome = gs->value( "reuseHome" ).toBool();
     }
 
-    if ( !m_config->sudoersGroup().isEmpty() )
-    {
-        m_status = tr( "Preparing sudo for user %1" ).arg( m_config->loginName() );
-        emit progress( 0.05 );
-        cDebug() << "[CREATEUSER]: preparing sudoers";
-
-        QString sudoersLine = QString( "%%1 ALL=(ALL) ALL\n" ).arg( m_config->sudoersGroup() );
-        auto fileResult
-            = CalamaresUtils::System::instance()->createTargetFile( QStringLiteral( "/etc/sudoers.d/10-installer" ),
-                                                                    sudoersLine.toUtf8().constData(),
-                                                                    CalamaresUtils::System::WriteMode::Overwrite );
-
-        if ( fileResult )
-        {
-            if ( !CalamaresUtils::Permissions::apply( fileResult.path(), 0440 ) )
-            {
-                return Calamares::JobResult::error( tr( "Cannot chmod sudoers file." ) );
-            }
-        }
-        else
-        {
-            return Calamares::JobResult::error( tr( "Cannot create sudoers file for writing." ) );
-        }
-    }
-
     cDebug() << "[CREATEUSER]: preparing groups";
 
     m_status = tr( "Preparing groups for user %1" ).arg( m_config->loginName() );
diff --git a/src/modules/users/MiscJobs.cpp b/src/modules/users/MiscJobs.cpp
new file mode 100644
index 000000000..4f9472cc6
--- /dev/null
+++ b/src/modules/users/MiscJobs.cpp
@@ -0,0 +1,57 @@
+/* === This file is part of Calamares - <https://calamares.io> ===
+ *
+ *   SPDX-FileCopyrightText: 2014-2015 Teo Mrnjavac <teo@kde.org>
+ *   SPDX-FileCopyrightText: 2020 Adriaan de Groot <groot@kde.org>
+ *   SPDX-License-Identifier: GPL-3.0-or-later
+ *
+ *   Calamares is Free Software: see the License-Identifier above.
+ *
+ */
+
+#include "MiscJobs.h"
+
+#include "Config.h"
+
+#include "utils/CalamaresUtilsSystem.h"
+#include "utils/Logger.h"
+#include "utils/Permissions.h"
+
+SetupSudoJob::SetupSudoJob( const QString& group )
+    : m_sudoGroup( group )
+{
+}
+
+QString
+SetupSudoJob::prettyName() const
+{
+    return tr( "Configure <pre>sudo</pre> users." );
+}
+
+Calamares::JobResult
+SetupSudoJob::exec()
+{
+    if ( m_sudoGroup.isEmpty() )
+    {
+        return Calamares::JobResult::ok();
+    }
+
+    QString sudoersLine = QString( "%%1 ALL=(ALL) ALL\n" ).arg( m_sudoGroup );
+    auto fileResult
+        = CalamaresUtils::System::instance()->createTargetFile( QStringLiteral( "/etc/sudoers.d/10-installer" ),
+                                                                sudoersLine.toUtf8().constData(),
+                                                                CalamaresUtils::System::WriteMode::Overwrite );
+
+    if ( fileResult )
+    {
+        if ( !CalamaresUtils::Permissions::apply( fileResult.path(), 0440 ) )
+        {
+            return Calamares::JobResult::error( tr( "Cannot chmod sudoers file." ) );
+        }
+    }
+    else
+    {
+        return Calamares::JobResult::error( tr( "Cannot create sudoers file for writing." ) );
+    }
+
+    return Calamares::JobResult::ok();
+}
diff --git a/src/modules/users/MiscJobs.h b/src/modules/users/MiscJobs.h
new file mode 100644
index 000000000..2f8055e8a
--- /dev/null
+++ b/src/modules/users/MiscJobs.h
@@ -0,0 +1,34 @@
+/* === This file is part of Calamares - <https://calamares.io> ===
+ *
+ *   SPDX-FileCopyrightText: 2014-2015 Teo Mrnjavac <teo@kde.org>
+ *   SPDX-FileCopyrightText: 2020 Adriaan de Groot <groot@kde.org>
+ *   SPDX-License-Identifier: GPL-3.0-or-later
+ *
+ *   Calamares is Free Software: see the License-Identifier above.
+ *
+ */
+
+/**@file Various small jobs
+ *
+ * This file collects miscellaneous jobs that need to be run to prepare
+ * the system for the user-creation job.
+ */
+
+#ifndef USERS_MISCJOBS_H
+#define USERS_MISCJOBS_H
+
+#include "Job.h"
+
+class SetupSudoJob : public Calamares::Job
+{
+    Q_OBJECT
+public:
+    SetupSudoJob( const QString& group );
+    QString prettyName() const override;
+    Calamares::JobResult exec() override;
+
+public:
+    QString m_sudoGroup;
+};
+
+#endif

From 6560ef00a1bbb38f1a5262e6d67289574b906881 Mon Sep 17 00:00:00 2001
From: Adriaan de Groot <groot@kde.org>
Date: Wed, 21 Oct 2020 14:46:07 +0200
Subject: [PATCH 18/30] [usersq] Fix build of usersq after refactor elsewhere

---
 src/modules/usersq/CMakeLists.txt | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/modules/usersq/CMakeLists.txt b/src/modules/usersq/CMakeLists.txt
index 26c270bfb..b6717e7b8 100644
--- a/src/modules/usersq/CMakeLists.txt
+++ b/src/modules/usersq/CMakeLists.txt
@@ -33,12 +33,13 @@ calamares_add_plugin( usersq
     TYPE viewmodule
     EXPORT_MACRO PLUGINDLLEXPORT_PRO
     SOURCES
-		${_users}/Config.cpp
-		${_users}/CreateUserJob.cpp
+        ${_users}/CheckPWQuality.cpp
+        ${_users}/Config.cpp
+        ${_users}/CreateUserJob.cpp
+        ${_users}/MiscJobs.cpp
+        ${_users}/SetHostNameJob.cpp
         ${_users}/SetPasswordJob.cpp
         UsersQmlViewStep.cpp
-        ${_users}/SetHostNameJob.cpp
-        ${_users}/CheckPWQuality.cpp
     RESOURCES
         usersq.qrc
     LINK_PRIVATE_LIBRARIES

From 3d289f345ad4d175842c2eb1448b9c869ca9c20e Mon Sep 17 00:00:00 2001
From: Adriaan de Groot <groot@kde.org>
Date: Wed, 21 Oct 2020 15:02:33 +0200
Subject: [PATCH 19/30] [users] Refactor users functionality to a static lib

- this makes it less fragile to share all the functionality
  with usersq, since it ends up in the library which is shared
  between the modules.
---
 src/modules/users/CMakeLists.txt  | 27 ++++++++++++++++++---------
 src/modules/users/Config.h        |  2 +-
 src/modules/usersq/CMakeLists.txt | 10 ++--------
 3 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/src/modules/users/CMakeLists.txt b/src/modules/users/CMakeLists.txt
index d758e2bb9..ca5d24d34 100644
--- a/src/modules/users/CMakeLists.txt
+++ b/src/modules/users/CMakeLists.txt
@@ -23,25 +23,35 @@ endif()
 
 include_directories( ${PROJECT_BINARY_DIR}/src/libcalamaresui )
 
-set( JOB_SRC
+set( _users_src
+    # Jobs
     CreateUserJob.cpp
     MiscJobs.cpp
     SetPasswordJob.cpp
     SetHostNameJob.cpp
-)
-set( CONFIG_SRC
+    # Configuration
     CheckPWQuality.cpp
     Config.cpp
 )
 
+calamares_add_library(
+    users_internal
+    EXPORT_MACRO PLUGINDLLEXPORT_PRO
+    TARGET_TYPE STATIC
+    NO_INSTALL
+    NO_VERSION
+    SOURCES
+        ${_users_src}
+    LINK_LIBRARIES
+        Qt5::DBus
+)
+
 calamares_add_plugin( users
     TYPE viewmodule
     EXPORT_MACRO PLUGINDLLEXPORT_PRO
     SOURCES
         UsersViewStep.cpp
         UsersPage.cpp
-        ${JOB_SRC}
-        ${CONFIG_SRC}
     UI
         page_usersetup.ui
     RESOURCES
@@ -50,7 +60,7 @@ calamares_add_plugin( users
         calamaresui
         ${CRYPT_LIBRARIES}
         ${USER_EXTRA_LIB}
-        Qt5::DBus
+        users_internal
     SHARED_LIB
 )
 
@@ -83,10 +93,9 @@ calamares_add_test(
     userstest
     SOURCES
         Tests.cpp
-        ${JOB_SRC}
-        ${CONFIG_SRC}
+        ${_users_src}  # Build again with test-visibility
     LIBRARIES
         ${USER_EXTRA_LIB}
-        Qt5::DBus  # HostName job can use DBus to systemd
         ${CRYPT_LIBRARIES}  # SetPassword job uses crypt()
+        Qt5::DBus  # HostName job can use DBus to systemd
 )
diff --git a/src/modules/users/Config.h b/src/modules/users/Config.h
index 04f37d231..57da73e30 100644
--- a/src/modules/users/Config.h
+++ b/src/modules/users/Config.h
@@ -84,7 +84,7 @@ private:
 };
 
 
-class Config : public QObject
+class PLUGINDLLEXPORT Config : public QObject
 {
     Q_OBJECT
 
diff --git a/src/modules/usersq/CMakeLists.txt b/src/modules/usersq/CMakeLists.txt
index b6717e7b8..951359c77 100644
--- a/src/modules/usersq/CMakeLists.txt
+++ b/src/modules/usersq/CMakeLists.txt
@@ -13,9 +13,8 @@ find_package( Crypt REQUIRED )
 
 # Add optional libraries here
 set( USER_EXTRA_LIB )
-set( _users ${CMAKE_CURRENT_SOURCE_DIR}/../users )
 
-include_directories( ${PROJECT_BINARY_DIR}/src/libcalamaresui ${CMAKE_CURRENT_SOURCE_DIR}/../../libcalamares ${_users} )
+include_directories( ${CMAKE_CURRENT_SOURCE_DIR}/../users )
 
 find_package( LibPWQuality )
 set_package_properties(
@@ -33,12 +32,6 @@ calamares_add_plugin( usersq
     TYPE viewmodule
     EXPORT_MACRO PLUGINDLLEXPORT_PRO
     SOURCES
-        ${_users}/CheckPWQuality.cpp
-        ${_users}/Config.cpp
-        ${_users}/CreateUserJob.cpp
-        ${_users}/MiscJobs.cpp
-        ${_users}/SetHostNameJob.cpp
-        ${_users}/SetPasswordJob.cpp
         UsersQmlViewStep.cpp
     RESOURCES
         usersq.qrc
@@ -47,5 +40,6 @@ calamares_add_plugin( usersq
         ${CRYPT_LIBRARIES}
         ${USER_EXTRA_LIB}
         Qt5::DBus
+        users_internal
     SHARED_LIB
 )

From fa1d314b13fb1f605bf7d29755067bfcc12919eb Mon Sep 17 00:00:00 2001
From: Adriaan de Groot <groot@kde.org>
Date: Thu, 22 Oct 2020 13:49:45 +0200
Subject: [PATCH 20/30] [users] Refactor group-creation into separate Job

---
 src/modules/users/MiscJobs.cpp | 126 +++++++++++++++++++++++++++++++++
 src/modules/users/MiscJobs.h   |  16 +++++
 2 files changed, 142 insertions(+)

diff --git a/src/modules/users/MiscJobs.cpp b/src/modules/users/MiscJobs.cpp
index 4f9472cc6..04880d1d8 100644
--- a/src/modules/users/MiscJobs.cpp
+++ b/src/modules/users/MiscJobs.cpp
@@ -12,10 +12,16 @@
 
 #include "Config.h"
 
+#include "GlobalStorage.h"
+#include "JobQueue.h"
 #include "utils/CalamaresUtilsSystem.h"
 #include "utils/Logger.h"
 #include "utils/Permissions.h"
 
+#include <QDir>
+#include <QFile>
+#include <QFileInfo>
+
 SetupSudoJob::SetupSudoJob( const QString& group )
     : m_sudoGroup( group )
 {
@@ -55,3 +61,123 @@ SetupSudoJob::exec()
 
     return Calamares::JobResult::ok();
 }
+
+STATICTEST QStringList
+groupsInTargetSystem()
+{
+    Calamares::GlobalStorage* gs = Calamares::JobQueue::instance()->globalStorage();
+    QDir targetRoot( gs->value( "rootMountPoint" ).toString() );
+
+    QFileInfo groupsFi( targetRoot.absoluteFilePath( "etc/group" ) );
+    QFile groupsFile( groupsFi.absoluteFilePath() );
+    if ( !groupsFile.open( QIODevice::ReadOnly | QIODevice::Text ) )
+    {
+        return QStringList();
+    }
+    QString groupsData = QString::fromLocal8Bit( groupsFile.readAll() );
+    QStringList groupsLines = groupsData.split( '\n' );
+    QStringList::iterator it = groupsLines.begin();
+    while ( it != groupsLines.end() )
+    {
+        if ( it->startsWith( '#' ) )
+        {
+            it = groupsLines.erase( it );
+            continue;
+        }
+        int indexOfFirstToDrop = it->indexOf( ':' );
+        if ( indexOfFirstToDrop < 1 )
+        {
+            it = groupsLines.erase( it );
+            continue;
+        }
+        it->truncate( indexOfFirstToDrop );
+        ++it;
+    }
+    return groupsLines;
+}
+
+/** @brief Create groups in target system as needed
+ *
+ * Given a list of groups that already exist, @p availableGroups,
+ * go through the @p wantedGroups and create each of them. Groups that
+ * fail, or which should have already been there, are added to
+ * @p missingGroups by name.
+ */
+static bool
+ensureGroupsExistInTarget( const QList< GroupDescription >& wantedGroups,
+                           const QStringList& availableGroups,
+                           QStringList& missingGroups )
+{
+    int failureCount = 0;
+
+    for ( const auto& group : wantedGroups )
+    {
+        int groupaddResult = 0;
+        if ( group.isValid() && !availableGroups.contains( group.name() ) )
+        {
+            if ( group.mustAlreadyExist() )
+            {
+                // Should have been there already: don't create it
+                missingGroups.append( group.name() );
+                continue;
+            }
+
+#ifdef __FreeBSD__
+            groupaddResult
+                = CalamaresUtils::System::instance()->targetEnvCall( { "pw", "groupadd", "-n", group.name() } );
+#else
+            groupaddResult = CalamaresUtils::System::instance()->targetEnvCall( { "groupadd", group.name() } );
+#endif
+            if ( groupaddResult )
+            {
+                failureCount++;
+                missingGroups.append( group.name() + QChar( '*' ) );
+            }
+        }
+    }
+    if ( !missingGroups.isEmpty() )
+    {
+        cWarning() << "Missing groups in target system (* for groupadd failure):" << Logger::DebugList( missingGroups );
+    }
+    return failureCount == 0;
+}
+
+SetupGroupsJob::SetupGroupsJob( const Config* config, const QString& autologinGroup )
+    : m_autologinGroup( autologinGroup )
+    , m_config( config )
+{
+}
+
+QString
+SetupGroupsJob::prettyName() const
+{
+    return tr( "Preparing groups." );
+}
+
+Calamares::JobResult
+SetupGroupsJob::exec()
+{
+    const auto& defaultGroups = m_config->defaultGroups();
+    QStringList availableGroups = groupsInTargetSystem();
+    QStringList missingGroups;
+
+    if ( !ensureGroupsExistInTarget( defaultGroups, availableGroups, missingGroups ) )
+    {
+        return Calamares::JobResult::error( tr( "Could not create groups in target system" ) );
+    }
+    if ( !missingGroups.isEmpty() )
+    {
+        return Calamares::JobResult::error(
+            tr( "Could not create groups in target system" ),
+            tr( "These groups are missing in the target system: %1" ).arg( missingGroups.join( ',' ) ) );
+    }
+
+    if ( m_config->doAutoLogin() && !m_config->autologinGroup().isEmpty() )
+    {
+        const QString autologinGroup = m_config->autologinGroup();
+        (void)ensureGroupsExistInTarget(
+            QList< GroupDescription >() << GroupDescription( autologinGroup ), availableGroups, missingGroups );
+    }
+
+    return Calamares::JobResult::ok();
+}
diff --git a/src/modules/users/MiscJobs.h b/src/modules/users/MiscJobs.h
index 2f8055e8a..8065079ce 100644
--- a/src/modules/users/MiscJobs.h
+++ b/src/modules/users/MiscJobs.h
@@ -19,6 +19,8 @@
 
 #include "Job.h"
 
+class Config;
+
 class SetupSudoJob : public Calamares::Job
 {
     Q_OBJECT
@@ -31,4 +33,18 @@ public:
     QString m_sudoGroup;
 };
 
+class SetupGroupsJob : public Calamares::Job
+{
+    Q_OBJECT
+
+public:
+    SetupGroupsJob( const Config* config, const QString& autologinGroup );
+    QString prettyName() const override;
+    Calamares::JobResult exec() override;
+
+public:
+    QString m_autologinGroup;
+    const Config* m_config;
+};
+
 #endif

From 03541470d53c63504ea6609217c1f98ba6078b18 Mon Sep 17 00:00:00 2001
From: Adriaan de Groot <groot@kde.org>
Date: Thu, 22 Oct 2020 14:07:40 +0200
Subject: [PATCH 21/30] [users] Handle system-group creating specially

---
 src/modules/users/Config.cpp   | 18 ++++++++++++++++++
 src/modules/users/Config.h     |  6 ++++++
 src/modules/users/MiscJobs.cpp | 18 +++++++++++++-----
 3 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/src/modules/users/Config.cpp b/src/modules/users/Config.cpp
index dc50f42c5..fff6e3960 100644
--- a/src/modules/users/Config.cpp
+++ b/src/modules/users/Config.cpp
@@ -136,6 +136,24 @@ Config::setAutologinGroup( const QString& group )
     }
 }
 
+QStringList
+Config::groupsForThisUser() const
+{
+    QStringList l;
+    l.reserve( defaultGroups().size() + 1 );
+
+    for ( const auto& g : defaultGroups() )
+    {
+        l << g.name();
+    }
+    if ( doAutoLogin() && !autologinGroup().isEmpty() )
+    {
+        l << autologinGroup();
+    }
+
+    return l;
+}
+
 void
 Config::setSudoersGroup( const QString& group )
 {
diff --git a/src/modules/users/Config.h b/src/modules/users/Config.h
index 57da73e30..d4bfee4a4 100644
--- a/src/modules/users/Config.h
+++ b/src/modules/users/Config.h
@@ -68,6 +68,7 @@ public:
 
     bool isValid() const { return m_isValid; }
     bool isSystemGroup() const { return m_isSystem; }
+    bool mustAlreadyExist() const { return m_mustAlreadyExist; }
     QString name() const { return m_name; }
 
     ///@brief Equality of groups depends only on name and kind
@@ -213,6 +214,11 @@ public:
     bool requireStrongPasswords() const { return m_requireStrongPasswords; }
 
     const QList< GroupDescription >& defaultGroups() const { return m_defaultGroups; }
+    /** @brief the names of all the groups for the current user
+     *
+     * Takes into account defaultGroups and autologin behavior.
+     */
+    QStringList groupsForThisUser() const;
 
     // The user enters a password (and again in a separate UI element)
     QString userPassword() const { return m_userPassword; }
diff --git a/src/modules/users/MiscJobs.cpp b/src/modules/users/MiscJobs.cpp
index 04880d1d8..b6fbe55db 100644
--- a/src/modules/users/MiscJobs.cpp
+++ b/src/modules/users/MiscJobs.cpp
@@ -112,7 +112,6 @@ ensureGroupsExistInTarget( const QList< GroupDescription >& wantedGroups,
 
     for ( const auto& group : wantedGroups )
     {
-        int groupaddResult = 0;
         if ( group.isValid() && !availableGroups.contains( group.name() ) )
         {
             if ( group.mustAlreadyExist() )
@@ -122,13 +121,22 @@ ensureGroupsExistInTarget( const QList< GroupDescription >& wantedGroups,
                 continue;
             }
 
+            QStringList cmd;
 #ifdef __FreeBSD__
-            groupaddResult
-                = CalamaresUtils::System::instance()->targetEnvCall( { "pw", "groupadd", "-n", group.name() } );
+            if ( group.isSystemGroup() )
+            {
+                cWarning() << "Ignoring must-be-a-system group for" << group.name() << "on FreeBSD";
+            }
+            cmd = QStringList { "pw", "groupadd", "-n", group.name() };
 #else
-            groupaddResult = CalamaresUtils::System::instance()->targetEnvCall( { "groupadd", group.name() } );
+            cmd << QStringLiteral( "groupadd" );
+            if ( group.isSystemGroup() )
+            {
+                cmd << "--system";
+            }
+            cmd << group.name();
 #endif
-            if ( groupaddResult )
+            if ( CalamaresUtils::System::instance()->targetEnvCall( cmd ) )
             {
                 failureCount++;
                 missingGroups.append( group.name() + QChar( '*' ) );

From f1772a7eae565ab8dc474c8b0d58fa889d80b152 Mon Sep 17 00:00:00 2001
From: Adriaan de Groot <groot@kde.org>
Date: Thu, 22 Oct 2020 14:11:01 +0200
Subject: [PATCH 22/30] [users] Create groups in a separate job

---
 src/modules/users/Config.cpp        |  3 ++
 src/modules/users/CreateUserJob.cpp | 71 +----------------------------
 src/modules/users/MiscJobs.cpp      |  5 +-
 src/modules/users/MiscJobs.h        |  3 +-
 4 files changed, 7 insertions(+), 75 deletions(-)

diff --git a/src/modules/users/Config.cpp b/src/modules/users/Config.cpp
index fff6e3960..0e70cc215 100644
--- a/src/modules/users/Config.cpp
+++ b/src/modules/users/Config.cpp
@@ -821,6 +821,9 @@ Config::createJobs() const
         jobs.append( Calamares::job_ptr( j ) );
     }
 
+    j = new SetupGroupsJob( this );
+    jobs.append( Calamares::job_ptr( j ) );
+
     j = new CreateUserJob( this );
     jobs.append( Calamares::job_ptr( j ) );
 
diff --git a/src/modules/users/CreateUserJob.cpp b/src/modules/users/CreateUserJob.cpp
index efbc4d9e3..e08108a46 100644
--- a/src/modules/users/CreateUserJob.cpp
+++ b/src/modules/users/CreateUserJob.cpp
@@ -50,53 +50,6 @@ CreateUserJob::prettyStatusMessage() const
     return m_status.isEmpty() ? tr( "Creating user %1." ).arg( m_config->loginName() ) : m_status;
 }
 
-STATICTEST QStringList
-groupsInTargetSystem( const QDir& targetRoot )
-{
-    QFileInfo groupsFi( targetRoot.absoluteFilePath( "etc/group" ) );
-    QFile groupsFile( groupsFi.absoluteFilePath() );
-    if ( !groupsFile.open( QIODevice::ReadOnly | QIODevice::Text ) )
-    {
-        return QStringList();
-    }
-    QString groupsData = QString::fromLocal8Bit( groupsFile.readAll() );
-    QStringList groupsLines = groupsData.split( '\n' );
-    QStringList::iterator it = groupsLines.begin();
-    while ( it != groupsLines.end() )
-    {
-        if ( it->startsWith( '#' ) )
-        {
-            it = groupsLines.erase( it );
-            continue;
-        }
-        int indexOfFirstToDrop = it->indexOf( ':' );
-        if ( indexOfFirstToDrop < 1 )
-        {
-            it = groupsLines.erase( it );
-            continue;
-        }
-        it->truncate( indexOfFirstToDrop );
-        ++it;
-    }
-    return groupsLines;
-}
-
-static void
-ensureGroupsExistInTarget( const QList< GroupDescription >& wantedGroups, const QStringList& availableGroups )
-{
-    for ( const auto& group : wantedGroups )
-    {
-        if ( group.isValid() && !availableGroups.contains( group.name() ) )
-        {
-#ifdef __FreeBSD__
-            CalamaresUtils::System::instance()->targetEnvCall( { "pw", "groupadd", "-n", group.name() } );
-#else
-            CalamaresUtils::System::instance()->targetEnvCall( { "groupadd", group.name() } );
-#endif
-        }
-    }
-}
-
 static Calamares::JobResult
 createUser( const QString& loginName, const QString& fullName, const QString& shell )
 {
@@ -166,28 +119,6 @@ CreateUserJob::exec()
         reuseHome = gs->value( "reuseHome" ).toBool();
     }
 
-    cDebug() << "[CREATEUSER]: preparing groups";
-
-    m_status = tr( "Preparing groups for user %1" ).arg( m_config->loginName() );
-    emit progress( 0.1 );
-    // loginName(), fullName().isEmpty() ? loginName() : fullName(), doAutoLogin(), groupNames );
-    const auto& defaultGroups = m_config->defaultGroups();
-    QStringList groupsForThisUser = std::accumulate(
-        defaultGroups.begin(),
-        defaultGroups.end(),
-        QStringList(),
-        []( const QStringList& l, const GroupDescription& g ) { return QStringList( l ) << g.name(); } );
-
-    QStringList availableGroups = groupsInTargetSystem( destDir );
-    ensureGroupsExistInTarget( defaultGroups, availableGroups );
-
-    if ( m_config->doAutoLogin() && !m_config->autologinGroup().isEmpty() )
-    {
-        const QString autologinGroup = m_config->autologinGroup();
-        groupsForThisUser << autologinGroup;
-        ensureGroupsExistInTarget( QList< GroupDescription >() << GroupDescription( autologinGroup ), availableGroups );
-    }
-
     // If we're looking to reuse the contents of an existing /home.
     // This GS setting comes from the **partitioning** module.
     if ( reuseHome )
@@ -219,7 +150,7 @@ CreateUserJob::exec()
 
     m_status = tr( "Configuring user %1" ).arg( m_config->loginName() );
     emit progress( 0.8 );
-    auto usergroupsResult = setUserGroups( m_config->loginName(), groupsForThisUser );
+    auto usergroupsResult = setUserGroups( m_config->loginName(), m_config->groupsForThisUser() );
     if ( !usergroupsResult )
     {
         return usergroupsResult;
diff --git a/src/modules/users/MiscJobs.cpp b/src/modules/users/MiscJobs.cpp
index b6fbe55db..4f2570a0a 100644
--- a/src/modules/users/MiscJobs.cpp
+++ b/src/modules/users/MiscJobs.cpp
@@ -150,9 +150,8 @@ ensureGroupsExistInTarget( const QList< GroupDescription >& wantedGroups,
     return failureCount == 0;
 }
 
-SetupGroupsJob::SetupGroupsJob( const Config* config, const QString& autologinGroup )
-    : m_autologinGroup( autologinGroup )
-    , m_config( config )
+SetupGroupsJob::SetupGroupsJob( const Config* config )
+    : m_config( config )
 {
 }
 
diff --git a/src/modules/users/MiscJobs.h b/src/modules/users/MiscJobs.h
index 8065079ce..fe4ff87c0 100644
--- a/src/modules/users/MiscJobs.h
+++ b/src/modules/users/MiscJobs.h
@@ -38,12 +38,11 @@ class SetupGroupsJob : public Calamares::Job
     Q_OBJECT
 
 public:
-    SetupGroupsJob( const Config* config, const QString& autologinGroup );
+    SetupGroupsJob( const Config* config );
     QString prettyName() const override;
     Calamares::JobResult exec() override;
 
 public:
-    QString m_autologinGroup;
     const Config* m_config;
 };
 

From 5b4f9d0b98ff6e0e71dad47b9afc00921c8d6e80 Mon Sep 17 00:00:00 2001
From: Adriaan de Groot <groot@kde.org>
Date: Thu, 22 Oct 2020 14:21:14 +0200
Subject: [PATCH 23/30] [users] Adjust tests for changed API

---
 src/modules/users/CMakeLists.txt        |  6 +++---
 src/modules/users/TestCreateUserJob.cpp | 21 +++++++++------------
 2 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/src/modules/users/CMakeLists.txt b/src/modules/users/CMakeLists.txt
index ca5d24d34..8680de73b 100644
--- a/src/modules/users/CMakeLists.txt
+++ b/src/modules/users/CMakeLists.txt
@@ -74,10 +74,10 @@ calamares_add_test(
 )
 
 calamares_add_test(
-    userscreatetest
+    usersgroupstest
     SOURCES
-        TestCreateUserJob.cpp
-        CreateUserJob.cpp
+        TestCreateUserJob.cpp  # Misnomer
+        MiscJobs.cpp
 )
 
 calamares_add_test(
diff --git a/src/modules/users/TestCreateUserJob.cpp b/src/modules/users/TestCreateUserJob.cpp
index fc2d74dcd..11bdfaa01 100644
--- a/src/modules/users/TestCreateUserJob.cpp
+++ b/src/modules/users/TestCreateUserJob.cpp
@@ -15,14 +15,14 @@
 #include <QtTest/QtTest>
 
 // Implementation details
-extern QStringList groupsInTargetSystem( const QDir& targetRoot );  // CreateUserJob
+extern QStringList groupsInTargetSystem();  // CreateUserJob
 
-class CreateUserTests : public QObject
+class GroupTests : public QObject
 {
     Q_OBJECT
 public:
-    CreateUserTests();
-    ~CreateUserTests() override {}
+    GroupTests();
+    ~GroupTests() override {}
 
 private Q_SLOTS:
     void initTestCase();
@@ -30,23 +30,20 @@ private Q_SLOTS:
     void testReadGroup();
 };
 
-CreateUserTests::CreateUserTests() {}
+GroupTests::GroupTests() {}
 
 void
-CreateUserTests::initTestCase()
+GroupTests::initTestCase()
 {
     Logger::setupLogLevel( Logger::LOGDEBUG );
     cDebug() << "Users test started.";
 }
 
 void
-CreateUserTests::testReadGroup()
+GroupTests::testReadGroup()
 {
-    QDir root( "/" );
-    QVERIFY( root.exists() );
-
     // Get the groups in the host system
-    QStringList groups = groupsInTargetSystem( root );
+    QStringList groups = groupsInTargetSystem();
     QVERIFY( groups.count() > 2 );
 #ifdef __FreeBSD__
     QVERIFY( groups.contains( QStringLiteral( "wheel" ) ) );
@@ -65,7 +62,7 @@ CreateUserTests::testReadGroup()
     }
 }
 
-QTEST_GUILESS_MAIN( CreateUserTests )
+QTEST_GUILESS_MAIN( GroupTests )
 
 #include "utils/moc-warnings.h"
 

From 89d495d5fb914fe6e3810fd9f3a053c207aa0782 Mon Sep 17 00:00:00 2001
From: Adriaan de Groot <groot@kde.org>
Date: Thu, 22 Oct 2020 14:22:11 +0200
Subject: [PATCH 24/30] [users] Rename file to match its purpose

---
 src/modules/users/CMakeLists.txt                                | 2 +-
 .../users/{TestCreateUserJob.cpp => TestGroupInformation.cpp}   | 0
 2 files changed, 1 insertion(+), 1 deletion(-)
 rename src/modules/users/{TestCreateUserJob.cpp => TestGroupInformation.cpp} (100%)

diff --git a/src/modules/users/CMakeLists.txt b/src/modules/users/CMakeLists.txt
index 8680de73b..34a0f6856 100644
--- a/src/modules/users/CMakeLists.txt
+++ b/src/modules/users/CMakeLists.txt
@@ -76,7 +76,7 @@ calamares_add_test(
 calamares_add_test(
     usersgroupstest
     SOURCES
-        TestCreateUserJob.cpp  # Misnomer
+        TestGroupInformation.cpp
         MiscJobs.cpp
 )
 
diff --git a/src/modules/users/TestCreateUserJob.cpp b/src/modules/users/TestGroupInformation.cpp
similarity index 100%
rename from src/modules/users/TestCreateUserJob.cpp
rename to src/modules/users/TestGroupInformation.cpp

From cd8c3089cbfa1f2b4d3a1ef85189f0fc162596a1 Mon Sep 17 00:00:00 2001
From: Adriaan de Groot <groot@kde.org>
Date: Fri, 23 Oct 2020 11:14:59 +0200
Subject: [PATCH 25/30] [users] Fix build: renamed moc file

---
 src/modules/users/TestGroupInformation.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/modules/users/TestGroupInformation.cpp b/src/modules/users/TestGroupInformation.cpp
index 11bdfaa01..3a04409d5 100644
--- a/src/modules/users/TestGroupInformation.cpp
+++ b/src/modules/users/TestGroupInformation.cpp
@@ -66,4 +66,4 @@ QTEST_GUILESS_MAIN( GroupTests )
 
 #include "utils/moc-warnings.h"
 
-#include "TestCreateUserJob.moc"
+#include "TestGroupInformation.moc"

From 63196ab58fc09b71163ec5ecd0e0f5c82dd3d180 Mon Sep 17 00:00:00 2001
From: Adriaan de Groot <groot@kde.org>
Date: Fri, 23 Oct 2020 12:19:28 +0200
Subject: [PATCH 26/30] [users] Avoid crashes in tests due to nullptr GS

---
 src/modules/users/MiscJobs.cpp             | 4 ++++
 src/modules/users/TestGroupInformation.cpp | 5 +++++
 2 files changed, 9 insertions(+)

diff --git a/src/modules/users/MiscJobs.cpp b/src/modules/users/MiscJobs.cpp
index 4f2570a0a..a62ada6d7 100644
--- a/src/modules/users/MiscJobs.cpp
+++ b/src/modules/users/MiscJobs.cpp
@@ -66,6 +66,10 @@ STATICTEST QStringList
 groupsInTargetSystem()
 {
     Calamares::GlobalStorage* gs = Calamares::JobQueue::instance()->globalStorage();
+    if ( !gs )
+    {
+        return QStringList();
+    }
     QDir targetRoot( gs->value( "rootMountPoint" ).toString() );
 
     QFileInfo groupsFi( targetRoot.absoluteFilePath( "etc/group" ) );
diff --git a/src/modules/users/TestGroupInformation.cpp b/src/modules/users/TestGroupInformation.cpp
index 3a04409d5..6f650049a 100644
--- a/src/modules/users/TestGroupInformation.cpp
+++ b/src/modules/users/TestGroupInformation.cpp
@@ -9,6 +9,7 @@
 
 #include "CreateUserJob.h"
 
+#include "JobQueue.h"
 #include "utils/Logger.h"
 
 #include <QDir>
@@ -37,6 +38,10 @@ GroupTests::initTestCase()
 {
     Logger::setupLogLevel( Logger::LOGDEBUG );
     cDebug() << "Users test started.";
+    if ( !Calamares::JobQueue::instance() )
+    {
+        (void)new Calamares::JobQueue();
+    }
 }
 
 void

From 0d4d3e3c4d14ad86bd2c27a9dcb3f720660c7ab2 Mon Sep 17 00:00:00 2001
From: Adriaan de Groot <groot@kde.org>
Date: Fri, 23 Oct 2020 12:27:44 +0200
Subject: [PATCH 27/30] [users] Set up GS rootMountPoint for test

---
 src/modules/users/TestGroupInformation.cpp | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/modules/users/TestGroupInformation.cpp b/src/modules/users/TestGroupInformation.cpp
index 6f650049a..3b2046bda 100644
--- a/src/modules/users/TestGroupInformation.cpp
+++ b/src/modules/users/TestGroupInformation.cpp
@@ -9,6 +9,7 @@
 
 #include "CreateUserJob.h"
 
+#include "GlobalStorage.h"
 #include "JobQueue.h"
 #include "utils/Logger.h"
 
@@ -47,6 +48,7 @@ GroupTests::initTestCase()
 void
 GroupTests::testReadGroup()
 {
+    Calamares::JobQueue::instance()->globalStorage()->insert( "rootMountPoint", "/" );
     // Get the groups in the host system
     QStringList groups = groupsInTargetSystem();
     QVERIFY( groups.count() > 2 );

From e66f81f6ee132a707d0ab2dc26afb9cd41dbe7db Mon Sep 17 00:00:00 2001
From: Adriaan de Groot <groot@kde.org>
Date: Mon, 2 Nov 2020 12:01:02 +0100
Subject: [PATCH 28/30] CMake: comment on BUILD_AS_TEST

---
 CMakeModules/CalamaresAddTest.cmake | 2 ++
 src/modules/users/Tests.cpp         | 1 +
 2 files changed, 3 insertions(+)

diff --git a/CMakeModules/CalamaresAddTest.cmake b/CMakeModules/CalamaresAddTest.cmake
index 56a45d7dc..228d7cbc0 100644
--- a/CMakeModules/CalamaresAddTest.cmake
+++ b/CMakeModules/CalamaresAddTest.cmake
@@ -42,6 +42,8 @@ function( calamares_add_test )
                 Qt5::Test
             )
         calamares_automoc( ${TEST_NAME} )
+        # We specifically pass in the source directory of the test-being-
+        # compiled, so that it can find test-files in that source dir.
         target_compile_definitions( ${TEST_NAME} PRIVATE -DBUILD_AS_TEST="${CMAKE_CURRENT_SOURCE_DIR}"  ${TEST_DEFINITIONS} )
         if( TEST_GUI )
             target_link_libraries( ${TEST_NAME} calamaresui Qt5::Gui )
diff --git a/src/modules/users/Tests.cpp b/src/modules/users/Tests.cpp
index 5259fb8a8..b687a6434 100644
--- a/src/modules/users/Tests.cpp
+++ b/src/modules/users/Tests.cpp
@@ -196,6 +196,7 @@ UserTests::testDefaultGroupsYAML()
     QFETCH( int, count );
     QFETCH( QString, group );
 
+    // BUILD_AS_TEST is the source-directory path
     QFile fi( QString( "%1/%2" ).arg( BUILD_AS_TEST, filename ) );
     QVERIFY( fi.exists() );
 

From 8127ae704ca93a0fc2fb92106ea6bd456f993c17 Mon Sep 17 00:00:00 2001
From: Adriaan de Groot <groot@kde.org>
Date: Mon, 2 Nov 2020 12:11:13 +0100
Subject: [PATCH 29/30] [users] Expand tests for groups a little

---
 src/modules/users/CMakeLists.txt           |  6 ++++-
 src/modules/users/TestGroupInformation.cpp | 28 +++++++++++++++++++++-
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/src/modules/users/CMakeLists.txt b/src/modules/users/CMakeLists.txt
index f2cb011f4..8b8e87080 100644
--- a/src/modules/users/CMakeLists.txt
+++ b/src/modules/users/CMakeLists.txt
@@ -77,7 +77,11 @@ calamares_add_test(
     usersgroupstest
     SOURCES
         TestGroupInformation.cpp
-        MiscJobs.cpp
+        ${_users_src}  # Build again with test-visibility
+    LIBRARIES
+        Qt5::DBus  # HostName job can use DBus to systemd
+        ${CRYPT_LIBRARIES}  # SetPassword job uses crypt()
+        ${USER_EXTRA_LIB}
 )
 
 calamares_add_test(
diff --git a/src/modules/users/TestGroupInformation.cpp b/src/modules/users/TestGroupInformation.cpp
index 3b2046bda..5e8bcf9ab 100644
--- a/src/modules/users/TestGroupInformation.cpp
+++ b/src/modules/users/TestGroupInformation.cpp
@@ -7,11 +7,14 @@
  *
  */
 
+#include "Config.h"
 #include "CreateUserJob.h"
+#include "MiscJobs.h"
 
 #include "GlobalStorage.h"
 #include "JobQueue.h"
 #include "utils/Logger.h"
+#include "utils/Yaml.h"
 
 #include <QDir>
 #include <QtTest/QtTest>
@@ -30,6 +33,7 @@ private Q_SLOTS:
     void initTestCase();
 
     void testReadGroup();
+    void testCreateGroup();
 };
 
 GroupTests::GroupTests() {}
@@ -43,12 +47,12 @@ GroupTests::initTestCase()
     {
         (void)new Calamares::JobQueue();
     }
+    Calamares::JobQueue::instance()->globalStorage()->insert( "rootMountPoint", "/" );
 }
 
 void
 GroupTests::testReadGroup()
 {
-    Calamares::JobQueue::instance()->globalStorage()->insert( "rootMountPoint", "/" );
     // Get the groups in the host system
     QStringList groups = groupsInTargetSystem();
     QVERIFY( groups.count() > 2 );
@@ -69,6 +73,28 @@ GroupTests::testReadGroup()
     }
 }
 
+void GroupTests::testCreateGroup()
+{
+    Config g;
+
+    // BUILD_AS_TEST is the source-directory path
+    QFile fi( QString( "%1/tests/5-issue-1523.conf" ).arg( BUILD_AS_TEST ) );
+    QVERIFY( fi.exists() );
+
+    bool ok = false;
+    const auto map = CalamaresUtils::loadYaml( fi, &ok );
+    QVERIFY( ok );
+    QVERIFY( map.count() > 0 );  // Just that it loaded, one key *defaultGroups*
+
+    Config c;
+    c.setConfigurationMap( map );
+
+    QCOMPARE( c.defaultGroups().count(), 4 );
+    QVERIFY( c.defaultGroups().contains( QStringLiteral( "adm" ) ) );
+}
+
+
+
 QTEST_GUILESS_MAIN( GroupTests )
 
 #include "utils/moc-warnings.h"

From b61b5f8650bdb3f4b57d997baa20715e115ded28 Mon Sep 17 00:00:00 2001
From: Adriaan de Groot <groot@kde.org>
Date: Mon, 2 Nov 2020 12:27:50 +0100
Subject: [PATCH 30/30] [users] Run an expected-to-fail test in creating groups

---
 src/modules/users/TestGroupInformation.cpp | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/modules/users/TestGroupInformation.cpp b/src/modules/users/TestGroupInformation.cpp
index 5e8bcf9ab..dd4bfe78f 100644
--- a/src/modules/users/TestGroupInformation.cpp
+++ b/src/modules/users/TestGroupInformation.cpp
@@ -75,8 +75,6 @@ GroupTests::testReadGroup()
 
 void GroupTests::testCreateGroup()
 {
-    Config g;
-
     // BUILD_AS_TEST is the source-directory path
     QFile fi( QString( "%1/tests/5-issue-1523.conf" ).arg( BUILD_AS_TEST ) );
     QVERIFY( fi.exists() );
@@ -91,6 +89,12 @@ void GroupTests::testCreateGroup()
 
     QCOMPARE( c.defaultGroups().count(), 4 );
     QVERIFY( c.defaultGroups().contains( QStringLiteral( "adm" ) ) );
+    QVERIFY( c.defaultGroups().contains( QStringLiteral( "bar" ) ) );
+
+    Calamares::JobQueue::instance()->globalStorage()->insert( "rootMountPoint", "/" );
+
+    SetupGroupsJob j(&c);
+    QVERIFY( !j.exec() );  // running as regular user this should fail
 }