Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update MediaSourceService.php #930

Open
wants to merge 5 commits into
base: 2.x
Choose a base branch
from
Open

Update MediaSourceService.php #930

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5585f89
Update MediaSourceService.php
DonRichards Feb 16, 2023
80391bc
Moved into existing check
DonRichards Feb 20, 2023
9bd4e8c
Update MediaSourceService.php
DonRichards Feb 20, 2023
c1ace5a
Replace the watchdog output
DonRichards Feb 23, 2023
e4639be
Fix phpcs errors
DonRichards Feb 23, 2023
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
12 changes: 11 additions & 1 deletion src/MediaSource/MediaSourceService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,8 +289,18 @@ public function putToNode(
}

$directory = $this->fileSystem->dirname($content_location);

// Check if the directory is writable.
if (!is_writable(dirname($directory))) {
$parent_directory = dirname($directory);
$error_current_user = shell_exec('whoami');
$sanitized_current_user = str_replace(array("\n", "\r"), '', $error_current_user);
throw new HttpException(500, "The user '$sanitized_current_user' does not have permissions to write to: '$parent_directory' . Error");
}
Copy link

Choose a reason for hiding this comment

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

This is kind of contrary to the next lines, where a given (sub)directory might not yet exist, and the ->prepareDirectory() call is intended to create it, with the FileSystemInterface::CREATE_DIRECTORY bit?

Copy link

Choose a reason for hiding this comment

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

Additionally, if there's any chance of these messages being emitted to browsers, I'm not sure that it's particularly appropriate to include things like the names of OS-level users and the directory? Like... sure, if we want to write them to logs for the consumption of users privileged to read them, that's one thing, but displaying to general users, not so much?

Copy link
Member

Choose a reason for hiding this comment

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

I think @adam-vessey is right; although I wouldn't object to the whoami bit being moved into the following if statement so that 500 error could read something like "The destination directory does not exist, could not be created, or is not writable by $current_user: $directory".

Copy link
Member

Choose a reason for hiding this comment

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

Good point. Instead of adding this onto the thrown 500 exception, we could use \Drupal::logger('islandora')->warning() or whatever the dependency injection equivalent is.

Copy link
Member Author

Choose a reason for hiding this comment

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

@adam-vessey I can move stuff into the existing one. I don't believe these are (I haven't seen them show up in the browser). I wasn't sure how much info is too much but right now it's not enough (in my opinion). Giving the user the directory is actionable. Telling them the username is probably fine but most people use usernames like nginx or www-data which is kinda standard so I wasn't sure how significant that would be for having in the logs.

Copy link
Member Author

Choose a reason for hiding this comment

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

After thinking about it I do think the added value shouldn't be in a new if statement but included (if at all) in the existing one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oddly enough, that error only really fires when derivatives generation is needing to create a new folder. It may just be how I'm testing this. But I thought it would be worth noting.



if (!$this->fileSystem->prepareDirectory($directory, FileSystemInterface::CREATE_DIRECTORY | FileSystemInterface::MODIFY_PERMISSIONS)) {
throw new HttpException(500, "The destination directory does not exist, could not be created, or is not writable");
throw new HttpException(500, "The destination directory does not exist, could not be created, or is not writable: $directory");
}

// Copy over the file content.
Expand Down