Moodle
  1. Moodle
  2. MDL-30214

File picker: Recent files option does not work

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.2
    • Fix Version/s: 2.2
    • Component/s: Files API, Forum
    • Labels:
    • Testing Instructions:
      Hide

      1. Create forum post and embed an image using filepicker 'Upload file'
      2. Create another post or use any editor in the site that supports html format and inserting of the images
      3. In Filepicker select 'Recent files' and insert an image from 1. Image must be successfully inserted in the editor field

      Show
      1. Create forum post and embed an image using filepicker 'Upload file' 2. Create another post or use any editor in the site that supports html format and inserting of the images 3. In Filepicker select 'Recent files' and insert an image from 1. Image must be successfully inserted in the editor field
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull from Repository:
      git@github.com:marinaglancy/moodle.git
    • Pull Master Branch:
      wip-MDL-30214-master
    • Rank:
      26471

      Description

      NOTE: Jira lost all my data because I accidentally hit the back shortcut, so this is a short bug report without testing instructions. Please could somebody else fill in more detail if required.

      Whenever you use the recent files feature you get the error 'File size cannot be determined'. This happens on 2.1.2 release version as well as on current qa.moodle.net, e.g.:

      http://qa.moodle.net/mod/forum/post.php?forum=2###

        Issue Links

          Activity

          Hide
          Helen Foster added a comment -

          This problem has also been reported by Mary Evans http://moodle.org/mod/forum/discuss.php?d=190201

          Show
          Helen Foster added a comment - This problem has also been reported by Mary Evans http://moodle.org/mod/forum/discuss.php?d=190201
          Hide
          Dan Poltawski added a comment -

          Looking into this, I tried with an example forum post attachment, and the problem is that there is no get_file_info() returned when trying to retrieve it from file_info_context_module.

          Here is a stacktrace of my debugging statement:

          Got to bottom of get_file_info..
          line 119 of /lib/filebrowser/file_info_context_module.php:
          line 225 of /lib/filebrowser/file_browser.php: call to file_info_context_module->get_file_info()
          line 90 of /lib/filebrowser/file_browser.php: call to file_browser->get_file_info_context_module()
          line 1375 of /repository/lib.php: call to file_browser->get_file_info()
          line 209 of /repository/repository_ajax.php: call to repository->get_file_size()

          Show
          Dan Poltawski added a comment - Looking into this, I tried with an example forum post attachment, and the problem is that there is no get_file_info() returned when trying to retrieve it from file_info_context_module. Here is a stacktrace of my debugging statement: Got to bottom of get_file_info.. line 119 of /lib/filebrowser/file_info_context_module.php: line 225 of /lib/filebrowser/file_browser.php: call to file_info_context_module->get_file_info() line 90 of /lib/filebrowser/file_browser.php: call to file_browser->get_file_info_context_module() line 1375 of /repository/lib.php: call to file_browser->get_file_info() line 209 of /repository/repository_ajax.php: call to repository->get_file_size()
          Hide
          Helen Foster added a comment -

          Sam, thanks for creating this issue. The linked QA test can be used for testing.

          Show
          Helen Foster added a comment - Sam, thanks for creating this issue. The linked QA test can be used for testing.
          Hide
          Marina Glancy added a comment -

          The problem was in absence of function forum_get_file_info().

          Actually recent files that were uploaded in other modules can be selected

          Show
          Marina Glancy added a comment - The problem was in absence of function forum_get_file_info(). Actually recent files that were uploaded in other modules can be selected
          Hide
          Martin Dougiamas added a comment -

          +1 from me!

          Show
          Martin Dougiamas added a comment - +1 from me!
          Hide
          Aparup Banerjee added a comment -

          just correcting the assignee & peer-reviewer here.

          Show
          Aparup Banerjee added a comment - just correcting the assignee & peer-reviewer here.
          Hide
          Aparup Banerjee added a comment -

          Hi Marina,
          heres some notes from review - i don't want to go too deep so hopefully i don't go around in circles into file api:

          • whitespace in phpdoc 2nd line
          • noting that $areas is not used, perhaps we should consider using $areas[$filearea] in file_info_stored() param ? - easier introduction of new fileareas.
          • There might be need for some capability checking here, for instance 'mod/forum:viewpost' might need to be checked for 'post' file area before we can get the file's info from the post. many modules also implement checking 'moodle/course:managefiles' - but this is possibly not applicable to forum?

          ps: we'll need to backport in a separate issue once we integrate this for 2.2 QA only.

          Show
          Aparup Banerjee added a comment - Hi Marina, heres some notes from review - i don't want to go too deep so hopefully i don't go around in circles into file api: whitespace in phpdoc 2nd line noting that $areas is not used, perhaps we should consider using $areas [$filearea] in file_info_stored() param ? - easier introduction of new fileareas. There might be need for some capability checking here, for instance 'mod/forum:viewpost' might need to be checked for 'post' file area before we can get the file's info from the post. many modules also implement checking 'moodle/course:managefiles' - but this is possibly not applicable to forum? ps: we'll need to backport in a separate issue once we integrate this for 2.2 QA only.
          Hide
          Dongsheng Cai added a comment -

          Hello

          Have a look at the code of function forum_pluginfile()

          Use forum_user_can_see_post to check the permission, plus you need to check group info too.

          Show
          Dongsheng Cai added a comment - Hello Have a look at the code of function forum_pluginfile() Use forum_user_can_see_post to check the permission, plus you need to check group info too.
          Hide
          Marina Glancy added a comment -

          I don't think there should be any permissions checks at all, or code should be fixed somewhere else.

          I describe a case here, since it is not likely to be reported by somebody:

          • As a teacher I edit the description to the assignment and insert image there.
          • As an admin I withdraw enrolment of this teacher into this course
          • As the same teacher I enter in another course and use any editor (create a forum post f.e.). I open filepicker, 'recent files' and see there an image I uploaded in that assignment. This is my image, uploaded from my computer but I'm just not enrolled any more in the course where I used it. And I get a very nice error that 'File size can not be determined'.

          It's all confusion here. It should be either image is not displayed in the list of recent files; or *_get_file_info() does not check permissions. But error with this text is completely unclear for people.

          And what I'm trying to say: this is not likely to happen with teacher being unenrolled, but it is more likely to happen in forums - students get enrolled back and forth, forum topics becomes hidden, etc. But as long as file exists on server we should return info about it to its owner.

          Show
          Marina Glancy added a comment - I don't think there should be any permissions checks at all, or code should be fixed somewhere else. I describe a case here, since it is not likely to be reported by somebody: As a teacher I edit the description to the assignment and insert image there. As an admin I withdraw enrolment of this teacher into this course As the same teacher I enter in another course and use any editor (create a forum post f.e.). I open filepicker, 'recent files' and see there an image I uploaded in that assignment. This is my image, uploaded from my computer but I'm just not enrolled any more in the course where I used it. And I get a very nice error that 'File size can not be determined'. It's all confusion here. It should be either image is not displayed in the list of recent files; or *_get_file_info() does not check permissions. But error with this text is completely unclear for people. And what I'm trying to say: this is not likely to happen with teacher being unenrolled, but it is more likely to happen in forums - students get enrolled back and forth, forum topics becomes hidden, etc. But as long as file exists on server we should return info about it to its owner.
          Hide
          Dongsheng Cai added a comment - - edited

          Hi Marina

          You made a valid point, but to avoid permission check is not the way to fix this issue, file picker not the only place use this callback, so it has to check permissions anyway.

          Surely file picker error message can be improved, for the unenrolled users case, I think we need to properly fix it in repostory/lib.php, the get_file_size() method should not use filebrowser class, it should use filestorge instead to avoid this permission checks.

          Show
          Dongsheng Cai added a comment - - edited Hi Marina You made a valid point, but to avoid permission check is not the way to fix this issue, file picker not the only place use this callback, so it has to check permissions anyway. Surely file picker error message can be improved, for the unenrolled users case, I think we need to properly fix it in repostory/lib.php, the get_file_size() method should not use filebrowser class, it should use filestorge instead to avoid this permission checks.
          Hide
          Aparup Banerjee added a comment -

          'as long as file exists on server we should return info about it to its owner.' - gd point.

          Show
          Aparup Banerjee added a comment - 'as long as file exists on server we should return info about it to its owner.' - gd point.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I agree with Marina that it's all or nothing.

          I agree with Dong that it's all.

          In other words, access should be checked everywhere, both listing and getting info/picking. Clearly they cannot be unbalanced nor they can give unauthorized access.

          My 2 cents, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I agree with Marina that it's all or nothing. I agree with Dong that it's all. In other words, access should be checked everywhere, both listing and getting info/picking. Clearly they cannot be unbalanced nor they can give unauthorized access. My 2 cents, ciao
          Hide
          Sam Marshall added a comment -

          Regarding missing function in forum, this error (recent files not working) also affects all the OU's custom modules because they don't have a _get_file_info function either. That's why I didn't realise it was limited to forum only (it wasn't, for me).

          The thing is, I could have sworn it used to work. Was there a change that suddenly required this API within the 2.1.x releases, or were we just imagining it working before?

          It would be nice if the message could be improved to say e.g. 'Cannot select file, missing function ouwiki_get_file_info' or whatever.

          In addition even in core I don't think this is just forum - there doesn't seem to be glossary_get_file_info either (to name one module, that everyone including students can add files to), or choice_get_file_info (students can't but staff can), or data_get_file_info, or...

          And, perhaps a lesser issue that could be handled separately but if get_file_info is going to check permissions, are we sure this is compatible with the method it is using to list the files in the first place, because it is certainly a bug if for example, we display a file in your recent files but then when you click it you get some error. (If it's not going to allow permission, someone needs to ensure the file is not included in the list to begin with.)

          Show
          Sam Marshall added a comment - Regarding missing function in forum, this error (recent files not working) also affects all the OU's custom modules because they don't have a _get_file_info function either. That's why I didn't realise it was limited to forum only (it wasn't, for me). The thing is, I could have sworn it used to work. Was there a change that suddenly required this API within the 2.1.x releases, or were we just imagining it working before? It would be nice if the message could be improved to say e.g. 'Cannot select file, missing function ouwiki_get_file_info' or whatever. In addition even in core I don't think this is just forum - there doesn't seem to be glossary_get_file_info either (to name one module, that everyone including students can add files to), or choice_get_file_info (students can't but staff can), or data_get_file_info, or... And, perhaps a lesser issue that could be handled separately but if get_file_info is going to check permissions, are we sure this is compatible with the method it is using to list the files in the first place, because it is certainly a bug if for example, we display a file in your recent files but then when you click it you get some error. (If it's not going to allow permission, someone needs to ensure the file is not included in the list to begin with.)
          Hide
          Dan Poltawski added a comment -

          Hi, it was me who just moved that issue into testing! There is a serious bug with the integration workflow allowing an anonymous user to change the state!

          Show
          Dan Poltawski added a comment - Hi, it was me who just moved that issue into testing! There is a serious bug with the integration workflow allowing an anonymous user to change the state!
          Hide
          Dan Poltawski added a comment -

          Sam: The recent files repo certainly was working

          Show
          Dan Poltawski added a comment - Sam: The recent files repo certainly was working
          Hide
          Petr Škoda added a comment -

          I have already commented in MDL-30469, the patch is missing necessary access control checks.

          Show
          Petr Škoda added a comment - I have already commented in MDL-30469 , the patch is missing necessary access control checks.
          Hide
          Aparup Banerjee added a comment -

          resetting the workflow (buggy!) , actually, reopening this.

          Show
          Aparup Banerjee added a comment - resetting the workflow (buggy!) , actually, reopening this.
          Hide
          Dan Poltawski added a comment -

          Hi Apu,

          Please don't test this change! Its not been integrated, I switched the state not an integrator!See MDLSITE-1587

          Show
          Dan Poltawski added a comment - Hi Apu, Please don't test this change! Its not been integrated, I switched the state not an integrator!See MDLSITE-1587
          Hide
          Dan Poltawski added a comment -

          Ah, ignore me...

          Show
          Dan Poltawski added a comment - Ah, ignore me...
          Hide
          Aparup Banerjee added a comment - - edited

          Marina, i'm reopening this, firstly so that no one tests this incorrectly (tracker workflow bug), and secondly so that we can consider the security (prior to calling function or within function) of this patch before putting this through.

          Show
          Aparup Banerjee added a comment - - edited Marina, i'm reopening this, firstly so that no one tests this incorrectly (tracker workflow bug), and secondly so that we can consider the security (prior to calling function or within function) of this patch before putting this through.
          Hide
          Aparup Banerjee added a comment -

          lol, np thanks Dan :-D

          Show
          Aparup Banerjee added a comment - lol, np thanks Dan :-D
          Hide
          Marina Glancy added a comment -

          I added permission checks to the function and created an issue MDL-30493 for population of recent files list.

          Show
          Marina Glancy added a comment - I added permission checks to the function and created an issue MDL-30493 for population of recent files list.
          Hide
          Aparup Banerjee added a comment -

          thanks all, this has been integrated

          Show
          Aparup Banerjee added a comment - thanks all, this has been integrated
          Hide
          Aparup Banerjee added a comment -

          tested perfect! now for QA test!

          Show
          Aparup Banerjee added a comment - tested perfect! now for QA test!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The master fixes corresponding to this issue have been sent upstream. Fixes for other branches (19, 20, 21 stable) will be sent in the very-next days.

          Thanks for the hard work! Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The master fixes corresponding to this issue have been sent upstream. Fixes for other branches (19, 20, 21 stable) will be sent in the very-next days. Thanks for the hard work! Closing, ciao
          Hide
          Mark Nielsen added a comment -

          This problem appears to exist for the assignment module as well, see MDL-30709

          Show
          Mark Nielsen added a comment - This problem appears to exist for the assignment module as well, see MDL-30709
          Hide
          Dominick Inglese added a comment -

          is this fixed?I have moodle 2.2 and when I select "recent files" under "insert image" in a glossary, I can select the image but when I click "insert image" an error occurs: "file size can not be determined"
          so after having this happen in several different places over the course of 2 months I found this bug tracker, but according to the results it says it is fixed.

          Show
          Dominick Inglese added a comment - is this fixed?I have moodle 2.2 and when I select "recent files" under "insert image" in a glossary, I can select the image but when I click "insert image" an error occurs: "file size can not be determined" so after having this happen in several different places over the course of 2 months I found this bug tracker, but according to the results it says it is fixed.
          Hide
          Helen Foster added a comment -

          Hi Dominick,

          Please check that you're using the latest stable version of Moodle 2.2, then if you find that the problem still occurs, please create a new issue for it.

          Show
          Helen Foster added a comment - Hi Dominick, Please check that you're using the latest stable version of Moodle 2.2, then if you find that the problem still occurs, please create a new issue for it.
          Hide
          Kirill Astashov added a comment -

          What about Wiki module? It's missing wiki_get_file_info() too...

          Show
          Kirill Astashov added a comment - What about Wiki module? It's missing wiki_get_file_info() too...
          Hide
          Kirill Astashov added a comment - - edited

          For modules which do not have modname_get_file_info() function (e.g. wiki), I tried to implement a generic method so that it allows to avoid a "File size cannot be determined" error and insert it anyway. Please see the attached patch "recent-files-generic-fix.diff". Any ideas whether this behaviour is worth attention? If it's deemed acceptable I can push it to github as well.

          Show
          Kirill Astashov added a comment - - edited For modules which do not have modname_get_file_info() function (e.g. wiki), I tried to implement a generic method so that it allows to avoid a "File size cannot be determined" error and insert it anyway. Please see the attached patch "recent-files-generic-fix.diff". Any ideas whether this behaviour is worth attention? If it's deemed acceptable I can push it to github as well.
          Hide
          Kirill Astashov added a comment -

          Generic fix for Recent Files.

          Show
          Kirill Astashov added a comment - Generic fix for Recent Files.
          Hide
          Marina Glancy added a comment -

          Kirill, thanks for pointing out about wiki module, I created an issue MDL-32378 for it.

          There was an issue MDL-30709 where I added file info check before the list of recent files is populated, so the error should not appear any more. But it also means that in case of wiki, the files attached to wiki module will not be shown in Recent files at all.

          Regarding your fix, I have to reject it. The whole idea of function xxx_get_file_info is checking the permission to view the file. We cannot avoid it. If module does not implement the function, we assume (for security reasons) that user is NOT allowed to view the file.

          Show
          Marina Glancy added a comment - Kirill, thanks for pointing out about wiki module, I created an issue MDL-32378 for it. There was an issue MDL-30709 where I added file info check before the list of recent files is populated, so the error should not appear any more. But it also means that in case of wiki, the files attached to wiki module will not be shown in Recent files at all. Regarding your fix, I have to reject it. The whole idea of function xxx_get_file_info is checking the permission to view the file. We cannot avoid it. If module does not implement the function, we assume (for security reasons) that user is NOT allowed to view the file.

            People

            • Votes:
              4 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: