Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Files API
    • Labels:
    • Testing Instructions:
      Hide

      Test prerequisites

      1. My private files with some files
      2. A file resource with some files

      Test steps

      1. Go to your privates files
      2. Click on each file and

      • make sure the button 'Set main file' is not displayed
      • using an HTML inspector (Firebug), unhide the button and make sure clicking on it does not do anything

      3. Go to the file resource

      • Make sure you can set one of the files to main file
      • Make sure you can change the main file
      • Save your changes and confirm it happened
      Show
      Test prerequisites My private files with some files A file resource with some files Test steps 1. Go to your privates files 2. Click on each file and make sure the button 'Set main file' is not displayed using an HTML inspector (Firebug), unhide the button and make sure clicking on it does not do anything 3. Go to the file resource Make sure you can set one of the files to main file Make sure you can change the main file Save your changes and confirm it happened
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-33582-master-integration
    • Rank:
      41513

      Description

      Replication steps:-

      1. Goto to edit private files area
      2. Open file manager and edit the css to remove "display:none" attribute on the "set as main file" button
      3. Now "set as main file" button is visible which should not be. Clicking on the button actually works and sets the current file as main file which should not be happening.
      4. I was also able to see "link to file directly" option following the same method in some cases, but was not really able to get that working. In any case both situations should be investigated.

        Activity

        Hide
        Michael de Raadt added a comment -

        Yes, ideally we should not be outputting controls and then hiding them. The control should not be output at all.

        Show
        Michael de Raadt added a comment - Yes, ideally we should not be outputting controls and then hiding them. The control should not be output at all.
        Hide
        Dan Poltawski added a comment -

        The key question is whether this is checked server side.

        Show
        Dan Poltawski added a comment - The key question is whether this is checked server side.
        Hide
        Dan Poltawski added a comment -

        Assigning to Marina for peer review.

        Show
        Dan Poltawski added a comment - Assigning to Marina for peer review.
        Hide
        Marina Glancy added a comment -

        Hi Fred,
        The function filemanager_js_templates() is called once per page even when there are several filemanagers. So with your changes it will be called with the options of the first filemanager on the page. I suggest you to discard changes to renderer.

        So let's assume that button is always present but it is shown conditionally with CSS. And inside

        selectnode.one('.fp-file-setmain').on('click', function(e) {
        

        after e.preventDefault(); you may insert the condition.

        Another thing that somebody mentioned that we can disable this on server side. It will be in repository/draftfiles_ajax.php, look for "case 'setmainfile'"

        Show
        Marina Glancy added a comment - Hi Fred, The function filemanager_js_templates() is called once per page even when there are several filemanagers. So with your changes it will be called with the options of the first filemanager on the page. I suggest you to discard changes to renderer. So let's assume that button is always present but it is shown conditionally with CSS. And inside selectnode.one('.fp-file-setmain').on('click', function(e) { after e.preventDefault(); you may insert the condition. Another thing that somebody mentioned that we can disable this on server side. It will be in repository/draftfiles_ajax.php, look for "case 'setmainfile'"
        Hide
        Frédéric Massart added a comment -

        Hi Marina,

        the patch is updated, but I wonder how to correctly disable this on the server side. As I understood, mainfile is a parameter set for Filemanager on the spot, which means that draftfile_ajax.php cannot figure out itself if the files can be reordered or not. And sending the mainfile parameter via Ajax does not make much sense as if someone hacks the HTML/JS to set a main file, he would hack the parameters sent too.

        How did you think about implementing this?

        Cheers!

        Show
        Frédéric Massart added a comment - Hi Marina, the patch is updated, but I wonder how to correctly disable this on the server side. As I understood, mainfile is a parameter set for Filemanager on the spot, which means that draftfile_ajax.php cannot figure out itself if the files can be reordered or not. And sending the mainfile parameter via Ajax does not make much sense as if someone hacks the HTML/JS to set a main file, he would hack the parameters sent too. How did you think about implementing this? Cheers!
        Hide
        Marina Glancy added a comment -

        Fred, that looks good.
        Probably you are right and it would be difficult on server side. Let's forget about it

        Show
        Marina Glancy added a comment - Fred, that looks good. Probably you are right and it would be difficult on server side. Let's forget about it
        Hide
        Dan Poltawski added a comment -

        (note, if this applied to anything like deleting then we clearly need to ensure there is a way to do it server side)

        Show
        Dan Poltawski added a comment - (note, if this applied to anything like deleting then we clearly need to ensure there is a way to do it server side)
        Hide
        Frédéric Massart added a comment -

        I confirmed with Marina that the delete button is not linked to this issue. Actually, the delete button is never hidden, and even if it doesn't appear that there are checks on server side, this would be only affecting the draft, which does not change the content until the form has been submitted. I tried to open two 'My private files' within the same session, deleting a file on one side and saving on the other, the file has not been deleted.

        Pushing for integration

        Show
        Frédéric Massart added a comment - I confirmed with Marina that the delete button is not linked to this issue. Actually, the delete button is never hidden, and even if it doesn't appear that there are checks on server side, this would be only affecting the draft, which does not change the content until the form has been submitted. I tried to open two 'My private files' within the same session, deleting a file on one side and saving on the other, the file has not been deleted. Pushing for integration
        Hide
        Dan Poltawski added a comment -

        Thanks Fred,

        integrated

        Show
        Dan Poltawski added a comment - Thanks Fred, integrated
        Hide
        Jason Fowler added a comment -

        All good

        Show
        Jason Fowler added a comment - All good
        Hide
        Eloy Lafuente (stronk7) added a comment -

        And this has been incorporated to all the weekly builds and also, to Moodle 2.3 Release Candidate 1, yay!

        Many, many thanks for your hard work!

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - And this has been incorporated to all the weekly builds and also, to Moodle 2.3 Release Candidate 1, yay! Many, many thanks for your hard work! Ciao

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: