Uploaded image for project: '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 Master Branch:
      MDL-38474-master

      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

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            fred 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
            fred 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
            schach 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
            schach 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
            gbridge Gordon Bridge added a comment -

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

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

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

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

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

            Show
            marina Marina Glancy added a comment - btw should not we add tests for Course files repository as well?
            Hide
            fred 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
            fred 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
            stronk7 Eloy Lafuente (stronk7) added a comment -

            +1, thanks Fred, for asking my concerns!

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

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

            Show
            salvetore Michael de Raadt added a comment - Thanks for responding quickly on this. Partners are now involved and following our actions on this one.
            Hide
            fred 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
            fred 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
            fred Frédéric Massart added a comment -

            Submitting for integration. Thanks!

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

            Starting review of this for integration out of cycle.

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

            Added an issue to create units tests for this regression.

            Show
            damyon Damyon Wiese added a comment - Added an issue to create units tests for this regression.
            Hide
            damyon 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 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
            dmonllao 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
            dmonllao 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
            salvetore Michael de Raadt added a comment -

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

            Show
            salvetore Michael de Raadt added a comment - I'll take MDK-37852. Adrian and Ankit, can you please split MDL-36426?
            Hide
            ankit_frenz 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_frenz 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 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 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
            abgreeve 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
            abgreeve 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 Damyon Wiese added a comment -

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

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

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

            Show
            ankit_frenz Ankit Agarwal added a comment - MDL-36426 Looks fine to me on 24 and master. Thanks
            Hide
            damyon 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 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
            dmonllao David Monllaó added a comment -

            MDL-38474 testing instructions working as expected

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

            MDL-37852 now completes as expected.

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

            Passing it

            Show
            dmonllao David Monllaó added a comment - Passing it
            Hide
            stronk7 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
            stronk7 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
            siete Jorge Ramos added a comment -

            Thank you very much!!!

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

            Show
            siete Jorge Ramos added a comment - Thank you very much!!! --------- Muchas Gracias a todos!!!
            Hide
            marina 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 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
            fred 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
            fred 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:
                  Fix Release Date:
                  18/Mar/13