From c410e4250266a04e7897ecd0c98c49f040f67205 Mon Sep 17 00:00:00 2001 From: Arttu Tarkiainen Date: Wed, 24 Jul 2019 16:45:39 +0300 Subject: Refactor setting and checking of file permissions Create methods to explicitly set default file permissions on Unix platforms, use these on installer created files and installation target directory. Add unit test for introduced functions. Remove method introduced in 46aecc23b2983c807ff2232ae9cb9651b4d2fdc2 as the same effect is achieved more efficiently by ensuring target directory will be written with explicit permissions on initial installation. Unlike the removed method, this will also not break if an installed component contains owner non-writable directories. Further simplify PackageManagerCore::directoryWritable() introduced in 89f772f819178ee2502768c3d259d22ecb910fbe. Remove orphan unit test for removed PackageManagerCore::subdirectoriesWritable(). This does not change permissions of files and directories extracted for installed components. Task-number: QTIFW-1412 Change-Id: I59698c78aceef874b1f79482bff5a618b9a1b536 Reviewed-by: Katja Marttila --- src/libs/installer/createdesktopentryoperation.cpp | 3 +- .../installer/createlocalrepositoryoperation.cpp | 3 +- src/libs/installer/fileutils.cpp | 37 +++++++ src/libs/installer/fileutils.h | 10 ++ src/libs/installer/packagemanagercore.cpp | 9 +- src/libs/installer/packagemanagercore.h | 1 - src/libs/installer/packagemanagercore_p.cpp | 43 +++------ src/libs/installer/packagemanagercore_p.h | 1 - src/libs/installer/utils.cpp | 3 + src/libs/kdtools/localpackagehub.cpp | 6 ++ tests/auto/installer/fileutils/fileutils.pro | 6 ++ tests/auto/installer/fileutils/tst_fileutils.cpp | 107 +++++++++++++++++++++ tests/auto/installer/installer.pro | 1 + .../packagemanagercore/tst_packagemanagercore.cpp | 29 ------ 14 files changed, 187 insertions(+), 72 deletions(-) create mode 100644 tests/auto/installer/fileutils/fileutils.pro create mode 100644 tests/auto/installer/fileutils/tst_fileutils.cpp diff --git a/src/libs/installer/createdesktopentryoperation.cpp b/src/libs/installer/createdesktopentryoperation.cpp index 17e165777..e6d3b16e7 100644 --- a/src/libs/installer/createdesktopentryoperation.cpp +++ b/src/libs/installer/createdesktopentryoperation.cpp @@ -137,8 +137,7 @@ bool CreateDesktopEntryOperation::performOperation() return false; } - QFile::setPermissions(filename, QFile::ReadOwner | QFile::WriteOwner | QFile::ReadUser | QFile::ReadGroup - | QFile::ReadOther | QFile::ExeOwner | QFile::ExeGroup | QFile::ExeOther); + setDefaultFilePermissions(filename, DefaultFilePermissions::Executable); QTextStream stream(&file); stream.setCodec("UTF-8"); diff --git a/src/libs/installer/createlocalrepositoryoperation.cpp b/src/libs/installer/createlocalrepositoryoperation.cpp index 0eabc8c35..8f9aa1ddb 100644 --- a/src/libs/installer/createlocalrepositoryoperation.cpp +++ b/src/libs/installer/createlocalrepositoryoperation.cpp @@ -82,8 +82,7 @@ static void fixPermissions(const QString &repoPath) if (!it.fileInfo().isFile()) continue; - if (!QFile::setPermissions(it.filePath(), QFile::ReadOwner | QFile::WriteOwner - | QFile::ReadUser | QFile::WriteUser | QFile::ReadGroup | QFile::ReadOther)) { + if (!setDefaultFilePermissions(it.filePath(), DefaultFilePermissions::NonExecutable)) { throw Error(CreateLocalRepositoryOperation::tr("Cannot set permissions for file \"%1\".") .arg(QDir::toNativeSeparators(it.filePath()))); } diff --git a/src/libs/installer/fileutils.cpp b/src/libs/installer/fileutils.cpp index 18c5f90fd..b000247e6 100644 --- a/src/libs/installer/fileutils.cpp +++ b/src/libs/installer/fileutils.cpp @@ -293,6 +293,43 @@ void QInstaller::removeSystemGeneratedFiles(const QString &path) #endif } +/*! + Sets permissions of file or directory specified by \a fileName to \c 644 or \c 755 + based by the value of \a permissions. +*/ +bool QInstaller::setDefaultFilePermissions(const QString &fileName, DefaultFilePermissions permissions) +{ + QFile file(fileName); + return setDefaultFilePermissions(&file, permissions); +} + +/*! + Sets permissions of file or directory specified by \a file to \c 644 or \c 755 + based by the value of \a permissions. This is effective only on Unix platforms + as \c setPermissions() does not manipulate ACLs. On Windows this does nothing + and always returns \c true. +*/ +bool QInstaller::setDefaultFilePermissions(QFile *file, DefaultFilePermissions permissions) +{ +#ifdef Q_OS_UNIX + if (!file->exists()) { + qWarning() << "Target" << file->fileName() << "does not exists."; + return false; + } + if (file->permissions() == static_cast(permissions)) + return true; + + if (!file->setPermissions(static_cast(permissions))) { + qWarning() << "Cannot set default permissions for target" + << file->fileName() << ":" << file->errorString(); + return false; + } + return true; +#else + return true; +#endif +} + void QInstaller::copyDirectoryContents(const QString &sourceDir, const QString &targetDir) { Q_ASSERT(QFileInfo(sourceDir).isDir()); diff --git a/src/libs/installer/fileutils.h b/src/libs/installer/fileutils.h index ae08f5ff3..3607a189b 100644 --- a/src/libs/installer/fileutils.h +++ b/src/libs/installer/fileutils.h @@ -36,10 +36,17 @@ QT_BEGIN_NAMESPACE class QFileInfo; +class QFile; class QUrl; QT_END_NAMESPACE namespace QInstaller { + +enum DefaultFilePermissions { + NonExecutable = 0x6644, + Executable = 0x7755 +}; + class INSTALLER_EXPORT TempDirDeleter { public: @@ -80,6 +87,9 @@ private: void INSTALLER_EXPORT removeDirectoryThreaded(const QString &path, bool ignoreErrors = false); void INSTALLER_EXPORT removeSystemGeneratedFiles(const QString &path); + bool INSTALLER_EXPORT setDefaultFilePermissions(const QString &fileName, DefaultFilePermissions permissions); + bool INSTALLER_EXPORT setDefaultFilePermissions(QFile *file, DefaultFilePermissions permissions); + QString INSTALLER_EXPORT generateTemporaryFileName(const QString &templ=QString()); void INSTALLER_EXPORT moveDirectoryContents(const QString &sourceDir, const QString &targetDir); diff --git a/src/libs/installer/packagemanagercore.cpp b/src/libs/installer/packagemanagercore.cpp index c154edf76..d3d3b5be7 100644 --- a/src/libs/installer/packagemanagercore.cpp +++ b/src/libs/installer/packagemanagercore.cpp @@ -1585,16 +1585,15 @@ Component *PackageManagerCore::componentByName(const QString &name, const QList< return nullptr; } +/*! + Returns \c true if directory specified by \a path is writable by + the current user. +*/ bool PackageManagerCore::directoryWritable(const QString &path) const { return d->directoryWritable(path); } -bool PackageManagerCore::subdirectoriesWritable(const QString &path) const -{ - return d->subdirectoriesWritable(path); -} - /*! Returns a list of components that are marked for installation. The list can be empty. diff --git a/src/libs/installer/packagemanagercore.h b/src/libs/installer/packagemanagercore.h index f4240fde0..fd74a6cf7 100644 --- a/src/libs/installer/packagemanagercore.h +++ b/src/libs/installer/packagemanagercore.h @@ -122,7 +122,6 @@ public: static Component *componentByName(const QString &name, const QList &components); bool directoryWritable(const QString &path) const; - bool subdirectoriesWritable(const QString &path) const; bool fetchLocalPackagesTree(); LocalPackagesHash localInstalledPackages(); diff --git a/src/libs/installer/packagemanagercore_p.cpp b/src/libs/installer/packagemanagercore_p.cpp index 308bfa097..7547a126f 100644 --- a/src/libs/installer/packagemanagercore_p.cpp +++ b/src/libs/installer/packagemanagercore_p.cpp @@ -347,23 +347,8 @@ QString PackageManagerCorePrivate::targetDir() const bool PackageManagerCorePrivate::directoryWritable(const QString &path) const { - QTemporaryFile tempFile(path + QStringLiteral("/tempFile") + QString::number(qrand() % 1000)); - if (!tempFile.open() || !tempFile.isWritable()) - return false; - else - return true; -} - -bool PackageManagerCorePrivate::subdirectoriesWritable(const QString &path) const -{ - // Iterate over target directory subdirectories for writing access - QDirIterator iterator(path, QDir::AllDirs | QDir::NoDotAndDotDot, QDirIterator::Subdirectories); - while (iterator.hasNext()) { - QTemporaryFile tempFile(iterator.next() + QLatin1String("/tempFile")); - if (!tempFile.open() || !tempFile.isWritable()) - return false; - } - return true; + QTemporaryFile tempFile(path + QLatin1String("/tempFile.XXXXXX")); + return (tempFile.open() && tempFile.isWritable()); } QString PackageManagerCorePrivate::configurationFileName() const @@ -805,6 +790,7 @@ void PackageManagerCorePrivate::writeMaintenanceConfigFiles() : tr("Format error"); throw Error(tr("Cannot write installer configuration to %1: %2").arg(iniPath, reason)); } + setDefaultFilePermissions(iniPath, DefaultFilePermissions::NonExecutable); QFile file(targetDir() + QLatin1Char('/') + QLatin1String("network.xml")); if (file.open(QIODevice::WriteOnly | QIODevice::Truncate)) { @@ -842,6 +828,7 @@ void PackageManagerCorePrivate::writeMaintenanceConfigFiles() writer.writeEndElement(); writer.writeEndElement(); } + setDefaultFilePermissions(&file, DefaultFilePermissions::NonExecutable); } void PackageManagerCorePrivate::readMaintenanceConfigFiles(const QString &targetDir) @@ -1073,8 +1060,7 @@ void PackageManagerCorePrivate::writeMaintenanceToolBinary(QFile *const input, q throw Error(tr("Cannot write maintenance tool data to %1: %2").arg(dataOut.fileName(), dataOut.errorString())); } - dataOut.setPermissions(dataOut.permissions() | QFile::WriteUser | QFile::ReadGroup - | QFile::ReadOther); + setDefaultFilePermissions(&dataOut, DefaultFilePermissions::NonExecutable); } { @@ -1091,12 +1077,10 @@ void PackageManagerCorePrivate::writeMaintenanceToolBinary(QFile *const input, q } QFile mt(maintenanceToolRenamedName); - if (mt.setPermissions(out.permissions() | QFile::WriteUser | QFile::ReadGroup | QFile::ReadOther - | QFile::ExeOther | QFile::ExeGroup | QFile::ExeUser)) { + if (setDefaultFilePermissions(&mt, DefaultFilePermissions::Executable)) qDebug() << "Wrote permissions for maintenance tool."; - } else { + else qDebug() << "Failed to write permissions for maintenance tool."; - } if (out.exists() && !out.remove()) { qWarning() << tr("Cannot remove temporary data file \"%1\": %2") @@ -1386,8 +1370,7 @@ void PackageManagerCorePrivate::writeMaintenanceTool(OperationList performedOper throw Error(tr("Cannot write maintenance tool binary data to %1: %2") .arg(file.fileName(), file.errorString())); } - file.setPermissions(file.permissions() | QFile::WriteUser | QFile::ReadGroup - | QFile::ReadOther); + setDefaultFilePermissions(&file, DefaultFilePermissions::NonExecutable); } catch (const Error &/*error*/) { if (!newBinaryWritten) { newBinaryWritten = true; @@ -1494,6 +1477,8 @@ bool PackageManagerCorePrivate::runInstaller() if (!performOperationThreaded(mkdirOp)) throw Error(mkdirOp->errorString()); } + setDefaultFilePermissions(target, DefaultFilePermissions::Executable); + const QString remove = m_core->value(scRemoveTargetDir); if (QVariant(remove).toBool()) addPerformed(takeOwnedOperation(mkdirOp)); @@ -1643,16 +1628,10 @@ bool PackageManagerCorePrivate::runPackageUpdater() //to have some progress for the cleanup/write component.xml step ProgressCoordinator::instance()->addReservePercentagePoints(1); -#if defined(Q_OS_LINUX) || defined(Q_OS_MACOS) - // check if we need admin rights and ask before the action happens - // on Linux and macOS also check target directory subdirectories - if (!directoryWritable(targetDir()) || !subdirectoriesWritable(targetDir())) - adminRightsGained = m_core->gainAdminRights(); -#else // check if we need admin rights and ask before the action happens if (!directoryWritable(targetDir())) adminRightsGained = m_core->gainAdminRights(); -#endif + const QList componentsToInstall = m_core->orderedComponentsToInstall(); qDebug() << "Install size:" << componentsToInstall.size() << "components"; diff --git a/src/libs/installer/packagemanagercore_p.h b/src/libs/installer/packagemanagercore_p.h index 3e8f831a3..42b1879c7 100644 --- a/src/libs/installer/packagemanagercore_p.h +++ b/src/libs/installer/packagemanagercore_p.h @@ -93,7 +93,6 @@ public: QString registerPath(); bool directoryWritable(const QString &path) const; - bool subdirectoriesWritable(const QString &path) const; QString maintenanceToolName() const; QString installerBinaryPath() const; diff --git a/src/libs/installer/utils.cpp b/src/libs/installer/utils.cpp index e4e3212ac..dae5f8d34 100644 --- a/src/libs/installer/utils.cpp +++ b/src/libs/installer/utils.cpp @@ -28,6 +28,8 @@ #include "utils.h" +#include "fileutils.h" + #include #include #include @@ -277,6 +279,7 @@ bool QInstaller::PlainVerboseWriterOutput::write(const QString &fileName, QIODev QFile output(fileName); if (output.open(openMode)) { output.write(data); + setDefaultFilePermissions(&output, DefaultFilePermissions::NonExecutable); return true; } return false; diff --git a/src/libs/kdtools/localpackagehub.cpp b/src/libs/kdtools/localpackagehub.cpp index 11ff36a88..14e6b5738 100644 --- a/src/libs/kdtools/localpackagehub.cpp +++ b/src/libs/kdtools/localpackagehub.cpp @@ -28,6 +28,7 @@ ****************************************************************************/ #include "localpackagehub.h" +#include "fileutils.h" #include "globals.h" #include "constants.h" @@ -431,6 +432,11 @@ void LocalPackageHub::writeToDisk() file.write(doc.toByteArray(4)); file.close(); + + // Write permissions for installation information file + QInstaller::setDefaultFilePermissions( + &file, DefaultFilePermissions::NonExecutable); + d->modified = false; } } diff --git a/tests/auto/installer/fileutils/fileutils.pro b/tests/auto/installer/fileutils/fileutils.pro new file mode 100644 index 000000000..eee7ec537 --- /dev/null +++ b/tests/auto/installer/fileutils/fileutils.pro @@ -0,0 +1,6 @@ +include(../../qttest.pri) + +QT -= gui +QT += testlib + +SOURCES += tst_fileutils.cpp diff --git a/tests/auto/installer/fileutils/tst_fileutils.cpp b/tests/auto/installer/fileutils/tst_fileutils.cpp new file mode 100644 index 000000000..feb96d30c --- /dev/null +++ b/tests/auto/installer/fileutils/tst_fileutils.cpp @@ -0,0 +1,107 @@ +/************************************************************************** +** +** Copyright (C) 2019 The Qt Company Ltd. +** Contact: https://www.qt.io/licensing/ +** +** This file is part of the Qt Installer Framework. +** +** $QT_BEGIN_LICENSE:GPL-EXCEPT$ +** Commercial License Usage +** Licensees holding valid commercial Qt licenses may use this file in +** accordance with the commercial license agreement provided with the +** Software or, alternatively, in accordance with the terms contained in +** a written agreement between you and The Qt Company. For licensing terms +** and conditions see https://www.qt.io/terms-conditions. For further +** information use the contact form at https://www.qt.io/contact-us. +** +** GNU General Public License Usage +** Alternatively, this file may be used under the terms of the GNU +** General Public License version 3 as published by the Free Software +** Foundation with exceptions as appearing in the file LICENSE.GPL3-EXCEPT +** included in the packaging of this file. Please review the following +** information to ensure the GNU General Public License requirements will +** be met: https://www.gnu.org/licenses/gpl-3.0.html. +** +** $QT_END_LICENSE$ +** +**************************************************************************/ + +#include +#include + +#include +#include +#include +#include + +using namespace QInstaller; + +class tst_fileutils : public QObject +{ + Q_OBJECT + +private slots: + void testSetDefaultFilePermissions() + { +#if defined(Q_OS_WIN) + QVERIFY(setDefaultFilePermissions(QInstaller::generateTemporaryFileName(), + DefaultFilePermissions::NonExecutable)); + + QVERIFY(setDefaultFilePermissions(QInstaller::generateTemporaryFileName(), + DefaultFilePermissions::Executable)); +#elif defined(Q_OS_UNIX) + // Need to include the "user" flags here as they will be returned + // by QFile::permissions(). Same as owner permissions of the file. + QFlags permissions(QFileDevice::ReadOwner + | QFileDevice::WriteOwner | QFileDevice::ReadUser | QFileDevice::WriteUser + | QFileDevice::ReadGroup | QFileDevice::ReadOther); + + QFlags exePermissions(permissions | QFileDevice::ExeOwner + | QFileDevice::ExeUser | QFileDevice::ExeGroup | QFileDevice::ExeOther); + + QString fileName = QInstaller::generateTemporaryFileName(); + QFile testFile(fileName); + + const QString message = "Target \"%1\" does not exists."; + + // Test non-existing file + QTest::ignoreMessage(QtWarningMsg, qPrintable(message.arg(fileName))); + QVERIFY(!setDefaultFilePermissions(fileName, DefaultFilePermissions::NonExecutable)); + + QTest::ignoreMessage(QtWarningMsg, qPrintable(message.arg(testFile.fileName()))); + QVERIFY(!setDefaultFilePermissions(&testFile, DefaultFilePermissions::NonExecutable)); + + QVERIFY(testFile.open(QIODevice::ReadWrite)); + QVERIFY(testFile.exists()); + testFile.close(); + + // Test with file name + QVERIFY(setDefaultFilePermissions(fileName, DefaultFilePermissions::NonExecutable)); + QCOMPARE(QFile().permissions(fileName), permissions); + + QVERIFY(setDefaultFilePermissions(fileName, DefaultFilePermissions::Executable)); + QCOMPARE(QFile().permissions(fileName), exePermissions); + + // Test with QFile object + QVERIFY(setDefaultFilePermissions(&testFile, DefaultFilePermissions::NonExecutable)); + QCOMPARE(QFile().permissions(fileName), permissions); + + QVERIFY(setDefaultFilePermissions(&testFile, DefaultFilePermissions::Executable)); + QCOMPARE(QFile().permissions(fileName), exePermissions); + + // Test with directory path + QString testDir = QDir().tempPath() + QLatin1String("/testDir"); + QVERIFY(QDir().mkdir(testDir)); + + QVERIFY(setDefaultFilePermissions(testDir, DefaultFilePermissions::Executable)); + QCOMPARE(QFile().permissions(testDir), exePermissions); + + QVERIFY(QDir().rmdir(testDir)); + QVERIFY(testFile.remove()); +#endif + } +}; + +QTEST_MAIN(tst_fileutils) + +#include "tst_fileutils.moc" diff --git a/tests/auto/installer/installer.pro b/tests/auto/installer/installer.pro index db94a23cb..6db98d9e9 100644 --- a/tests/auto/installer/installer.pro +++ b/tests/auto/installer/installer.pro @@ -10,6 +10,7 @@ SUBDIRS += \ messageboxhandler \ extractarchiveoperationtest \ lib7zfacade \ + fileutils \ unicodeexecutable \ scriptengine \ consumeoutputoperationtest \ diff --git a/tests/auto/installer/packagemanagercore/tst_packagemanagercore.cpp b/tests/auto/installer/packagemanagercore/tst_packagemanagercore.cpp index 67fa7e2c5..cf5a53b8b 100644 --- a/tests/auto/installer/packagemanagercore/tst_packagemanagercore.cpp +++ b/tests/auto/installer/packagemanagercore/tst_packagemanagercore.cpp @@ -284,35 +284,6 @@ private slots: #endif QVERIFY(QDir().rmdir(testDirectory)); } - - void testSubdirectoriesWritable() - { - PackageManagerCore core; - - const QString testDirectory = QInstaller::generateTemporaryFileName(); - QVERIFY(QDir().mkpath(testDirectory)); - QVERIFY(QDir(testDirectory).exists()); - - const QString testSubdirectory = testDirectory + "/" + QString::number(qrand() % 1000); - - QVERIFY(QDir().mkpath(testSubdirectory)); - QVERIFY(QDir(testSubdirectory).exists()); - - // should be writable - QVERIFY(core.subdirectoriesWritable(testDirectory)); - -#if defined(Q_OS_LINUX) || defined(Q_OS_MACOS) - QFile dirDevice(testSubdirectory); - dirDevice.setPermissions(QFileDevice::ReadOwner | QFileDevice::ExeOwner); - - // should not be writable - QVERIFY(!core.subdirectoriesWritable(testDirectory)); - - dirDevice.setPermissions(QFileDevice::ReadOwner | QFileDevice::WriteOwner | QFileDevice::ExeOwner); -#endif - QVERIFY(QDir().rmdir(testSubdirectory)); - QVERIFY(QDir().rmdir(testDirectory)); - } }; -- cgit v1.2.3