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

Category creation is only allowed by permisisons defined at system context

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9.1
    • Fix Version/s: 1.9.4
    • Component/s: Roles / Access
    • Labels:
      None
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE

      Description

      First noticed by the following steps:

      1/ Assign a user a role which has moodle/category:create allowed to a category
      2/ As that user, try to add a subcategory from the category
      3/ The form will be displayed to add a new category
      4/ Submiting the form returns you back to the form

      Looking at the top of categoryedit.php seems to indicate that when categories are being added with also use system context.. Need to dig further..

        Gliffy Diagrams

        1. categories.patch.txt
          88 kB
          Tim Hunt
        2. editcategoryfixes2.patch
          21 kB
          Dan Poltawski

          Issue Links

            Activity

            Hide
            howardsmiller Howard Miller added a comment -

            When I looked at this I was actually assigning Admin rights at category level (which works). At the time there wasn't even a UI for adding categories at other than site level.

            Show
            howardsmiller Howard Miller added a comment - When I looked at this I was actually assigning Admin rights at category level (which works). At the time there wasn't even a UI for adding categories at other than site level.
            Hide
            poltawski Dan Poltawski added a comment -

            Hmm, i'm looking more at this, and the editcategory form is just broken in multiple places so i'm refactoring, patch on the way

            Show
            poltawski Dan Poltawski added a comment - Hmm, i'm looking more at this, and the editcategory form is just broken in multiple places so i'm refactoring, patch on the way
            Hide
            poltawski Dan Poltawski added a comment - - edited

            Big patch to fix some of the problems with the current edit category form which are:

            • Category context permissions are not respected (e.g. creating sub-categories can only be done by a user with permissions at system-level)
            • A category can be 'lost' by changing the parent to itself in the edit form
            • Lots of obsolete code which hasn't been implemented

            I've refactored it a bit to address these issues (primarily to allow category ctx permission to be respected) and checks permissions where the parent is changed.

            course/editcategory.php | 289 ++++++------------------------------------
            course/editcategory_form.php | 47 +++++++-
            lang/en_utf8/error.php | 3 +
            3 files changed, 87 insertions, 252 deletions

            Show
            poltawski Dan Poltawski added a comment - - edited Big patch to fix some of the problems with the current edit category form which are: Category context permissions are not respected (e.g. creating sub-categories can only be done by a user with permissions at system-level) A category can be 'lost' by changing the parent to itself in the edit form Lots of obsolete code which hasn't been implemented I've refactored it a bit to address these issues (primarily to allow category ctx permission to be respected) and checks permissions where the parent is changed. course/editcategory.php | 289 ++++++------------------------------------ course/editcategory_form.php | 47 +++++++- lang/en_utf8/error.php | 3 + 3 files changed, 87 insertions , 252 deletions
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            just linking this bug with MDL-14580 because of potential relation, to have them under control.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - just linking this bug with MDL-14580 because of potential relation, to have them under control.
            Hide
            skodak Petr Skoda added a comment -

            I would prefer to have two optional parameters:

            • id for category - 0 means create new category
            • parent for category parent
              and do some verifications in definitional_after_data() and validation().
            Show
            skodak Petr Skoda added a comment - I would prefer to have two optional parameters: id for category - 0 means create new category parent for category parent and do some verifications in definitional_after_data() and validation().
            Hide
            poltawski Dan Poltawski added a comment -

            Makes sense, I was keeping with the meaning of the original parms, I will refactor this some more

            Show
            poltawski Dan Poltawski added a comment - Makes sense, I was keeping with the meaning of the original parms, I will refactor this some more
            Hide
            skodak Petr Skoda added a comment -

            thanks for working on this!

            Show
            skodak Petr Skoda added a comment - thanks for working on this!
            Hide
            poltawski Dan Poltawski added a comment -

            New patch with Petrs suggestions (which make it 100000x more logical!)

            danp@cream:~/git/moodle-r2$ git diff --stat d5176
            course/category.php | 3 +-
            course/editcategory.php | 325 ++++++++----------------------------------
            course/editcategory_form.php | 47 ++++++-
            course/index.php | 7 +-
            lang/en_utf8/error.php | 3 +
            5 files changed, 112 insertions, 273 deletions

            Show
            poltawski Dan Poltawski added a comment - New patch with Petrs suggestions (which make it 100000x more logical!) danp@cream:~/git/moodle-r2$ git diff --stat d5176 course/category.php | 3 +- course/editcategory.php | 325 ++++++++---------------------------------- course/editcategory_form.php | 47 ++++++- course/index.php | 7 +- lang/en_utf8/error.php | 3 + 5 files changed, 112 insertions , 273 deletions
            Hide
            poltawski Dan Poltawski added a comment -

            Hi, has anyone had a chance to review this patch?

            I think the problems mentioned are still broken in head, so I was thinking of updating this for HEAD so we can test for breakage

            thanks

            Show
            poltawski Dan Poltawski added a comment - Hi, has anyone had a chance to review this patch? I think the problems mentioned are still broken in head, so I was thinking of updating this for HEAD so we can test for breakage thanks
            Hide
            howardsmiller Howard Miller added a comment -

            AFAIK, it's working fine in 1.9 Weekly. I did test it.

            Unfortunately, you still can't delete them or move them about at Category level.

            Show
            howardsmiller Howard Miller added a comment - AFAIK, it's working fine in 1.9 Weekly. I did test it. Unfortunately, you still can't delete them or move them about at Category level.
            Hide
            timhunt Tim Hunt added a comment -

            Grabbing a bunch of related course category editing bugs that I plan to work on. Sorry for all the emails.

            Show
            timhunt Tim Hunt added a comment - Grabbing a bunch of related course category editing bugs that I plan to work on. Sorry for all the emails.
            Hide
            timhunt Tim Hunt added a comment -

            After some more testing tomorrow, and merging it to HEAD, I intend to commit this patch (categories.patch.txt) to the 1.9 stable branch.

            It fixes MDL-17479, MDL-16426, MDL-16063, MDL-16013, MDL-15658, MDL-15556, MDL-15161, MDL-14925, MDL-13742, MDL-11557.

            I chose this bug to attach the patch to because it had the most watchers!

            Apologies for the fact it is one big patch, rather than a series of smaller patches. However, categoryedit.php, category.php and index.php where in pretty bad shape and needed significant cleaning up. categoryedit.php, in particular, was almost completely rewritten to bring it in line with modern Moodle coding practices. Doing that fixed a lot of bugs at once.

            There are still about a dozen category related bugs on my list, but I am able to separate them out, and hope to be committing fixes for them in the next few days.

            If anyone has some spare cycles to review or test this before tomorrow, that would be great, but I am not expecting anything. Thanks.

            Show
            timhunt Tim Hunt added a comment - After some more testing tomorrow, and merging it to HEAD, I intend to commit this patch (categories.patch.txt) to the 1.9 stable branch. It fixes MDL-17479 , MDL-16426 , MDL-16063 , MDL-16013 , MDL-15658 , MDL-15556 , MDL-15161 , MDL-14925 , MDL-13742 , MDL-11557 . I chose this bug to attach the patch to because it had the most watchers! Apologies for the fact it is one big patch, rather than a series of smaller patches. However, categoryedit.php, category.php and index.php where in pretty bad shape and needed significant cleaning up. categoryedit.php, in particular, was almost completely rewritten to bring it in line with modern Moodle coding practices. Doing that fixed a lot of bugs at once. There are still about a dozen category related bugs on my list, but I am able to separate them out, and hope to be committing fixes for them in the next few days. If anyone has some spare cycles to review or test this before tomorrow, that would be great, but I am not expecting anything. Thanks.
            Hide
            howardsmiller Howard Miller added a comment -

            Putting my sycophant hat on for a moment - that's excellent news, Tim The grimness of these pages has been a big hassle for a long time.

            Show
            howardsmiller Howard Miller added a comment - Putting my sycophant hat on for a moment - that's excellent news, Tim The grimness of these pages has been a big hassle for a long time.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  28/Jan/09