Merge branch 'issue-1523' into calamares

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 )

src/modules/users/CMakeLists.txt

@@ -23,24 +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
-)
+    # Configuration
-set( CONFIG_SRC
     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
@@ -49,7 +60,7 @@ calamares_add_plugin( users
         calamaresui
         ${CRYPT_LIBRARIES}
         ${USER_EXTRA_LIB}
-        Qt5::DBus
+        users_internal
     SHARED_LIB
 )
 
@@ -63,10 +74,14 @@ calamares_add_test(
 )
 
 calamares_add_test(
-    userscreatetest
+    usersgroupstest
     SOURCES
-        TestCreateUserJob.cpp
+        TestGroupInformation.cpp
-        CreateUserJob.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(
@@ -82,8 +97,7 @@ calamares_add_test(
     userstest
     SOURCES
         Tests.cpp
-        ${JOB_SRC}
+        ${_users_src}  # Build again with test-visibility
-        ${CONFIG_SRC}
     LIBRARIES
         Qt5::DBus  # HostName job can use DBus to systemd
         ${CRYPT_LIBRARIES}  # SetPassword job uses crypt()

src/modules/users/Config.cpp

@@ -10,6 +10,7 @@
 #include "Config.h"
 
 #include "CreateUserJob.h"
+#include "MiscJobs.h"
 #include "SetHostNameJob.h"
 #include "SetPasswordJob.h"
 
@@ -34,6 +35,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() )
     {
@@ -95,13 +101,18 @@ 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
+    if ( shell != m_userShell )
+    {
+        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 );
         }
     }
+}
 
 static inline void
 insertInGlobalStorage( const QString& key, const QString& group )
@@ -117,16 +128,42 @@ insertInGlobalStorage( const QString& key, const QString& group )
 void
 Config::setAutologinGroup( const QString& group )
 {
+    if ( group != m_autologinGroup )
+    {
+        m_autologinGroup = group;
         insertInGlobalStorage( QStringLiteral( "autologinGroup" ), group );
         emit autologinGroupChanged( 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 )
 {
+    if ( group != m_sudoersGroup )
+    {
+        m_sudoersGroup = group;
         insertInGlobalStorage( QStringLiteral( "sudoersGroup" ), group );
         emit sudoersGroupChanged( group );
     }
+}
 
 
 void
@@ -134,10 +171,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() );
     }
@@ -189,6 +225,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() )
         {
@@ -198,9 +236,6 @@ Config::setHostName( const QString& host )
         {
             gs->insert( "hostname", host );
         }
-
-        m_customHostName = !host.isEmpty();
-        m_hostName = host;
         emit hostNameChanged( host );
         emit hostNameStatusChanged( hostNameStatus() );
     }
@@ -369,8 +404,8 @@ Config::setAutoLogin( bool b )
 {
     if ( b != m_doAutoLogin )
     {
-        updateGSAutoLogin( b, loginName() );
         m_doAutoLogin = b;
+        updateGSAutoLogin( b, loginName() );
         emit autoLoginChanged( b );
     }
 }
@@ -590,16 +625,59 @@ Config::checkReady()
 
 
 STATICTEST void
-setConfigurationDefaultGroups( const QVariantMap& map, QStringList& defaultGroups )
+setConfigurationDefaultGroups( const QVariantMap& map, QList< GroupDescription >& defaultGroups )
 {
-    // '#' is not a valid group name; use that to distinguish an empty-list
+    defaultGroups.clear();
-    // in the configuration (which is a legitimate, if unusual, choice)
+
-    // from a bad or missing configuration value.
+    const QString key( "defaultGroups" );
-    defaultGroups = CalamaresUtils::getStringList( map, QStringLiteral( "defaultGroups" ), QStringList { "#" } );
+    auto groupsFromConfig = map.value( key ).toList();
-    if ( defaultGroups.contains( QStringLiteral( "#" ) ) )
+    if ( groupsFromConfig.isEmpty() )
     {
-        cWarning() << "Using fallback groups. Please check *defaultGroups* in users.conf";
+        if ( map.contains( key ) && map.value( key ).isValid() && map.value( key ).canConvert( QVariant::List ) )
-        defaultGroups = QStringList { "lp", "video", "network", "storage", "wheel", "audio" };
+        {
+            // Explicitly set, but empty: this is valid, but unusual.
+            cDebug() << key << "has explicit empty value.";
+        }
+        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 {} ) );
+            }
+        }
+    }
+    else
+    {
+        for ( const auto& v : groupsFromConfig )
+        {
+            if ( v.type() == QVariant::String )
+            {
+                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;
+            }
+        }
     }
 }
 
@@ -737,8 +815,16 @@ Config::createJobs() const
 
     Calamares::Job* j;
 
-    j = new CreateUserJob(
+    if ( m_sudoersGroup.isEmpty() )
-        loginName(), fullName().isEmpty() ? loginName() : fullName(), doAutoLogin(), defaultGroups() );
+    {
+        j = new SetupSudoJob( m_sudoersGroup );
+        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 ) );
 
     j = new SetPasswordJob( loginName(), userPassword() );

src/modules/users/Config.h

@@ -15,6 +15,7 @@
 #include "Job.h"
 #include "utils/NamedEnum.h"
 
+#include <QList>
 #include <QObject>
 #include <QVariantMap>
 
@@ -30,7 +31,61 @@ Q_DECLARE_OPERATORS_FOR_FLAGS( HostNameActions )
 
 const NamedEnumTable< HostNameAction >& hostNameActionNames();
 
-class Config : public QObject
+/** @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:
+    // 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 = CreateIfNeeded {}, bool isSystem = UserGroup {} )
+        : 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; }
+    bool mustAlreadyExist() const { return m_mustAlreadyExist; }
+    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 PLUGINDLLEXPORT Config : public QObject
 {
     Q_OBJECT
 
@@ -158,7 +213,12 @@ 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; }
+    /** @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; }
@@ -242,7 +302,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;

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,
+CreateUserJob::CreateUserJob( const Config* config )
-                              const QString& fullName,
-                              bool autologin,
-                              const QStringList& defaultGroups )
     : Calamares::Job()
-    , m_userName( userName )
+    , m_config( config )
-    , m_fullName( fullName )
-    , m_autologin( autologin )
-    , m_defaultGroups( defaultGroups )
 {
 }
 
@@ -37,68 +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 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 QStringList& wantedGroups, const QStringList& availableGroups )
-{
-    for ( const QString& group : wantedGroups )
-    {
-        if ( !availableGroups.contains( group ) )
-        {
-#ifdef __FreeBSD__
-            CalamaresUtils::System::instance()->targetEnvCall( { "pw", "groupadd", "-n", group } );
-#else
-            CalamaresUtils::System::instance()->targetEnvCall( { "groupadd", group } );
-#endif
-        }
-    }
 }
 
 static Calamares::JobResult
@@ -160,48 +109,23 @@ setUserGroups( const QString& loginName, const QStringList& groups )
 
 Calamares::JobResult
 CreateUserJob::exec()
+{
+    QDir destDir;
+    bool reuseHome = false;
+
     {
         Calamares::GlobalStorage* gs = Calamares::JobQueue::instance()->globalStorage();
-    QDir destDir( gs->value( "rootMountPoint" ).toString() );
+        destDir = QDir( gs->value( "rootMountPoint" ).toString() );
-
+        reuseHome = gs->value( "reuseHome" ).toBool();
-    if ( gs->contains( "sudoersGroup" ) && !gs->value( "sudoersGroup" ).toString().isEmpty() )
-    {
-        cDebug() << "[CREATEUSER]: preparing sudoers";
-
-        QString sudoersLine = QString( "%%1 ALL=(ALL) ALL\n" ).arg( gs->value( "sudoersGroup" ).toString() );
-        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";
-
-    QStringList availableGroups = groupsInTargetSystem( destDir );
-    QStringList groupsForThisUser = m_defaultGroups;
-    if ( m_autologin && gs->contains( "autologinGroup" ) && !gs->value( "autologinGroup" ).toString().isEmpty() )
-    {
-        groupsForThisUser << gs->value( "autologinGroup" ).toString();
-    }
-    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() )
+    if ( reuseHome )
     {
-        QString shellFriendlyHome = "/home/" + m_userName;
+        m_status = tr( "Preserving home directory" );
+        emit progress( 0.2 );
+        QString shellFriendlyHome = "/home/" + m_config->loginName();
         QDir existingHome( destDir.absolutePath() + shellFriendlyHome );
         if ( existingHome.exists() )
         {
@@ -216,20 +140,26 @@ CreateUserJob::exec()
 
     cDebug() << "[CREATEUSER]: creating user";
 
-    auto useraddResult = createUser( m_userName, m_fullName, gs->value( "userShell" ).toString() );
+    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;
     }
 
-    auto usergroupsResult = setUserGroups( m_userName, groupsForThisUser );
+    m_status = tr( "Configuring user %1" ).arg( m_config->loginName() );
+    emit progress( 0.8 );
+    auto usergroupsResult = setUserGroups( m_config->loginName(), m_config->groupsForThisUser() );
     if ( !usergroupsResult )
     {
         return usergroupsResult;
     }
 
-    QString userGroup = QString( "%1:%2" ).arg( m_userName ).arg( m_userName );
+    m_status = tr( "Setting file permissions" );
-    QString homeDir = QString( "/home/%1" ).arg( m_userName );
+    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 } );
     if ( commandResult.getExitCode() )
     {

src/modules/users/CreateUserJob.h

@@ -12,23 +12,21 @@
 
 #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;
+    const Config* m_config;
-    QString m_fullName;
+    QString m_status;
-    bool m_autologin;
-    QStringList m_defaultGroups;
 };
 
 #endif /* CREATEUSERJOB_H */

src/modules/users/MiscJobs.cpp

@@ -0,0 +1,194 @@
+/* === 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 "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 )
+{
+}
+
+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();
+}
+
+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" ) );
+    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 )
+    {
+        if ( group.isValid() && !availableGroups.contains( group.name() ) )
+        {
+            if ( group.mustAlreadyExist() )
+            {
+                // Should have been there already: don't create it
+                missingGroups.append( group.name() );
+                continue;
+            }
+
+            QStringList cmd;
+#ifdef __FreeBSD__
+            if ( group.isSystemGroup() )
+            {
+                cWarning() << "Ignoring must-be-a-system group for" << group.name() << "on FreeBSD";
+            }
+            cmd = QStringList { "pw", "groupadd", "-n", group.name() };
+#else
+            cmd << QStringLiteral( "groupadd" );
+            if ( group.isSystemGroup() )
+            {
+                cmd << "--system";
+            }
+            cmd << group.name();
+#endif
+            if ( CalamaresUtils::System::instance()->targetEnvCall( cmd ) )
+            {
+                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 )
+    : 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();
+}

src/modules/users/MiscJobs.h

@@ -0,0 +1,49 @@
+/* === 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 Config;
+
+class SetupSudoJob : public Calamares::Job
+{
+    Q_OBJECT
+public:
+    SetupSudoJob( const QString& group );
+    QString prettyName() const override;
+    Calamares::JobResult exec() override;
+
+public:
+    QString m_sudoGroup;
+};
+
+class SetupGroupsJob : public Calamares::Job
+{
+    Q_OBJECT
+
+public:
+    SetupGroupsJob( const Config* config );
+    QString prettyName() const override;
+    Calamares::JobResult exec() override;
+
+public:
+    const Config* m_config;
+};
+
+#endif

src/modules/users/TestCreateUserJob.cpp

@@ -1,72 +0,0 @@
-/* === This file is part of Calamares - <https://calamares.io> ===
- *
- *   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 "CreateUserJob.h"
-
-#include "utils/Logger.h"
-
-#include <QDir>
-#include <QtTest/QtTest>
-
-// Implementation details
-extern QStringList groupsInTargetSystem( const QDir& targetRoot );  // CreateUserJob
-
-class CreateUserTests : public QObject
-{
-    Q_OBJECT
-public:
-    CreateUserTests();
-    ~CreateUserTests() override {}
-
-private Q_SLOTS:
-    void initTestCase();
-
-    void testReadGroup();
-};
-
-CreateUserTests::CreateUserTests() {}
-
-void
-CreateUserTests::initTestCase()
-{
-    Logger::setupLogLevel( Logger::LOGDEBUG );
-    cDebug() << "Users test started.";
-}
-
-void
-CreateUserTests::testReadGroup()
-{
-    QDir root( "/" );
-    QVERIFY( root.exists() );
-
-    // Get the groups in the host system
-    QStringList groups = groupsInTargetSystem( root );
-    QVERIFY( groups.count() > 2 );
-#ifdef __FreeBSD__
-    QVERIFY( groups.contains( QStringLiteral( "wheel" ) ) );
-#else
-    QVERIFY( groups.contains( QStringLiteral( "root" ) ) );
-#endif
-    // openSUSE doesn't have "sys"
-    // QVERIFY( groups.contains( QStringLiteral( "sys" ) ) );
-    QVERIFY( groups.contains( QStringLiteral( "nogroup" ) ) );
-    QVERIFY( groups.contains( QStringLiteral( "tty" ) ) );
-
-    for ( const QString& s : groups )
-    {
-        QVERIFY( !s.isEmpty() );
-        QVERIFY( !s.contains( '#' ) );
-    }
-}
-
-QTEST_GUILESS_MAIN( CreateUserTests )
-
-#include "utils/moc-warnings.h"
-
-#include "TestCreateUserJob.moc"

src/modules/users/TestGroupInformation.cpp

@@ -0,0 +1,106 @@
+/* === This file is part of Calamares - <https://calamares.io> ===
+ *
+ *   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 "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>
+
+// Implementation details
+extern QStringList groupsInTargetSystem();  // CreateUserJob
+
+class GroupTests : public QObject
+{
+    Q_OBJECT
+public:
+    GroupTests();
+    ~GroupTests() override {}
+
+private Q_SLOTS:
+    void initTestCase();
+
+    void testReadGroup();
+    void testCreateGroup();
+};
+
+GroupTests::GroupTests() {}
+
+void
+GroupTests::initTestCase()
+{
+    Logger::setupLogLevel( Logger::LOGDEBUG );
+    cDebug() << "Users test started.";
+    if ( !Calamares::JobQueue::instance() )
+    {
+        (void)new Calamares::JobQueue();
+    }
+    Calamares::JobQueue::instance()->globalStorage()->insert( "rootMountPoint", "/" );
+}
+
+void
+GroupTests::testReadGroup()
+{
+    // Get the groups in the host system
+    QStringList groups = groupsInTargetSystem();
+    QVERIFY( groups.count() > 2 );
+#ifdef __FreeBSD__
+    QVERIFY( groups.contains( QStringLiteral( "wheel" ) ) );
+#else
+    QVERIFY( groups.contains( QStringLiteral( "root" ) ) );
+#endif
+    // openSUSE doesn't have "sys"
+    // QVERIFY( groups.contains( QStringLiteral( "sys" ) ) );
+    QVERIFY( groups.contains( QStringLiteral( "nogroup" ) ) );
+    QVERIFY( groups.contains( QStringLiteral( "tty" ) ) );
+
+    for ( const QString& s : groups )
+    {
+        QVERIFY( !s.isEmpty() );
+        QVERIFY( !s.contains( '#' ) );
+    }
+}
+
+void GroupTests::testCreateGroup()
+{
+    // 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" ) ) );
+    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
+}
+
+
+
+QTEST_GUILESS_MAIN( GroupTests )
+
+#include "utils/moc-warnings.h"
+
+#include "TestGroupInformation.moc"

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 );
 
@@ -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();
@@ -50,35 +53,97 @@ UserTests::initTestCase()
 {
     Logger::setupLogLevel( Logger::LOGDEBUG );
     cDebug() << "Users test started.";
+
+    if ( !Calamares::JobQueue::instance() )
+    {
+        (void)new Calamares::JobQueue();
     }
+}
+
+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 );  // .. so unchanged
+        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() );  // empty login name is ok
+        c.setLoginName( lg );
+        c.setFullName( ful );
+        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
+    }
+}
+
 
 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" ) );
+        QVERIFY( !groups.contains( GroupDescription( "hweel" ) ) );
-        QCOMPARE( groups, desired );
+        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 +152,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", false, GroupDescription::SystemGroup {} ) ) );
     }
 
     {
-        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", false, GroupDescription::SystemGroup {} ) ) );
     }
 }
 
@@ -116,6 +181,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
@@ -130,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() );
 

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

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,7 @@ calamares_add_plugin( usersq
     TYPE viewmodule
     EXPORT_MACRO PLUGINDLLEXPORT_PRO
     SOURCES
-		${_users}/Config.cpp
-		${_users}/CreateUserJob.cpp
-        ${_users}/SetPasswordJob.cpp
         UsersQmlViewStep.cpp
-        ${_users}/SetHostNameJob.cpp
-        ${_users}/CheckPWQuality.cpp
     RESOURCES
         usersq.qrc
     LINK_PRIVATE_LIBRARIES
@@ -46,5 +40,6 @@ calamares_add_plugin( usersq
         ${CRYPT_LIBRARIES}
         ${USER_EXTRA_LIB}
         Qt5::DBus
+        users_internal
     SHARED_LIB
 )