Update MediaSourceService.php #930

DonRichards · 2023-02-16T15:56:55Z

I'm not sure if this is ideal or if it's just overwriting the existing check

islandora/src/MediaSource/MediaSourceService.php

Line 302 in 5585f89

    
           if (!$this->fileSystem->prepareDirectory($directory, FileSystemInterface::CREATE_DIRECTORY | FileSystemInterface::MODIFY_PERMISSIONS)) {

GitHub Issue: N/A

What does this Pull Request do?

When the media script tries to write to a directory (like public://) and the path is not set or has the wrong permissions, the error message could provide more informative information and be less cryptic.

What's new?

Checking if the directory is writeable by the PHP script.
If it doesn't have permissions then it outputs the username that the script is running as and the directory it's trying to write to in the thrown error.

How should this be tested?

# Change the permissions to not allow nginx to write a new directory to the public folders.
chown -R 1000:nginx web/sites/default/files
# Clear your logs
drush watchdog:delete all -y
drush cr

Trigger media script to generate derivative

Go to /admin/content and click the drop-down and select "Image - Generate a thumbnail from an original file" and in the list of content items check the box next to an object that has a model = Image

It really doesn't matter which derivative you generate. This just needs to create a file. You just create a new media or something. All that matters is that the media script is trying to write to the error logs when a file save/update fails due to permissions issues.

To check that it worked

 # Run this to see the error message or go to /admin/reports/dblog
drush watchdog:show --extended

# output should include " user 'xxxx' does not have permissions to write to: 'xxxx' .
.... Symfony\Component\HttpKernel\Exception\HttpException: The user 'nginx' does not have permission to write to: 'public:' . Error .....

In this example, the directory resolves to public:// instead of a real absolute path.

To reset back and test with the proper permissions to verify this doesn't trigger when it shouldn't

# clear the logs first
drush watchdog:delete all -y
# Change permissions back 
chown -R nginx:nginx web/sites/default/files
# You may need to reset your read/write permission
chmod -R 2775 web/sites/default/files
drush cr

An explanation for the above chmod command.

The 2 means that the group id will be preserved for any new files created in this directory. What that means is that nginx(or whatever your HTTP user is) will always be the group on any files, thereby ensuring that web server and the user will both always have write permissions to any new files that are placed in this directory. The first 7 means that the owner (example) can R (Read) W (Write) and X (Execute) any files in here. The second 7 means that group (www-data) can also R W and X any files in this directory. Finally, the 5 means that other users can R and X files, but not write.

Now run the instructions above on "Trigger media script to generate derivative" and then check the error logs. There should be no error message.

Documentation Status

Does this change existing behaviour that's currently documented? No
Does this change require new pages or sections of documentation? No
Who does this need to be documented for? N/A
Associated documentation pull request(s): 0 or documentation issue 0

Additional Notes:

N/A

Interested parties

@Islandora/committers

adam-vessey · 2023-02-16T16:03:18Z

src/MediaSource/MediaSourceService.php

+      // 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");
+      }


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?

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?

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".

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.

@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.

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.

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.

seth-shaw-asu · 2023-04-11T17:37:06Z

src/MediaSource/MediaSourceService.php

+        $error_current_user = shell_exec('whoami');
+        $sanitized_current_user = str_replace(["\n", "\r"], '', $error_current_user);
+        $error_message = "The destination directory does not exist, could not be created, or is not writable by $sanitized_current_user: $directory";
+        $this->logger->error($error_message);


This is throwing a null error:

[Tue Apr 11 17:28:11.009894 2023] [php:warn] [pid 36294] [client 127.0.0.1:42242] PHP Warning: Undefined property: Drupal\\islandora\\MediaSource\\MediaSourceService::$logger in /var/www/html/drupal/web/modules/contrib/islandora/src/MediaSource/MediaSourceService.php on line 298 [Tue Apr 11 17:28:11.014440 2023] [php:notice] [pid 36294] [client 127.0.0.1:42242] Error: Call to a member function error() on null in /var/www/html/drupal/web/modules/contrib/islandora/src/MediaSource/MediaSourceService.php on line 298 #0 /var/www/html/drupal/web/modules/contrib/islandora/src/Controller/MediaSourceController.php(168): Drupal\\islandora\\MediaSource\\MediaSourceService->putToNode(Object(Drupal\\node\\Entity\\Node), Object(Drupal\\media\\Entity\\MediaType), Object(Drupal\\taxonomy\\Entity\\Term), Resource id #24, 'image/jpeg', 'public://2023-0...')\n#1 [internal function]: Drupal\\islandora\\Controller\\MediaSourceController->putToNode(Object(Drupal\\node\\Entity\\Node), Object(Drupal\\media\\Entity\\MediaType), Object(Drupal\\taxonomy\\Entity\\Term), Object(Symfony\\Component\\HttpFoundation\\Request))\n#2 /var/www/html/drupal/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(123): call_user_func_array(Array, Array)\n#3 /var/www/html/drupal/web/core/lib/Drupal/Core/Render/Renderer.php(580): Drupal\\Core\\EventSubscriber\\EarlyRenderingControllerWrapperSubscriber->Drupal\\Core\\EventSubscriber\\{closure}()\n#4 /var/www/html/drupal/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(124): Drupal\\Core\\Render\\Renderer->executeInRenderContext(Object(Drupal\\Core\\Render\\RenderContext), Object(Closure))\n#5 /var/www/html/drupal/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(97): Drupal\\Core\\EventSubscriber\\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array)\n#6 /var/www/html/drupal/vendor/symfony/http-kernel/HttpKernel.php(169): Drupal\\Core\\EventSubscriber\\EarlyRenderingControllerWrapperSubscriber->Drupal\\Core\\EventSubscriber\\{closure}()\n#7 /var/www/html/drupal/vendor/symfony/http-kernel/HttpKernel.php(81): Symfony\\Component\\HttpKernel\\HttpKernel->handleRaw(Object(Symfony\\Component\\HttpFoundation\\Request), 1)\n#8 /var/www/html/drupal/web/core/lib/Drupal/Core/StackMiddleware/Session.php(58): Symfony\\Component\\HttpKernel\\HttpKernel->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)\n#9 /var/www/html/drupal/web/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php(48): Drupal\\Core\\StackMiddleware\\Session->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)\n#10 /var/www/html/drupal/web/core/modules/page_cache/src/StackMiddleware/PageCache.php(106): Drupal\\Core\\StackMiddleware\\KernelPreHandle->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)\n#11 /var/www/html/drupal/web/core/modules/page_cache/src/StackMiddleware/PageCache.php(85): Drupal\\page_cache\\StackMiddleware\\PageCache->pass(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)\n#12 /var/www/html/drupal/web/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(48): Drupal\\page_cache\\StackMiddleware\\PageCache->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)\n#13 /var/www/html/drupal/web/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(51): Drupal\\Core\\StackMiddleware\\ReverseProxyMiddleware->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)\n#14 /var/www/html/drupal/vendor/stack/builder/src/Stack/StackedHttpKernel.php(23): Drupal\\Core\\StackMiddleware\\NegotiationMiddleware->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)\n#15 /var/www/html/drupal/web/core/lib/Drupal/Core/DrupalKernel.php(713): Stack\\StackedHttpKernel->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)\n#16 /var/www/html/drupal/web/index.php(19): Drupal\\Core\\DrupalKernel->handle(Object(Symfony\\Component\\HttpFoundation\\Request))\n#17 {main}

Also, despite this not working, the file module is already giving me an error about file permissions for the folder in question: "The file permissions could not be set on public://2023-04."

You need to setup a private variable of $logger and then add an argument of type LoggerInterface to the __constructor and then add @logger.channel.islandora to the array of arguments in the islandora.services.yml

Also, $parent_directory is never referenced and I'm not sure why we need to go through the work to get the username. It should be the user running your webserver, which you should know or be able to check with a ps -ef | grep (nginx|httpd|apache2)

xurizaemon · 2024-10-07T02:39:38Z

It seems like this issue relates to handling filesystem restrictions with more clarity. (It would be helpful to give this PR a meaningful subject line, rather than naming the modified file; that could better communicate intent.)

There's no existing usage of shell_exec in the Islandora codebase that I can see, and it's a function which may be disabled for security reasons (PHP disable_functions). I don't think introducing its use here for an error message will add much.

The user that PHP runs as is unlikely to be "dynamic" - the important information to capture IMO is that the webserver/application was unable to create a directory in that location. (I see this is noted in #930 (comment) also.)

Update MediaSourceService.php

5585f89

adam-vessey reviewed Feb 16, 2023

View reviewed changes

DonRichards added 4 commits February 20, 2023 13:34

Moved into existing check

80391bc

Update MediaSourceService.php

9bd4e8c

Replace the watchdog output

c1ace5a

Fix phpcs errors

e4639be

wgilling self-requested a review March 1, 2023 18:16

seth-shaw-asu self-requested a review April 5, 2023 17:12

seth-shaw-asu requested changes Apr 11, 2023

View reviewed changes

xurizaemon mentioned this pull request Oct 7, 2024

[FEATURE] Improve handling of requests where Media lacks an associated File #1059

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update MediaSourceService.php #930

Update MediaSourceService.php #930

DonRichards commented Feb 16, 2023 •

edited

Loading

adam-vessey Feb 16, 2023

adam-vessey Feb 16, 2023

seth-shaw-asu Feb 16, 2023

seth-shaw-asu Feb 16, 2023

DonRichards Feb 20, 2023

DonRichards Feb 20, 2023

DonRichards Feb 20, 2023

seth-shaw-asu Apr 11, 2023

whikloj Apr 11, 2023

xurizaemon commented Oct 7, 2024 •

edited

Loading

Update MediaSourceService.php #930

Are you sure you want to change the base?

Update MediaSourceService.php #930

Conversation

DonRichards commented Feb 16, 2023 • edited Loading

What does this Pull Request do?

What's new?

How should this be tested?

Trigger media script to generate derivative

To check that it worked

Documentation Status

Additional Notes:

Interested parties

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xurizaemon commented Oct 7, 2024 • edited Loading

DonRichards commented Feb 16, 2023 •

edited

Loading

xurizaemon commented Oct 7, 2024 •

edited

Loading