Moodle

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 2.4 Diff URL:
  • Pull Master Branch:
    MDL-38474-master
  • Pull Master Diff URL:

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

has a non-specific relationship to

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-38452 Teachers cannot upload files on behalf of the students any more

  • Major - Major loss of function, incorrect output
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.

Improvement - An enhancement to an existing Moodle feature. MDL-38498 Add units tests for repository check_capability

  • Major - Major loss of function, incorrect output
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.
testing discovered

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-38500 Repository names have to be unique (not sure why) but no check on create, only edit.

  • Major - Major loss of function, incorrect output
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
Frédéric Massart added a comment -

Submitting for integration. Thanks!

Show
Frédéric Massart added a comment - Submitting for integration. Thanks!
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
David Monllaó added a comment -

Passing it

Show
David Monllaó added a comment - Passing it
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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

Vote (6)
Watch (15)

Dates

  • Created:
    Updated:
    Resolved: