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. as admin enable $CFG->navshowmycoursecategories
      11. As teacher make sure you can see the categories names in the 'server files' only for courses that have all visible parents.
      12. 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. as admin enable $CFG->navshowmycoursecategories As teacher make sure you can see the categories names in the 'server files' only for courses that have all visible parents. 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, MOODLE_30_STABLE, MOODLE_31_STABLE
    • Fixed Branches:
      MOODLE_30_STABLE, MOODLE_31_STABLE
    • Pull 3.0 Branch:
      wip-MDL-33741-m30
    • Pull 3.1 Branch:
      wip-MDL-33741-m31
    • Pull Master Branch:
      wip-MDL-33741-master
    • Sprint:
      3.2 Sprint 3
    • Story Points (Obsolete):
      3.67
    • Sprint:
      3.2 Sprint 3

      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

          Attachments

            Issue Links

              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.
              Hide
              marina Marina Glancy added a comment -

              I have completely re-wrote my previous patch. Petr was correct, I was showing the names of hidden categories in case when $CFG->navshowmycoursecategories was enabled. I added this to the testing instructions too.

              There are couple of other small bugs that I found in file_info and fixed on the way. The explanations are in commit messages

              Show
              marina Marina Glancy added a comment - I have completely re-wrote my previous patch. Petr was correct, I was showing the names of hidden categories in case when $CFG->navshowmycoursecategories was enabled. I added this to the testing instructions too. There are couple of other small bugs that I found in file_info and fixed on the way. The explanations are in commit messages
              Hide
              cibot CiBoT added a comment -

              Fails against automated checks.

              Checked MDL-33741 using repository: git://github.com/marinaglancy/moodle.git

              More information about this report

              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-33741 using repository: git://github.com/marinaglancy/moodle.git master (1 errors / 1 warnings) [branch: wip-MDL-33741-master | CI Job ] phplint (0/0) , phpcs (0/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (1/1) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , More information about this report
              Hide
              cibot CiBoT added a comment -

              Code verified against automated checks.

              Checked MDL-33741 using repository: git://github.com/marinaglancy/moodle.git

              More information about this report

              Show
              cibot CiBoT added a comment - Code verified against automated checks. Checked MDL-33741 using repository: git://github.com/marinaglancy/moodle.git master (0 errors / 0 warnings) [branch: wip-MDL-33741-master | CI Job ] More information about this report
              Hide
              cameron1729 Cameron Ball added a comment -

              Thanks Marina, overall the patch looks fine and seems to work properly from my testing. Only some minor points:

              1. In a commit message you say "check cap when serving file in cat description" - I personally feel like we should say "capability" and "category" rather than "cap" and "cat". But up to you.
              2. Another commit message: "This also has couple bugfixes:" -> "This also has a couple of bugfixes"

              That's it really. Feel free to send for integration when you're ready. Cheers.

              Show
              cameron1729 Cameron Ball added a comment - Thanks Marina, overall the patch looks fine and seems to work properly from my testing. Only some minor points: In a commit message you say "check cap when serving file in cat description" - I personally feel like we should say "capability" and "category" rather than "cap" and "cat". But up to you. Another commit message: "This also has couple bugfixes:" -> "This also has a couple of bugfixes" That's it really. Feel free to send for integration when you're ready. Cheers.
              Hide
              marina Marina Glancy added a comment -

              Thanks for the review, Cameron, I corrected the commit mesages

              Show
              marina Marina Glancy added a comment - Thanks for the review, Cameron, I corrected the commit mesages
              Hide
              cibot CiBoT added a comment -
              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-33741 using repository: git://github.com/marinaglancy/moodle.git MOODLE_30_STABLE (1 errors / 0 warnings) [branch: wip-MDL-33741-m30 | CI Job ] phplint (0/0) , phpcs (0/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (1/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , MOODLE_31_STABLE (1 errors / 0 warnings) [branch: wip-MDL-33741-m31 | CI Job ] phplint (0/0) , phpcs (0/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (1/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , master (1 errors / 0 warnings) [branch: wip-MDL-33741-master | CI Job ] phplint (0/0) , phpcs (0/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (1/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , More information about this report
              Hide
              cibot CiBoT added a comment -

              Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

              Show
              cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
              Hide
              cibot CiBoT added a comment -
              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-33741 using repository: git://github.com/marinaglancy/moodle.git MOODLE_30_STABLE (1 errors / 0 warnings) [branch: wip-MDL-33741-m30 | CI Job ] phplint (0/0) , phpcs (0/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (1/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , MOODLE_31_STABLE (1 errors / 0 warnings) [branch: wip-MDL-33741-m31 | CI Job ] phplint (0/0) , phpcs (0/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (1/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , master (1 errors / 0 warnings) [branch: wip-MDL-33741-master | CI Job ] phplint (0/0) , phpcs (0/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (1/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , More information about this report
              Hide
              poltawski Dan Poltawski added a comment -

              Hi Marina,

              The code looks good - can we add any test coverage to this?

              Show
              poltawski Dan Poltawski added a comment - Hi Marina, The code looks good - can we add any test coverage to this?
              Hide
              marina Marina Glancy added a comment -

              I could not find any existing unittests for file_info, there are also neither unittests nor behat for repository_local
              I tried to create behat tests for repository_local but got stuck because there are no steps for checking repository contents in filepicker.

              I ran out of time today so if you think that bug fix issue should be rejected because of the missing tests - feel free to do it

              Show
              marina Marina Glancy added a comment - I could not find any existing unittests for file_info, there are also neither unittests nor behat for repository_local I tried to create behat tests for repository_local but got stuck because there are no steps for checking repository contents in filepicker. I ran out of time today so if you think that bug fix issue should be rejected because of the missing tests - feel free to do it
              Hide
              poltawski Dan Poltawski added a comment -

              Integrated to master, 31 and 30

              Show
              poltawski Dan Poltawski added a comment - Integrated to master, 31 and 30
              Hide
              marina Marina Glancy added a comment -

              I created MDL-55427 for behat tests

              Show
              marina Marina Glancy added a comment - I created MDL-55427 for behat tests
              Hide
              jpataleta Jun Pataleta added a comment -

              Thanks for working on this Marina. Works as expected.

              Tested on master, 31 and 30. Passing!

              Show
              jpataleta Jun Pataleta added a comment - Thanks for working on this Marina. Works as expected. Tested on master, 31 and 30. Passing!
              Hide
              poltawski Dan Poltawski added a comment -

              Thanks for your contributions! This change is now available from the main moodle.git repository and will shortly be available on download.moodle.org.

              Why do we never have time to do it right, but always have time to do it over?.
              --Anonymous

              Show
              poltawski Dan Poltawski added a comment - Thanks for your contributions! This change is now available from the main moodle.git repository and will shortly be available on download.moodle.org. Why do we never have time to do it right, but always have time to do it over?. --Anonymous

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    12/Sep/16