Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-30214

File picker: Recent files option does not work

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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

      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###

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            tsala Helen Foster added a comment -

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

            Show
            tsala Helen Foster added a comment - This problem has also been reported by Mary Evans http://moodle.org/mod/forum/discuss.php?d=190201
            Hide
            poltawski 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
            poltawski 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
            tsala Helen Foster added a comment -

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

            Show
            tsala Helen Foster added a comment - Sam, thanks for creating this issue. The linked QA test can be used for testing.
            Hide
            marina 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 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
            dougiamas Martin Dougiamas added a comment -

            +1 from me!

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

            just correcting the assignee & peer-reviewer here.

            Show
            nebgor Aparup Banerjee added a comment - just correcting the assignee & peer-reviewer here.
            Hide
            nebgor 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
            nebgor 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 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 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 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 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 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 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
            nebgor Aparup Banerjee added a comment -

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

            Show
            nebgor Aparup Banerjee added a comment - 'as long as file exists on server we should return info about it to its owner.' - gd point.
            Hide
            stronk7 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
            stronk7 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
            quen 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
            quen 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
            poltawski 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
            poltawski 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
            poltawski Dan Poltawski added a comment -

            Sam: The recent files repo certainly was working

            Show
            poltawski Dan Poltawski added a comment - Sam: The recent files repo certainly was working
            Hide
            skodak Petr Skoda added a comment -

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

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

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

            Show
            nebgor Aparup Banerjee added a comment - resetting the workflow (buggy!) , actually, reopening this.
            Hide
            poltawski 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
            poltawski 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
            poltawski Dan Poltawski added a comment -

            Ah, ignore me...

            Show
            poltawski Dan Poltawski added a comment - Ah, ignore me...
            Hide
            nebgor 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
            nebgor 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
            nebgor Aparup Banerjee added a comment -

            lol, np thanks Dan :-D

            Show
            nebgor Aparup Banerjee added a comment - lol, np thanks Dan :-D
            Hide
            marina 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 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
            nebgor Aparup Banerjee added a comment -

            thanks all, this has been integrated

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

            tested perfect! now for QA test!

            Show
            nebgor Aparup Banerjee added a comment - tested perfect! now for QA test!
            Hide
            stronk7 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
            stronk7 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
            bushido Mark Nielsen added a comment -

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

            Show
            bushido Mark Nielsen added a comment - This problem appears to exist for the assignment module as well, see MDL-30709
            Hide
            nimaipandit 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
            nimaipandit 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
            tsala 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
            tsala 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
            badblock Kirill Astashov added a comment -

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

            Show
            badblock Kirill Astashov added a comment - What about Wiki module? It's missing wiki_get_file_info() too...
            Hide
            badblock 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
            badblock 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
            badblock Kirill Astashov added a comment -

            Generic fix for Recent Files.

            Show
            badblock Kirill Astashov added a comment - Generic fix for Recent Files.
            Hide
            marina 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 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:
                  Fix Release Date:
                  5/Dec/11