Moodle
  1. Moodle
  2. MDL-38474

Teachers cannot access Server files (No permission to access)

    Details

    • Testing Instructions:
      Hide

      Test pre-requisites

      1. Create a 1.9 and add some course files
      2. Use the 1.9 instance and upgrade it to 2.2, then to 2.3, 2.4 and master to test each version against it.
      3. Enable Server files repository

      Test steps

      1. Login as a teacher
      2. Add a folder to a course and add files:
        • Make sure you can browse the Server files repository
        • Make sure you can browse the Course files repository (legacy from 1.9)
        • Make sure any other repository does not throw the error 'No permission to access'

      Test 2

      1. Repeat MDL-36426 testing instructions (I'm sorry!)

      Test 3

      1. Repeat MDL-37852 testing instructions (I'm very sorry!)
      Show
      Test pre-requisites Create a 1.9 and add some course files Use the 1.9 instance and upgrade it to 2.2, then to 2.3, 2.4 and master to test each version against it. Enable Server files repository Test steps Login as a teacher Add a folder to a course and add files: Make sure you can browse the Server files repository Make sure you can browse the Course files repository (legacy from 1.9) Make sure any other repository does not throw the error 'No permission to access' Test 2 Repeat MDL-36426 testing instructions (I'm sorry!) Test 3 Repeat MDL-37852 testing instructions (I'm very sorry!)
    • Workaround:
      Hide
      1. Navigate to Home ► Site administration ► Users ► Permissions ► Define roles
      2. Create a new role
        • Call it Server files
        • Allow the capability repository/local:view
      3. Navigate to Home ► Site administration ► Users ► Permissions ► Assign system roles
      4. Select Server files
      5. Add the users who are teachers in some courses in this list
      Show
      Navigate to Home ► Site administration ► Users ► Permissions ► Define roles Create a new role Call it Server files Allow the capability repository/local:view Navigate to Home ► Site administration ► Users ► Permissions ► Assign system roles Select Server files Add the users who are teachers in some courses in this list
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
    • Pull Master Branch:
      MDL-38474-master
    • Rank:
      48444

      Description

      Teachers (or any other role on a course context), are not able to browse the Server files repository. Error message: "No permission to access this repository"

      This is a regression created by MDL-36426 when the security of the repositories have been enhanced.

      The problem here being that the Server files repository (repository/local) is defined on a system context, but browse course files. So the logic checks that the user has the capability on a system context, who doesn't (cannot be a teacher on a system level).

      A workaround is to create a new role, with the only capability repository/local:view set to allowed, and give that role to teachers but on a system level.

      This could apply to coursefiles too

        Issue Links

          Activity

          Hide
          Frédéric Massart added a comment -

          Sending for peer review for a sanity check. The repositories got me really confused and I'd like to make sure I'm doing the right thing. Once reviewed and fixed, I'll backport the branches.

          Show
          Frédéric Massart added a comment - Sending for peer review for a sanity check. The repositories got me really confused and I'd like to make sure I'm doing the right thing. Once reviewed and fixed, I'll backport the branches.
          Hide
          Heiko Schach added a comment -

          Hi Frédéric
          Good to see this bug getting fixed.
          I suppose you could raise the priority to major.

          Show
          Heiko Schach added a comment - Hi Frédéric Good to see this bug getting fixed. I suppose you could raise the priority to major.
          Hide
          Gordon Bridge added a comment -

          I updated to 2.4.2 yesterday and am experiencing this same bug.

          Show
          Gordon Bridge added a comment - I updated to 2.4.2 yesterday and am experiencing this same bug.
          Hide
          Marina Glancy added a comment -

          looks good Fred. We are backporting it to previous versions as well, right?

          Show
          Marina Glancy added a comment - looks good Fred. We are backporting it to previous versions as well, right?
          Hide
          Marina Glancy added a comment -

          btw should not we add tests for Course files repository as well?

          Show
          Marina Glancy added a comment - btw should not we add tests for Course files repository as well?
          Hide
          Frédéric Massart added a comment -

          Yes, I am currently preparing 1.9 to test the course files repository. I will backport the patches down to 2.2 after a successful peer review.

          Show
          Frédéric Massart added a comment - Yes, I am currently preparing 1.9 to test the course files repository. I will backport the patches down to 2.2 after a successful peer review.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          +1, thanks Fred, for asking my concerns!

          Show
          Eloy Lafuente (stronk7) added a comment - +1, thanks Fred, for asking my concerns!
          Hide
          Michael de Raadt added a comment -

          Thanks for responding quickly on this. Partners are now involved and following our actions on this one.

          Show
          Michael de Raadt added a comment - Thanks for responding quickly on this. Partners are now involved and following our actions on this one.
          Hide
          Frédéric Massart added a comment -

          Sorry for breaking your Moodle guys, I have secured the repositories a bit too much apparently . Anyway, here are patches for 2.2 onwards. That would be great if some of the watchers of this issue could (quickly?) test the patch.

          In a nutshell, this is what happens:

          When browsing a repository using the File Manager (repository_ajax.php, filepicker.js), the context passed to the repository instance is the current one. This context is used to verify that the user can access its content in repository::check_capability(). To ensure that the user is not trying to pass another $contextid (as a GET parameter) to be granted an access to a repository, I've added some context validation. So we're not using the context of the repository to check the capability, but to validate the context passed. This solves the problem of teachers not having a capability on a system context.

          Show
          Frédéric Massart added a comment - Sorry for breaking your Moodle guys, I have secured the repositories a bit too much apparently . Anyway, here are patches for 2.2 onwards. That would be great if some of the watchers of this issue could (quickly?) test the patch. In a nutshell, this is what happens: When browsing a repository using the File Manager (repository_ajax.php, filepicker.js), the context passed to the repository instance is the current one. This context is used to verify that the user can access its content in repository::check_capability(). To ensure that the user is not trying to pass another $contextid (as a GET parameter) to be granted an access to a repository, I've added some context validation. So we're not using the context of the repository to check the capability, but to validate the context passed. This solves the problem of teachers not having a capability on a system context.
          Hide
          Frédéric Massart added a comment -

          Submitting for integration. Thanks!

          Show
          Frédéric Massart added a comment - Submitting for integration. Thanks!
          Hide
          Damyon Wiese added a comment -

          Starting review of this for integration out of cycle.

          Show
          Damyon Wiese added a comment - Starting review of this for integration out of cycle.
          Hide
          Damyon Wiese added a comment -

          Added an issue to create units tests for this regression.

          Show
          Damyon Wiese added a comment - Added an issue to create units tests for this regression.
          Hide
          Damyon Wiese added a comment -

          Thanks Fred,

          I have integrated this to 22, 23, 24 and master branches. I set the fix version to the released versions because I think we will do something special to get this patch out in those versions.

          Show
          Damyon Wiese added a comment - Thanks Fred, I have integrated this to 22, 23, 24 and master branches. I set the fix version to the released versions because I think we will do something special to get this patch out in those versions.
          Hide
          David Monllaó added a comment -

          I'll test this issue's testing instructions with the upgrades and server files and you (Michael, Adrian and Ankit) can split MDL-36426 and MDL-37852 as you wish, between branches or repository types.

          Show
          David Monllaó added a comment - I'll test this issue's testing instructions with the upgrades and server files and you (Michael, Adrian and Ankit) can split MDL-36426 and MDL-37852 as you wish, between branches or repository types.
          Hide
          Michael de Raadt added a comment -

          I'll take MDK-37852. Adrian and Ankit, can you please split MDL-36426?

          Show
          Michael de Raadt added a comment - I'll take MDK-37852. Adrian and Ankit, can you please split MDL-36426?
          Hide
          Ankit Agarwal added a comment -

          Noticed that webdav repos are not listing in the non-js version , but seems it was the same before these changes were made

          Show
          Ankit Agarwal added a comment - Noticed that webdav repos are not listing in the non-js version , but seems it was the same before these changes were made
          Hide
          Damyon Wiese added a comment -

          I just pushed an additional patch from Fred for the 23 and 24 branches to fix an undefined variable.

          Show
          Damyon Wiese added a comment - I just pushed an additional patch from Fred for the 23 and 24 branches to fix an undefined variable.
          Hide
          Adrian Greeve added a comment -

          I ran through the MDL-36426 testing instructions on 2.2 and 2.3. After a couple of hiccups everything is now working properly.

          Show
          Adrian Greeve added a comment - I ran through the MDL-36426 testing instructions on 2.2 and 2.3. After a couple of hiccups everything is now working properly.
          Hide
          Damyon Wiese added a comment -

          Added an issue MDL-38500 - but it is an existing bug and should be worked on separately.

          Show
          Damyon Wiese added a comment - Added an issue MDL-38500 - but it is an existing bug and should be worked on separately.
          Hide
          Ankit Agarwal added a comment -

          MDL-36426 Looks fine to me on 24 and master.
          Thanks

          Show
          Ankit Agarwal added a comment - MDL-36426 Looks fine to me on 24 and master. Thanks
          Hide
          Damyon Wiese added a comment -

          I just pushed an additional patch from Fred for the master branch to fix editing a repository instance in a course.

          Show
          Damyon Wiese added a comment - I just pushed an additional patch from Fred for the master branch to fix editing a repository instance in a course.
          Hide
          David Monllaó added a comment -

          MDL-38474 testing instructions working as expected

          Show
          David Monllaó added a comment - MDL-38474 testing instructions working as expected
          Hide
          Michael de Raadt added a comment -

          MDL-37852 now completes as expected.

          Show
          Michael de Raadt added a comment - MDL-37852 now completes as expected.
          Hide
          David Monllaó added a comment -

          Passing it

          Show
          David Monllaó added a comment - Passing it
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Closing thanks!

          Note: weeklies with this fix have been already rolled to git repositories and http://download.moodle.org . Also new point releases 2.4.3, 2.3.6 and 2.2.9 will happen next Monday 18th March.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Closing thanks! Note: weeklies with this fix have been already rolled to git repositories and http://download.moodle.org . Also new point releases 2.4.3, 2.3.6 and 2.2.9 will happen next Monday 18th March. Ciao
          Hide
          Jorge Ramos added a comment -

          Thank you very much!!!

          ---------
          Muchas Gracias a todos!!!

          Show
          Jorge Ramos added a comment - Thank you very much!!! --------- Muchas Gracias a todos!!!
          Hide
          Marina Glancy added a comment -

          I know this has been integrated, don't beat me please, I did mention it to Fred but was not persuasive enough. When can it happen that filepicker is accessed from block context? And if it ever happens, why are you sure that block is on the course page and not on system page? There would be a fatal error when you call $coursecontext->instanceid

          Show
          Marina Glancy added a comment - I know this has been integrated, don't beat me please, I did mention it to Fred but was not persuasive enough. When can it happen that filepicker is accessed from block context? And if it ever happens, why are you sure that block is on the course page and not on system page? There would be a fatal error when you call $coursecontext->instanceid
          Hide
          Frédéric Massart added a comment -

          That's a good point Marina. But I don't think that would ever cause an issue. If you are accessing the File Manager from the block context, assuming it's not in a course, then the repositories listed should not contain the course instances. If you want to access course instances, you have to be in a course, whether it's from a block, or somewhere else.

          Show
          Frédéric Massart added a comment - That's a good point Marina. But I don't think that would ever cause an issue. If you are accessing the File Manager from the block context, assuming it's not in a course, then the repositories listed should not contain the course instances. If you want to access course instances, you have to be in a course, whether it's from a block, or somewhere else.

            People

            • Votes:
              6 Vote for this issue
              Watchers:
              15 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: