Moodle
  1. Moodle
  2. MDL-26890

Forum file size limit is not used if a file is added from privat area.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0.4, 2.1.1, 2.2
    • Fix Version/s: 2.0.5, 2.1.2
    • Component/s: Files API, Forum
    • Labels:
    • Testing Instructions:
      Hide

      1) Login as admin>goto my profile> private files
      2) Upload atleast two files, one less than 100 kb in size and another more than 100kb in size.
      3) Goto any forum click on edit settings under forum administration
      4) Change max attachment size to 100 kb and save.
      5) try to create a new topic and/or reply to existing topic
      6) try to attach files to the post from your private area
      7) You should be able to attach the file that has a size <100kb easily and should get an error for the other one.
      8) In the later case make sure the file is not present in the post after you close the file picker and make the post.

      Show
      1) Login as admin>goto my profile> private files 2) Upload atleast two files, one less than 100 kb in size and another more than 100kb in size. 3) Goto any forum click on edit settings under forum administration 4) Change max attachment size to 100 kb and save. 5) try to create a new topic and/or reply to existing topic 6) try to attach files to the post from your private area 7) You should be able to attach the file that has a size <100kb easily and should get an error for the other one. 8) In the later case make sure the file is not present in the post after you close the file picker and make the post.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull Master Branch:
      MDL-26890-master
    • Rank:
      16517

      Description

      moodle .org Using Moodle course forum file upload is limited to 100 KB.So I uploaded a file (140 KB) to my private folder, and added this file from private folder as forum posting attachment. File picker showed to me that the file is to large for the forum and didn't show the file as attachment in edit mode of forum posting. After sending the post to the forum the file was shown as attachment in the forum.

      We've two options to handle this:

      • accept that bigger files can be used by uploading from private area, In this case the error message should be deleted..
      • but normally a teacher will define the max size for file . Then the attachment shouldn't be added to the posting.
      1. 0001-patch-for-MDL-26890.patch
        3 kB
        Ankit Agarwal
      2. patch for MDL--26890.patch
        3 kB
        Ankit Agarwal

        Issue Links

          Activity

          Ralf Hilgenstock created issue -
          Petr Škoda made changes -
          Field Original Value New Value
          Assignee Petr Škoda (skodak) [ skodak ] Dongsheng Cai [ dongsheng ]
          Hide
          Ankit Agarwal added a comment -

          suggested patch for the issue

          Show
          Ankit Agarwal added a comment - suggested patch for the issue
          Ankit Agarwal made changes -
          Attachment 0001-patch-for-MDL-26890.patch [ 23366 ]
          Hide
          Dongsheng Cai added a comment -

          Hi, Ankit

          Thanks for the patch, problems in your patch:
          1. code indent problem, we use four spaces (not tab) in moodle code, and use unix file format all the time (see more: http://docs.moodle.org/en/Development:Coding_style)

          2. The patch is not a appropriate fix, it is basically copying the code from local/lib.php, the problem is actaully copy_to_area is called before filesize limit check.

          Show
          Dongsheng Cai added a comment - Hi, Ankit Thanks for the patch, problems in your patch: 1. code indent problem, we use four spaces (not tab) in moodle code, and use unix file format all the time (see more: http://docs.moodle.org/en/Development:Coding_style ) 2. The patch is not a appropriate fix, it is basically copying the code from local/lib.php, the problem is actaully copy_to_area is called before filesize limit check.
          Hide
          Ankit Agarwal added a comment -

          Hi Dongsheng,

          Thanks for the comments.

          I figured out that we need to check the filesize before we make a call to copy_to_area function.But I didnt get any pre-defined function of directly checking the filesize of the local files.Did i missed something in the docs?

          On the other note..i am trying to get familiar with the moodle coding style..will keep your suggestion in mind.

          Thanks

          Show
          Ankit Agarwal added a comment - Hi Dongsheng, Thanks for the comments. I figured out that we need to check the filesize before we make a call to copy_to_area function.But I didnt get any pre-defined function of directly checking the filesize of the local files.Did i missed something in the docs? On the other note..i am trying to get familiar with the moodle coding style..will keep your suggestion in mind. Thanks
          Hide
          Ankit Agarwal added a comment -

          As suggested by Dongsheng ,i have used a method instead of hard coding the process.
          attached patch for the same.
          Thanks

          Show
          Ankit Agarwal added a comment - As suggested by Dongsheng ,i have used a method instead of hard coding the process. attached patch for the same. Thanks
          Ankit Agarwal made changes -
          Attachment patch for MDL--26890.patch [ 23422 ]
          Hide
          Dongsheng Cai added a comment -

          Thanks for the patch, it looks good to me, raised the priority of this issue, added to stable backlog.

          Show
          Dongsheng Cai added a comment - Thanks for the patch, it looks good to me, raised the priority of this issue, added to stable backlog.
          Dongsheng Cai made changes -
          Labels triaged
          Fix Version/s STABLE backlog [ 10463 ]
          Priority Major [ 3 ] Critical [ 2 ]
          Helen Foster made changes -
          Labels triaged patch triaged
          Martin Dougiamas made changes -
          Workflow MDL Workflow [ 68740 ] MDL Full Workflow [ 76165 ]
          moodle.com made changes -
          Fix Version/s STABLE Sprint 14 [ 11050 ]
          Fix Version/s STABLE backlog [ 10463 ]
          moodle.com made changes -
          Assignee Dongsheng Cai [ dongsheng ] Ankit Agarwal [ ankit_frenz ]
          Ankit Agarwal made changes -
          Status Open [ 1 ] Development in progress [ 3 ]
          Ankit Agarwal made changes -
          Affects Version/s 2.1.1 [ 10750 ]
          Affects Version/s 2.0.4 [ 10652 ]
          Affects Version/s 2.0.2 [ 10421 ]
          Ankit Agarwal made changes -
          Pull Master Diff URL https://github.com/ankitagarwal/moodle/compare/master...MDL-26890-master
          Pull Master Branch MDL-26890-master
          Testing Instructions 1) Login as admin>goto my profile> private files
          2) Upload atleast two files, one less than 100 kb in size and another more than 100kb in size.
          3) Goto any forum click on edit settings under forum administration
          4) Change max attachment size to 100 kb and save.
          5) try to create a new topic and/or reply to existing topic
          6) try to attach files to the post from your private area
          7) You should be able to attach the file that has a size <100kb easily and should get an error for the other one.
          8) In the later case make sure the file is not present in the post after you close the file picker and make the post.
          Pull from Repository git://github.com/ankitagarwal/moodle.git
          Ankit Agarwal made changes -
          Status Development in progress [ 3 ] Waiting for peer review [ 10012 ]
          Sam Hemelryk made changes -
          Original Estimate 0 minutes [ 0 ]
          Remaining Estimate 0 minutes [ 0 ]
          Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
          Peer reviewer samhemelryk
          Hide
          Sam Hemelryk added a comment -

          Hi Ankit looks good to me.
          Only thing to do before submitting for integration is clean up the indenting of `return $filesize` lib.php line 1381.
          Other than that spot on.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Ankit looks good to me. Only thing to do before submitting for integration is clean up the indenting of `return $filesize` lib.php line 1381. Other than that spot on. Cheers Sam
          Sam Hemelryk made changes -
          Status Peer review in progress [ 10013 ] Development in progress [ 3 ]
          Hide
          Ankit Agarwal added a comment -

          Thanks Sam for the review

          Show
          Ankit Agarwal added a comment - Thanks Sam for the review
          Ankit Agarwal made changes -
          Status Development in progress [ 3 ] Waiting for integration review [ 10010 ]
          Hide
          Petr Škoda added a comment -

          oh!
          $filearea = clean_param($params['filearea'], PARAM_ALPHAEXT);
          $component = clean_param($params['component'], PARAM_ALPHAEXT);

          is very wrong, it is already in repository/lib.php:
          1/ unfortuantely we do not have PARAM_FRANKENSTYLE yet for component
          2/ PARAM_SAFEDIR might be best for the filearea

          I have create a new issue for that MDL-29401

          Show
          Petr Škoda added a comment - oh! $filearea = clean_param($params ['filearea'] , PARAM_ALPHAEXT); $component = clean_param($params ['component'] , PARAM_ALPHAEXT); is very wrong, it is already in repository/lib.php: 1/ unfortuantely we do not have PARAM_FRANKENSTYLE yet for component 2/ PARAM_SAFEDIR might be best for the filearea I have create a new issue for that MDL-29401
          Sam Hemelryk made changes -
          Currently in integration Yes
          Aparup Banerjee made changes -
          Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
          Integrator nebgor
          Hide
          Aparup Banerjee added a comment -

          just linking Petr's issue.

          Show
          Aparup Banerjee added a comment - just linking Petr's issue.
          Aparup Banerjee made changes -
          Link This issue has a non-specific relationship to MDL-29401 [ MDL-29401 ]
          Hide
          Aparup Banerjee added a comment - - edited

          Hi Ankit,
          I've just had a look at your patch and I have some notes:

          • there seems to be a consistent space after calls to the empty function, perhaps this could be your editor setting ?
          • as Petr said, PARAM_SAFEDIR does seem appropriate for filearea. (checkout clean_param() function)

          ps: you may want to watch MDL-29401 in case he's already got a patch there to validate component names.
          cheers,
          Aparup

          Show
          Aparup Banerjee added a comment - - edited Hi Ankit, I've just had a look at your patch and I have some notes: there seems to be a consistent space after calls to the empty function, perhaps this could be your editor setting ? as Petr said, PARAM_SAFEDIR does seem appropriate for filearea. (checkout clean_param() function) ps: you may want to watch MDL-29401 in case he's already got a patch there to validate component names. cheers, Aparup
          Hide
          Aparup Banerjee added a comment -

          Thanks for the fixes , they've now been integrated.

          Show
          Aparup Banerjee added a comment - Thanks for the fixes , they've now been integrated.
          Aparup Banerjee made changes -
          Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
          Affects Version/s 2.2 [ 10656 ]
          Michael de Raadt made changes -
          Tester salvetore
          Michael de Raadt made changes -
          Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
          Hide
          Michael de Raadt added a comment -

          Test result: passed. Well done!

          Show
          Michael de Raadt added a comment - Test result: passed. Well done!
          Michael de Raadt made changes -
          Status Testing in progress [ 10011 ] Tested [ 10006 ]
          Eloy Lafuente (stronk7) made changes -
          Fix Version/s 2.0.5 [ 10950 ]
          Fix Version/s 2.1.2 [ 10851 ]
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And this code has been spread to all Moodle git and cvs repositories. Many thanks! Closing.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - And this code has been spread to all Moodle git and cvs repositories. Many thanks! Closing. Ciao
          Eloy Lafuente (stronk7) made changes -
          Status Tested [ 10006 ] Closed [ 6 ]
          Resolution Fixed [ 1 ]
          Currently in integration Yes
          Integration date 21/Sep/10
          Eloy Lafuente (stronk7) made changes -
          Fix Version/s STABLE Sprint 14 [ 11050 ]

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: