Uploaded image for project: '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 (Obsolete):
      100

      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.

        Gliffy Diagrams

          Activity

          Hide
          marina 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 Marina Glancy added a comment - Is this a new issue in 2.3 or was it the same in 2.2 as well?
          Hide
          avilela 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
          avilela 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
          salvetore Michael de Raadt added a comment -

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

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

          this sounds like a problem with file_info and file_browser classes

          Show
          marina Marina Glancy added a comment - this sounds like a problem with file_info and file_browser classes
          Hide
          cfollin 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
          cfollin 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
          salvetore Michael de Raadt added a comment -

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

          Show
          salvetore 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 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 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 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 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 CiBoT added a comment -

          Results for MDL-33741

          Show
          cibot 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
          skodak Petr Skoda 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
          skodak Petr Skoda 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 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 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
          skodak Petr Skoda 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
          skodak Petr Skoda 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.
          Hide
          salvetore Michael de Raadt added a comment -

          Removing this from the "Filepicker UI Polishing" epic so it can be worked on independently.

          Show
          salvetore Michael de Raadt added a comment - Removing this from the "Filepicker UI Polishing" epic so it can be worked on independently.

            People

            • Votes:
              5 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated: