Moodle
  1. Moodle
  2. MDL-33741

Teacher can't access course files in hidden categories using the Filepicker with the server files repository

    Details

    • Testing Instructions:
      Hide

      Test 1. Teacher

      1. As admin create two course categories
      2. Edit categories descriptions and add files in them
      3. In each category create a course, enrol user as a teacher in both of them.
      4. Make sure each course contains at least one file (in course or section summary or in a module)
      5. Hide one of the categories
      6. Copy the URL to the file in the description of the hidden category.
      7. Log in as a teacher
      8. You should be able to access both courses
      9. Inside one of the courses open filepicker and make sure in 'Server files' you can access files in both courses.
      10. Try to access file in the description of the hidden category by typing URL, this should be prohibited.

      Test 2. Manager

      1. Create a user who can access the hidden categories and manage all categories
      2. Make sure this user can access files in categories descriptions through Server files.
      Show
      Test 1. Teacher As admin create two course categories Edit categories descriptions and add files in them In each category create a course, enrol user as a teacher in both of them. Make sure each course contains at least one file (in course or section summary or in a module) Hide one of the categories Copy the URL to the file in the description of the hidden category. Log in as a teacher You should be able to access both courses Inside one of the courses open filepicker and make sure in 'Server files' you can access files in both courses. Try to access file in the description of the hidden category by typing URL, this should be prohibited. Test 2. Manager Create a user who can access the hidden categories and manage all categories Make sure this user can access files in categories descriptions through Server files.
    • Affected Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      wip-MDL-33741-master
    • Story Points:
      100
    • Rank:
      59

      Description

      Replication instructions:

      1. Login as administrator
      2. Create 2 categories
      3. Create two courses, one in each category and assign the same teacher to both.
      4. Create 1 file resource (with 1 file) on each course.
      5. Hide one of the categories
      6. Logout and login as the teacher
      7. Enter the course on the visible category
      8. Create a file resource
      9. Click on add file button to access the file Picker
      10. Select the server files repository
      11. Click on system to access the root dir

      Result: The course on the hidden category is not listed. If you try to do steps 7 to 11 as an admin, you will be able to see the course and the files.

        Activity

        Hide
        Marina Glancy added a comment -

        Is this a new issue in 2.3 or was it the same in 2.2 as well?

        Show
        Marina Glancy added a comment - Is this a new issue in 2.3 or was it the same in 2.2 as well?
        Hide
        Antonio Vilela added a comment -

        I didn't try it in 2.2. I am testing 2.3 and as FilePicker is having some changes on 2.3 I assumed it was a 2.3 neww issue.

        Show
        Antonio Vilela added a comment - I didn't try it in 2.2. I am testing 2.3 and as FilePicker is having some changes on 2.3 I assumed it was a 2.3 neww issue.
        Hide
        Michael de Raadt added a comment -

        This sounds like a problem with the repository, rather than the filepicker.

        Show
        Michael de Raadt added a comment - This sounds like a problem with the repository, rather than the filepicker.
        Hide
        Marina Glancy added a comment -

        this sounds like a problem with file_info and file_browser classes

        Show
        Marina Glancy added a comment - this sounds like a problem with file_info and file_browser classes
        Hide
        Chris Follin added a comment -

        We have this problem in 2.2.3, but we're upgrading to 2.3 before the end of this month so our concern is for 2.3 rather than 2.2.

        Show
        Chris Follin added a comment - We have this problem in 2.2.3, but we're upgrading to 2.3 before the end of this month so our concern is for 2.3 rather than 2.2.
        Hide
        Michael de Raadt added a comment -

        This issue looks like it was left behind. It would be good to give it some attention.

        Show
        Michael de Raadt added a comment - This issue looks like it was left behind. It would be good to give it some attention.
        Hide
        Marina Glancy added a comment -

        Petr do you mind reviewing it please.

        I removed the viewhiddencategories check for everything except category description (btw fixed 'manage' cap check for category description too)

        Please note that course categories names will be collapsed anyway in Server files if $CFG->navshowmycoursecategories is false. And if it is true, the hidden category name already appears in the breadcrumb when user accesses his own course.

        If this fix is fine with you we can backport it.

        Show
        Marina Glancy added a comment - Petr do you mind reviewing it please. I removed the viewhiddencategories check for everything except category description (btw fixed 'manage' cap check for category description too) Please note that course categories names will be collapsed anyway in Server files if $CFG->navshowmycoursecategories is false. And if it is true, the hidden category name already appears in the breadcrumb when user accesses his own course. If this fix is fine with you we can backport it.
        Hide
        Marina Glancy added a comment -

        I wrote the test and then tested it myself. Apparently there is no check for viewhiddencategories when serving a file in category description. Fixed in the second commit.

        Show
        Marina Glancy added a comment - I wrote the test and then tested it myself. Apparently there is no check for viewhiddencategories when serving a file in category description. Fixed in the second commit.
        Hide
        CiBoT added a comment -

        Results for MDL-33741

        Show
        CiBoT added a comment - Results for MDL-33741 Branch wip- MDL-33741 -master to be integrated into upstream master Executed job http://ci.stronk7.com/job/Precheck%20remote%20branch/429 Execution status: 0 Details: http://ci.stronk7.com/job/Precheck%20remote%20branch/429/artifact/work/smurf.html
        Hide
        Petr Škoda added a comment -

        Hmm, I am not sure this is a correct approach. What happens when teacher looks into the hidden category? They should not see any courses they are not enrolled in or if they do not at least have course:view capability. The category hiding is supposed to prevent listing of courses.

        Show
        Petr Škoda added a comment - Hmm, I am not sure this is a correct approach. What happens when teacher looks into the hidden category? They should not see any courses they are not enrolled in or if they do not at least have course:view capability. The category hiding is supposed to prevent listing of courses.
        Hide
        Marina Glancy added a comment -

        Well Petr, apparently they can see all courses they are enrolled in regardless on category visibility. We can't change it now as very many people use this bug as a feature to hide the courses from "Courses" list. Also it is possible to put course in a hidden category, and they manually change the course visibility status back to visible. In this case students can also access this course.

        At the if $CFG->navshowmycoursecategories is on, the hidden category name is displayed in the navbar as a link. If they click on the link they get an error "don't have permission". But it should be a separate bug.

        Show
        Marina Glancy added a comment - Well Petr, apparently they can see all courses they are enrolled in regardless on category visibility. We can't change it now as very many people use this bug as a feature to hide the courses from "Courses" list. Also it is possible to put course in a hidden category, and they manually change the course visibility status back to visible. In this case students can also access this course. At the if $CFG->navshowmycoursecategories is on, the hidden category name is displayed in the navbar as a link. If they click on the link they get an error "don't have permission". But it should be a separate bug.
        Hide
        Petr Škoda added a comment -

        These are the basic rules when dealing with hidden categories:
        1/ hidden categories prevent disclosing of its existence and browsing of courses
        2/ user may access course in hidden category if they are enrolled
        3/ file_browser is a tree that is similar to context tree - you can walk it from top down or upwards
        4/ if you are inside your course in hidden category the parent should be the system context (it is not at the moment)
        5/ my courses should be somehow presented at the top of the file_browser tree

        If I read your patch correctly it completely removes the prevention of hidden category browsing (this is a security issue imo) and replaces it with a hack in repository. Instead I believe the file_browser should be fixed to include list of my courses and the parent link from course in hidden category should be fixed too. Then the repository would have to remove duplicates before displaying the simplified flat structure to users.

        Show
        Petr Škoda added a comment - These are the basic rules when dealing with hidden categories: 1/ hidden categories prevent disclosing of its existence and browsing of courses 2/ user may access course in hidden category if they are enrolled 3/ file_browser is a tree that is similar to context tree - you can walk it from top down or upwards 4/ if you are inside your course in hidden category the parent should be the system context (it is not at the moment) 5/ my courses should be somehow presented at the top of the file_browser tree If I read your patch correctly it completely removes the prevention of hidden category browsing (this is a security issue imo) and replaces it with a hack in repository. Instead I believe the file_browser should be fixed to include list of my courses and the parent link from course in hidden category should be fixed too. Then the repository would have to remove duplicates before displaying the simplified flat structure to users.

          People

          • Votes:
            4 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

            • Created:
              Updated: