Moodle
  1. Moodle
  2. MDL-35144

course/index.php doesn't check capabilities properly when permitting editing

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.1, 2.4.3
    • Fix Version/s: 2.3.7, 2.4.4
    • Component/s: Administration
    • Labels:
    • Testing Instructions:
      Hide

      1. Create a new user
      2. Create a new role with only moodle/category:manage allowed and assignable only at Site
      3. Assign the user to that role at Site context
      4. Log in as that user. They should have the ability to see the edit courses page and they should see it in editing mode.
      5. Repeat for cap moodle/course:create

      Show
      1. Create a new user 2. Create a new role with only moodle/category:manage allowed and assignable only at Site 3. Assign the user to that role at Site context 4. Log in as that user. They should have the ability to see the edit courses page and they should see it in editing mode. 5. Repeat for cap moodle/course:create
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull 2.4 Branch:
      MOODLE_24_MDL35144
    • Rank:
      43776

      Description

      The only capability that is checked to see if editing is permitted on this page (of anything) is moodle/site:manageblocks. This doesn't matter for much for an admin or Manager who will have that cap but this will make it impossible to switch on editing if a more restrictive role is being set up.

      There are other caps that may permit editing on this page - e.g. moodle/category:manage. This is not checked and will not work.

      The page should really call $PAGE->set_other_editing_capability('capability') to permit reasonably additional caps to allow editing.

        Activity

        Hide
        Michael de Raadt added a comment -

        Your summary and description were unclear to me. I've tried to write an improved summary, but you should check that it still reflects your intention.

        From what I can interpret, there is not a bug at present, but you feel the capabilities related to editing a page could be more granular to provide more control.

        Show
        Michael de Raadt added a comment - Your summary and description were unclear to me. I've tried to write an improved summary, but you should check that it still reflects your intention. From what I can interpret, there is not a bug at present, but you feel the capabilities related to editing a page could be more granular to provide more control.
        Hide
        Howard Miller added a comment -

        No... it's a bug. I should have supplied instructions to reproduce the initial problem so here we go...

        1. create a test user
        2. create a role with a single capability allowed, moodle/category:manage
        3. assign the user that role at the site level
        4. go to the course/category edit page

        Result: the "Turn editing on" button appears. Pressing does nothing - no error, no nothing.

        Analysis:
        Would you expect turn editing on to work in this situation? I would suggest that at a minimum there's a bug that the button should not appear if it shouldn't work, however...

        Looking at the code in course/index.php. About 50 lines down, the code checks $PAGE->user_is_editing(). This returns FALSE and this is the crux of the matter. How does this work...

        • now in lib/pagelib.php
        • user_is_editing() does little more than check user_allowed_editing()
        • user_allowed_editing checks that the user has any capability returned by all_editing_caps()
        • all_editing_caps() returns an array of capabilities comprising _othereditingcaps and _blockseditingcaps
        • _otherediting caps is empty in this case because nothing fills it, I'll come back to this
        • _blocks editingcaps contains the single cap 'moodle/site:manageblocks'

        So... the only cap we are checking to see if editing is permitted on the courses page is 'moodle/site:manageblocks. Referring to http://docs.moodle.org/23/en/Capabilities/moodle/site:manageblocks this doesn't say anything about editing courses or categories so this must surely be wrong. A bug?

        It looks as though _othereditingcaps is provided so that pages with an editing button can specify the caps allowed to switch on editing. This has not been done on this page at all. Another bug?

        Show
        Howard Miller added a comment - No... it's a bug. I should have supplied instructions to reproduce the initial problem so here we go... 1. create a test user 2. create a role with a single capability allowed, moodle/category:manage 3. assign the user that role at the site level 4. go to the course/category edit page Result: the "Turn editing on" button appears. Pressing does nothing - no error, no nothing. Analysis: Would you expect turn editing on to work in this situation? I would suggest that at a minimum there's a bug that the button should not appear if it shouldn't work, however... Looking at the code in course/index.php. About 50 lines down, the code checks $PAGE->user_is_editing(). This returns FALSE and this is the crux of the matter. How does this work... now in lib/pagelib.php user_is_editing() does little more than check user_allowed_editing() user_allowed_editing checks that the user has any capability returned by all_editing_caps() all_editing_caps() returns an array of capabilities comprising _othereditingcaps and _blockseditingcaps _otherediting caps is empty in this case because nothing fills it, I'll come back to this _blocks editingcaps contains the single cap 'moodle/site:manageblocks' So... the only cap we are checking to see if editing is permitted on the courses page is 'moodle/site:manageblocks. Referring to http://docs.moodle.org/23/en/Capabilities/moodle/site:manageblocks this doesn't say anything about editing courses or categories so this must surely be wrong. A bug? It looks as though _othereditingcaps is provided so that pages with an editing button can specify the caps allowed to switch on editing. This has not been done on this page at all. Another bug?
        Hide
        Michael de Raadt added a comment -

        OK. That's clearer now.

        Thanks for adding the extra information. I was able to reproduce what you found on the courses page.

        Show
        Michael de Raadt added a comment - OK. That's clearer now. Thanks for adding the extra information. I was able to reproduce what you found on the courses page.
        Hide
        Howard Miller added a comment - - edited

        Have checked in (master) 2.5 and the problem seems to be fixed.Possibly because 2.5 no longer has a 'turn editing on' button for the Add/edit courses page.

        Show
        Howard Miller added a comment - - edited Have checked in (master) 2.5 and the problem seems to be fixed.Possibly because 2.5 no longer has a 'turn editing on' button for the Add/edit courses page.
        Hide
        Howard Miller added a comment -

        This code has been changed completely in 2.5 and the problem doesn't exist.

        Show
        Howard Miller added a comment - This code has been changed completely in 2.5 and the problem doesn't exist.
        Hide
        Rajesh Taneja added a comment -

        Thanks for the patch Howard,

        Looks good to me, pushing it for integration review.
        [y] Syntax
        [y] Output
        [y] Whitespace
        [-] Language
        [-] Databases
        [y] Testing
        [y] Security
        [-] Documentation
        [y] Git
        [y] Sanity check

        Show
        Rajesh Taneja added a comment - Thanks for the patch Howard, Looks good to me, pushing it for integration review. [y] Syntax [y] Output [y] Whitespace [-] Language [-] Databases [y] Testing [y] Security [-] Documentation [y] Git [y] Sanity check
        Hide
        Dan Poltawski added a comment -

        Does this need to be backported to 2.3?

        Show
        Dan Poltawski added a comment - Does this need to be backported to 2.3?
        Hide
        Rajesh Taneja added a comment -

        Yes, Dan.

        I should have created that. Let me know if you want me to do this.

        Show
        Rajesh Taneja added a comment - Yes, Dan. I should have created that. Let me know if you want me to do this.
        Hide
        Dan Poltawski added a comment -

        Integrated to 24 and 23. Thanks Howard/Raj

        Show
        Dan Poltawski added a comment - Integrated to 24 and 23. Thanks Howard/Raj
        Hide
        Damyon Wiese added a comment -

        I'll test this.

        Show
        Damyon Wiese added a comment - I'll test this.
        Hide
        Damyon Wiese added a comment -

        Thanks for the patch howard - it worked for the top level category page, but not for a sub category page.

        Adding the same change to course/category.php should fix it.

        Show
        Damyon Wiese added a comment - Thanks for the patch howard - it worked for the top level category page, but not for a sub category page. Adding the same change to course/category.php should fix it.
        Show
        Damyon Wiese added a comment - Added branches with the extra commit: Repo: https://github.com/damyon/moodle.git 24 Branch: MDL-35144 -24 24 Diff: https://github.com/damyon/moodle/compare/MDL-35144-24~1...MDL-35144-24 23 Branch: MDL-35144 -23 23 Diff: https://github.com/damyon/moodle/compare/MDL-35144-23~1...MDL-35144-23
        Hide
        Dan Poltawski added a comment -

        Thanks, i've pulled those in

        Show
        Dan Poltawski added a comment - Thanks, i've pulled those in
        Hide
        Damyon Wiese added a comment -

        Tested on 23 and 24 and passed.

        Thanks!

        Show
        Damyon Wiese added a comment - Tested on 23 and 24 and passed. Thanks!
        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
        Howard Miller added a comment -

        Wooh Hooh - thanks guys :-D

        Show
        Howard Miller added a comment - Wooh Hooh - thanks guys :-D

          People

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

            Dates

            • Created:
              Updated:
              Resolved: