Moodle
  1. Moodle
  2. MDL-38452

Teachers cannot upload files on behalf of the students any more

    Details

    • Testing Instructions:
      Hide

      Test pre-requisites

      1. Grant authenticated users permission to view webdav repositories
      2. Enable the following repositories:
        • Webdav
        • Dropbox
        • Upload
        • Private files
      3. As Student A create a user instance of Webdav
      4. As Student A browse your Dropbox repository (do not logout)
      5. As Student A confirm you can upload a file to your private files

      Test steps

      1. As a teacher
      2. Add an assignment (with file upload enabled) to a course
      3. Use the login as feature to login as Student A
      4. Visit the assignment, and confirm
        • You can upload a file
        • You can browse the private files
        • You cannot browse the Webdav instance
        • You cannot browse the Dropbox repository

      Test 2

      1. Keep the configuration of the previous test
      2. Use the login as feature to login as the Student A
      3. Go to Home ► My profile ► Repositories ► Student A ► Repositories
      4. Confirm
        • You cannot edit the webdav instance
        • You cannot delete the webdav instance
      5. Login as the student (Do not use login as feature this time)
      6. Go to Home ► My profile ► Repositories ► Student A ► Repositories
      7. Confirm
        • You can edit the webdav instance
        • You can delete the webdav instance
      Show
      Test pre-requisites Grant authenticated users permission to view webdav repositories Enable the following repositories: Webdav Dropbox Upload Private files As Student A create a user instance of Webdav As Student A browse your Dropbox repository (do not logout) As Student A confirm you can upload a file to your private files Test steps As a teacher Add an assignment (with file upload enabled) to a course Use the login as feature to login as Student A Visit the assignment, and confirm You can upload a file You can browse the private files You cannot browse the Webdav instance You cannot browse the Dropbox repository Test 2 Keep the configuration of the previous test Use the login as feature to login as the Student A Go to Home ► My profile ► Repositories ► Student A ► Repositories Confirm You cannot edit the webdav instance You cannot delete the webdav instance Login as the student (Do not use login as feature this time) Go to Home ► My profile ► Repositories ► Student A ► Repositories Confirm You can edit the webdav instance You can delete the webdav instance
    • 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:
    • Rank:
      48407

      Description

      after patching to 2.4.2 our course managers can no longer use the loginas feature to login as a student and upload an assignment. The loginas appears to still work but when the upload is attempted "No Permission to access this repository" is displayed. Students can still login and upload files with no problem. This was working before we upgraded last week and I can find no information if a change to this was applied. We updated from 2.4.1+(Build: 20130118). This error is produced for managers in both the course or system context. This is a real work stopper as we upload files for students every day. This error is still present after upgrading to Moodle 2.4.3 (Build: 20130318)

      Steps to reproduce:
      Log in as Manager
      Browse to Course and Assignment you need to upload file for
      go into View/grade all submissions
      Click on Student you want to upload for
      Use "Login As" for that student and continue
      Go to Assignment
      Click add Submission
      then attempt to add by clicking add in filepicker or dragging file,

      Returns "No permission to access this repository"

        Issue Links

          Activity

          Hide
          Heiko Schach added a comment -

          After upgrading from 2.3.4 to 2.3.5 we have received reports from our teachers.
          Adding file from File picker doesn't work as it did before. The user gets an error: "No permission to access this repository."

          Steps to reproduce:
          Log in as teacher
          Turn editing on
          Add a resource... > File
          Content > Select Files > Add...

          Error message: "No permission to access this repository."

          Show
          Heiko Schach added a comment - After upgrading from 2.3.4 to 2.3.5 we have received reports from our teachers. Adding file from File picker doesn't work as it did before. The user gets an error: "No permission to access this repository." Steps to reproduce: Log in as teacher Turn editing on Add a resource... > File Content > Select Files > Add... Error message: "No permission to access this repository."
          Hide
          Trevor Cunnnigham added a comment -

          Looks like this is happening to us as well. Luckily I caught it locally before pushing the update.

          Upgraded from 2.3.4 to 2.3.5 and noticed the issue as Heiko describes.

          Show
          Trevor Cunnnigham added a comment - Looks like this is happening to us as well. Luckily I caught it locally before pushing the update. Upgraded from 2.3.4 to 2.3.5 and noticed the issue as Heiko describes.
          Hide
          steve miley added a comment -

          We just updated from 2.4.1 to 2.4.2 and ran into this problem for
          all teachers accessing server files.

          I believe this is of priority critical.

          It fails on the teacher role, accessing server files.
          It fails even without a "login as".

          We believe the failure is at this line -
          $can = has_capability('repository/'.$this->type.':view', $repocontext);
          in repository/lib.php.
          --------------------------

          if ($can) {
          if ($repocontext->contextlevel == CONTEXT_USER && $repocontext->instanceid != $USER->id)

          { // Prevent URL hijack to access someone else's repository. $can = false; }

          else

          { $can = has_capability('repository/'.$this->type.':view', $repocontext); error_log('in else this->type [' . $this->type . '] repocontext[' . print_r($repocontext,1)) . ']'; }

          }
          if ($can)

          { return true; }

          --------------------------

          The output from error_log is

          [13-Mar-2013 17:14:51] in else this->type [local] repocontext[context_system Object
          (
          [_id:protected] => 1
          [_contextlevel:protected] => 10
          [_instanceid:protected] => 0
          [_path:protected] => /1
          [_depth:protected] => 1
          )

          The teacher has allow access to repository/local:view

          Steve

          This stops all access of server files by all teachers.
          help!

          Show
          steve miley added a comment - We just updated from 2.4.1 to 2.4.2 and ran into this problem for all teachers accessing server files. I believe this is of priority critical. It fails on the teacher role, accessing server files. It fails even without a "login as". We believe the failure is at this line - $can = has_capability('repository/'.$this->type.':view', $repocontext); in repository/lib.php. -------------------------- if ($can) { if ($repocontext->contextlevel == CONTEXT_USER && $repocontext->instanceid != $USER->id) { // Prevent URL hijack to access someone else's repository. $can = false; } else { $can = has_capability('repository/'.$this->type.':view', $repocontext); error_log('in else this->type [' . $this->type . '] repocontext[' . print_r($repocontext,1)) . ']'; } } if ($can) { return true; } -------------------------- The output from error_log is [13-Mar-2013 17:14:51] in else this->type [local] repocontext[context_system Object ( [_id:protected] => 1 [_contextlevel:protected] => 10 [_instanceid:protected] => 0 [_path:protected] => /1 [_depth:protected] => 1 ) The teacher has allow access to repository/local:view Steve This stops all access of server files by all teachers. help!
          Hide
          Frédéric Massart added a comment -

          Hi guys,

          I have raised MDL-38474 to take care of the non login as issue.

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - Hi guys, I have raised MDL-38474 to take care of the non login as issue. Cheers, Fred
          Hide
          Heiko Schach added a comment -

          Good to see we are not alone with this bug.
          Could you please raise the priority of this issue to major?

          Show
          Heiko Schach added a comment - Good to see we are not alone with this bug. Could you please raise the priority of this issue to major?
          Hide
          Frédéric Massart added a comment -

          The issue MDL-38474 is now closed, and part of the most recent releases of Moodle 2.2 onwards. Could you confirm that this fix this issue as well?

          Many thanks!
          Fred

          Show
          Frédéric Massart added a comment - The issue MDL-38474 is now closed, and part of the most recent releases of Moodle 2.2 onwards. Could you confirm that this fix this issue as well? Many thanks! Fred
          Hide
          Fernando Oliveira added a comment -

          I just installed the latest update, which was released today (Build: 20130315), and the problem persists.

          Thanks,
          Fernando

          Show
          Fernando Oliveira added a comment - I just installed the latest update, which was released today (Build: 20130315), and the problem persists. Thanks, Fernando
          Hide
          Medex added a comment -

          We installed Moodle 2.4.3 (Build: 20130318) and this error still exists.

          John

          Show
          Medex added a comment - We installed Moodle 2.4.3 (Build: 20130318) and this error still exists. John
          Hide
          Frédéric Massart added a comment -

          Hi everyone,

          thanks you for the precise reproduction steps, that was very handy!

          unfortunately, this is not a bug, but was considered a feature when integrated. It was decided in MDL-36426 (which I don't think you have access to because it's a security issue), to be strict on the login as leading to access the repositories. While we were trying to prevent admins (or anyone logged in as) to access personal repositories of users (such as Dropbox, etc...) we decided to block all repositories when logged in as someone. I understand that this was perhaps not the best idea, but at that time we thought it was. Though, we were aware that this should not be that strict, that's why the solution implemented in 2.5 (still in development) is more flexible and only blocks repositories containing sensitive data. (Yes, upload is a repository, and I agree that this should not have been blocked).

          Why the solution differs between 2.5 and the rest? That's because we are having stricter policies about new features (understand new piece of code) landing in stable versions, and so extending the code to allow only some repositories to be accessible was a no, no for stable releases. In that case, that was probably not a good idea, but I guess we tried to achieve the best between fixing a security bug, and maintaining the code as stable as possible.

          As a workaround for now and if you feel comfortable with PHP and/or Git, here is a patch that solves your issue. BUT it also reverts the security fix preventing a logged in as user to access a personal repository, so apply with care!

          diff --git a/repository/lib.php b/repository/lib.php
          index b4d904b..461121b 100644
          --- a/repository/lib.php
          +++ b/repository/lib.php
          @@ -650,11 +650,6 @@ abstract class repository {
                   // Context in which the repository has been created.
                   $repocontext = context::instance_by_id($this->instance->contextid);
           
          -        // Prevent access to private repositories when logged in as.
          -        if (session_is_loggedinas()) {
          -            $can = false;
          -        }
          -
                   // We are going to ensure that the current context was legit, and reliable to check
                   // the capability against. (No need to do that if we already cannot).
                   if ($can) {
          

          I am sure we will work on a solution in a very foreseeable future, meantime please don't be tempted to upgrade to 2.5dev, that would not be a strategic move. I feel sorry about this regression, I realise that Moodle has so many different users, with so many different use cases, that it's pretty much impossible to fix a bug without causing trouble to some users.

          Show
          Frédéric Massart added a comment - Hi everyone, thanks you for the precise reproduction steps, that was very handy! unfortunately, this is not a bug , but was considered a feature when integrated. It was decided in MDL-36426 (which I don't think you have access to because it's a security issue), to be strict on the login as leading to access the repositories. While we were trying to prevent admins (or anyone logged in as) to access personal repositories of users (such as Dropbox, etc...) we decided to block all repositories when logged in as someone. I understand that this was perhaps not the best idea, but at that time we thought it was. Though, we were aware that this should not be that strict, that's why the solution implemented in 2.5 (still in development) is more flexible and only blocks repositories containing sensitive data. (Yes, upload is a repository, and I agree that this should not have been blocked). Why the solution differs between 2.5 and the rest? That's because we are having stricter policies about new features (understand new piece of code) landing in stable versions, and so extending the code to allow only some repositories to be accessible was a no, no for stable releases. In that case, that was probably not a good idea, but I guess we tried to achieve the best between fixing a security bug, and maintaining the code as stable as possible. As a workaround for now and if you feel comfortable with PHP and/or Git, here is a patch that solves your issue. BUT it also reverts the security fix preventing a logged in as user to access a personal repository, so apply with care! diff --git a/repository/lib.php b/repository/lib.php index b4d904b..461121b 100644 --- a/repository/lib.php +++ b/repository/lib.php @@ -650,11 +650,6 @@ abstract class repository { // Context in which the repository has been created. $repocontext = context::instance_by_id($this->instance->contextid); - // Prevent access to private repositories when logged in as. - if (session_is_loggedinas()) { - $can = false; - } - // We are going to ensure that the current context was legit, and reliable to check // the capability against. (No need to do that if we already cannot). if ($can) { I am sure we will work on a solution in a very foreseeable future, meantime please don't be tempted to upgrade to 2.5dev, that would not be a strategic move. I feel sorry about this regression, I realise that Moodle has so many different users, with so many different use cases, that it's pretty much impossible to fix a bug without causing trouble to some users.
          Hide
          Adrian Greeve added a comment - - edited

          As a developer I didn't realise how often I log in as a student and upload files for basic testing of issues. I can recognise that this was deemed a security risk, but I think that there are some use cases where being able to log in and post an assignment would be useful as the description points out, and it would help us few developers who use this functionality as well. I know that the MDK software allows for quick logging in and out of student accounts, but it's random and sometimes I just want to select a specific student with out having to manually find the user name, log out and then log in again.

          Show
          Adrian Greeve added a comment - - edited As a developer I didn't realise how often I log in as a student and upload files for basic testing of issues. I can recognise that this was deemed a security risk, but I think that there are some use cases where being able to log in and post an assignment would be useful as the description points out, and it would help us few developers who use this functionality as well. I know that the MDK software allows for quick logging in and out of student accounts, but it's random and sometimes I just want to select a specific student with out having to manually find the user name, log out and then log in again.
          Hide
          Frédéric Massart added a comment -

          Sending for peer review.

          • I added a missed check in 2.2 when editing repositories.
          • I hardcoded the list of allowed repositories based on the repositories in master which answer false to contains_private_data().
          • Any user instance is considered private.

          Reviewers, integrators, please have a thorough look at each different patch. 2.2 differs a lot from the rest.

          Many thanks!
          Fred

          Show
          Frédéric Massart added a comment - Sending for peer review. I added a missed check in 2.2 when editing repositories. I hardcoded the list of allowed repositories based on the repositories in master which answer false to contains_private_data() . Any user instance is considered private. Reviewers, integrators, please have a thorough look at each different patch. 2.2 differs a lot from the rest. Many thanks! Fred
          Hide
          Martin Dougiamas added a comment -

          +1 - Makes sense to me to just disallow things with tokens that are problematic, like Dropbox etc.

          Show
          Martin Dougiamas added a comment - +1 - Makes sense to me to just disallow things with tokens that are problematic, like Dropbox etc.
          Hide
          Medex added a comment -

          We applied this and it worked.

          I would like to raise the point that not being able to see trackers that deal with this issue left us in the dark.
          There should be an option to allow admins and managers to override this lockout. Not all programs may allow unfettered use of the private file area and may need to check for abuse or help with a technical issue. Another solution would be to allow managers and teachers to attach a file in an assignment directly from the assignment page, thus avoiding the issue of logging in as a student. Would it be helpful for me to add these as improvement suggestions?

          Show
          Medex added a comment - We applied this and it worked. I would like to raise the point that not being able to see trackers that deal with this issue left us in the dark. There should be an option to allow admins and managers to override this lockout. Not all programs may allow unfettered use of the private file area and may need to check for abuse or help with a technical issue. Another solution would be to allow managers and teachers to attach a file in an assignment directly from the assignment page, thus avoiding the issue of logging in as a student. Would it be helpful for me to add these as improvement suggestions?
          Hide
          Michael de Raadt added a comment -

          Hi, all.

          Apologies for those people who felt in the dark about this. We try to follow a responsible security process, hiding the details of reported security issues until they have been resolved in Moodle and allowing some time for administrators to update their sites.

          It looks like our fix for hiding potentially personal information through the repositories was overly aggressive. In the new master version (to be released as 2.5) repositories will be able to define themselves as private. For earlier versions, where repositories do not have this potential, we have created a whitelist of non-personal repositories that can be accessed when using "login as". For some of the repositories we also check that the instance of the repository being accessed is a site-wide or course-level repository rather than a personal instance of the repository. The whitelist currently looks as follows.

          • Legacy course files
          • Equella (site and course)
          • File system (site and course)
          • Flickr public
          • Server files
          • Merlot.org (site and course)
          • Recent files
          • Amazon S3 (site and course)
          • Upload a file
          • URL downloader
          • Private files
          • Webdav (site and course)
          • Wikimedia
          • YouTube
          Show
          Michael de Raadt added a comment - Hi, all. Apologies for those people who felt in the dark about this. We try to follow a responsible security process, hiding the details of reported security issues until they have been resolved in Moodle and allowing some time for administrators to update their sites. It looks like our fix for hiding potentially personal information through the repositories was overly aggressive. In the new master version (to be released as 2.5) repositories will be able to define themselves as private. For earlier versions, where repositories do not have this potential, we have created a whitelist of non-personal repositories that can be accessed when using "login as". For some of the repositories we also check that the instance of the repository being accessed is a site-wide or course-level repository rather than a personal instance of the repository. The whitelist currently looks as follows. Legacy course files Equella (site and course) File system (site and course) Flickr public Server files Merlot.org (site and course) Recent files Amazon S3 (site and course) Upload a file URL downloader Private files Webdav (site and course) Wikimedia YouTube
          Hide
          Jason Fowler added a comment -

          [Y] Syntax
          [Y] Output
          [Y] Whitespace
          [-] Language
          [-] Databases
          [Y] Testing
          [Y] Security
          [Y] Documentation
          [Y] Git
          [Y] Sanity check

          Show
          Jason Fowler added a comment - [Y] Syntax [Y] Output [Y] Whitespace [-] Language [-] Databases [Y] Testing [Y] Security [Y] Documentation [Y] Git [Y] Sanity check
          Hide
          Eloy Lafuente (stronk7) added a comment -

          +1 here, looks perfect and make things flexible enough without compromising privacy.

          Show
          Eloy Lafuente (stronk7) added a comment - +1 here, looks perfect and make things flexible enough without compromising privacy.
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Damyon Wiese added a comment - - edited

          Hi Fred,

          I found 2 issues when testing this.

          First - while I am logged in using "Login as" I can create a webdav repository for the current user - once it is created I cannot edit or delete it.

          Second - According to the check_capability function now I should never be able to manage a user repository that is not my own. This should be removed from the navigation as it currently shows a link that will always give an error.

          This is from lib/navigationlib.php (I think the first if statement is inverted - should only apply for the current user).

                  // Repository                                                                                                               
                  if (!$currentuser && $usercontext->contextlevel == CONTEXT_USER) {                                                          
                      if (!$this->cache->cached('contexthasrepos'.$usercontext->id)) {                                                        
                          require_once($CFG->dirroot . '/repository/lib.php');                                                                
                          $editabletypes = repository::get_editable_types($usercontext);                                                      
                          $haseditabletypes = !empty($editabletypes);                                                                         
                          unset($editabletypes);                                                                                              
                          $this->cache->set('contexthasrepos'.$usercontext->id, $haseditabletypes);                                           
                      } else {                                                                                                                
                          $haseditabletypes = $this->cache->{'contexthasrepos'.$usercontext->id};                                             
                      }                                                                                                                       
                      if ($haseditabletypes) {                                                                                                
                          $url = new moodle_url('/repository/manage_instances.php', array('contextid'=>$usercontext->id));                    
                          $usersetting->add(get_string('repositories', 'repository'), $url, self::TYPE_SETTING);                              
                      }                                                                                                                       
                  }
          
          Show
          Damyon Wiese added a comment - - edited Hi Fred, I found 2 issues when testing this. First - while I am logged in using "Login as" I can create a webdav repository for the current user - once it is created I cannot edit or delete it. Second - According to the check_capability function now I should never be able to manage a user repository that is not my own. This should be removed from the navigation as it currently shows a link that will always give an error. This is from lib/navigationlib.php (I think the first if statement is inverted - should only apply for the current user). // Repository if (!$currentuser && $usercontext->contextlevel == CONTEXT_USER) { if (!$ this ->cache->cached('contexthasrepos'.$usercontext->id)) { require_once($CFG->dirroot . '/repository/lib.php'); $editabletypes = repository::get_editable_types($usercontext); $haseditabletypes = !empty($editabletypes); unset($editabletypes); $ this ->cache->set('contexthasrepos'.$usercontext->id, $haseditabletypes); } else { $haseditabletypes = $ this ->cache->{'contexthasrepos'.$usercontext->id}; } if ($haseditabletypes) { $url = new moodle_url('/repository/manage_instances.php', array('contextid'=>$usercontext->id)); $usersetting->add(get_string('repositories', 'repository'), $url, self::TYPE_SETTING); } }
          Hide
          Damyon Wiese added a comment -

          Got Dans opinion on this one - and we agreed these two points raised above should considered improvements so we can minimise the changes introduced by the backports.

          Show
          Damyon Wiese added a comment - Got Dans opinion on this one - and we agreed these two points raised above should considered improvements so we can minimise the changes introduced by the backports.
          Hide
          Damyon Wiese added a comment -

          Thanks Fred,

          Integrated to 22, 23 and 24. As noted above I created 2 new improvement issues.

          Show
          Damyon Wiese added a comment - Thanks Fred, Integrated to 22, 23 and 24. As noted above I created 2 new improvement issues.
          Hide
          Dan Poltawski added a comment -

          Taking this off Andrew

          Show
          Dan Poltawski added a comment - Taking this off Andrew
          Hide
          Dan Poltawski added a comment -

          It passes, thanks Fred!

          (I think the missing step in the testing instructions which Andrew might've missed was enabling the user repos in the repository settings for webdav)

          Show
          Dan Poltawski added a comment - It passes, thanks Fred! (I think the missing step in the testing instructions which Andrew might've missed was enabling the user repos in the repository settings for webdav)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Your awesome contributions are now part of Moodle, your fav LMS out there.

          Closing this as fixed.

          Many thanks for all the hard work, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Your awesome contributions are now part of Moodle, your fav LMS out there. Closing this as fixed. Many thanks for all the hard work, ciao
          Hide
          Roderick Young added a comment -

          Hello! I am still unable to upload on behalf of students, after upgrading to the latest 2.4.3+. If I login as a student, I am unable to see the "Add Submission" button. I don't even get to see all the stuff about repositories. Please help!

          Show
          Roderick Young added a comment - Hello! I am still unable to upload on behalf of students, after upgrading to the latest 2.4.3+. If I login as a student, I am unable to see the "Add Submission" button. I don't even get to see all the stuff about repositories. Please help!
          Hide
          Nick Gault added a comment -

          We're running 2.4.3 and are still experiencing this problem. Anyone else?

          Show
          Nick Gault added a comment - We're running 2.4.3 and are still experiencing this problem. Anyone else?

            People

            • Votes:
              25 Vote for this issue
              Watchers:
              31 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: