Moodle
  1. Moodle
  2. MDL-15161

Category creation is only allowed by permisisons defined at system context

    Details

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

      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..

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

        Issue Links

          Activity

          Hide
          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
          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
          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
          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
          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
          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
          Eloy Lafuente (stronk7) added a comment -

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

          Show
          Eloy Lafuente (stronk7) added a comment - just linking this bug with MDL-14580 because of potential relation, to have them under control.
          Hide
          Petr Škoda 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
          Petr Škoda 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
          Dan Poltawski added a comment -

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

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

          thanks for working on this!

          Show
          Petr Škoda added a comment - thanks for working on this!
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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: