Moodle
  1. Moodle
  2. MDL-27156

File System Repository files limited by PHP upload_max_filesize limits

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0.2
    • Fix Version/s: 2.3
    • Component/s: Repositories
    • Labels:
    • Environment:
      Ubuntu/10.10; PHP/5.3.3-1ubuntu9.3 with Suhosin-Patch; Apache/2.2.16
    • Testing Instructions:
      Hide

      Set these PHP settings in your php.in as follows: upload_max_filesize=2M, post_max_size=8M

      For this test you'll need a teacher and your admin user.

      Check your site's maximum uploaded file size. This is a site setting called "maxbytes". Set maxbytes to 2MB.

      Also set the course setting "Maximum upload size" to 2MB.

      You'll need a course backup. Check the file size and make sure that it is bigger than 10MB.

      As admin go to course admin > users > permissions > check permissions and check "moodle/course:ignorefilesizelimits" for a teacher. The teacher should NOT have moodle/course:ignorefilesizelimits.

      File system repository set up
      1. create a directory at moodledata/repository/examplerepository then set up a file system repository using that directory by going to: Site administration > Plugins > Repositories > Manage repositories > File System > Settings > Create a Repository Instance
      2. Outside of Moodle copy your course backup into the repository directory.

      As a teacher go into a course and do a restore into a new course. Select the course backup from the file repo.
      You should get an error about the file being too big.

      As admin give teacher moodle/course:ignorefilesizelimits in the course.
      Teacher should now be able to select the backup file and proceed with the restore.

      Additional testing.

      As any user but admin go to My private files.
      You should see something like "Maximum size for newfiles: 2MB - drag and drop available" near the top right of the file manager.

      As admin you should see something like "Maximum size for new files: -1 bytes - drag and drop available"
      As admin drag the course backup into your private files. You should get an error message about PHP.
      As admin click "Add..." and add add the course backup from your file repository. It should succeed.

      Show
      Set these PHP settings in your php.in as follows: upload_max_filesize=2M, post_max_size=8M For this test you'll need a teacher and your admin user. Check your site's maximum uploaded file size. This is a site setting called "maxbytes". Set maxbytes to 2MB. Also set the course setting "Maximum upload size" to 2MB. You'll need a course backup. Check the file size and make sure that it is bigger than 10MB. As admin go to course admin > users > permissions > check permissions and check "moodle/course:ignorefilesizelimits" for a teacher. The teacher should NOT have moodle/course:ignorefilesizelimits. File system repository set up 1. create a directory at moodledata/repository/examplerepository then set up a file system repository using that directory by going to: Site administration > Plugins > Repositories > Manage repositories > File System > Settings > Create a Repository Instance 2. Outside of Moodle copy your course backup into the repository directory. As a teacher go into a course and do a restore into a new course. Select the course backup from the file repo. You should get an error about the file being too big. As admin give teacher moodle/course:ignorefilesizelimits in the course. Teacher should now be able to select the backup file and proceed with the restore. Additional testing. As any user but admin go to My private files. You should see something like "Maximum size for newfiles: 2MB - drag and drop available" near the top right of the file manager. As admin you should see something like "Maximum size for new files: -1 bytes - drag and drop available" As admin drag the course backup into your private files. You should get an error message about PHP. As admin click "Add..." and add add the course backup from your file repository. It should succeed.
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-27156_part2
    • Rank:
      16809

      Description

      Files in the file system repository should not be limited by the PHP max file size.

      Steps:

      1. Setup file system repository in the moodledata/repository/examplerepository and enable the folder in: Site administration > Plugins > Repositories > Manage repositories > File System > Settings > Create a Repository Instance
      2. Copy a file to the repository using the file system that is larger than the PHP upload_max_file_size setting
      3. Create a new Page in a Course
      4. Attempt to link to the file from the Page using the File Picker

      Expected result:

      • Link to the file from the file system repository is created in the page

      Actual result:

      • File is apparently bigger than max file size

      This problem has been discussed in the forum, with workarounds requiring access to either .htaccess, php.ini, or a non-moodle file hosting site. http://moodle.org/mod/forum/discuss.php?d=166497 http://moodle.org/mod/forum/discuss.php?d=158886

      In my testing, the PHP max file size upload is 2MB and the PHP post max size is 8MB (ie., upload_max_filesize=2M and the post_max_size=8M)

      Any files larger than 2MB fail to be included from the file system repository, even though they are already on the file system and shouldn't in principle be limited to the PHP upload or post limits.

      The applicable lines in code are:

      repository/filepicker.php;96:
      $moodle_maxbytes = get_max_upload_file_size();
      repository/repository_ajax.php;86:
      $moodle_maxbytes = get_max_upload_file_size();

      If the user interface repository file pickers do not assume that all file transfers need to go through the PHP upload routine, then the file system repository links should succeed regardless of upload_max_filesize.

        Issue Links

          Activity

          Hide
          Helen Foster added a comment -

          Peter, thanks for your report with detailed description and links to forum discussions with workarounds.

          Increasing priority and hoping this issue can be looked into soon.

          Show
          Helen Foster added a comment - Peter, thanks for your report with detailed description and links to forum discussions with workarounds. Increasing priority and hoping this issue can be looked into soon.
          Hide
          Guillermo M. added a comment -

          Backup files bigger than the PHP upload_max_file_size are actually processed. So not only the .mbz file is read and uncompressed, but the contained files are also processed, many of which exceed the PHP file size limitation.

          Show
          Guillermo M. added a comment - Backup files bigger than the PHP upload_max_file_size are actually processed. So not only the .mbz file is read and uncompressed, but the contained files are also processed, many of which exceed the PHP file size limitation.
          Hide
          George Chen added a comment -

          I have experienced the same issue while tried to add a PPT file (96 MB) that was in the file repository on the server into the course and received the error "This file is bigger than the maximum size".

          Show
          George Chen added a comment - I have experienced the same issue while tried to add a PPT file (96 MB) that was in the file repository on the server into the course and received the error "This file is bigger than the maximum size".
          Hide
          Joseph Jacelone added a comment -

          I second the comment made by George. This issue is currently affecting multiple clients of ours and would love to see a resolution to this issue. The new file picker system could be a even greater asset to Moodle users should some of these quirks be worked out.

          Show
          Joseph Jacelone added a comment - I second the comment made by George. This issue is currently affecting multiple clients of ours and would love to see a resolution to this issue. The new file picker system could be a even greater asset to Moodle users should some of these quirks be worked out.
          Hide
          Martin Dougiamas added a comment -

          Can this be prioritised for the STABLE team please?

          Show
          Martin Dougiamas added a comment - Can this be prioritised for the STABLE team please?
          Hide
          Joseph Jacelone added a comment -

          Martin to the rescue!! Thank you!!

          Show
          Joseph Jacelone added a comment - Martin to the rescue!! Thank you!!
          Hide
          George Chen added a comment -

          Thanks Martin for your quick response!

          Show
          George Chen added a comment - Thanks Martin for your quick response!
          Hide
          George Chen added a comment -

          in my case, it affected our clients on Moodle 2.1.

          Show
          George Chen added a comment - in my case, it affected our clients on Moodle 2.1.
          Hide
          Andrew Davis added a comment -

          Just recording some notes on how this area of Moodle works

          I've raised MDL-30792 which I stumbled on while exploring this area of Moodle. Its a security issue so you may not be able to see it.

          Assuming ajax is enabled repository/filepicker.js sends an ajax request to /repository/repository_ajax.php Within repository_ajax.php the lines actually generating the error are...

          // check if exceed maxbytes
          if (($maxbytes!==-1) && (filesize($file['path']) > $maxbytes)) {
              throw new file_exception('maxbytes');
          }
          

          so if repository_ajax.php asked the repository it could potentially set the value of $maxbytes to -1.

          This could be done in one of a few ways.

          We can use repository::static_function to call a function on the repo to ask for whether Moodle file size limits should be applied. Something like

          $applysizelimits = repository::static_function($this->plugin, 'get_apply_file_size_limits');
          

          Or we could use $repo->get_option(). get_option() retrieves repository options from the repository_instance_config table. The file repo could put an option in there when its created.

          I'm not terribly familiar with this area of Moodle so I will record more as I learn more.

          Show
          Andrew Davis added a comment - Just recording some notes on how this area of Moodle works I've raised MDL-30792 which I stumbled on while exploring this area of Moodle. Its a security issue so you may not be able to see it. Assuming ajax is enabled repository/filepicker.js sends an ajax request to /repository/repository_ajax.php Within repository_ajax.php the lines actually generating the error are... // check if exceed maxbytes if (($maxbytes!==-1) && (filesize($file['path']) > $maxbytes)) { throw new file_exception('maxbytes'); } so if repository_ajax.php asked the repository it could potentially set the value of $maxbytes to -1. This could be done in one of a few ways. We can use repository::static_function to call a function on the repo to ask for whether Moodle file size limits should be applied. Something like $applysizelimits = repository::static_function($ this ->plugin, 'get_apply_file_size_limits'); Or we could use $repo->get_option(). get_option() retrieves repository options from the repository_instance_config table. The file repo could put an option in there when its created. I'm not terribly familiar with this area of Moodle so I will record more as I learn more.
          Hide
          Martin Dougiamas added a comment -

          Without looking at code I don't know which method is best, but it's definitely a good idea to ask the repository.

          I would actually DEFAULT to having this limit disabled for all plugins, as I think only the "upload" repository actually should have this limit.

          Show
          Martin Dougiamas added a comment - Without looking at code I don't know which method is best, but it's definitely a good idea to ask the repository. I would actually DEFAULT to having this limit disabled for all plugins, as I think only the "upload" repository actually should have this limit.
          Hide
          Andrew Davis added a comment -

          I'm not certain this is correct but I have added a branch that contains a solution that at least appears to work at first glance.

          Show
          Andrew Davis added a comment - I'm not certain this is correct but I have added a branch that contains a solution that at least appears to work at first glance.
          Hide
          Peter Ansell added a comment -

          Do you need to configure the new option for the file system repository plugin in or around repository/filesystem/lib.php?

          Also, do you need to add the new code to repository/filepicker.php around line 96 to avoid the maxbytes check there?

          Show
          Peter Ansell added a comment - Do you need to configure the new option for the file system repository plugin in or around repository/filesystem/lib.php? Also, do you need to add the new code to repository/filepicker.php around line 96 to avoid the maxbytes check there?
          Hide
          Dan Poltawski added a comment -

          Hmm, I think this and MDL-30792 are closely related. Gonna comment in the other bug.

          Show
          Dan Poltawski added a comment - Hmm, I think this and MDL-30792 are closely related. Gonna comment in the other bug.
          Hide
          Dan Poltawski added a comment -

          In fact, it looks like we already do check the filesize on upload in the upload repository, so I wonder if this functionally can be taken out of the repository ajax script wholesale.

          Show
          Dan Poltawski added a comment - In fact, it looks like we already do check the filesize on upload in the upload repository, so I wonder if this functionally can be taken out of the repository ajax script wholesale.
          Hide
          Dan Poltawski added a comment - - edited

          Hmm thinking about it more can disable filesize checking for all plugins?

          Use case where I think site admins might want to be able to restrict filesize: A forum thread where students can upload 1GB zip of mp3 files frm their dropbox account..

          Show
          Dan Poltawski added a comment - - edited Hmm thinking about it more can disable filesize checking for all plugins? Use case where I think site admins might want to be able to restrict filesize: A forum thread where students can upload 1GB zip of mp3 files frm their dropbox account..
          Hide
          Andrew Davis added a comment -

          Peter, no. The new option I have proposed is actually in the upload repository. My change says "dont do size checks unless I specifically say so" and only the upload repository says so.

          And if Im looking at the piece of code you mean when you say line 96 (could have been reshuffled since your comment) that is just calculating the max file size. I can probably do something to avoid doing that if its not necessary but it does no real harm.

          Dan, this check in repository_ajax.php (around line 62) is interesting.

          // if uploaded file is larger than post_max_size (php.ini) setting, $_POST content will lost
          if (empty($_POST) && !empty($action)) {
              $err->error = get_string('errorpostmaxsize', 'repository');
              die(json_encode($err));
          }
          

          I commented out the other ajaxy size check and then uploaded a large video and the above code executed and threw the error. I need to do some more experimenting but if the ajax size check occurs before the upload actually begins and the code above only executes once the upload is under way then we should probably retain the ajax size checks (where relevant). Im just worried about the case where someone is uploading a large video over a slow net connection. It would be better to tell them up front "this file is too big" rather than waiting for them to upload a few MB, hit a size limit then be told.

          Show
          Andrew Davis added a comment - Peter, no. The new option I have proposed is actually in the upload repository. My change says "dont do size checks unless I specifically say so" and only the upload repository says so. And if Im looking at the piece of code you mean when you say line 96 (could have been reshuffled since your comment) that is just calculating the max file size. I can probably do something to avoid doing that if its not necessary but it does no real harm. Dan, this check in repository_ajax.php (around line 62) is interesting. // if uploaded file is larger than post_max_size (php.ini) setting, $_POST content will lost if (empty($_POST) && !empty($action)) { $err->error = get_string('errorpostmaxsize', 'repository'); die(json_encode($err)); } I commented out the other ajaxy size check and then uploaded a large video and the above code executed and threw the error. I need to do some more experimenting but if the ajax size check occurs before the upload actually begins and the code above only executes once the upload is under way then we should probably retain the ajax size checks (where relevant). Im just worried about the case where someone is uploading a large video over a slow net connection. It would be better to tell them up front "this file is too big" rather than waiting for them to upload a few MB, hit a size limit then be told.
          Hide
          Andrew Davis added a comment -

          Im not sure if I'm learning or just confusing myself.

          When you upload a file (using the upload repo) filepicker.js calls repository_ajax.php with action == upload.

          When you link to a file in a file system repo filepicker.js calls repository_ajax.php with action == download. It is quite possible that the file size check within the 'download' branch of the switch is entirely unnecessary. This code:

          // check if exceed maxbytes
          if (($maxbytes!==-1) && (filesize($file['path']) > $maxbytes)) {
              throw new file_exception('maxbytes');
          }
          

          At this time I dont know whether to simply strip out that check or to go with the idea of adding a repository option that makes that check optional.

          Show
          Andrew Davis added a comment - Im not sure if I'm learning or just confusing myself. When you upload a file (using the upload repo) filepicker.js calls repository_ajax.php with action == upload. When you link to a file in a file system repo filepicker.js calls repository_ajax.php with action == download. It is quite possible that the file size check within the 'download' branch of the switch is entirely unnecessary. This code: // check if exceed maxbytes if (($maxbytes!==-1) && (filesize($file['path']) > $maxbytes)) { throw new file_exception('maxbytes'); } At this time I dont know whether to simply strip out that check or to go with the idea of adding a repository option that makes that check optional.
          Hide
          Andrew Davis added a comment -

          Ok, here are branches containing the two proposed solutions.

          1) Adding an option to the repositories (actually only the upload repository) that demands strict enforcement of file size limits in the "download" branch of repository_ajax.php
          https://github.com/andyjdavis/moodle/compare/master...MDL-27156_file_limitation

          2) Simply removing the file size check in the "download" branch of repository_ajax.php as the upload repository upload actually occurs in the "upload" branch of repository_ajax.php so the check seems, afaik, fairly pointless.
          https://github.com/andyjdavis/moodle/compare/master...MDL-27156_remove_file_size_check

          Show
          Andrew Davis added a comment - Ok, here are branches containing the two proposed solutions. 1) Adding an option to the repositories (actually only the upload repository) that demands strict enforcement of file size limits in the "download" branch of repository_ajax.php https://github.com/andyjdavis/moodle/compare/master...MDL-27156_file_limitation 2) Simply removing the file size check in the "download" branch of repository_ajax.php as the upload repository upload actually occurs in the "upload" branch of repository_ajax.php so the check seems, afaik, fairly pointless. https://github.com/andyjdavis/moodle/compare/master...MDL-27156_remove_file_size_check
          Hide
          Jason Fowler added a comment -

          The code in both branches makes sense, and looks clean. It might be worthwhile getting Eloy or Sam to give advice on which one to use

          Show
          Jason Fowler added a comment - The code in both branches makes sense, and looks clean. It might be worthwhile getting Eloy or Sam to give advice on which one to use
          Hide
          Sam Hemelryk added a comment -

          Hi Andrew,

          I've just been looking at the two options now.
          First up I'm seriously no repository expert, I've never really looked to deep into its code. Likely Dongsheng is the best person to ask.
          However I have had a good look into things because it is always interesting to learn something new.
          I started by looking at option 2 as it seemed the most straight forward.

          During my review/research I looked at how the filesystem, youtube and dropbox repo's worked in regards to repository_ajax and the download action.
          For both the filesystem and dropbox repos when using the editor and selecting an image to embed the image is copied into the draft files area (in the case of dropbox downloaded) and then when saving copied into the appropriate area. In the case of youtube it is linked as expected and nothing copied.
          In thinking about it there is no PHP conf limitation to the size of the file for either the dropbox or filesystem repositories, there is only the limit we impose.
          So my initial thoughts are that removing the check is fine.

          Next I looked at the first set of changes, which is of course just an option to avoid the check. I'm not entirely sure about this option. I tested the upload option with the editor in the same way as the dropbox and filesystem repo's however the upload doesn't execute the download action (which makes sense when you type it out).
          I didn't look more into this path either as too be truthful I'm not partial to it. The option to me doesn't feel necessary at the moment and I feel it is only adding clutter.

          So at the moment I prefer the patch that simply removes the check.
          HOWEVER I wonder whether there is a need for an option here. The repo's I looked at would not be limited by php conf such as upload_max_size or post_max_size, and for dropbox which uses curl well curl doesn't have a max size php option so theres no conf to limit it there either, however perhaps there are other means of `downloading` files onto the server that would be limited. I wonder whether we should give the repository an option to specify the maximum download size. Perhaps at the moment there is no need however. I do wonder still why that check was there in the first place.

          In summary my +1 goes to removing the check pending Dongsheng or someone knowledgeable in all repositories confirming that there are no repositories using mechanisms other than curl or local file transfer to `download` files and that if there are that they won't have a limit imposed through other settings.
          Hopefully that all makes sense.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Andrew, I've just been looking at the two options now. First up I'm seriously no repository expert, I've never really looked to deep into its code. Likely Dongsheng is the best person to ask. However I have had a good look into things because it is always interesting to learn something new. I started by looking at option 2 as it seemed the most straight forward. During my review/research I looked at how the filesystem, youtube and dropbox repo's worked in regards to repository_ajax and the download action. For both the filesystem and dropbox repos when using the editor and selecting an image to embed the image is copied into the draft files area (in the case of dropbox downloaded) and then when saving copied into the appropriate area. In the case of youtube it is linked as expected and nothing copied. In thinking about it there is no PHP conf limitation to the size of the file for either the dropbox or filesystem repositories, there is only the limit we impose. So my initial thoughts are that removing the check is fine. Next I looked at the first set of changes, which is of course just an option to avoid the check. I'm not entirely sure about this option. I tested the upload option with the editor in the same way as the dropbox and filesystem repo's however the upload doesn't execute the download action (which makes sense when you type it out). I didn't look more into this path either as too be truthful I'm not partial to it. The option to me doesn't feel necessary at the moment and I feel it is only adding clutter. So at the moment I prefer the patch that simply removes the check. HOWEVER I wonder whether there is a need for an option here. The repo's I looked at would not be limited by php conf such as upload_max_size or post_max_size, and for dropbox which uses curl well curl doesn't have a max size php option so theres no conf to limit it there either, however perhaps there are other means of `downloading` files onto the server that would be limited. I wonder whether we should give the repository an option to specify the maximum download size. Perhaps at the moment there is no need however. I do wonder still why that check was there in the first place. In summary my +1 goes to removing the check pending Dongsheng or someone knowledgeable in all repositories confirming that there are no repositories using mechanisms other than curl or local file transfer to `download` files and that if there are that they won't have a limit imposed through other settings. Hopefully that all makes sense. Cheers Sam
          Hide
          Andrew Davis added a comment -

          Thanks Sam. Ive updated the git urls and added testing instructions. Im putting this up for integration pending feedback from Dongsheng.

          Show
          Andrew Davis added a comment - Thanks Sam. Ive updated the git urls and added testing instructions. Im putting this up for integration pending feedback from Dongsheng.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Uhm,

          I don't understand this completely...

          1) There are still 'maxbytes' occurrences in the /repository directory (apart from the upload one).

          2) Offtopic: Does drag and drop observe the limits (aka is considered "upload").

          3) What happens with activity restrictions? Say forum only allows 200K attachments... with the patch that setting will be ignored completely if I'm not wrong.

          4) Reflexion: While I don't like restrictions... if they are there, they must be observed. If somebody puts a 2GB file in the filesystem repo, or if somebody has one 3GB file in dropbox... their size should be checked. 100% sure. No problem at all with links, of course, but any file "loaded" (added) should observe the restriction.

          5) And of course, we must not lose activity restrictions at all, no matter of all the rest. They are part of the learning experience and can be good reasons for having that restricted (from a teacher pov).

          So... I'm going to reopen this. This needs some more thinking IMO...

          Show
          Eloy Lafuente (stronk7) added a comment - Uhm, I don't understand this completely... 1) There are still 'maxbytes' occurrences in the /repository directory (apart from the upload one). 2) Offtopic: Does drag and drop observe the limits (aka is considered "upload"). 3) What happens with activity restrictions? Say forum only allows 200K attachments... with the patch that setting will be ignored completely if I'm not wrong. 4) Reflexion: While I don't like restrictions... if they are there, they must be observed. If somebody puts a 2GB file in the filesystem repo, or if somebody has one 3GB file in dropbox... their size should be checked. 100% sure. No problem at all with links, of course, but any file "loaded" (added) should observe the restriction. 5) And of course, we must not lose activity restrictions at all, no matter of all the rest. They are part of the learning experience and can be good reasons for having that restricted (from a teacher pov). So... I'm going to reopen this. This needs some more thinking IMO...
          Hide
          Andrew Davis added a comment -

          Some responses.

          2) drag and drop appears to implement the activity file size restriction through some other mechanism. This change does not break that mechanism.

          3) The change does however give users a way around activity file size restrictions. You can have a much larger file in a file system repo and the user can use it for an upload assignment that has a file size check that it should fail

          Show
          Andrew Davis added a comment - Some responses. 2) drag and drop appears to implement the activity file size restriction through some other mechanism. This change does not break that mechanism. 3) The change does however give users a way around activity file size restrictions. You can have a much larger file in a file system repo and the user can use it for an upload assignment that has a file size check that it should fail
          Hide
          David Mudrak added a comment -

          Students having their assignments uploaded to the file system prior to submitting them via web? Let's admit its very minor use-case, if ever. On contrary to what MD states above, I see all plugin controlled by upload_max_filesize. Note there is no conceptual difference between, say, Upload repo and URL Downloader repo. I mean, there would be no point to have Upload restricted and leave URL Downloader unrestricted (as there is an obvious way to bypass the restriction). So having the upload_max_filesize check disabled in the File system repository should be implemented, said that it is an exception - as we assume only admins having access to the file system so things are still under their control.

          The "IMHO" disclaimer applies as usually.

          Show
          David Mudrak added a comment - Students having their assignments uploaded to the file system prior to submitting them via web? Let's admit its very minor use-case, if ever. On contrary to what MD states above, I see all plugin controlled by upload_max_filesize. Note there is no conceptual difference between, say, Upload repo and URL Downloader repo. I mean, there would be no point to have Upload restricted and leave URL Downloader unrestricted (as there is an obvious way to bypass the restriction). So having the upload_max_filesize check disabled in the File system repository should be implemented, said that it is an exception - as we assume only admins having access to the file system so things are still under their control. The "IMHO" disclaimer applies as usually.
          Hide
          Andrew Davis added a comment -

          Maybe we're coming back around to an options that came up earlier. From above...

          1) Adding an option to the repositories (actually only the upload repository) that demands strict enforcement of file size limits in the "download" branch of repository_ajax.php
          https://github.com/andyjdavis/moodle/compare/master...MDL-27156_file_limitation

          Show
          Andrew Davis added a comment - Maybe we're coming back around to an options that came up earlier. From above... 1) Adding an option to the repositories (actually only the upload repository) that demands strict enforcement of file size limits in the "download" branch of repository_ajax.php https://github.com/andyjdavis/moodle/compare/master...MDL-27156_file_limitation
          Hide
          Andrew Davis added a comment -

          Ill be back shortly for a few more hours but will then be away for a week. I've rebased https://github.com/andyjdavis/moodle/compare/master...MDL-27156_file_limitation and updated the diff url just in case its decided that that is a viable decision while I am away.

          Show
          Andrew Davis added a comment - Ill be back shortly for a few more hours but will then be away for a week. I've rebased https://github.com/andyjdavis/moodle/compare/master...MDL-27156_file_limitation and updated the diff url just in case its decided that that is a viable decision while I am away.
          Hide
          Andrew Davis added a comment -

          As I'm going to be away for a week I'm putting this up for integration just to make sure it gets looked at. If anyone wants to take this issue over, modify my code or whatever, feel free to have a go

          Show
          Andrew Davis added a comment - As I'm going to be away for a week I'm putting this up for integration just to make sure it gets looked at. If anyone wants to take this issue over, modify my code or whatever, feel free to have a go
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Some hours ago...

          the main moodle.git repository has been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Some hours ago... the main moodle.git repository has been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

          This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

          This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

          Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Dan Poltawski added a comment -

          Hi,

          I think what we have come around to - the filesystem repository the only one being removed from this restriction by default makes sense - however maybe this should be an option that is visible to administrators so that they can override it.

          If we are not making this configurable then I don't think that using this $options array is the right way to do it (as a user configurable option could potentially override it in third party plugin). I think it would be preferable to introduce a function definition observe_filesize_limits() or something like that where this would be statically defined in the API much like supported_returntypes().

          Anyway I am interested on opinions on this.

          Show
          Dan Poltawski added a comment - Hi, I think what we have come around to - the filesystem repository the only one being removed from this restriction by default makes sense - however maybe this should be an option that is visible to administrators so that they can override it. If we are not making this configurable then I don't think that using this $options array is the right way to do it (as a user configurable option could potentially override it in third party plugin). I think it would be preferable to introduce a function definition observe_filesize_limits() or something like that where this would be statically defined in the API much like supported_returntypes(). Anyway I am interested on opinions on this.
          Hide
          Martin Dougiamas added a comment - - edited

          There are several things getting confused here into one.

          Currently we have:

          1. HTTP: The upload limits imposed by PHP/Apache.
          2. SERVER: The upload limit imposed by the admin. Must be < HTTP.
          3. ACTIVITY: The final limits imposed by activities (and courses). Must be < SERVER.

          For upload scenario this always made sense and the calculations work.

          What I think we need to do is

          A. Decouple HTTP from SERVER/ACTIVITY.

          1. HTTP: The upload limits imposed by PHP/Apache.
          2. SERVER: The upload limit imposed by the admin. Can be anything!
          3. ACTIVITY: The final limits imposed by activities (and courses). Must be < SERVER.

          B. Change the calculations so that:

          1. get_max_upload_file_size() IGNORES HTTP. ACTIVITY could be bigger than HTTP. All repositories must respect this limit as they do now.
          2. Upload plugin (only) uses maxsize is the minimum of HTTP and ACTIVITY, and prints that info somewhere near the upload form.
          Show
          Martin Dougiamas added a comment - - edited There are several things getting confused here into one. Currently we have: HTTP: The upload limits imposed by PHP/Apache. SERVER: The upload limit imposed by the admin. Must be < HTTP. ACTIVITY: The final limits imposed by activities (and courses). Must be < SERVER. For upload scenario this always made sense and the calculations work. What I think we need to do is A. Decouple HTTP from SERVER/ACTIVITY. HTTP: The upload limits imposed by PHP/Apache. SERVER: The upload limit imposed by the admin. Can be anything! ACTIVITY: The final limits imposed by activities (and courses). Must be < SERVER. B. Change the calculations so that: get_max_upload_file_size() IGNORES HTTP. ACTIVITY could be bigger than HTTP. All repositories must respect this limit as they do now. Upload plugin (only) uses maxsize is the minimum of HTTP and ACTIVITY, and prints that info somewhere near the upload form.
          Hide
          Dan Poltawski added a comment -

          It seems this needs more discussion to find the right solution so i'm reopening this issue again.

          Show
          Dan Poltawski added a comment - It seems this needs more discussion to find the right solution so i'm reopening this issue again.
          Hide
          Peter Ansell added a comment - - edited

          The use case for file system repository in my impression, is to allow arbitrary sized files if necessary for administrators. In the calculations above it looks like SERVER is not clearly used in this way.

          In the case of the file system repository, the SERVER limit would be set by administrators based on their needs for the largest video or powerpoint that they will ever need to publish. They would then have the impression that they need to set ACTIVITY to limit users. However, they would actually need to set ACTIVITY to be just as large as SERVER to be useful, as "All repositories must respect this limit as they do now" includes the file system repository. Hence they would be limiting their use of the file system repository by limiting users with ACTIVITY, which was the point this bug was trying to fix. To obtain their desired behaviour they would infact need to rely on HTTP to limit users, as that is the only limit that will not limit the file system repository.

          I don't understand why the "All repositories must respect this limit as they do now" is so important and why the algorithm must use ACTIVITY instead of SERVER in get_max_upload_file_size(). If get_max_upload_file_size() used SERVER the calculations above would be closer to a naive impression of the three limits in my opinion.

          Show
          Peter Ansell added a comment - - edited The use case for file system repository in my impression, is to allow arbitrary sized files if necessary for administrators. In the calculations above it looks like SERVER is not clearly used in this way. In the case of the file system repository, the SERVER limit would be set by administrators based on their needs for the largest video or powerpoint that they will ever need to publish. They would then have the impression that they need to set ACTIVITY to limit users. However, they would actually need to set ACTIVITY to be just as large as SERVER to be useful, as "All repositories must respect this limit as they do now" includes the file system repository. Hence they would be limiting their use of the file system repository by limiting users with ACTIVITY, which was the point this bug was trying to fix. To obtain their desired behaviour they would infact need to rely on HTTP to limit users, as that is the only limit that will not limit the file system repository. I don't understand why the "All repositories must respect this limit as they do now" is so important and why the algorithm must use ACTIVITY instead of SERVER in get_max_upload_file_size(). If get_max_upload_file_size() used SERVER the calculations above would be closer to a naive impression of the three limits in my opinion.
          Hide
          Martin Dougiamas added a comment - - edited

          Peter,

          Activities can able to set their own limits for educational reasons, and this is really important.

          eg If a teacher says they want an assignment submission to be an image under 1 Mb, then that limit must be enforced.

          eg If a site like moodle.org sets global limits of 100k attachments to keep the overall storage/size requirements down, and to encourage people to use links instead, then it should apply everywhere

          People should not be able to get around these limitations by uploading via Dropbox. Nor should certain repositories have special exclusions from restrictions. Thus all repositories should obey the limits.

          I do hear what you are saying, but we need a consistent design that is clear to understand (by developers and users).

          There are at least two ways we can solve the particular use case you mention (that the File System repo should not have any size restrictions).

          1. Add a override filesize limit setting for each repository which, if set, trumps all other ACTIVITY settings. The more I think about this the messier it seems, in that file size decisions need to know and check the repository settings, and different repositories will work inconsistently for the same person, etc. And we'll have people disabling Dropbox just because of the way it could be used to bypass restrictions etc etc ... It's confusing.
          2. My preferred way would be to simply add a new capability like "Immune to file size limits". This could be given to certain roles by the admin and would allow those people to bypass any limits. It can easily be checked in get_max_upload_file_size() and elsewhere, and keeps capabilities where it belongs, in the capabilities system. My +10 for this.
          Show
          Martin Dougiamas added a comment - - edited Peter, Activities can able to set their own limits for educational reasons, and this is really important. eg If a teacher says they want an assignment submission to be an image under 1 Mb, then that limit must be enforced. eg If a site like moodle.org sets global limits of 100k attachments to keep the overall storage/size requirements down, and to encourage people to use links instead, then it should apply everywhere People should not be able to get around these limitations by uploading via Dropbox. Nor should certain repositories have special exclusions from restrictions. Thus all repositories should obey the limits. I do hear what you are saying, but we need a consistent design that is clear to understand (by developers and users). There are at least two ways we can solve the particular use case you mention (that the File System repo should not have any size restrictions). Add a override filesize limit setting for each repository which, if set, trumps all other ACTIVITY settings. The more I think about this the messier it seems, in that file size decisions need to know and check the repository settings, and different repositories will work inconsistently for the same person, etc. And we'll have people disabling Dropbox just because of the way it could be used to bypass restrictions etc etc ... It's confusing. My preferred way would be to simply add a new capability like "Immune to file size limits". This could be given to certain roles by the admin and would allow those people to bypass any limits. It can easily be checked in get_max_upload_file_size() and elsewhere, and keeps capabilities where it belongs, in the capabilities system. My +10 for this.
          Hide
          Andrew Davis added a comment - - edited

          Updating the branch info. https://github.com/andyjdavis/moodle/compare/master...MDL-27156_file_cap

          This doesnt work quite how it should. The global $COURSE hasnt been initiated correctly so $COURSE->id is always 1 (system). This makes it impossible to grant the user the new capability within a single course. I'm not sure how to remedy this just yet. If anyone else knows feel free to jump in. I have emailed Michael and Dongsheng about this as Im a bit stuck at this point.

          I was worried that exempting users from file size restrictions across all repository types would behave badly. The upload repository at least still handles it gracefully and still handles you trying to upload a file above PHP's file size limits (which we cannot bypass from within PHP)

          Show
          Andrew Davis added a comment - - edited Updating the branch info. https://github.com/andyjdavis/moodle/compare/master...MDL-27156_file_cap This doesnt work quite how it should. The global $COURSE hasnt been initiated correctly so $COURSE->id is always 1 (system). This makes it impossible to grant the user the new capability within a single course. I'm not sure how to remedy this just yet. If anyone else knows feel free to jump in. I have emailed Michael and Dongsheng about this as Im a bit stuck at this point. I was worried that exempting users from file size restrictions across all repository types would behave badly. The upload repository at least still handles it gracefully and still handles you trying to upload a file above PHP's file size limits (which we cannot bypass from within PHP)
          Hide
          Dongsheng Cai added a comment -

          Hello Andrew

          That's actually a bug introduced by accesslib.php refactor.

          File picker requires $context parameter, before it's just $context standard class, after refactor, $context object has private id, depth and level, so when json_encode try to encode $context, all private properties got lost, so filepicker don't know the real context and $COURSE.

          The bug is here: MDL-31789/MDL-30869, not really the same bug, but the same cause. To fix this:

          1. implement _toString() magic method in context class, so it works with json_encode/echo/print
          2. Assign a standard object to filepicker
          Show
          Dongsheng Cai added a comment - Hello Andrew That's actually a bug introduced by accesslib.php refactor. File picker requires $context parameter, before it's just $context standard class, after refactor, $context object has private id, depth and level, so when json_encode try to encode $context, all private properties got lost, so filepicker don't know the real context and $COURSE. The bug is here: MDL-31789 / MDL-30869 , not really the same bug, but the same cause. To fix this: implement _toString() magic method in context class, so it works with json_encode/echo/print Assign a standard object to filepicker
          Hide
          Eric Merrill added a comment -

          This is a big issue for us - as file system is how we plan to get around limits for uploading backups and what not (files that we, as admins, vet). The other way is going to have to be to setup a second sever link to the same file store and DB with different limits.

          I add my +0.001 to Martin's option #2 (capability to ignore limits).

          Show
          Eric Merrill added a comment - This is a big issue for us - as file system is how we plan to get around limits for uploading backups and what not (files that we, as admins, vet). The other way is going to have to be to setup a second sever link to the same file store and DB with different limits. I add my +0.001 to Martin's option #2 (capability to ignore limits).
          Hide
          Andrew Davis added a comment -

          It appears that the issues Dongsheng mentioned have been integrated and the problem with course ID in the file picker has been resolved. I'll do some testing myself and update the testing instructions.

          Show
          Andrew Davis added a comment - It appears that the issues Dongsheng mentioned have been integrated and the problem with course ID in the file picker has been resolved. I'll do some testing myself and update the testing instructions.
          Hide
          Andrew Davis added a comment -

          Ill need to do some more work on this. I have the check for the new capability in get_max_upload_file_size() but then the user's individual capability alters the site admin pages. Specifically if get_max_upload_file_size() returns -1 because the user is immune to file size restrictions the only options you get for maxbytes are server limit and -1. You're meant to get 8MB, 5MB, 2MB etc.

          I could rework how the admin stuff works but I get the feeling that get_max_upload_file_size() should continue to behave the same for all users. Individual user exemptions can be dealt with outside of it.

          Show
          Andrew Davis added a comment - Ill need to do some more work on this. I have the check for the new capability in get_max_upload_file_size() but then the user's individual capability alters the site admin pages. Specifically if get_max_upload_file_size() returns -1 because the user is immune to file size restrictions the only options you get for maxbytes are server limit and -1. You're meant to get 8MB, 5MB, 2MB etc. I could rework how the admin stuff works but I get the feeling that get_max_upload_file_size() should continue to behave the same for all users. Individual user exemptions can be dealt with outside of it.
          Hide
          Eric Merrill added a comment -

          Maybe add a optional param 'allowoverride'?

          Then you just set it in the places where get_max_upload_file_size are used to inform the filepicker (it actually is only used in a handful of places in moodle)

          Show
          Eric Merrill added a comment - Maybe add a optional param 'allowoverride'? Then you just set it in the places where get_max_upload_file_size are used to inform the filepicker (it actually is only used in a handful of places in moodle)
          Hide
          Andrew Davis added a comment -

          Updated the branch and testing instructions. Putting this up for peer review.

          Show
          Andrew Davis added a comment - Updated the branch and testing instructions. Putting this up for peer review.
          Hide
          Jason Fowler added a comment -

          repository/repository_ajax.php line 87 is extra whitespace where as repository/filepicker.php doesn't have the blank line (near line 96)

          Other than that the code looks good andrew - love the name of the new capability

          Show
          Jason Fowler added a comment - repository/repository_ajax.php line 87 is extra whitespace where as repository/filepicker.php doesn't have the blank line (near line 96) Other than that the code looks good andrew - love the name of the new capability
          Hide
          Andrew Davis added a comment -

          Fixed the white space. Putting this up for integration. As it involves introducing a new capability I'm not sure if this will be master only. I suspect it will as its rather new feature-ish.

          Show
          Andrew Davis added a comment - Fixed the white space. Putting this up for integration. As it involves introducing a new capability I'm not sure if this will be master only. I suspect it will as its rather new feature-ish.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Marina Glancy added a comment - - edited

          (comment removed, sorry, did not read the description carefully)

          Show
          Marina Glancy added a comment - - edited (comment removed, sorry, did not read the description carefully)
          Hide
          Andrew Davis added a comment -

          rebased.

          Show
          Andrew Davis added a comment - rebased.
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Andrew Davis added a comment -

          re-rebased.

          Show
          Andrew Davis added a comment - re-rebased.
          Hide
          Dan Poltawski added a comment -

          Hi Andrew,

          The main concern about this change is that by using the $COURSE global, it is going in the wrong direction (course centric, rather than context centric).

          I'm still thinking about this, but I think my gut feeling i that we want this permission to only be a system level permission. The admin can give certain users the permission to override the course/activity limits.

          Show
          Dan Poltawski added a comment - Hi Andrew, The main concern about this change is that by using the $COURSE global, it is going in the wrong direction (course centric, rather than context centric). I'm still thinking about this, but I think my gut feeling i that we want this permission to only be a system level permission. The admin can give certain users the permission to override the course/activity limits.
          Hide
          Andrew Davis added a comment -

          "The main concern about this change is that by using the $COURSE global, it is going in the wrong direction (course centric, rather than context centric)."

          I dont understand what you mean. Can you explain that in another way?

          Show
          Andrew Davis added a comment - "The main concern about this change is that by using the $COURSE global, it is going in the wrong direction (course centric, rather than context centric)." I dont understand what you mean. Can you explain that in another way?
          Hide
          Dan Poltawski added a comment -

          I dont understand what you mean. Can you explain that in another way?

          Well, lets say an admin wants to allow a user to upload a big file in one specific activity rather than in the whole course then this is not possible using the course global to work out the context.

          Where as if there was a specific context param then the admin could do both at course level and at course module level.

          Show
          Dan Poltawski added a comment - I dont understand what you mean. Can you explain that in another way? Well, lets say an admin wants to allow a user to upload a big file in one specific activity rather than in the whole course then this is not possible using the course global to work out the context. Where as if there was a specific context param then the admin could do both at course level and at course module level.
          Hide
          Dan Poltawski added a comment -

          Discussed this a bit more between integrators and agreed that this course centric approach does not seem right, especially as we need to know the context in order to create a file in the filesystem as every file is tied to a context.

          We came up with the following idea:

          • Introduce a new function get_user_max_upload_file_size, passing context as the first param and user as an optional fifth.
          • Check the capability to bypass file limits in the context passed
          • Investigate all the calls to get_max_upload_file_size and convert to the new max_upload_file_size
          • (optionally, probably advisable Depreciate the old function and mark with developer warnings).

          Sorry for my incompetence causing the delay of this issue. I will be happy to help with this to get it resolved ASAP.

          Show
          Dan Poltawski added a comment - Discussed this a bit more between integrators and agreed that this course centric approach does not seem right, especially as we need to know the context in order to create a file in the filesystem as every file is tied to a context. We came up with the following idea: Introduce a new function get_user_max_upload_file_size, passing context as the first param and user as an optional fifth. Check the capability to bypass file limits in the context passed Investigate all the calls to get_max_upload_file_size and convert to the new max_upload_file_size (optionally, probably advisable Depreciate the old function and mark with developer warnings). Sorry for my incompetence causing the delay of this issue. I will be happy to help with this to get it resolved ASAP.
          Hide
          Andrew Davis added a comment -

          Mostly done.

          "Investigate all the calls to get_max_upload_file_size and convert to the new max_upload_file_size"

          I think there's some confusion here. I'm adding a new function called get_user_max_upload_file_size() that will exist alongside the current get_max_upload_file_size(). I dont think anything is getting deprecated. We cant always use the new get_user_max_upload_file_size() as using it makes viewing the system settings pages not work (admin looks at them, is immune to file size limits, all limits show -1)

          Show
          Andrew Davis added a comment - Mostly done. "Investigate all the calls to get_max_upload_file_size and convert to the new max_upload_file_size" I think there's some confusion here. I'm adding a new function called get_user_max_upload_file_size() that will exist alongside the current get_max_upload_file_size(). I dont think anything is getting deprecated. We cant always use the new get_user_max_upload_file_size() as using it makes viewing the system settings pages not work (admin looks at them, is immune to file size limits, all limits show -1)
          Hide
          Dan Poltawski added a comment -

          Hmm, good point. My concern was that there is a risk of inconsistency here if everywhere isn't using this new function.

          Show
          Dan Poltawski added a comment - Hmm, good point. My concern was that there is a risk of inconsistency here if everywhere isn't using this new function.
          Hide
          Dan Poltawski added a comment -

          By the way, I am not sure about the capability name - trying to think of a similar capability which might have a style we can copy.

          Show
          Dan Poltawski added a comment - By the way, I am not sure about the capability name - trying to think of a similar capability which might have a style we can copy.
          Hide
          Andrew Davis added a comment - - edited

          Ive altered the capability name. Hopefully the new one is more moodley. I did like the old one though. It reminded me of dungeons and dragons

          "My concern was that there is a risk of inconsistency here if everywhere isn't using this new function."

          There is some risk of inconsistency but its the good kind, if there is such a thing. If somewhere should be using the new function but isn't then the user will be dealt with more strictly than they should be. Going through calls to the old function now.

          Show
          Andrew Davis added a comment - - edited Ive altered the capability name. Hopefully the new one is more moodley. I did like the old one though. It reminded me of dungeons and dragons "My concern was that there is a risk of inconsistency here if everywhere isn't using this new function." There is some risk of inconsistency but its the good kind, if there is such a thing. If somewhere should be using the new function but isn't then the user will be dealt with more strictly than they should be. Going through calls to the old function now.
          Hide
          Andrew Davis added a comment -

          I'm increasingly convinced that it would be a bad idea to make any further code changes in regards to this issue at this time. We've fixed the problem that saw this issue raised. I'd be happy to see this go in as is. We can flush out places where we are being overly strict over time.

          My concern is that by switching from more places over from get_max_upload_file_size() to get_user_max_upload_file_size() we're placing the burden of additional error handling on the calling code. If you call get_max_upload_file_size() PHP's own file limits will be factored in. If you call get_user_max_upload_file_size() and the user is exempt from file limits you need to deal with the possibility that the user will then try to upload something so large that PHP itself throws an error. I'm not confident that the existing error handling is up to scratch. Maybe it is but I'm not confident. I feel like Im making a lot of changes deep in the bowels of Moodle in areas I dont understand and that are going to affect basically everywhere.

          Plus the testing would essentially be "test everywhere that a user can upload a file" which is an awful lot of places :\

          Show
          Andrew Davis added a comment - I'm increasingly convinced that it would be a bad idea to make any further code changes in regards to this issue at this time. We've fixed the problem that saw this issue raised. I'd be happy to see this go in as is. We can flush out places where we are being overly strict over time. My concern is that by switching from more places over from get_max_upload_file_size() to get_user_max_upload_file_size() we're placing the burden of additional error handling on the calling code. If you call get_max_upload_file_size() PHP's own file limits will be factored in. If you call get_user_max_upload_file_size() and the user is exempt from file limits you need to deal with the possibility that the user will then try to upload something so large that PHP itself throws an error. I'm not confident that the existing error handling is up to scratch. Maybe it is but I'm not confident. I feel like Im making a lot of changes deep in the bowels of Moodle in areas I dont understand and that are going to affect basically everywhere. Plus the testing would essentially be "test everywhere that a user can upload a file" which is an awful lot of places :\
          Hide
          Dan Poltawski added a comment -

          We're just chatting about this - man this issue just keeps throwing hurdles in our way as we discover them!

          One (not sounding very elegant solution) would be to have a parameter, defaulting to on of whether we observe php file limits. Then when users have the capability to bypass restrictions they could go all the way up to the max php limit in places restricted by that. Places that don't need to care about the php restriction would be truely unlimited if they passed the param.

          Show
          Dan Poltawski added a comment - We're just chatting about this - man this issue just keeps throwing hurdles in our way as we discover them! One (not sounding very elegant solution) would be to have a parameter, defaulting to on of whether we observe php file limits. Then when users have the capability to bypass restrictions they could go all the way up to the max php limit in places restricted by that. Places that don't need to care about the php restriction would be truely unlimited if they passed the param.
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Andrew Davis added a comment -

          Ive made some minor tweaks. I'm trying to identify a subset of the calls to get_max_upload_file_size() that are reasonably easy to to switch over and test comprehensively.

          Show
          Andrew Davis added a comment - Ive made some minor tweaks. I'm trying to identify a subset of the calls to get_max_upload_file_size() that are reasonably easy to to switch over and test comprehensively.
          Hide
          Andrew Davis added a comment -

          Ive switched over 2 instances of get_max_upload_file_size() and added additional testing instructions. A lot of the other calls to get_max_upload_file_size() I cannot for the life of me figure out how to get the code to be executed from the UI.

          Show
          Andrew Davis added a comment - Ive switched over 2 instances of get_max_upload_file_size() and added additional testing instructions. A lot of the other calls to get_max_upload_file_size() I cannot for the life of me figure out how to get the code to be executed from the UI.
          Hide
          Marina Glancy added a comment -

          Hi Andrew, how about filemanager and filepicker form elements?

          In form_filepicker we call get_max_upload_file_size() always:
          https://github.com/moodle/moodle/blob/master/lib/form/filepicker.php#L82
          and acually it is often called in other places in the code before ->addElement('filepicker',

          And in form_filemanager we also call get_max_upload_file_size() several times:
          https://github.com/marinaglancy/moodle/blob/master/lib/form/filemanager.php

          Show
          Marina Glancy added a comment - Hi Andrew, how about filemanager and filepicker form elements? In form_filepicker we call get_max_upload_file_size() always: https://github.com/moodle/moodle/blob/master/lib/form/filepicker.php#L82 and acually it is often called in other places in the code before ->addElement('filepicker', And in form_filemanager we also call get_max_upload_file_size() several times: https://github.com/marinaglancy/moodle/blob/master/lib/form/filemanager.php
          Hide
          Marina Glancy added a comment - - edited

          another thing is that I would exclude lines like

          'maxbytes' => get_max_upload_file_size($CFG->maxbytes),
          

          completely or set to -1 (in rubrics and lesson), because now it makes no sense. Maxbytes will be adjusted to server/php/apache limits anyway.

          Show
          Marina Glancy added a comment - - edited another thing is that I would exclude lines like 'maxbytes' => get_max_upload_file_size($CFG->maxbytes), completely or set to -1 (in rubrics and lesson), because now it makes no sense. Maxbytes will be adjusted to server/php/apache limits anyway.
          Hide
          Andrew Davis added a comment -

          Ive added a new commit and some more testing instructions.

          Show
          Andrew Davis added a comment - Ive added a new commit and some more testing instructions.
          Hide
          Andrew Davis added a comment -

          I'm afraid that "'maxbytes' => get_max_upload_file_size($CFG->maxbytes)" is still necessary. The function that determines max file size doesnt actually retrieve the site maximum file limit. It relies on having it passed in. There's probably a reason why it was implemented that way but I dont know what it is.

          Show
          Andrew Davis added a comment - I'm afraid that "'maxbytes' => get_max_upload_file_size($CFG->maxbytes)" is still necessary. The function that determines max file size doesnt actually retrieve the site maximum file limit. It relies on having it passed in. There's probably a reason why it was implemented that way but I dont know what it is.
          Hide
          Jason Fowler added a comment -

          There is a bit of whitespace at line 859 in lib/db/access.php - extra line is all, once that is cleaned up, it's good to go for integration

          Show
          Jason Fowler added a comment - There is a bit of whitespace at line 859 in lib/db/access.php - extra line is all, once that is cleaned up, it's good to go for integration
          Hide
          Andrew Davis added a comment -

          For whatever reason most of the capability definitions in lib/db/access.php have an empty line there. I just copied what was already there. Not sure why...

          Submitting for integration.

          Show
          Andrew Davis added a comment - For whatever reason most of the capability definitions in lib/db/access.php have an empty line there. I just copied what was already there. Not sure why... Submitting for integration.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Ho,

          while for sure more work is needed to get the limits applied properly site-wide, I think current patch achieves the goals for this issue: To be able to bypass the calculated maxbytes via capability.

          I don't know if the code achieves that (haven't looked/know much), but the testing instructions do (IMO). So this gets my +1 if works as expected in the instructions.

          Only 2 comments:

          1) Does all this work with JS disabled?
          2) Does people with the capability really see "-1 bytes". If so, it's really horrible. Please fix that to be "unlimited" or whatever. Here or in another issue.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Ho, while for sure more work is needed to get the limits applied properly site-wide, I think current patch achieves the goals for this issue: To be able to bypass the calculated maxbytes via capability. I don't know if the code achieves that (haven't looked/know much), but the testing instructions do (IMO). So this gets my +1 if works as expected in the instructions. Only 2 comments: 1) Does all this work with JS disabled? 2) Does people with the capability really see "-1 bytes". If so, it's really horrible. Please fix that to be "unlimited" or whatever. Here or in another issue. Ciao
          Hide
          Andrew Davis added a comment -

          I've raised MDL-33717 to deal with the -1/unlimited issue.

          I'm just doing some additional testing with JS disabled to make sure it all works fine.

          Show
          Andrew Davis added a comment - I've raised MDL-33717 to deal with the -1/unlimited issue. I'm just doing some additional testing with JS disabled to make sure it all works fine.
          Hide
          Andrew Davis added a comment -

          Slightly altered testing instructions. Looks fine with JS disabled.

          Show
          Andrew Davis added a comment - Slightly altered testing instructions. Looks fine with JS disabled.
          Hide
          Andrew Davis added a comment -

          fyi, I rebased just to make sure there were no problems (aside from the inevitable conflict with version.php)

          Show
          Andrew Davis added a comment - fyi, I rebased just to make sure there were no problems (aside from the inevitable conflict with version.php)
          Hide
          Dan Poltawski added a comment -

          I've integrated this now.

          Thanks Andrew for all your work on this, I know this was a bit of a beast, we got there eventually!

          I agree we need to sort out the -1 issue, perhaps even as a blocker.

          Show
          Dan Poltawski added a comment - I've integrated this now. Thanks Andrew for all your work on this, I know this was a bit of a beast, we got there eventually! I agree we need to sort out the -1 issue, perhaps even as a blocker.
          Hide
          Frédéric Massart added a comment - - edited

          Failed when addind a video/audio to a page resource. The file size can be anything when using the file repository. I tried the upload, and the max upload limit error is raised.

          I did not check the capability as the first steps failed.

          I tried hard to track why this fails but did not find much. What happens is that the maxbytes setting is wrong. It appears to be zero even though I had set it to 2MB. And the call to repository_ajax.php sends maxbytes = -1.

          Edit:

          Found that in repository/filepicker.js:545,

          params['maxbytes'] = this.options.maxbytes?this.options.maxbytes:-1;
          

          sets maxbytes to -1 when it is equal to 0. It shouldn't.

          Show
          Frédéric Massart added a comment - - edited Failed when addind a video/audio to a page resource. The file size can be anything when using the file repository. I tried the upload, and the max upload limit error is raised. I did not check the capability as the first steps failed. I tried hard to track why this fails but did not find much. What happens is that the maxbytes setting is wrong. It appears to be zero even though I had set it to 2MB. And the call to repository_ajax.php sends maxbytes = -1. Edit: Found that in repository/filepicker.js:545, params['maxbytes'] = this .options.maxbytes? this .options.maxbytes:-1; sets maxbytes to -1 when it is equal to 0. It shouldn't.
          Hide
          Andrew Davis added a comment - - edited

          Investigating now.

          UPDATE: When the user doesn't have the moodle/course:ignorefilesizelimits capability the site/activty file size limit is enforced in the page's content but not the page description.

          Show
          Andrew Davis added a comment - - edited Investigating now. UPDATE: When the user doesn't have the moodle/course:ignorefilesizelimits capability the site/activty file size limit is enforced in the page's content but not the page description.
          Hide
          Andrew Davis added a comment -

          Ive raised MDL-33760 to deal with the page description not observing file size limits. It appears to be an old but that we've just now noticed. Im also updating the testing instructions to specifically exclude the page description from the testing of this issue.

          Show
          Andrew Davis added a comment - Ive raised MDL-33760 to deal with the page description not observing file size limits. It appears to be an old but that we've just now noticed. Im also updating the testing instructions to specifically exclude the page description from the testing of this issue.
          Hide
          Frédéric Massart added a comment - - edited

          Hi Andrew,

          Ok, now it makes sense about the content and description, and it works.
          I am just concerned about My private files where I cannot reproduce your steps.

          My settings
          moodlecourse | maxbytes = 2MB
          userquota = 104857600
          upload_max_filesize = 120M
          post_max_size = 128M

          As a teacher, in my private files "Maximum size for new files: 100MB" which is the userquota. "You have reached your file quota limit" is displayed but I can still upload any file with any file size, exception made for the files which exceed max_upload_size. I have removed the capability from all the roles and checked that the teacher didn't have it.

          Edit: It appears that My private files uses the userquota and nothing else when validating the form.

          More edit: I have linked this issue with a new one (MDL-33766), I'll let you decide if that new one blocks this issue or not. If not, the test passes.

          Show
          Frédéric Massart added a comment - - edited Hi Andrew, Ok, now it makes sense about the content and description, and it works. I am just concerned about My private files where I cannot reproduce your steps. My settings moodlecourse | maxbytes = 2MB userquota = 104857600 upload_max_filesize = 120M post_max_size = 128M As a teacher, in my private files "Maximum size for new files: 100MB" which is the userquota. "You have reached your file quota limit" is displayed but I can still upload any file with any file size, exception made for the files which exceed max_upload_size. I have removed the capability from all the roles and checked that the teacher didn't have it. Edit: It appears that My private files uses the userquota and nothing else when validating the form. More edit: I have linked this issue with a new one ( MDL-33766 ), I'll let you decide if that new one blocks this issue or not. If not, the test passes.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi,

          was testing other issues when I discovered that picking files from the filesystem repository was not working any more. Bisecting the problem back, it seems that this commit (part of this issue): 845c2ae

          is the one causing the problem.

          To reproduce:

          1/ create filesystem repository instance.
          2/ add the any backup file (1.1MB or bigger)
          3/ click settings->restore
          4/ pick the file (apparently it's picked ok)
          5/ click "restore"

          Expected:

          • A page showing the backup summary information is show.

          Current:

          • error/invalidrestorefile with information:
          Debug info: 
          Error code: invalidrestorefile
          $a contents:
          Stack trace:
          line 157 of /backup/util/ui/restore_ui_stage.class.php: restore_ui_exception thrown
          line 42 of /backup/restore.php: call to restore_ui_stage_confirm->process()
          

          Note it's restore the one throwing the error, but surely caused because of some previous error (silently happen, I don't get any other information nor on screen nor in logs).

          The exact previous commmit 1812ea3 (also belonging to this issue) allows the restore to happen, so...

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi, was testing other issues when I discovered that picking files from the filesystem repository was not working any more. Bisecting the problem back, it seems that this commit (part of this issue): 845c2ae is the one causing the problem. To reproduce: 1/ create filesystem repository instance. 2/ add the any backup file (1.1MB or bigger) 3/ click settings->restore 4/ pick the file (apparently it's picked ok) 5/ click "restore" Expected: A page showing the backup summary information is show. Current: error/invalidrestorefile with information: Debug info: Error code: invalidrestorefile $a contents: Stack trace: line 157 of /backup/util/ui/restore_ui_stage.class.php: restore_ui_exception thrown line 42 of /backup/restore.php: call to restore_ui_stage_confirm->process() Note it's restore the one throwing the error, but surely caused because of some previous error (silently happen, I don't get any other information nor on screen nor in logs). The exact previous commmit 1812ea3 (also belonging to this issue) allows the restore to happen, so... Ciao
          Hide
          Andrew Davis added a comment -

          Theres further work that needs doing here. How do I get this issue back into "currently in development"?

          Show
          Andrew Davis added a comment - Theres further work that needs doing here. How do I get this issue back into "currently in development"?
          Hide
          Helen Foster added a comment -

          I also obtained error/invalidrestorefile (with same debug info and stack trace) when attempting to upload a small (less than 1MB) backup file to http://qa.moodle.net then clicking restore.

          Show
          Helen Foster added a comment - I also obtained error/invalidrestorefile (with same debug info and stack trace) when attempting to upload a small (less than 1MB) backup file to http://qa.moodle.net then clicking restore.
          Hide
          Martin Dougiamas added a comment -

          Can you work on this atm Andrew? (make a new commit to fix?)

          Show
          Martin Dougiamas added a comment - Can you work on this atm Andrew? (make a new commit to fix?)
          Hide
          Dan Poltawski added a comment -

          Hi Andrew,

          We are not going to revert this change and going to leave master in this 'broken' state because we'd really like to see this improvement into 2.3 and not delayed any further. So if you could work on a fix with urgency then hopefully we can get this integrated and signed off for release.

          Show
          Dan Poltawski added a comment - Hi Andrew, We are not going to revert this change and going to leave master in this 'broken' state because we'd really like to see this improvement into 2.3 and not delayed any further. So if you could work on a fix with urgency then hopefully we can get this integrated and signed off for release.
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Andrew et all, FYI, I'm adding this commit right now, to allow the picker to continue working as it was before the patch. It simply avoids the -1 to be returned and I've verified here that it makes both upload and fs repos to, at least, work.

          Without it nothing can be picked properly. Of course, the final solution for this should uncomment that commit. It's just one interim solution to keep things working.

          Ciao

          diff --git a/lib/moodlelib.php b/lib/moodlelib.php
          index 708c613..6978a42 100644
          --- a/lib/moodlelib.php
          +++ b/lib/moodlelib.php
          @@ -5860,9 +5860,10 @@ function get_user_max_upload_file_size($context, $sitebytes=0, $coursebytes=0, $
                   $user = $USER;
               }
           
          -    if (has_capability('moodle/course:ignorefilesizelimits', $context, $user)) {
          -        return -1;
          -    }
          +    // Temp. commenting this until MDL-27156 fixes it!
          +    // if (has_capability('moodle/course:ignorefilesizelimits', $context, $user)) {
          +    //     return -1;
          +    // }
           
               return get_max_upload_file_size($sitebytes, $coursebytes, $modulebytes);
           }
          
          Show
          Eloy Lafuente (stronk7) added a comment - Andrew et all, FYI, I'm adding this commit right now, to allow the picker to continue working as it was before the patch. It simply avoids the -1 to be returned and I've verified here that it makes both upload and fs repos to, at least, work. Without it nothing can be picked properly. Of course, the final solution for this should uncomment that commit. It's just one interim solution to keep things working. Ciao diff --git a/lib/moodlelib.php b/lib/moodlelib.php index 708c613..6978a42 100644 --- a/lib/moodlelib.php +++ b/lib/moodlelib.php @@ -5860,9 +5860,10 @@ function get_user_max_upload_file_size($context, $sitebytes=0, $coursebytes=0, $ $user = $USER; } - if (has_capability('moodle/course:ignorefilesizelimits', $context, $user)) { - return -1; - } + // Temp. commenting this until MDL-27156 fixes it! + // if (has_capability('moodle/course:ignorefilesizelimits', $context, $user)) { + // return -1; + // } return get_max_upload_file_size($sitebytes, $coursebytes, $modulebytes); }
          Hide
          Helen Foster added a comment -

          Just tried again to upload a backup file to http://qa.moodle.net then clicked restore. No error this time! The course backup restored without any problem.

          Show
          Helen Foster added a comment - Just tried again to upload a backup file to http://qa.moodle.net then clicked restore. No error this time! The course backup restored without any problem.
          Hide
          Andrew Davis added a comment -

          Hi all. I'm recording the old branch info here for antiquity as Ill create a new branch for subsequent fixes.

          https://github.com/andyjdavis/moodle/compare/master...MDL-27156_file_cap

          Show
          Andrew Davis added a comment - Hi all. I'm recording the old branch info here for antiquity as Ill create a new branch for subsequent fixes. https://github.com/andyjdavis/moodle/compare/master...MDL-27156_file_cap
          Hide
          Andrew Davis added a comment - - edited

          Just recording some notes. If anyone has any theories feel free to jump in.

          This is the code thats producing the visible error. From restore_ui_stage.class.php

          if (!file_exists("$CFG->tempdir/backup/".$this->filename)) {
             throw new restore_ui_exception('invalidrestorefile');
          }
          

          This is the problem in moodleform::save_file()

          if (!$files = $fs->get_area_files($context->id, 'user', 'draft', $draftid, 'id DESC', false)) {
              return false;
          }
          

          Later code is meant to copy the backup file to /home/andrew/Desktop/tempdata/moodledata/dev/master/temp/backup/61761eb5e161d33ee4a570acb151b7b6 however $fs->get_area_files() is returning an empty array so we're returning.

          As a side note, /backup/restorefile.php should probably be checking the return value of $form->save_file('backupfile', $pathname); UPDATE: raised MDL-33799 to deal with this.

          Show
          Andrew Davis added a comment - - edited Just recording some notes. If anyone has any theories feel free to jump in. This is the code thats producing the visible error. From restore_ui_stage.class.php if (!file_exists( "$CFG->tempdir/backup/" .$ this ->filename)) { throw new restore_ui_exception('invalidrestorefile'); } This is the problem in moodleform::save_file() if (!$files = $fs->get_area_files($context->id, 'user', 'draft', $draftid, 'id DESC', false )) { return false ; } Later code is meant to copy the backup file to /home/andrew/Desktop/tempdata/moodledata/dev/master/temp/backup/61761eb5e161d33ee4a570acb151b7b6 however $fs->get_area_files() is returning an empty array so we're returning. As a side note, /backup/restorefile.php should probably be checking the return value of $form->save_file('backupfile', $pathname); UPDATE: raised MDL-33799 to deal with this.
          Hide
          Andrew Davis added a comment - - edited

          repository_ajax.php is actually adding the backup file to the file table. Then the user is redirected to /backup/restorefile.php and something is actually deleting the row from the file table before the backup code is able to access the file.

          update: the get_data() call backup/restorefile.php is deleting rows from the file table

          $form = new course_restore_form(null, array('contextid'=>$contextid));
          $data = $form->get_data();

          Show
          Andrew Davis added a comment - - edited repository_ajax.php is actually adding the backup file to the file table. Then the user is redirected to /backup/restorefile.php and something is actually deleting the row from the file table before the backup code is able to access the file. update: the get_data() call backup/restorefile.php is deleting rows from the file table $form = new course_restore_form(null, array('contextid'=>$contextid)); $data = $form->get_data();
          Hide
          Andrew Davis added a comment -

          Updated the branch with a possible fix. Not 100% sure of what the constant's name should be.

          Show
          Andrew Davis added a comment - Updated the branch with a possible fix. Not 100% sure of what the constant's name should be.
          Hide
          Andrew Davis added a comment -

          Im going to heavily update the testing instructions. Just in case here are the old ones.

          For this test you'll need a teacher and your admin user. A 3rd user with very basic capabilities ie a student, will also be handy.

          Check your site's maximum uploaded file size. This is a site setting called "maxbytes".

          Set maxbytes to 2MB.

          You'll need 2 files.
          1 small file ie smaller than 2 MB.
          1 large file. I'm using an 82MB video. The precise size doesn't matter but it should be bigger than your PHP max upload size.
          They both need to be media files (video or music).

          As admin go to course admin > users > permissions > check permissions and check "moodle/course:ignorefilesizelimits" for a teacher. The teacher should NOT have moodle/course:ignorefilesizelimits.

          File system repository set up
          1. create a directory at moodledata/repository/examplerepository then set up a file system repository using that directory by going to: Site administration > Plugins > Repositories > Manage repositories > File System > Settings > Create a Repository Instance
          2. Outside of Moodle copy your 2 files into the repository directory.

          Create a new Page in a Course

          All of the following uses the page content field. Do NOT use page description due to an outstanding bug (MDL-33760).

          User without moodle/course:ignorefilesizelimits test
          1. Edit the page. Using the media button add a link to the smaller file in your file repository in the page content. It should succeed.
          2. Repeat this with the bigger file. It should fail with a message like "This file is bigger than the maximum size"

          Has capability test
          1. As admin give the teacher role moodle/course:ignorefilesizelimits within the course. course administration > users > permissions.
          2. As teacher edit the page and check that you can now use the media button to link to both the small and the large file when they are in a file system repository.

          Upload repository test
          1. Edit your page again but this time on the file picker click "upload a file" and link to the smaller file. It should succeed.
          2. Repeat this with the larger file. You should get an error saying that the file is too big. This is due to PHP's max file upload size which we cannot easily avoid but which we should handle gracefully.

          Additional tests for other areas using the new function

          1.create a lesson and add a content page. after the page is created, edit the page and put an image in the page content. Just checking that image insertion still works.

          2. create assignment, set grading to rubric and create a new rubric.

          3. With a user who has moodle/course:ignorefilesizelimits backup a course. Outside of Moodle copy it into your file repository. Start a course restore and you should be able to access the backup file in the file repository.

          4. My private files. Bear in mind the context where you gave the user the capability. my files is outside of a course.
          1) Go to my private files. As a user without moodle/course:ignorefilesizelimits you should see
          something like "Maximum size for newfiles: 2MB - drag and drop available" near the top right of the file manager.
          2) As a user with moodle/course:ignorefilesizelimits (or admin) you should see something like
          "Maximum size for new files: -1 bytes - drag and drop available"
          3) As the user with the capability drag your large file into your private files. You should get an error message about PHP.
          4) As the user with the capability click "Add..." and add a large file from your file repository. It should succeed.

          Show
          Andrew Davis added a comment - Im going to heavily update the testing instructions. Just in case here are the old ones. For this test you'll need a teacher and your admin user. A 3rd user with very basic capabilities ie a student, will also be handy. Check your site's maximum uploaded file size. This is a site setting called "maxbytes". Set maxbytes to 2MB. You'll need 2 files. 1 small file ie smaller than 2 MB. 1 large file. I'm using an 82MB video. The precise size doesn't matter but it should be bigger than your PHP max upload size. They both need to be media files (video or music). As admin go to course admin > users > permissions > check permissions and check "moodle/course:ignorefilesizelimits" for a teacher. The teacher should NOT have moodle/course:ignorefilesizelimits. File system repository set up 1. create a directory at moodledata/repository/examplerepository then set up a file system repository using that directory by going to: Site administration > Plugins > Repositories > Manage repositories > File System > Settings > Create a Repository Instance 2. Outside of Moodle copy your 2 files into the repository directory. Create a new Page in a Course All of the following uses the page content field. Do NOT use page description due to an outstanding bug ( MDL-33760 ). User without moodle/course:ignorefilesizelimits test 1. Edit the page. Using the media button add a link to the smaller file in your file repository in the page content. It should succeed. 2. Repeat this with the bigger file. It should fail with a message like "This file is bigger than the maximum size" Has capability test 1. As admin give the teacher role moodle/course:ignorefilesizelimits within the course. course administration > users > permissions. 2. As teacher edit the page and check that you can now use the media button to link to both the small and the large file when they are in a file system repository. Upload repository test 1. Edit your page again but this time on the file picker click "upload a file" and link to the smaller file. It should succeed. 2. Repeat this with the larger file. You should get an error saying that the file is too big. This is due to PHP's max file upload size which we cannot easily avoid but which we should handle gracefully. Additional tests for other areas using the new function 1.create a lesson and add a content page. after the page is created, edit the page and put an image in the page content. Just checking that image insertion still works. 2. create assignment, set grading to rubric and create a new rubric. 3. With a user who has moodle/course:ignorefilesizelimits backup a course. Outside of Moodle copy it into your file repository. Start a course restore and you should be able to access the backup file in the file repository. 4. My private files. Bear in mind the context where you gave the user the capability. my files is outside of a course. 1) Go to my private files. As a user without moodle/course:ignorefilesizelimits you should see something like "Maximum size for newfiles: 2MB - drag and drop available" near the top right of the file manager. 2) As a user with moodle/course:ignorefilesizelimits (or admin) you should see something like "Maximum size for new files: -1 bytes - drag and drop available" 3) As the user with the capability drag your large file into your private files. You should get an error message about PHP. 4) As the user with the capability click "Add..." and add a large file from your file repository. It should succeed.
          Hide
          Andrew Davis added a comment -

          Updated testing instructions.

          Show
          Andrew Davis added a comment - Updated testing instructions.
          Hide
          Andrew Davis added a comment -

          This is now up for peer review. afaik, this can go straight to integration if there are no issues found.

          Integrators, if you're not happy with the constant name feel free to change it if Im not online

          Show
          Andrew Davis added a comment - This is now up for peer review. afaik, this can go straight to integration if there are no issues found. Integrators, if you're not happy with the constant name feel free to change it if Im not online
          Hide
          Andrew Davis added a comment -

          Ive raised MDL-33803. I'm intending to fix that once this issue is done with. Its only a problem if JS is disabled in the browser so I dont think its massively important. I will certainly fix it however this issue doesnt need to be dragged out any longer.

          Show
          Andrew Davis added a comment - Ive raised MDL-33803 . I'm intending to fix that once this issue is done with. Its only a problem if JS is disabled in the browser so I dont think its massively important. I will certainly fix it however this issue doesnt need to be dragged out any longer.
          Hide
          Jason Fowler added a comment -

          all looks good to me Andrew

          Show
          Jason Fowler added a comment - all looks good to me Andrew
          Hide
          Andrew Davis added a comment -

          Submitting for integration.

          For anyone interested, I introduced the constant as I can see theres going to be more places where we need check against maxbytes. Trying to avoid having mysterious -1's scattered throughout the code.

          Show
          Andrew Davis added a comment - Submitting for integration. For anyone interested, I introduced the constant as I can see theres going to be more places where we need check against maxbytes. Trying to avoid having mysterious -1's scattered throughout the code.
          Hide
          Dan Poltawski added a comment -

          Thanks Andrew, integrated that now.

          Commence testing!!

          Show
          Dan Poltawski added a comment - Thanks Andrew, integrated that now. Commence testing!!
          Hide
          Frédéric Massart added a comment -

          All right guys, we're getting closer to closing this issue, but there are still a few bugs.

          • When a teacher, without the capability, on the restore backup page, the maxbytes limit is the php limit, not the maxbytes setting. Probably because this is a filepicker, not a filemanager.
          • When an admin, dragging/dropping files larger than the php post limit result in sesskey error. Probably because $_POST is empty as it exceeded the post limit.

          Not failing or passing yet.

          (That'd be nice if the test instructions mentioned the course max upload size, and the php post/upload limit to use)

          Show
          Frédéric Massart added a comment - All right guys, we're getting closer to closing this issue, but there are still a few bugs. When a teacher, without the capability, on the restore backup page, the maxbytes limit is the php limit, not the maxbytes setting. Probably because this is a filepicker, not a filemanager. When an admin, dragging/dropping files larger than the php post limit result in sesskey error. Probably because $_POST is empty as it exceeded the post limit. Not failing or passing yet. (That'd be nice if the test instructions mentioned the course max upload size, and the php post/upload limit to use)
          Hide
          Andrew Davis added a comment -

          Looking at these now.

          We're a little stuck here. Ultimately what we probably need is to switch to using an object (or something) to represent file size restrictions instead of a single number. Returning the calling code an object containing the activity/course file size limit, PHP's file limit plus a flag indicating whether or not the user can ignore limits would allow a more nuanced approach. Overloading an int with multiple meanings has limitations which we're discovering

          Anyhow, trying to figure out the best way forward now.

          Show
          Andrew Davis added a comment - Looking at these now. We're a little stuck here. Ultimately what we probably need is to switch to using an object (or something) to represent file size restrictions instead of a single number. Returning the calling code an object containing the activity/course file size limit, PHP's file limit plus a flag indicating whether or not the user can ignore limits would allow a more nuanced approach. Overloading an int with multiple meanings has limitations which we're discovering Anyhow, trying to figure out the best way forward now.
          Hide
          Andrew Davis added a comment -

          "When a teacher, without the capability, on the restore backup page, the maxbytes limit is the php limit, not the maxbytes setting. Probably because this is a filepicker, not a filemanager."

          Im not sure about filepicker Vs file manager however the file size limit displayed on the restore page is definitely the site setting "maxbytes" even if the course setting "maximum upload size" is lower than maxbytes. Im checking the logic but Im not entirely sure whether or not that is correct.

          Show
          Andrew Davis added a comment - "When a teacher, without the capability, on the restore backup page, the maxbytes limit is the php limit, not the maxbytes setting. Probably because this is a filepicker, not a filemanager." Im not sure about filepicker Vs file manager however the file size limit displayed on the restore page is definitely the site setting "maxbytes" even if the course setting "maximum upload size" is lower than maxbytes. Im checking the logic but Im not entirely sure whether or not that is correct.
          Hide
          Frédéric Massart added a comment -

          In my case, the maxbytes setting for course and site are 2MB, and the PHP limit is 120MB. I can upload larger files than 2MB on the restore page as a teacher. Larger than 120MB fails.

          Show
          Frédéric Massart added a comment - In my case, the maxbytes setting for course and site are 2MB, and the PHP limit is 120MB. I can upload larger files than 2MB on the restore page as a teacher. Larger than 120MB fails.
          Hide
          Andrew Davis added a comment -

          Added details about php settings and course max upload size to the testing instructions.

          Show
          Andrew Davis added a comment - Added details about php settings and course max upload size to the testing instructions.
          Hide
          Andrew Davis added a comment -

          I have added a 2nd commit to the branch. https://github.com/andyjdavis/moodle/commit/703ffaea05dd73cff661cbad8d58e6286fc6b20a

          This makes the file picker observe the course file size limits. However, I quite literally have no idea why it wasn't working before.

          This was always returning false for some unknown reason.
          if (!empty($PAGE->course)) {

          This works
          if (!empty($PAGE->course->maxbytes)) {

          Show
          Andrew Davis added a comment - I have added a 2nd commit to the branch. https://github.com/andyjdavis/moodle/commit/703ffaea05dd73cff661cbad8d58e6286fc6b20a This makes the file picker observe the course file size limits. However, I quite literally have no idea why it wasn't working before. This was always returning false for some unknown reason. if (!empty($PAGE->course)) { This works if (!empty($PAGE->course->maxbytes)) {
          Hide
          Andrew Davis added a comment -

          re the poor handling if the user uploads a file above PHP's post_max_size, I'm not sure we are going to be able to find a good way to handle that in a timely fashion.

          Show
          Andrew Davis added a comment - re the poor handling if the user uploads a file above PHP's post_max_size, I'm not sure we are going to be able to find a good way to handle that in a timely fashion.
          Hide
          Dan Poltawski added a comment -

          I've pulled that commit in. Thanks Andrew for sticking with this!!

          Show
          Dan Poltawski added a comment - I've pulled that commit in. Thanks Andrew for sticking with this!!
          Hide
          Andrew Davis added a comment -

          Ive raised and linked MDL-33889. I suggest we attempt to deal with PHP post_max_size issue in that separate issue.

          Show
          Andrew Davis added a comment - Ive raised and linked MDL-33889 . I suggest we attempt to deal with PHP post_max_size issue in that separate issue.
          Hide
          Frédéric Massart added a comment -

          The test now passes. An new issue has been raised to solve the problem of exceeded post/upload size. MDL-33889

          Show
          Frédéric Massart added a comment - The test now passes. An new issue has been raised to solve the problem of exceeded post/upload size. MDL-33889
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And this has been incorporated to all the weekly builds and also, to Moodle 2.3 Release Candidate 1, yay!

          Many, many thanks for your hard work!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - And this has been incorporated to all the weekly builds and also, to Moodle 2.3 Release Candidate 1, yay! Many, many thanks for your hard work! Ciao
          Hide
          Mary Cooch added a comment -

          I am about to remove the docs_required label but just checking - although the title here refers to the Filesystem repository, this conversation seems to go much further and actually relates more to the new capability http://docs.moodle.org/23/en/Capabilities/moodle/course:ignorefilesizelimits Am I correct in this or have I missed something out?

          Show
          Mary Cooch added a comment - I am about to remove the docs_required label but just checking - although the title here refers to the Filesystem repository, this conversation seems to go much further and actually relates more to the new capability http://docs.moodle.org/23/en/Capabilities/moodle/course:ignorefilesizelimits Am I correct in this or have I missed something out?
          Hide
          Dan Poltawski added a comment -

          Mary, you are correct - it relates to the ignorefilesizelimits capability. This is a solution for the 'I want to upload files bigger than the server limits (php)'.

          This allows the person granted this capability to bypass the moodle restrictions where they are able to - it won't work for things like upload a file as that is restricted by php.

          Show
          Dan Poltawski added a comment - Mary, you are correct - it relates to the ignorefilesizelimits capability. This is a solution for the 'I want to upload files bigger than the server limits (php)'. This allows the person granted this capability to bypass the moodle restrictions where they are able to - it won't work for things like upload a file as that is restricted by php.
          Hide
          Daniel Neis added a comment -

          Hello,

          i think we should review this capability as it is confusing and tell users about "php.ini" stuff is not that helpful. Please take a look at the linked issues.

          Kind regards,
          Daniel

          Show
          Daniel Neis added a comment - Hello, i think we should review this capability as it is confusing and tell users about "php.ini" stuff is not that helpful. Please take a look at the linked issues. Kind regards, Daniel

            People

            • Votes:
              65 Vote for this issue
              Watchers:
              26 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: