Skip to content
This repository has been archived by the owner on Mar 15, 2020. It is now read-only.

Allow checking for updates with an underprivileged user. Resolve Issue #20 #21

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Allow checking for updates with an underprivileged user. Resolve Issue #20 #21

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a7596d9
Delay checking if local phar file is writable until it needs to be wr…
Mar 8, 2017
2be819f
delay checking if temp directory is writable until it needs to be wri…
Mar 8, 2017
850cccd
Merge branch 'master' into fix-update-checker
padraic May 2, 2017
d12a15a
Merge remote-tracking branch 'cannonical/master' into fix-update-checker
Jun 23, 2017
f557fac
Merge branch 'fix-update-checker' of github.com:jxn/phar-updater into…
Jun 23, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 38 additions & 13 deletions src/Updater.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,13 @@ protected function newVersionAvailable()

protected function backupPhar()
{
$tempDirectory = $this->getTempDirectory();
Copy link
Contributor

@aik099 aik099 Mar 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block is copy-pasted several times. Maybe it can be moved out to separate method?

I see that this is moved out from setTempDirectory method. Instead you can:

  1. move that check into getTempDirectory method
  2. use getTempDirectory method internally instead of accessing tempDirectory property directly

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concerning #1, I don't think that will work, because getTempDirectory() method is called in the update check process. That would cause the update check to fail in the same way that required the change (Issue #20 ).

Concerning #2, here in line 330 I am using the getTempDirectory() method as requested and assigning it to a local variable in the method's scope. I don't see a place where I am using the tempDirectory property directly, but please clarify if you see a line where that is done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concerning #1, I don't think that will work, because getTempDirectory() method is called in the update check process. That would cause the update check to fail in the same way that required the change (Issue #20 ).

I've tracked back every call of \Humbug\SelfUpdate\Updater::getTempDirectory and each of them is prefixed by exception throwing when path isn't writable in one way or another. So moving exception throwing into that method would reduce duplication and preserve logic introduced in this PR.

Concerning #2, here in line 330 I am using the getTempDirectory() method as requested and assigning it to a local variable in the method's scope. I don't see a place where I am using the tempDirectory property directly, but please clarify if you see a line where that is done.

I've double checked again and the place is gone. Probably something in the code was changed or I've written my comment theoretically (without looking at the code).

if (!is_writable($tempDirectory)) {
throw new FilesystemException(sprintf(
'The directory is not writeable: %s.', $tempDirectory
));
}

$result = copy($this->getLocalPharFile(), $this->getBackupPharFile());
if ($result === false) {
$this->cleanupAfterError();
Expand All @@ -346,6 +353,13 @@ protected function backupPhar()

protected function downloadPhar()
{
$tempDirectory = $this->getTempDirectory();
if (!is_writable($tempDirectory)) {
throw new FilesystemException(sprintf(
'The directory is not writeable: %s.', $tempDirectory
));
}

$this->strategy->download($this);

if (!file_exists($this->getTempPharFile())) {
Expand Down Expand Up @@ -386,19 +400,41 @@ protected function downloadPhar()

protected function replacePhar()
{
rename($this->getTempPharFile(), $this->getLocalPharFile());
$localPharFile = $this->getLocalPharFile();
if (!is_writable($localPharFile)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code block was moved out from setLocalPharFile and you can move it to getLocalPharFile method to avoid duplication.

Copy link
Author

@jxn jxn May 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getLocalPharFile() is also called by setTempDirectory(), which would cause this to fail unnecessarily during a check also. I understand wanting to reduce code duplication, but I think it also makes sense to have these exceptions associated with the code that requires writability, because it prevents these methods from being run without the possibility of running the check first. If you'd really like to avoid duplicating the exceptions, I would propose an interstitial method, like protected isDirectoryWritable($this->getTempDirectory); which just does the writability check and throws the exception. I decided not to do that originally because it actually creates more lines of code, but perhaps you'd prefer that to the current PR?

throw new FilesystemException(sprintf(
'The current phar file is not writeable and cannot be replaced: %s.',
$localPharFile
));
}
rename($this->getTempPharFile(), $localPharFile);
}

protected function restorePhar()
{
$tempDirectory = $this->getTempDirectory();
if (!is_writable($tempDirectory)) {
throw new FilesystemException(sprintf(
'The directory is not writeable: %s.', $tempDirectory
));
}

$backup = $this->getRestorePharFile();
if (!file_exists($backup)) {
throw new RuntimeException(sprintf(
'The backup file does not exist: %s.', $backup
));
}
$this->validatePhar($backup);
return rename($backup, $this->getLocalPharFile());

$localPharFile = $this->getLocalPharFile();
if (!is_writable($localPharFile)) {
throw new FilesystemException(sprintf(
'The current phar file is not writeable and cannot be replaced: %s.',
$localPharFile
));
}
return rename($backup, $localPharFile);
}

protected function getBackupPharFile()
Expand Down Expand Up @@ -441,12 +477,6 @@ protected function setLocalPharFile($localPharFile)
'The set phar file does not exist: %s.', $localPharFile
));
}
if (!is_writable($localPharFile)) {
throw new FilesystemException(sprintf(
'The current phar file is not writeable and cannot be replaced: %s.',
$localPharFile
));
}
$this->localPharFile = $localPharFile;
$this->localPharFileBasename = basename($localPharFile, '.phar');
}
Expand All @@ -465,11 +495,6 @@ protected function setLocalPubKeyFile()
protected function setTempDirectory()
{
$tempDirectory = dirname($this->getLocalPharFile());
if (!is_writable($tempDirectory)) {
throw new FilesystemException(sprintf(
'The directory is not writeable: %s.', $tempDirectory
));
}
$this->tempDirectory = $tempDirectory;
}

Expand Down