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

          Attachments

            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