diff --git a/doc/xml/release.xml b/doc/xml/release.xml index 4b7b3b698..13efe1ba1 100644 --- a/doc/xml/release.xml +++ b/doc/xml/release.xml @@ -37,6 +37,10 @@ + +

Change file ownership only when required.

+
+ diff --git a/lib/pgBackRest/Storage/Posix/Driver.pm b/lib/pgBackRest/Storage/Posix/Driver.pm index 4b189d998..e85e4b42b 100644 --- a/lib/pgBackRest/Storage/Posix/Driver.pm +++ b/lib/pgBackRest/Storage/Posix/Driver.pm @@ -698,19 +698,16 @@ sub owner # If the user or group is not defined then get it by stat'ing the file. This is because the chown function requires that # both user and group be set. - if (!(defined($strUser) && defined($strGroup))) + my $oStat = $self->info($strFilePath); + + if (!defined($strUser)) { - my $oStat = $self->info($strFilePath); + $iUserId = $oStat->uid; + } - if (!defined($strUser)) - { - $iUserId = $oStat->uid; - } - - if (!defined($strGroup)) - { - $iGroupId = $oStat->gid; - } + if (!defined($strGroup)) + { + $iGroupId = $oStat->gid; } # Lookup user if specified @@ -735,15 +732,13 @@ sub owner } } - # Set ownership on the file - if (!chown($iUserId, $iGroupId, $strFilePath)) + # Set ownership on the file if the user or group would be changed + if ($iUserId != $oStat->uid || $iGroupId != $oStat->gid) { - if ($OS_ERROR{ENOENT}) + if (!chown($iUserId, $iGroupId, $strFilePath)) { - logErrorResult(ERROR_FILE_OWNER, "${strMessage} because it is missing"); + logErrorResult(ERROR_FILE_OWNER, "${strMessage}", $OS_ERROR); } - - logErrorResult(ERROR_FILE_OWNER, "${strMessage}", $OS_ERROR); } } diff --git a/src/perl/embed.auto.c b/src/perl/embed.auto.c index 1eb85f30c..c66650e7c 100644 --- a/src/perl/embed.auto.c +++ b/src/perl/embed.auto.c @@ -19996,8 +19996,6 @@ static const EmbeddedModule embeddedModule[] = "my $iUserId;\n" "my $iGroupId;\n" "\n\n\n" - "if (!(defined($strUser) && defined($strGroup)))\n" - "{\n" "my $oStat = $self->info($strFilePath);\n" "\n" "if (!defined($strUser))\n" @@ -20009,7 +20007,6 @@ static const EmbeddedModule embeddedModule[] = "{\n" "$iGroupId = $oStat->gid;\n" "}\n" - "}\n" "\n\n" "if (defined($strUser))\n" "{\n" @@ -20031,16 +20028,14 @@ static const EmbeddedModule embeddedModule[] = "}\n" "}\n" "\n\n" + "if ($iUserId != $oStat->uid || $iGroupId != $oStat->gid)\n" + "{\n" "if (!chown($iUserId, $iGroupId, $strFilePath))\n" "{\n" - "if ($OS_ERROR{ENOENT})\n" - "{\n" - "logErrorResult(ERROR_FILE_OWNER, \"${strMessage} because it is missing\");\n" - "}\n" - "\n" "logErrorResult(ERROR_FILE_OWNER, \"${strMessage}\", $OS_ERROR);\n" "}\n" "}\n" + "}\n" "\n\n" "return logDebugReturn($strOperation);\n" "}\n" diff --git a/test/define.yaml b/test/define.yaml index 8bc2af125..0037c3562 100644 --- a/test/define.yaml +++ b/test/define.yaml @@ -458,7 +458,7 @@ unit: # ---------------------------------------------------------------------------------------------------------------------------- - name: posix-perl - total: 9 + total: 10 coverage: Storage/Posix/Driver: partial diff --git a/test/lib/pgBackRestTest/Module/Storage/StoragePosixPerlTest.pm b/test/lib/pgBackRestTest/Module/Storage/StoragePosixPerlTest.pm index 6007ab4a4..a216f1e10 100644 --- a/test/lib/pgBackRestTest/Module/Storage/StoragePosixPerlTest.pm +++ b/test/lib/pgBackRestTest/Module/Storage/StoragePosixPerlTest.pm @@ -19,6 +19,7 @@ use pgBackRest::Common::Exception; use pgBackRest::Common::Log; use pgBackRest::Storage::Posix::Driver; +use pgBackRestTest::Common::ContainerTest; use pgBackRestTest::Common::ExecuteTest; use pgBackRestTest::Common::RunTest; @@ -313,6 +314,42 @@ sub run sub {$oPosixIo->close()}, ERROR_FILE_WRITE, "unable to set time for '${strFile}': No such file or directory"); } + ################################################################################################################################ + if ($self->begin('owner()')) + { + my $strFile = $self->testPath() . "/test.txt"; + + $self->testException( + sub {$oPosix->owner($strFile, {strUser => 'root'})}, ERROR_FILE_MISSING, + "unable to stat '${strFile}': No such file or directory"); + + executeTest("touch ${strFile}"); + + $self->testException( + sub {$oPosix->owner($strFile, {strUser => BOGUS})}, ERROR_FILE_OWNER, + "unable to set ownership for '${strFile}' because user 'bogus' does not exist"); + $self->testException( + sub {$oPosix->owner($strFile, {strGroup => BOGUS})}, ERROR_FILE_OWNER, + "unable to set ownership for '${strFile}' because group 'bogus' does not exist"); + + $self->testResult(sub {$oPosix->owner($strFile)}, undef, "no ownership changes"); + $self->testResult(sub {$oPosix->owner($strFile, {strUser => TEST_USER})}, undef, "same user"); + $self->testResult(sub {$oPosix->owner($strFile, {strGroup => TEST_GROUP})}, undef, "same group"); + $self->testResult( + sub {$oPosix->owner($strFile, {strUser => TEST_USER, strGroup => TEST_GROUP})}, undef, "same user, group"); + + $self->testException( + sub {$oPosix->owner($strFile, {strUser => 'root'})}, ERROR_FILE_OWNER, + "unable to set ownership for '${strFile}': Operation not permitted"); + $self->testException( + sub {$oPosix->owner($strFile, {strGroup => 'root'})}, ERROR_FILE_OWNER, + "unable to set ownership for '${strFile}': Operation not permitted"); + + executeTest("sudo chown :root ${strFile}"); + $self->testResult( + sub {$oPosix->owner($strFile, {strGroup => TEST_GROUP})}, undef, "change group back from root"); + } + ################################################################################################################################ if ($self->begin('pathCreate()')) {