From f34f58702f6b2da9f40aab9e187414e0bdc43947 Mon Sep 17 00:00:00 2001 From: Danil Shein Date: Thu, 11 Feb 2021 12:40:47 +0300 Subject: [PATCH 1/4] Add shared folder permissions helper Performs shared folder permissions checks whether guest access is enabled or access have been granted for user other than owner. If shared folder permissions are insufficient for given configuration, details is displayed and user is asked for confirmation of automatic permissions changing. --- .../sambausershareplugin.cpp | 172 ++++++++++++++++++ 1 file changed, 172 insertions(+) diff --git a/samba/filepropertiesplugin/sambausershareplugin.cpp b/samba/filepropertiesplugin/sambausershareplugin.cpp index f7b7140..b13fc5c 100644 --- a/samba/filepropertiesplugin/sambausershareplugin.cpp +++ b/samba/filepropertiesplugin/sambausershareplugin.cpp @@ -41,6 +41,133 @@ K_PLUGIN_CLASS_WITH_JSON(SambaUserSharePlugin, "sambausershareplugin.json") +class SharePermissionsHelper +{ +public: + explicit SharePermissionsHelper(const QString &path, const QString &acl, const QString &user): + m_acl(acl), + m_user(user) + { + sharePermsResolve(path); + } + + QString sharePermsCheck() const + { + QStringList filePermsCheckResult; + + for (auto it = m_sharePerms.constBegin(); it != m_sharePerms.constEnd(); ++it) { + QString fname = it.key(); + QFile::Permissions newPerm = it.value(); + QFile::Permissions oldPerm = QFileInfo(fname).permissions(); + + filePermsCheckResult += QStringLiteral("File:\t%1\nMode:\t%2 -> %3\n") + .arg(fname) + .arg(prettyPrintFilePerms(oldPerm)) + .arg(prettyPrintFilePerms(newPerm)); + } + + return filePermsCheckResult.join(QLatin1Char('\n')); + } + + QString sharePermsChange() const + { + QStringList filePermsChangeResult; + + for (auto it = m_sharePerms.constBegin(); it != m_sharePerms.constEnd(); ++it) { + QString fname = it.key(); + QFile::Permissions perms = it.value(); + + if (!QFile::setPermissions(fname, perms)) { + // failed to change permissions + filePermsChangeResult += fname; + } + } + + return filePermsChangeResult.join(QLatin1Char('\n')); + } + +private: + void sharePermsResolve(const QString &path) + { + QFile::Permissions permsForShare; + QFile::Permissions oldPerm, newPerm; + QStringList filesWOProperPermissions; + + // parse acl + QStringList lstAcl(m_acl.split(QStringLiteral(","))); + for (const auto &it : lstAcl) { + const auto aclVal = it.split(QStringLiteral(":")); + if (aclVal.at(0).compare(m_user, Qt::CaseSensitive) != 0) { + if (aclVal.at(1).endsWith(QStringLiteral("r"))) { + permsForShare |= (QFile::ExeOther | QFile::ReadOther); + } else if (aclVal.at(1).endsWith(QStringLiteral("f"))) { + permsForShare |= (QFile::ExeOther | QFile::ReadOther | QFile::WriteOther); + } + } + } + // store share path if permissions are insufficient + if (!QFileInfo(path).permission(permsForShare)) { + filesWOProperPermissions.append(path); + } + // check if share path could be resolved for others (has 'o+x' all the way through) + if (permsForShare) { + QStringList lstPath = path.split(QStringLiteral("/"), QString::SkipEmptyParts); + QString curPath; + for (const auto &it : lstPath) { + curPath.append(QStringLiteral("/%1").arg(it)); + const QFileInfo info(curPath); + if (!info.permission(QFile::ExeOther)) { + filesWOProperPermissions.append(curPath); + } + } + } + + if (!filesWOProperPermissions.isEmpty()) { + // we have to change permissions for share and share's path elements + for (const auto it : filesWOProperPermissions) { + oldPerm = QFileInfo(it).permissions(); + if (QString::compare(it, path, Qt::CaseSensitive) == 0) { + newPerm = oldPerm | permsForShare; + } else { + newPerm = oldPerm | QFile::ExeOther; + } + // store files and permissions to change later on + m_sharePerms.insert(it, newPerm); + } + } + } + + QString prettyPrintFilePerms(QFile::Permissions perm) const + { + QString res = QLatin1String((perm & QFileDevice::ReadOwner) ? "r" : "-") + + QLatin1String((perm & QFileDevice::WriteOwner) ? "w" : "-") + + QLatin1String((perm & QFileDevice::ExeOwner) ? "x" : "-") + + QLatin1String((perm & QFileDevice::ReadGroup) ? "r" : "-") + + QLatin1String((perm & QFileDevice::WriteGroup) ? "w" : "-") + + QLatin1String((perm & QFileDevice::ExeGroup) ? "x" : "-") + + QLatin1String((perm & QFileDevice::ReadOther) ? "r" : "-") + + QLatin1String((perm & QFileDevice::WriteOther) ? "w" : "-") + + QLatin1String((perm & QFileDevice::ExeOther) ? "x" : "-"); + + int p = ((perm & QFileDevice::ReadOwner) ? 0400 : 0) + + ((perm & QFileDevice::WriteOwner) ? 0200 : 0) + + ((perm & QFileDevice::ExeOwner) ? 0100 : 0) + + ((perm & QFileDevice::ReadGroup) ? 0040 : 0) + + ((perm & QFileDevice::WriteGroup) ? 0020 : 0) + + ((perm & QFileDevice::ExeGroup) ? 0010 : 0) + + ((perm & QFileDevice::ReadOther) ? 0004 : 0) + + ((perm & QFileDevice::WriteOther) ? 0002 : 0) + + ((perm & QFileDevice::ExeOther) ? 0001 : 0); + + return res + QStringLiteral(" (0%1)").arg(QString::number(p, 8)); + } + +private: + QString m_acl; + QString m_user; + QMap m_sharePerms; +}; + class ShareContext : public QObject { Q_OBJECT @@ -254,6 +381,51 @@ void SambaUserSharePlugin::applyChanges() return; } + // Check and change permissions for shared folder + SharePermissionsHelper helper(m_context->m_shareData.path(), + m_model->getAcl(), + m_userManager->currentUser()->name()); + + QString res = helper.sharePermsCheck(); + + if (!res.isEmpty()) { + QString warningMessage = xi18nc("@info detailed warning message", + "The folder \"%1\"" + "needs the extra permissions for sharing to work." + "Do you want Dolphin to add these permissions to " + "the folder automatically?" + "Required permission changes are listed in details " + "below:", + m_context->m_shareData.path()); + + KGuiItem contButton(i18n("Change permissions"), QString(), QString(), QString()); + KGuiItem cancelButton(i18n("Cancel"), QString(), QString(), QString()); + + KMessageBox::ButtonCode ret = KMessageBox::warningContinueCancelDetailed( + qobject_cast(parent()), + warningMessage, + i18nc("@info warning message caption", + "File permissions changes required"), + contButton, + cancelButton, + QString(), + KMessageBox::Notify, + res); + + if (ret == KMessageBox::Continue) { + res = helper.sharePermsChange(); + + if (!res.isEmpty()) { + QString errorMessage = i18nc("@info detailed error message", + "Could not change the permissions for:\n\n%1", + res); + KMessageBox::sorry(qobject_cast(parent()), + errorMessage, + i18nc("@info/title", "Failed to change permissions")); + } + } + } + // TODO: should run this through reportAdd() as well, ACLs may be invalid and then we shouldn't try to save m_context->m_shareData.setAcl(m_model->getAcl()); reportAdd(m_context->m_shareData.save()); -- GitLab From 04d19f005acf1c0a35c279b4b0ae5b8be990c4d4 Mon Sep 17 00:00:00 2001 From: Danil Shein Date: Thu, 11 Mar 2021 09:41:36 +0300 Subject: [PATCH 2/4] Handle permissions for groups and check POSIX ACL + Added permissions check and change for users with same prinary group. + Added extended POSIX ACL check for shared folder and it's path. + Added user notification if extended POSIX ACLs are involved. --- .../sambausershareplugin.cpp | 209 +++++++++++++++--- 1 file changed, 181 insertions(+), 28 deletions(-) diff --git a/samba/filepropertiesplugin/sambausershareplugin.cpp b/samba/filepropertiesplugin/sambausershareplugin.cpp index b13fc5c..bf7b8a1 100644 --- a/samba/filepropertiesplugin/sambausershareplugin.cpp +++ b/samba/filepropertiesplugin/sambausershareplugin.cpp @@ -30,6 +30,8 @@ #include #include #include +#include +#include #include "model.h" #include "usermanager.h" @@ -48,6 +50,7 @@ public: m_acl(acl), m_user(user) { + m_group = getUserPrimaryGroup(m_user); sharePermsResolve(path); } @@ -57,13 +60,11 @@ public: for (auto it = m_sharePerms.constBegin(); it != m_sharePerms.constEnd(); ++it) { QString fname = it.key(); - QFile::Permissions newPerm = it.value(); - QFile::Permissions oldPerm = QFileInfo(fname).permissions(); filePermsCheckResult += QStringLiteral("File:\t%1\nMode:\t%2 -> %3\n") .arg(fname) - .arg(prettyPrintFilePerms(oldPerm)) - .arg(prettyPrintFilePerms(newPerm)); + .arg(prettyPrintFilePerms(it.value().oldPerm)) + .arg(prettyPrintFilePerms(it.value().newPerm)); } return filePermsCheckResult.join(QLatin1Char('\n')); @@ -72,36 +73,90 @@ public: QString sharePermsChange() const { QStringList filePermsChangeResult; + bool rollback = false; for (auto it = m_sharePerms.constBegin(); it != m_sharePerms.constEnd(); ++it) { QString fname = it.key(); - QFile::Permissions perms = it.value(); + QFile::Permissions perms = it.value().newPerm; if (!QFile::setPermissions(fname, perms)) { // failed to change permissions filePermsChangeResult += fname; + rollback = true; + } + } + + // roll back files permissions if failed + if (rollback) { + for (auto it = m_sharePerms.constBegin(); it != m_sharePerms.constEnd(); ++it) { + QString fname = it.key(); + QFile::Permissions perms = it.value().oldPerm; + QFile::setPermissions(fname, perms); } } return filePermsChangeResult.join(QLatin1Char('\n')); } + bool shareOrPathHasACL() const + { + bool hasACL = false; + for (auto it = m_sharePosixACL.constBegin(); it != m_sharePosixACL.constEnd(); ++it) { + QString fname = it.key(); + if (it.value().hasExtendedACL) { + hasACL = true; + } + } + return hasACL; + } + + QString sharePermsForElemsWithACL() const + { + QStringList allACLs; + for (auto it = m_sharePosixACL.constBegin(); it != m_sharePosixACL.constEnd(); ++it) { + QString fname = it.key(); + if (it.value().hasExtendedACL) { + allACLs += QStringLiteral("File:\t%1\nMode:\t%2\n") + .arg(fname) + .arg(it.value().permsAsString); + } + } + return allACLs.join(QLatin1Char('\n')); + } + private: void sharePermsResolve(const QString &path) { QFile::Permissions permsForShare; + QFile::Permissions permsForSharePath; QFile::Permissions oldPerm, newPerm; QStringList filesWOProperPermissions; + QString curUser; + KFileItem fitem; - // parse acl + // parse samba acl QStringList lstAcl(m_acl.split(QStringLiteral(","))); for (const auto &it : lstAcl) { const auto aclVal = it.split(QStringLiteral(":")); - if (aclVal.at(0).compare(m_user, Qt::CaseSensitive) != 0) { - if (aclVal.at(1).endsWith(QStringLiteral("r"))) { - permsForShare |= (QFile::ExeOther | QFile::ReadOther); - } else if (aclVal.at(1).endsWith(QStringLiteral("f"))) { - permsForShare |= (QFile::ExeOther | QFile::ReadOther | QFile::WriteOther); + curUser = aclVal.at(0); + if (curUser.compare(m_user, Qt::CaseSensitive) != 0) { + // check if user has same primary group as current user + if (getUserPrimaryGroup(curUser).compare(m_group, Qt::CaseSensitive) == 0) { + if (aclVal.at(1).endsWith(QStringLiteral("r"))) { + permsForShare |= (QFile::ExeGroup | QFile::ReadGroup); + permsForSharePath |= QFile::ExeGroup; + } else if (aclVal.at(1).endsWith(QStringLiteral("f"))) { + permsForShare |= (QFile::ExeGroup | QFile::ReadGroup | QFile::WriteGroup); + permsForSharePath |= QFile::ExeGroup; + } + } else { + if (aclVal.at(1).endsWith(QStringLiteral("r"))) { + permsForShare |= (QFile::ExeOther | QFile::ReadOther); + permsForSharePath |= QFile::ExeOther; + } else if (aclVal.at(1).endsWith(QStringLiteral("f"))) { + permsForShare |= (QFile::ExeOther | QFile::ReadOther | QFile::WriteOther); + permsForSharePath |= QFile::ExeOther; + } } } } @@ -109,16 +164,30 @@ private: if (!QFileInfo(path).permission(permsForShare)) { filesWOProperPermissions.append(path); } - // check if share path could be resolved for others (has 'o+x' all the way through) + // check and store share POSIX ACL + fitem = getCompleteFileItem(path); + m_sharePosixACL.insert(path, VV(fitem.hasExtendedACL(), + fitem.permissionsString(), + fitem.ACL().asString(), + fitem.defaultACL().asString()) + ); + // check if share path could be resolved (has 'g+x' or 'o+x' all the way through) if (permsForShare) { - QStringList lstPath = path.split(QStringLiteral("/"), QString::SkipEmptyParts); + QStringList lstPath = path.split(QStringLiteral("/"), Qt::SkipEmptyParts); QString curPath; for (const auto &it : lstPath) { curPath.append(QStringLiteral("/%1").arg(it)); const QFileInfo info(curPath); - if (!info.permission(QFile::ExeOther)) { + if (!info.permission(permsForSharePath)) { filesWOProperPermissions.append(curPath); } + // check and store share path element's POSIX ACL + fitem = getCompleteFileItem(curPath); + m_sharePosixACL.insert(curPath, VV(fitem.hasExtendedACL(), + fitem.permissionsString(), + fitem.ACL().asString(), + fitem.defaultACL().asString()) + ); } } @@ -129,14 +198,40 @@ private: if (QString::compare(it, path, Qt::CaseSensitive) == 0) { newPerm = oldPerm | permsForShare; } else { - newPerm = oldPerm | QFile::ExeOther; + newPerm = oldPerm | permsForSharePath; } // store files and permissions to change later on - m_sharePerms.insert(it, newPerm); + m_sharePerms.insert(it, V(oldPerm, newPerm)); } } } + QString getCurrentUserGroup() const + { + return m_group; + } + + QString getUserPrimaryGroup(QString const user) + { + KUser kuser = KUser(user); + QStringList groups = kuser.groupNames(); + if (groups.length() != 0) { + return groups.at(0); + } + // if we can't fetch user's groups than assume that group name is the same witn user name + return user; + } + + KFileItem getCompleteFileItem(QString path) + { + const QUrl url = QUrl::fromLocalFile(path); + auto job = KIO::stat(url); + job->exec(); + KIO::UDSEntry entry = job->statResult(); + KFileItem item(entry, url); + return item; + } + QString prettyPrintFilePerms(QFile::Permissions perm) const { QString res = QLatin1String((perm & QFileDevice::ReadOwner) ? "r" : "-") @@ -163,9 +258,33 @@ private: } private: + struct V { + QFile::Permissions oldPerm; + QFile::Permissions newPerm; + V(QFile::Permissions o, QFile::Permissions n) { + oldPerm = o; + newPerm = n; + } + V() = default; + }; + struct VV { + bool hasExtendedACL = false; + QString permsAsString; + QString extACL; + QString defACL; + VV(bool h, QString p, QString ea = QString(), QString da = QString()) { + hasExtendedACL = h; + permsAsString = p; + extACL = ea; + defACL = da; + } + VV() = default; + }; QString m_acl; QString m_user; - QMap m_sharePerms; + QString m_group; + QMap m_sharePerms; + QMap m_sharePosixACL; }; class ShareContext : public QObject @@ -387,25 +506,49 @@ void SambaUserSharePlugin::applyChanges() m_userManager->currentUser()->name()); QString res = helper.sharePermsCheck(); + bool aclFound = helper.shareOrPathHasACL(); if (!res.isEmpty()) { - QString warningMessage = xi18nc("@info detailed warning message", - "The folder \"%1\"" - "needs the extra permissions for sharing to work." - "Do you want Dolphin to add these permissions to " - "the folder automatically?" - "Required permission changes are listed in details " - "below:", - m_context->m_shareData.path()); + QString warningMessage; + + if (aclFound) { + warningMessage = xi18nc("@info detailed warning message", + "The folder \"%1\"" + "needs the extra permissions for sharing to work." + "Do you want Dolphin to add these permissions to " + "the folder automatically?" + "Notice: Shared folder or it's path elements has " + "extended ACL permissions." + "Network share might not work properly." + "Required permission changes are listed in details " + "below:", + m_context->m_shareData.path()); + } else { + warningMessage = xi18nc("@info detailed warning message", + "The folder \"%1\"" + "needs the extra permissions for sharing to work." + "Do you want Dolphin to add these permissions to " + "the folder automatically?" + "Required permission changes are listed in details " + "below:", + m_context->m_shareData.path()); + } KGuiItem contButton(i18n("Change permissions"), QString(), QString(), QString()); KGuiItem cancelButton(i18n("Cancel"), QString(), QString(), QString()); + // formatting message details + res = QStringLiteral("File permissions changes:\n\n%1").arg(res); + if (aclFound) { + res = res + + QStringLiteral("\nFiles with extended ACL permissions:\n\n%1") + .arg(helper.sharePermsForElemsWithACL()); + } KMessageBox::ButtonCode ret = KMessageBox::warningContinueCancelDetailed( qobject_cast(parent()), warningMessage, i18nc("@info warning message caption", - "File permissions changes required"), + "File permissions changes required"), contButton, cancelButton, QString(), @@ -414,16 +557,26 @@ void SambaUserSharePlugin::applyChanges() if (ret == KMessageBox::Continue) { res = helper.sharePermsChange(); - if (!res.isEmpty()) { QString errorMessage = i18nc("@info detailed error message", - "Could not change the permissions for:\n\n%1", + "Could not change the permissions for:\n\n%1\n\n" + "All permission changes were reverted to initial state.", res); KMessageBox::sorry(qobject_cast(parent()), errorMessage, i18nc("@info/title", "Failed to change permissions")); } } + } else if (aclFound) { + QString aclMessage = i18nc("@info acl info message", + "Shared folder or it's path elements\n" + "has extended ACL permissions.\n" + "Network share might not work properly.\n\n" + "Files with extended ACL permissions:\n\n%1", + helper.sharePermsForElemsWithACL()); + KMessageBox::information(qobject_cast(parent()), + aclMessage, + i18nc("@info/title", "Extended ACL permissions found")); } // TODO: should run this through reportAdd() as well, ACLs may be invalid and then we shouldn't try to save -- GitLab From 60c741f0605d52102be4aa94267aed98c05b5b02 Mon Sep 17 00:00:00 2001 From: Danil Shein Date: Wed, 7 Apr 2021 15:01:16 +0300 Subject: [PATCH 3/4] Permission helper code refactor. User messages rewording. --- .../sambausershareplugin.cpp | 181 +++++++++--------- 1 file changed, 87 insertions(+), 94 deletions(-) diff --git a/samba/filepropertiesplugin/sambausershareplugin.cpp b/samba/filepropertiesplugin/sambausershareplugin.cpp index bf7b8a1..8c806c8 100644 --- a/samba/filepropertiesplugin/sambausershareplugin.cpp +++ b/samba/filepropertiesplugin/sambausershareplugin.cpp @@ -48,9 +48,9 @@ class SharePermissionsHelper public: explicit SharePermissionsHelper(const QString &path, const QString &acl, const QString &user): m_acl(acl), - m_user(user) + m_user(user), + m_group(getUserPrimaryGroup(user)) { - m_group = getUserPrimaryGroup(m_user); sharePermsResolve(path); } @@ -129,31 +129,31 @@ private: { QFile::Permissions permsForShare; QFile::Permissions permsForSharePath; - QFile::Permissions oldPerm, newPerm; + QFile::Permissions oldPerm; + QFile::Permissions newPerm; QStringList filesWOProperPermissions; - QString curUser; - KFileItem fitem; // parse samba acl - QStringList lstAcl(m_acl.split(QStringLiteral(","))); - for (const auto &it : lstAcl) { - const auto aclVal = it.split(QStringLiteral(":")); - curUser = aclVal.at(0); - if (curUser.compare(m_user, Qt::CaseSensitive) != 0) { + const QStringList lstSambaACLs(m_acl.split(QStringLiteral(","))); + for (const auto &acl : lstSambaACLs) { + const auto sambaACLRecord = acl.split(QStringLiteral(":")); + QString aclUserName = sambaACLRecord.at(0); + QString aclAccessMode = sambaACLRecord.at(1); + if (aclUserName != m_user) { // check if user has same primary group as current user - if (getUserPrimaryGroup(curUser).compare(m_group, Qt::CaseSensitive) == 0) { - if (aclVal.at(1).endsWith(QStringLiteral("r"))) { + if (getUserPrimaryGroup(aclUserName) == m_group) { + if (aclAccessMode.endsWith(QStringLiteral("r"))) { permsForShare |= (QFile::ExeGroup | QFile::ReadGroup); permsForSharePath |= QFile::ExeGroup; - } else if (aclVal.at(1).endsWith(QStringLiteral("f"))) { + } else if (aclAccessMode.endsWith(QStringLiteral("f"))) { permsForShare |= (QFile::ExeGroup | QFile::ReadGroup | QFile::WriteGroup); permsForSharePath |= QFile::ExeGroup; } } else { - if (aclVal.at(1).endsWith(QStringLiteral("r"))) { + if (aclAccessMode.endsWith(QStringLiteral("r"))) { permsForShare |= (QFile::ExeOther | QFile::ReadOther); permsForSharePath |= QFile::ExeOther; - } else if (aclVal.at(1).endsWith(QStringLiteral("f"))) { + } else if (aclAccessMode.endsWith(QStringLiteral("f"))) { permsForShare |= (QFile::ExeOther | QFile::ReadOther | QFile::WriteOther); permsForSharePath |= QFile::ExeOther; } @@ -165,43 +165,43 @@ private: filesWOProperPermissions.append(path); } // check and store share POSIX ACL - fitem = getCompleteFileItem(path); - m_sharePosixACL.insert(path, VV(fitem.hasExtendedACL(), - fitem.permissionsString(), - fitem.ACL().asString(), - fitem.defaultACL().asString()) + KFileItem currentFileItem = getCompleteFileItem(path); + m_sharePosixACL.insert(path, fileACLsStruct{currentFileItem.hasExtendedACL(), + currentFileItem.permissionsString(), + currentFileItem.ACL().asString(), + currentFileItem.defaultACL().asString()} ); // check if share path could be resolved (has 'g+x' or 'o+x' all the way through) if (permsForShare) { - QStringList lstPath = path.split(QStringLiteral("/"), Qt::SkipEmptyParts); - QString curPath; + const QStringList lstPath = path.split(QStringLiteral("/"), Qt::SkipEmptyParts); + QString currentPath; for (const auto &it : lstPath) { - curPath.append(QStringLiteral("/%1").arg(it)); - const QFileInfo info(curPath); + currentPath.append(QStringLiteral("/%1").arg(it)); + const QFileInfo info(currentPath); if (!info.permission(permsForSharePath)) { - filesWOProperPermissions.append(curPath); + filesWOProperPermissions.append(currentPath); } // check and store share path element's POSIX ACL - fitem = getCompleteFileItem(curPath); - m_sharePosixACL.insert(curPath, VV(fitem.hasExtendedACL(), - fitem.permissionsString(), - fitem.ACL().asString(), - fitem.defaultACL().asString()) + currentFileItem = getCompleteFileItem(currentPath); + m_sharePosixACL.insert(currentPath, fileACLsStruct{currentFileItem.hasExtendedACL(), + currentFileItem.permissionsString(), + currentFileItem.ACL().asString(), + currentFileItem.defaultACL().asString()} ); } } if (!filesWOProperPermissions.isEmpty()) { // we have to change permissions for share and share's path elements - for (const auto it : filesWOProperPermissions) { - oldPerm = QFileInfo(it).permissions(); - if (QString::compare(it, path, Qt::CaseSensitive) == 0) { + for (const auto ¤tPath : qAsConst(filesWOProperPermissions)) { + oldPerm = QFileInfo(currentPath).permissions(); + if (currentPath == path) { newPerm = oldPerm | permsForShare; } else { newPerm = oldPerm | permsForSharePath; } // store files and permissions to change later on - m_sharePerms.insert(it, V(oldPerm, newPerm)); + m_sharePerms.insert(currentPath, filePermsStruct{oldPerm, newPerm}); } } } @@ -211,18 +211,18 @@ private: return m_group; } - QString getUserPrimaryGroup(QString const user) + QString getUserPrimaryGroup(QString const user) const { - KUser kuser = KUser(user); - QStringList groups = kuser.groupNames(); - if (groups.length() != 0) { + const KUser kuser = KUser(user); + const QStringList groups = kuser.groupNames(); + if (!groups.isEmpty()) { return groups.at(0); } // if we can't fetch user's groups than assume that group name is the same witn user name return user; } - KFileItem getCompleteFileItem(QString path) + KFileItem getCompleteFileItem(QString path) const { const QUrl url = QUrl::fromLocalFile(path); auto job = KIO::stat(url); @@ -234,17 +234,22 @@ private: QString prettyPrintFilePerms(QFile::Permissions perm) const { - QString res = QLatin1String((perm & QFileDevice::ReadOwner) ? "r" : "-") - + QLatin1String((perm & QFileDevice::WriteOwner) ? "w" : "-") - + QLatin1String((perm & QFileDevice::ExeOwner) ? "x" : "-") - + QLatin1String((perm & QFileDevice::ReadGroup) ? "r" : "-") - + QLatin1String((perm & QFileDevice::WriteGroup) ? "w" : "-") - + QLatin1String((perm & QFileDevice::ExeGroup) ? "x" : "-") - + QLatin1String((perm & QFileDevice::ReadOther) ? "r" : "-") - + QLatin1String((perm & QFileDevice::WriteOther) ? "w" : "-") - + QLatin1String((perm & QFileDevice::ExeOther) ? "x" : "-"); - - int p = ((perm & QFileDevice::ReadOwner) ? 0400 : 0) + const QString read = QStringLiteral("r"); + const QString write = QStringLiteral("w"); + const QString exe = QStringLiteral("x"); + const QString none = QStringLiteral("-"); + + const QString permsAsRWX = ((perm & QFileDevice::ReadOwner) ? read : none) + + ((perm & QFileDevice::WriteOwner) ? write : none) + + ((perm & QFileDevice::ExeOwner) ? exe : none) + + ((perm & QFileDevice::ReadGroup) ? read : none) + + ((perm & QFileDevice::WriteGroup) ? write : none) + + ((perm & QFileDevice::ExeGroup) ? exe : none) + + ((perm & QFileDevice::ReadOther) ? read : none) + + ((perm & QFileDevice::WriteOther) ? write : none) + + ((perm & QFileDevice::ExeOther) ? exe : none); + + const int permsAsNum = ((perm & QFileDevice::ReadOwner) ? 0400 : 0) + ((perm & QFileDevice::WriteOwner) ? 0200 : 0) + ((perm & QFileDevice::ExeOwner) ? 0100 : 0) + ((perm & QFileDevice::ReadGroup) ? 0040 : 0) @@ -254,37 +259,25 @@ private: + ((perm & QFileDevice::WriteOther) ? 0002 : 0) + ((perm & QFileDevice::ExeOther) ? 0001 : 0); - return res + QStringLiteral(" (0%1)").arg(QString::number(p, 8)); + return permsAsRWX + QStringLiteral(" (0%1)").arg(QString::number(permsAsNum, 8)); } private: - struct V { + struct filePermsStruct { QFile::Permissions oldPerm; QFile::Permissions newPerm; - V(QFile::Permissions o, QFile::Permissions n) { - oldPerm = o; - newPerm = n; - } - V() = default; }; - struct VV { - bool hasExtendedACL = false; + struct fileACLsStruct { + bool hasExtendedACL; QString permsAsString; QString extACL; QString defACL; - VV(bool h, QString p, QString ea = QString(), QString da = QString()) { - hasExtendedACL = h; - permsAsString = p; - extACL = ea; - defACL = da; - } - VV() = default; }; - QString m_acl; - QString m_user; - QString m_group; - QMap m_sharePerms; - QMap m_sharePosixACL; + const QString m_acl; + const QString m_user; + const QString m_group; + QMap m_sharePerms; + QMap m_sharePosixACL; }; class ShareContext : public QObject @@ -505,10 +498,10 @@ void SambaUserSharePlugin::applyChanges() m_model->getAcl(), m_userManager->currentUser()->name()); - QString res = helper.sharePermsCheck(); - bool aclFound = helper.shareOrPathHasACL(); + QString unmetSharePerms = helper.sharePermsCheck(); + const bool aclFound = helper.shareOrPathHasACL(); - if (!res.isEmpty()) { + if (!unmetSharePerms.isEmpty()) { QString warningMessage; if (aclFound) { @@ -518,7 +511,7 @@ void SambaUserSharePlugin::applyChanges() "Do you want Dolphin to add these permissions to " "the folder automatically?" "Notice: Shared folder or it's path elements has " - "extended ACL permissions." + "extended Advanced Permissions." "Network share might not work properly." "Required permission changes are listed in details " "below:", @@ -534,13 +527,13 @@ void SambaUserSharePlugin::applyChanges() m_context->m_shareData.path()); } - KGuiItem contButton(i18n("Change permissions"), QString(), QString(), QString()); - KGuiItem cancelButton(i18n("Cancel"), QString(), QString(), QString()); + KGuiItem contButton(i18nc("@button", "Change permissions"), QString(), QString(), QString()); + KGuiItem cancelButton(i18nc("@button", "Cancel"), QString(), QString(), QString()); // formatting message details - res = QStringLiteral("File permissions changes:\n\n%1").arg(res); + QString sharePermsDetails = i18n("File permissions changes:\n\n%1").arg(unmetSharePerms); if (aclFound) { - res = res - + QStringLiteral("\nFiles with extended ACL permissions:\n\n%1") + sharePermsDetails = sharePermsDetails + + i18n("\nFiles with extended Advanced Permissions:\n\n%1") .arg(helper.sharePermsForElemsWithACL()); } @@ -553,30 +546,30 @@ void SambaUserSharePlugin::applyChanges() cancelButton, QString(), KMessageBox::Notify, - res); + sharePermsDetails); if (ret == KMessageBox::Continue) { - res = helper.sharePermsChange(); - if (!res.isEmpty()) { - QString errorMessage = i18nc("@info detailed error message", - "Could not change the permissions for:\n\n%1\n\n" - "All permission changes were reverted to initial state.", - res); + QString permsChangeResult = helper.sharePermsChange(); + if (!permsChangeReult.isEmpty()) { + const QString errorMessage = i18nc("@info detailed error message", + "Could not change the permissions for:\n%1\n" + "All permission changes were reverted to initial state.", + permsChangeResult); KMessageBox::sorry(qobject_cast(parent()), errorMessage, i18nc("@info/title", "Failed to change permissions")); } } } else if (aclFound) { - QString aclMessage = i18nc("@info acl info message", - "Shared folder or it's path elements\n" - "has extended ACL permissions.\n" - "Network share might not work properly.\n\n" - "Files with extended ACL permissions:\n\n%1", - helper.sharePermsForElemsWithACL()); + const QString aclMessage = i18nc("@info acl info message", + "Shared folder or it's path elements\n" + "has extended Advanced Permissions.\n" + "Network share might not work properly.\n\n" + "Files with extended Advanced Permissions:\n\n%1", + helper.sharePermsForElemsWithACL()); KMessageBox::information(qobject_cast(parent()), aclMessage, - i18nc("@info/title", "Extended ACL permissions found")); + i18nc("@info/title", "Extended Advanced Permissions found")); } // TODO: should run this through reportAdd() as well, ACLs may be invalid and then we shouldn't try to save -- GitLab From bffcef2738f5aedf38866ce26386b5441b409522 Mon Sep 17 00:00:00 2001 From: Danil Shein Date: Mon, 26 Apr 2021 13:35:20 +0300 Subject: [PATCH 4/4] Permission helper code refactor --- .../sambausershareplugin.cpp | 113 +++++++++--------- 1 file changed, 56 insertions(+), 57 deletions(-) diff --git a/samba/filepropertiesplugin/sambausershareplugin.cpp b/samba/filepropertiesplugin/sambausershareplugin.cpp index 8c806c8..81fe59e 100644 --- a/samba/filepropertiesplugin/sambausershareplugin.cpp +++ b/samba/filepropertiesplugin/sambausershareplugin.cpp @@ -4,6 +4,7 @@ SPDX-FileCopyrightText: 2011 Rodrigo Belem SPDX-FileCopyrightText: 2015-2020 Harald Sitter SPDX-FileCopyrightText: 2019 Nate Graham + SPDX-FileCopyrightText: 2021 Danil Shein */ #include "sambausershareplugin.h" @@ -59,7 +60,7 @@ public: QStringList filePermsCheckResult; for (auto it = m_sharePerms.constBegin(); it != m_sharePerms.constEnd(); ++it) { - QString fname = it.key(); + const QString &fname = it.key(); filePermsCheckResult += QStringLiteral("File:\t%1\nMode:\t%2 -> %3\n") .arg(fname) @@ -76,8 +77,8 @@ public: bool rollback = false; for (auto it = m_sharePerms.constBegin(); it != m_sharePerms.constEnd(); ++it) { - QString fname = it.key(); - QFile::Permissions perms = it.value().newPerm; + const QString &fname = it.key(); + const QFile::Permissions perms = it.value().newPerm; if (!QFile::setPermissions(fname, perms)) { // failed to change permissions @@ -89,9 +90,11 @@ public: // roll back files permissions if failed if (rollback) { for (auto it = m_sharePerms.constBegin(); it != m_sharePerms.constEnd(); ++it) { - QString fname = it.key(); - QFile::Permissions perms = it.value().oldPerm; - QFile::setPermissions(fname, perms); + const QString &fname = it.key(); + const QFile::Permissions perms = it.value().oldPerm; + if (!QFile::setPermissions(fname, perms)) { + qWarning("SharePermissionsHelper::sharePermsChange: failed to restore permissions for %s", fname); + } } } @@ -100,21 +103,19 @@ public: bool shareOrPathHasACL() const { - bool hasACL = false; - for (auto it = m_sharePosixACL.constBegin(); it != m_sharePosixACL.constEnd(); ++it) { - QString fname = it.key(); - if (it.value().hasExtendedACL) { - hasACL = true; + for (const auto &fileACL : m_sharePosixACL) { + if (fileACL.hasExtendedACL) { + return true; } } - return hasACL; + return false; } - QString sharePermsForElemsWithACL() const + QString sharePermsForElementsWithACL() const { QStringList allACLs; for (auto it = m_sharePosixACL.constBegin(); it != m_sharePosixACL.constEnd(); ++it) { - QString fname = it.key(); + const QString &fname = it.key(); if (it.value().hasExtendedACL) { allACLs += QStringLiteral("File:\t%1\nMode:\t%2\n") .arg(fname) @@ -131,14 +132,14 @@ private: QFile::Permissions permsForSharePath; QFile::Permissions oldPerm; QFile::Permissions newPerm; - QStringList filesWOProperPermissions; + QStringList filesWithoutProperPermissions; // parse samba acl - const QStringList lstSambaACLs(m_acl.split(QStringLiteral(","))); - for (const auto &acl : lstSambaACLs) { + const QStringList sambaACLs(m_acl.split(QStringLiteral(","))); + for (const auto &acl : sambaACLs) { const auto sambaACLRecord = acl.split(QStringLiteral(":")); - QString aclUserName = sambaACLRecord.at(0); - QString aclAccessMode = sambaACLRecord.at(1); + const QString &aclUserName = sambaACLRecord.at(0); + const QString &aclAccessMode = sambaACLRecord.at(1); if (aclUserName != m_user) { // check if user has same primary group as current user if (getUserPrimaryGroup(aclUserName) == m_group) { @@ -162,7 +163,7 @@ private: } // store share path if permissions are insufficient if (!QFileInfo(path).permission(permsForShare)) { - filesWOProperPermissions.append(path); + filesWithoutProperPermissions.append(path); } // check and store share POSIX ACL KFileItem currentFileItem = getCompleteFileItem(path); @@ -173,13 +174,13 @@ private: ); // check if share path could be resolved (has 'g+x' or 'o+x' all the way through) if (permsForShare) { - const QStringList lstPath = path.split(QStringLiteral("/"), Qt::SkipEmptyParts); + const QStringList pathParts = path.split(QStringLiteral("/"), Qt::SkipEmptyParts); QString currentPath; - for (const auto &it : lstPath) { + for (const auto &it : pathParts) { currentPath.append(QStringLiteral("/%1").arg(it)); const QFileInfo info(currentPath); if (!info.permission(permsForSharePath)) { - filesWOProperPermissions.append(currentPath); + filesWithoutProperPermissions.append(currentPath); } // check and store share path element's POSIX ACL currentFileItem = getCompleteFileItem(currentPath); @@ -191,9 +192,9 @@ private: } } - if (!filesWOProperPermissions.isEmpty()) { + if (!filesWithoutProperPermissions.isEmpty()) { // we have to change permissions for share and share's path elements - for (const auto ¤tPath : qAsConst(filesWOProperPermissions)) { + for (const auto ¤tPath : qAsConst(filesWithoutProperPermissions)) { oldPerm = QFileInfo(currentPath).permissions(); if (currentPath == path) { newPerm = oldPerm | permsForShare; @@ -206,23 +207,22 @@ private: } } - QString getCurrentUserGroup() const + QString currentUserGroup() const { return m_group; } - QString getUserPrimaryGroup(QString const user) const + static QString getUserPrimaryGroup(const QString &user) { - const KUser kuser = KUser(user); - const QStringList groups = kuser.groupNames(); + const QStringList groups = KUser(user).groupNames(); if (!groups.isEmpty()) { return groups.at(0); } - // if we can't fetch user's groups than assume that group name is the same witn user name + // if we can't fetch the user's groups then assume the group name is the same as the user name return user; } - KFileItem getCompleteFileItem(QString path) const + static KFileItem getCompleteFileItem(const QString &path) { const QUrl url = QUrl::fromLocalFile(path); auto job = KIO::stat(url); @@ -249,20 +249,19 @@ private: + ((perm & QFileDevice::WriteOther) ? write : none) + ((perm & QFileDevice::ExeOther) ? exe : none); - const int permsAsNum = ((perm & QFileDevice::ReadOwner) ? 0400 : 0) - + ((perm & QFileDevice::WriteOwner) ? 0200 : 0) - + ((perm & QFileDevice::ExeOwner) ? 0100 : 0) - + ((perm & QFileDevice::ReadGroup) ? 0040 : 0) - + ((perm & QFileDevice::WriteGroup) ? 0020 : 0) - + ((perm & QFileDevice::ExeGroup) ? 0010 : 0) - + ((perm & QFileDevice::ReadOther) ? 0004 : 0) - + ((perm & QFileDevice::WriteOther) ? 0002 : 0) - + ((perm & QFileDevice::ExeOther) ? 0001 : 0); + const int permsAsNum = ((perm & QFileDevice::ReadOwner) ? S_IRUSR : 0) + + ((perm & QFileDevice::WriteOwner) ? S_IWUSR : 0) + + ((perm & QFileDevice::ExeOwner) ? S_IXUSR : 0) + + ((perm & QFileDevice::ReadGroup) ? S_IRGRP : 0) + + ((perm & QFileDevice::WriteGroup) ? S_IWGRP : 0) + + ((perm & QFileDevice::ExeGroup) ? S_IXGRP : 0) + + ((perm & QFileDevice::ReadOther) ? S_IROTH : 0) + + ((perm & QFileDevice::WriteOther) ? S_IWOTH : 0) + + ((perm & QFileDevice::ExeOther) ? S_IXOTH : 0); return permsAsRWX + QStringLiteral(" (0%1)").arg(QString::number(permsAsNum, 8)); } -private: struct filePermsStruct { QFile::Permissions oldPerm; QFile::Permissions newPerm; @@ -494,11 +493,11 @@ void SambaUserSharePlugin::applyChanges() } // Check and change permissions for shared folder - SharePermissionsHelper helper(m_context->m_shareData.path(), + const SharePermissionsHelper helper(m_context->m_shareData.path(), m_model->getAcl(), m_userManager->currentUser()->name()); - QString unmetSharePerms = helper.sharePermsCheck(); + const QString unmetSharePerms = helper.sharePermsCheck(); const bool aclFound = helper.shareOrPathHasACL(); if (!unmetSharePerms.isEmpty()) { @@ -527,49 +526,49 @@ void SambaUserSharePlugin::applyChanges() m_context->m_shareData.path()); } - KGuiItem contButton(i18nc("@button", "Change permissions"), QString(), QString(), QString()); - KGuiItem cancelButton(i18nc("@button", "Cancel"), QString(), QString(), QString()); + const KGuiItem applyButton(i18nc("@button", "Change permissions"), QString(), QString(), QString()); + const KGuiItem cancelButton(i18nc("@button", "Cancel"), QString(), QString(), QString()); // formatting message details QString sharePermsDetails = i18n("File permissions changes:\n\n%1").arg(unmetSharePerms); if (aclFound) { sharePermsDetails = sharePermsDetails + i18n("\nFiles with extended Advanced Permissions:\n\n%1") - .arg(helper.sharePermsForElemsWithACL()); + .arg(helper.sharePermsForElementsWithACL()); } - KMessageBox::ButtonCode ret = KMessageBox::warningContinueCancelDetailed( - qobject_cast(parent()), + const KMessageBox::ButtonCode ret = KMessageBox::warningContinueCancelDetailed( + properties, warningMessage, i18nc("@info warning message caption", "File permissions changes required"), - contButton, + applyButton, cancelButton, QString(), KMessageBox::Notify, sharePermsDetails); if (ret == KMessageBox::Continue) { - QString permsChangeResult = helper.sharePermsChange(); - if (!permsChangeReult.isEmpty()) { + const QString permsChangeResult = helper.sharePermsChange(); + if (!permsChangeResult.isEmpty()) { const QString errorMessage = i18nc("@info detailed error message", "Could not change the permissions for:\n%1\n" "All permission changes were reverted to initial state.", permsChangeResult); - KMessageBox::sorry(qobject_cast(parent()), + KMessageBox::sorry(properties, errorMessage, - i18nc("@info/title", "Failed to change permissions")); + i18nc("@info/title", "Failed to Change Permissions")); } } } else if (aclFound) { const QString aclMessage = i18nc("@info acl info message", - "Shared folder or it's path elements\n" + "Shared folder, or it's path elements\n" "has extended Advanced Permissions.\n" "Network share might not work properly.\n\n" "Files with extended Advanced Permissions:\n\n%1", - helper.sharePermsForElemsWithACL()); - KMessageBox::information(qobject_cast(parent()), + helper.sharePermsForElementsWithACL()); + KMessageBox::information(properties, aclMessage, - i18nc("@info/title", "Extended Advanced Permissions found")); + i18nc("@info/title", "Advanced Permissions Found")); } // TODO: should run this through reportAdd() as well, ACLs may be invalid and then we shouldn't try to save -- GitLab