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

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

    Details

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

      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.

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            salvetore 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
            salvetore 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
            howardsmiller 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
            howardsmiller 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
            salvetore 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
            salvetore 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
            howardsmiller 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
            howardsmiller 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
            howardsmiller Howard Miller added a comment -

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

            Show
            howardsmiller Howard Miller added a comment - This code has been changed completely in 2.5 and the problem doesn't exist.
            Hide
            rajeshtaneja 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
            rajeshtaneja 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
            poltawski Dan Poltawski added a comment -

            Does this need to be backported to 2.3?

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

            Yes, Dan.

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

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

            Integrated to 24 and 23. Thanks Howard/Raj

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

            I'll test this.

            Show
            damyon Damyon Wiese added a comment - I'll test this.
            Hide
            damyon 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 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 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
            poltawski Dan Poltawski added a comment -

            Thanks, i've pulled those in

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

            Tested on 23 and 24 and passed.

            Thanks!

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

            Wooh Hooh - thanks guys :-D

            Show
            howardsmiller 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:
                  Fix Release Date:
                  13/May/13