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.

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

Uploadlimit of course and site is not overriden by block #20

Closed
tobiasreischmann opened this issue Apr 4, 2018 · 10 comments
Closed

Uploadlimit of course and site is not overriden by block #20

tobiasreischmann opened this issue Apr 4, 2018 · 10 comments

Comments

@tobiasreischmann
Copy link
Contributor

The problem is that get_user_max_upload_file_size, which is called frequently within the implementation of filemanager, sets the max upload size to the minimum out of php, site, course and module limit. This way the block is not able to override the other limits in the moodle site.

@tobiasreischmann
Copy link
Contributor Author

The branch feature/uploadlimit contains a current implementation of a custom QuickFormElement, which extends the existing filemanager. Unfortunately, we needs to copy lots of code to insert one single line.

@Dagefoerde
Copy link
Contributor

Apache seems to hold uploaded files in memory until the upload completes, so at least maintaining that limit may be advisable (and blindly increasing it may not). Therefore it will become necessary to implement our own file manager with support for chunked uploads, each chunk being smaller than the upload limit. It could be based e.g. on http://www.resumablejs.com .

@tobiasreischmann
Copy link
Contributor Author

tobiasreischmann commented Apr 4, 2018

Or even better. Bring chunked uploads into core!! :)

@tobiasreischmann
Copy link
Contributor Author

tobiasreischmann commented Apr 4, 2018

The current branch makes also the following core changes necessary....I can not see any other way!

diff --git a/repository/repository_ajax.php b/repository/repository_ajax.php
--- a/repository/repository_ajax.php
+++ b/repository/repository_ajax.php
@@ -88,7 +88,14 @@ if (!empty($course)) {
     $coursemaxbytes = $course->maxbytes;
 }
 // Make sure maxbytes passed is within site filesize limits.
-$maxbytes = get_user_max_upload_file_size($context, $CFG->maxbytes, $coursemaxbytes, $maxbytes);
+if ($context instanceof context_block &&
+    $block = $DB->get_record('block_instances', array('id' => $context->instanceid)) &&
+    $block->blockname = 'block_opencast') {
+    $maxbytes = get_user_max_upload_file_size($context, -1, -1, $maxbytes);
+} else {
+    $maxbytes = get_user_max_upload_file_size($context, $CFG->maxbytes, $coursemaxbytes, $maxbytes);
+}
 
 // Wait as long as it takes for this script to finish
 core_php_time_limit::raise();

@synergy-wagner
Copy link

Hi Tobias,

You are right, it would need a core change to limit the upload size to specific values, but it is possible to unlimit the upload size in total:

Our solution:

  • created a role with suitable capability and assign this role in context of the block.
  • changed the page context of upload form page to the block context to make this work.

Kind regards
Andi

@t-schroeder
Copy link
Contributor

t-schroeder commented Sep 19, 2018

Our current workaround is adding this .htaccess file into the repository directory:

<If "%{HTTP_REFERER} == '%{REQUEST_SCHEME}://%{SERVER_NAME}/blocks/opencast/addvideo.php'">
php_value post_max_size 10G
php_value upload_max_filesize 10G
</If>

Not 100% secure because people can just send custom HTTP requests with that referrer to upload larger-than-allowed files in other places. But it's just a workaround and this way we don't have to change the core.

Edit:
Another reason for doing it this way is: We have 250M as the limit in out php.ini and if we increased that to 10G we'd have to restrict the upload size to 250M again in all places but the opencast video upload. But e.g. when we limit the uploaded files size in the course settings, this would effect files added to a course via drag-and-drop but not files added as a new "File" resource. So in order to ensure the 250M limit is kept everywhere except for the Opencast video upload we set it in the php.ini and only override it in the .htaccess for the opencast video upload.

@t-schroeder
Copy link
Contributor

I noticed that the solution of @synergy-wagner works only for blocks that are added to a course. However, you can also add a block to the site and configure it to only be displayed in course main pages. In that case the parent context of the block is not a course but the system. But the get_opencast_upload_context function doesn't account for that so it returns the course context instead of the block context.

I fixed that in #72.

@tobiasreischmann
Copy link
Contributor Author

I can see that both of your versions work.
However, the one of @synergy-wagner does not really scale well, if you have many courses, which use this feature, since you have to set these capabilities for every single block context instance.
The one of @do-you-even-curl completely disables the upload management of moodle, since you have to set the site and the course limit to very high amounts and they are then only restricted by the value in the php.ini .
My version is also not perfect, since a moodle core change is definitely not possible or advisable for everyone. However, if someone is interested the relevant code is in https://github.com/unirz-tu-ilmenau/moodle-block_opencast/tree/feature/uploadlimit and the necessary code changes are listed above.

In my point of view, the best way would be to integrate a way to allow plugins to exceed the site and course uploadlimits in core. But I can already see that the HQ might not be happy about that.

@abias
Copy link
Member

abias commented Jun 28, 2021

As this enhancement can't be implemented without changing Moodle core first and as https://github.com/Opencast-Moodle/moodle-local_chunkupload is now there to solve this issue, I am wondering if we should close this issue with a reference to https://github.com/Opencast-Moodle/moodle-local_chunkupload as the recommended solution.

@TamaraGunkel
Copy link
Contributor

Yes, I think we can do that. Chunkupload is well integrated with the block and provides a clean solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants