Moodle
  1. Moodle
  2. MDL-31789

Teacher can not access a local system file repository in his course.

    Details

    • Database:
      MySQL
    • Testing Instructions:
      Hide

      Test case 1:

      Clean installation Moodle 2.2.1.
      Configure the repositoyr extension with System File repository. And check permission to create a repository folder into a course.
      Create a simple course with one teacher enrol it.
      Create a repository folder into a course.
      Acces Moodle with user teacher into this course. And try to create a file resource.
      Editing form using filepicker to attach a file. In the filepicker teacher can acces a repository folder course.

      Test case 2:

      1. As admin or teacher, open Filepicker inside any course (from TinyMCE or filemanager) and make sure that repository 'Server files' is in the current course folder by default.
      2. As admin enable legacy files in settings
      3. As admin enable Legacy files repository
      4. In one of the courses enable legacy files in settings (last one!)
      5. Add some files to course legacy files
      5. As teacher/admin make sure you can access legacy files in current course from filepicker

      Show
      Test case 1: Clean installation Moodle 2.2.1. Configure the repositoyr extension with System File repository. And check permission to create a repository folder into a course. Create a simple course with one teacher enrol it. Create a repository folder into a course. Acces Moodle with user teacher into this course. And try to create a file resource. Editing form using filepicker to attach a file. In the filepicker teacher can acces a repository folder course. Test case 2: 1. As admin or teacher, open Filepicker inside any course (from TinyMCE or filemanager) and make sure that repository 'Server files' is in the current course folder by default. 2. As admin enable legacy files in settings 3. As admin enable Legacy files repository 4. In one of the courses enable legacy files in settings (last one!) 5. Add some files to course legacy files 5. As teacher/admin make sure you can access legacy files in current course from filepicker
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull Master Branch:
      wip-MDL-31789-master

      Description

      In Moodle 2.2.1+ latest stable release a teacher can access into filepicker in a local system file repository in his course.
      Moodle 2.2.1+ (Build: 20120223)

        Gliffy Diagrams

        1. Repository.odt
          657 kB
          Beverley Booker
        1. moodle221repository.png
          92 kB

          Issue Links

            Activity

            Carlos Escobedo Orea created issue -
            Carlos Escobedo Orea made changes -
            Field Original Value New Value
            Attachment moodle221repository.png [ 27091 ]
            Carlos Escobedo Orea made changes -
            Labels partner
            Hide
            Carlos Escobedo Orea added a comment -

            Attachments LoginForumView.jmw is wrong, please remove it.

            Show
            Carlos Escobedo Orea added a comment - Attachments LoginForumView.jmw is wrong, please remove it.
            Helen Foster made changes -
            Attachment LoginForumView.jmx [ 27090 ]
            Hide
            Helen Foster added a comment -

            Attachment removed as requested.

            Show
            Helen Foster added a comment - Attachment removed as requested.
            Hide
            Carlos Escobedo Orea added a comment -

            Thanks a lot Helen.

            Show
            Carlos Escobedo Orea added a comment - Thanks a lot Helen.
            Beverley Booker made changes -
            Attachment Repository.odt [ 27350 ]
            Hide
            Beverley Booker added a comment -

            There is a discussion on this:

            http://moodle.org/mod/forum/discuss.php?d=195768

            Allowing server file access to authenticated users is a security issue, this is not an acceptable workaround.

            This is a major bug!

            Show
            Beverley Booker added a comment - There is a discussion on this: http://moodle.org/mod/forum/discuss.php?d=195768 Allowing server file access to authenticated users is a security issue, this is not an acceptable workaround. This is a major bug!
            Stephen Bourget made changes -
            Link This issue has been marked as being related by MDL-31980 [ MDL-31980 ]
            Dongsheng Cai made changes -
            Link This issue is a regression caused by MDL-30452 [ MDL-30452 ]
            Hide
            Heikki Wilenius added a comment -

            Agreed, this should be a major issue.

            What are the security risks in allowing system file repository for authenticated users? We are thinking whether to enable this in our 2.2 test install.

            Show
            Heikki Wilenius added a comment - Agreed, this should be a major issue. What are the security risks in allowing system file repository for authenticated users? We are thinking whether to enable this in our 2.2 test install.
            Hide
            Juan Leyva added a comment -

            This seems to be related to:

            http://tracker.moodle.org/browse/MDL-30452

            Show
            Juan Leyva added a comment - This seems to be related to: http://tracker.moodle.org/browse/MDL-30452
            Hide
            Juan Leyva added a comment -

            This is what happened:

            • This is stopping current Teachers to use their course levels file system repositories
            • The workaround is:
              • Change the authenticated user's repository/filesystem:view at system level to Allow

            Optional:
            – Change the student's repository/filesystem:view at course level to Not allow
            – Change the teacher's repository/filesystem:view at system level to Not Allow

            The current workaround is just to change permissions and test if it works

            I've talked with Dong (the Repos maintainer) and he is going to perform some tests on this issue, once completed, he'll update this issue

            I'm going to ask to Michael also to raise the priority of this issue

            Show
            Juan Leyva added a comment - This is what happened: New Moodle versions since January 2012 has the system authenticated user permissions for view file repositories disabled http://tracker.moodle.org/browse/MDL-30452 This is stopping current Teachers to use their course levels file system repositories The workaround is: Change the authenticated user's repository/filesystem:view at system level to Allow Optional: – Change the student's repository/filesystem:view at course level to Not allow – Change the teacher's repository/filesystem:view at system level to Not Allow The current workaround is just to change permissions and test if it works I've talked with Dong (the Repos maintainer) and he is going to perform some tests on this issue, once completed, he'll update this issue I'm going to ask to Michael also to raise the priority of this issue
            Martin Dougiamas made changes -
            Link This issue is duplicated by MDL-31980 [ MDL-31980 ]
            Martin Dougiamas made changes -
            Link This issue has been marked as being related by MDL-31980 [ MDL-31980 ]
            Martin Dougiamas made changes -
            Priority Minor [ 4 ] Blocker [ 1 ]
            Martin Dougiamas made changes -
            Assignee Dongsheng Cai [ dongsheng ] moodle.com [ moodle.com ]
            Michael de Raadt made changes -
            Fix Version/s STABLE backlog [ 10463 ]
            Labels partner partner triaged
            Hide
            Dongsheng Cai added a comment - - edited

            OK, I figured out, MDL-30452 triggered this issue, but the real problem is the refactor of accesslib.php: MDL-29602.

            context is an object with protected id, level, depth after refactoring, when formlib generated options for filepicker/editor/filemanager using json_encode(), we got an empty object, then no contextid send to filepicker ajax script, when no context id provided, ajax script will assume this is in system context, two solution for this issue:

            1. implement __toString() in context class to dump json string, doesn't look quite nice in this way
            2. Manually construct context object with public properties in formlib, make sure json_encode works as expected
            Show
            Dongsheng Cai added a comment - - edited OK, I figured out, MDL-30452 triggered this issue, but the real problem is the refactor of accesslib.php: MDL-29602 . context is an object with protected id, level, depth after refactoring, when formlib generated options for filepicker/editor/filemanager using json_encode(), we got an empty object, then no contextid send to filepicker ajax script, when no context id provided, ajax script will assume this is in system context, two solution for this issue: implement __toString() in context class to dump json string, doesn't look quite nice in this way Manually construct context object with public properties in formlib, make sure json_encode works as expected
            Dongsheng Cai made changes -
            Link This issue has a non-specific relationship to MDL-29602 [ MDL-29602 ]
            Marina Glancy made changes -
            Assignee moodle.com [ moodle.com ] Marina Glancy [ marina ]
            Marina Glancy made changes -
            Comment [ ugly solution I can think so far of:
            {code}

            diff --git a/lib/editor/tinymce/lib.php b/lib/editor/tinymce/lib.php
            index 77a2756..062dfc5 100644
            --- a/lib/editor/tinymce/lib.php
            +++ b/lib/editor/tinymce/lib.php
            @@ -77,6 +77,9 @@ class tinymce_texteditor extends texteditor {
                     }
                     $PAGE->requires->js_init_call('M.editor_tinymce.init_editor', array($elementid, $this->get_init_params($elementid, $options)), true);
                     if ($fpoptions) {
            + $fpoptions['image']->context = array('id' => $fpoptions['image']->context->id);
            + $fpoptions['media']->context = array('id' => $fpoptions['media']->context->id);
            + $fpoptions['link']->context = array('id' => $fpoptions['link']->context->id);
                         $PAGE->requires->js_init_call('M.editor_tinymce.init_filepicker', array($elementid, $fpoptions), true);
                     }
                 }
            diff --git a/lib/form/filemanager.php b/lib/form/filemanager.php
            index 213ecc6..483674c 100644
            --- a/lib/form/filemanager.php
            +++ b/lib/form/filemanager.php
            @@ -365,6 +365,7 @@ function form_filemanager_render($options) {
                 } else {
                     $course_maxbytes = $CFG->maxbytes;
                 }
            + $options->context = array('id' => $options->context->id);
             
                 if ($options->maxbytes == -1 || empty($options->maxbytes)) {
                     $options->maxbytes = $CFG->maxbytes;
            {code}

            I'll have to come back to this issue after my vacation ]
            Rajesh Taneja made changes -
            Link This issue is duplicated by MDL-30869 [ MDL-30869 ]
            Hide
            Rajesh Taneja added a comment -

            Please include testing instructions covering MDL-30869 (Duplicate bug)

            Show
            Rajesh Taneja added a comment - Please include testing instructions covering MDL-30869 (Duplicate bug)
            Dongsheng Cai made changes -
            Link This issue has been marked as being related by MDL-27156 [ MDL-27156 ]
            Marina Glancy made changes -
            Hide
            Marina Glancy added a comment -

            Petr, can you please peer review it.
            This issue is a regression caused by MDL-29602
            Thanks
            Marina

            Show
            Marina Glancy added a comment - Petr, can you please peer review it. This issue is a regression caused by MDL-29602 Thanks Marina
            Marina Glancy made changes -
            Status Open [ 1 ] Waiting for peer review [ 10012 ]
            Peer reviewer skodak
            Pull 2.2 Diff URL https://github.com/marinaglancy/moodle/compare/MOODLE_22_STABLE...wip-MDL-31789-MOODLE_22_STABLE
            Pull 2.2 Branch wip-MDL-31789-MOODLE_22_STABLE
            Marina Glancy made changes -
            Testing Instructions Clean installation Moodle 2.2.1.
            Configure the repositoyr extension with System File repository. And check permission to create a repository folder into a course.
            Create a simple course with one teacher enrol it.
            Create a repository folder into a course.
            Acces Moodle with user teacher into this course. And try to create a file resource.
            Editing form using filepicker to attach a file. In the filepicker teacher can acces a repository folder course.

            Test case 1:

            Clean installation Moodle 2.2.1.
            Configure the repositoyr extension with System File repository. And check permission to create a repository folder into a course.
            Create a simple course with one teacher enrol it.
            Create a repository folder into a course.
            Acces Moodle with user teacher into this course. And try to create a file resource.
            Editing form using filepicker to attach a file. In the filepicker teacher can acces a repository folder course.

            Test case 2:

            1. As admin or teacher, open Filepicker inside any course (from TinyMCE or filemanager) and make sure that repository 'Server files' is in the current course folder by default.
            2. As admin enable legacy files in settings
            3. As admin enable Legacy files repository
            4. In one of the courses enable legacy files in settings (last one!)
            5. Add some files to course legacy files
            5. As teacher/admin make sure you can access legacy files in current course from filepicker
            Hide
            Petr Skoda added a comment -

            +1

            Show
            Petr Skoda added a comment - +1
            Marina Glancy made changes -
            Status Waiting for peer review [ 10012 ] Waiting for integration review [ 10010 ]
            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
            Aparup Banerjee made changes -
            Integrator nebgor
            Sam Hemelryk made changes -
            Currently in integration Yes [ 10041 ]
            Aparup Banerjee made changes -
            Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
            Hide
            Aparup Banerjee added a comment -

            Thanks for this fix Marina.

            This is up for integration testing on MOODLE_22_STABLE and master.

            Show
            Aparup Banerjee added a comment - Thanks for this fix Marina. This is up for integration testing on MOODLE_22_STABLE and master.
            Aparup Banerjee made changes -
            Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
            Affects Version/s 2.2.2 [ 11552 ]
            Affects Version/s 2.3 [ 10657 ]
            Affects Version/s 2.2.1 [ 11456 ]
            Fix Version/s 2.2.3 [ 12053 ]
            Michael de Raadt made changes -
            Tester ankit_frenz
            Ankit Agarwal made changes -
            Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
            Hide
            Ankit Agarwal added a comment -

            Hi,
            I and Apu tried to test it, seems to be working expect a few things:-

            ->"In the file-picker teacher can access a repository folder course." I was able to access the file system repo via the file picker, but it displayed only the files that were uploaded to the folder via the linux file system. All files uploaded via moodle (course>folder res>edit>add) doesn't seem to be present here. I guess that is expected behavior?
            -> We are not sure what exactly am supposed to verify by this "make sure that repository 'Server files' is in the current course folder by default."

            -> If you create a file system repo based on a existing folder on your system. You can see its files if you goto course>folder resources>edit>add>filepicker but when you try to access it from tinymce>insert image> upload image> filepicker , the files donot appear. This seems to be an issue even before the patch.

            Thanks

            Show
            Ankit Agarwal added a comment - Hi, I and Apu tried to test it, seems to be working expect a few things:- ->"In the file-picker teacher can access a repository folder course." I was able to access the file system repo via the file picker, but it displayed only the files that were uploaded to the folder via the linux file system. All files uploaded via moodle (course>folder res>edit>add) doesn't seem to be present here. I guess that is expected behavior? -> We are not sure what exactly am supposed to verify by this "make sure that repository 'Server files' is in the current course folder by default." -> If you create a file system repo based on a existing folder on your system. You can see its files if you goto course>folder resources>edit>add>filepicker but when you try to access it from tinymce>insert image> upload image> filepicker , the files donot appear. This seems to be an issue even before the patch. Thanks
            Hide
            Dan Poltawski added a comment -

            These testing instructions are confusing

            All files uploaded via moodle (course>folder res>edit>add) doesn't seem to be present here. I guess that is expected behavior?

            Yes - the filesystem repository only contains files uploaded to the filesystem, it does not interact with moodle files uploads.

            We are not sure what exactly am supposed to verify by this "make sure that repository 'Server files' is in the current course folder by default."

            I think that this means ensure that the server files repository is enabled for the current course.

            If you create a file system repo based on a existing folder on your system. You can see its files if you goto course>folder resources>edit>add>filepicker but when you try to access it from tinymce>insert image> upload image> filepicker , the files donot appear. This seems to be an issue even before the patch.

            Yes, this repository doesn't define 'supported_filetypes' I do not think this testing instruciton is valid and so I think that this can be passed.

            Show
            Dan Poltawski added a comment - These testing instructions are confusing All files uploaded via moodle (course>folder res>edit>add) doesn't seem to be present here. I guess that is expected behavior? Yes - the filesystem repository only contains files uploaded to the filesystem, it does not interact with moodle files uploads. We are not sure what exactly am supposed to verify by this "make sure that repository 'Server files' is in the current course folder by default." I think that this means ensure that the server files repository is enabled for the current course. If you create a file system repo based on a existing folder on your system. You can see its files if you goto course>folder resources>edit>add>filepicker but when you try to access it from tinymce>insert image> upload image> filepicker , the files donot appear. This seems to be an issue even before the patch. Yes, this repository doesn't define 'supported_filetypes' I do not think this testing instruciton is valid and so I think that this can be passed.
            Hide
            Ankit Agarwal added a comment -

            Had a talk with Dan and Rajesh...Looks like this issue can be passed now.
            Thanks

            Show
            Ankit Agarwal added a comment - Had a talk with Dan and Rajesh...Looks like this issue can be passed now. Thanks
            Ankit Agarwal made changes -
            Status Testing in progress [ 10011 ] Tested [ 10006 ]
            Hide
            Aparup Banerjee added a comment -

            The code here has been spread to upstream moodle repositories and mirrors for anyone to use .

            Closing, have a good weekend!

            Show
            Aparup Banerjee added a comment - The code here has been spread to upstream moodle repositories and mirrors for anyone to use . Closing, have a good weekend!
            Aparup Banerjee made changes -
            Status Tested [ 10006 ] Closed [ 6 ]
            Resolution Fixed [ 1 ]
            Currently in integration Yes [ 10041 ]
            Integration date 05/Apr/12
            Rajesh Taneja made changes -
            Comment [ One of the linked issues (MDL-30869) was reported for 2.1. Can you please suggest why this was not ported on 2.1 ? ]
            Hide
            Gabriel Mazetto added a comment -

            convert_to_array messes with valid data.

            For example, it doesn't accept:

            array('first' => array('second'), 'third' => array('second'));

            Please check MDL-35001 for more details.

            Show
            Gabriel Mazetto added a comment - convert_to_array messes with valid data. For example, it doesn't accept: array('first' => array('second'), 'third' => array('second')); Please check MDL-35001 for more details.
            Gabriel Mazetto made changes -
            Link This issue caused a regression MDL-34518 [ MDL-34518 ]
            Gabriel Mazetto made changes -
            Link This issue testing discovered MDL-35001 [ MDL-35001 ]
            Hide
            Jorge Ramos added a comment -

            Hi! We've upgrade 2.2.7 -> 2.2.8 and this bug is suddenly happening, with teacher role we cant access to Server Files nor Legacy Files...

            Can you reopened the issue??

            Show
            Jorge Ramos added a comment - Hi! We've upgrade 2.2.7 -> 2.2.8 and this bug is suddenly happening, with teacher role we cant access to Server Files nor Legacy Files... Can you reopened the issue??
            Hide
            Heiko Schach added a comment -

            After upgrading from 2.3.4 to 2.3.5 we have received reports from our teachers.
            Adding file from File picker doesn't work as it did before. The user gets an error: "No permission to access this repository."

            Steps to reproduce:
            Log in as teacher
            Turn editing on
            Add a resource... > File
            Content > Select Files > Add...

            Error message: "No permission to access this repository."

            Show
            Heiko Schach added a comment - After upgrading from 2.3.4 to 2.3.5 we have received reports from our teachers. Adding file from File picker doesn't work as it did before. The user gets an error: "No permission to access this repository." Steps to reproduce: Log in as teacher Turn editing on Add a resource... > File Content > Select Files > Add... Error message: "No permission to access this repository."
            Hide
            Dan Marsden added a comment -

            Hi Jorge, We don't re-open issues when a patch has been submitted/integrated into code - you should create a new bug describing the issues you are having and can add a link back to this bug as a reference.

            Show
            Dan Marsden added a comment - Hi Jorge, We don't re-open issues when a patch has been submitted/integrated into code - you should create a new bug describing the issues you are having and can add a link back to this bug as a reference.
            Hide
            Frédéric Massart added a comment -

            Hi, I have raised the issue MDL-38474 to take care of that error.

            Show
            Frédéric Massart added a comment - Hi, I have raised the issue MDL-38474 to take care of that error.
            Hide
            Jorge Ramos added a comment -

            Thank you very much Dan&Fréderic!!

            Show
            Jorge Ramos added a comment - Thank you very much Dan&Fréderic!!

              People

              • Votes:
                19 Vote for this issue
                Watchers:
                22 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: