Moodle
  1. Moodle
  2. MDL-39084

Gradebook - Capability checks are incoherent for the "Course grade settings" section

    Details

    • Testing Instructions:
      Hide

      1. As an administrator, go to "Site administration > Users > Permissions > Define roles" and uncheck "Update course settings" capability (moodle/course:update) for teacher's role.
      2. As a teacher, go in a course and click on Grades link in Course administration.
      3. TEST : Make sure that you see "Course grade settings" link in the Grade administration and "Settings > Course" in the dropdown list of the page (top left).
      4. TEST : Make sure that you have access to the page when you click on "Course grade settings" link and "Settings > Course" in the dropdown list.

      Show
      1. As an administrator, go to "Site administration > Users > Permissions > Define roles" and uncheck "Update course settings" capability (moodle/course:update) for teacher's role. 2. As a teacher, go in a course and click on Grades link in Course administration. 3. TEST : Make sure that you see "Course grade settings" link in the Grade administration and "Settings > Course" in the dropdown list of the page (top left). 4. TEST : Make sure that you have access to the page when you click on "Course grade settings" link and "Settings > Course" in the dropdown list.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
      git@github.com:StudiUM/moodle.git
    • Pull 2.4 Branch:
      MDL-39084-moodle24
    • Pull Master Branch:
      MDL-39084-master
    • Rank:
      49220

      Description

      It seems there's a problem with capabilitiy checks between "Course grade settings" in the navigation (navigation bar and dropdown menu) and in the page.

      Navigation (grade/lib.php) :

          public static function get_info_manage_settings($courseid) {
              ...
              if (has_capability('moodle/course:update', $context)) {
              ...
      

      Page (grade/edit/settings/index.php) :

      ...
      require_capability('moodle/grade:manage', $context);
      ...
      

      Although I'm not sure which cabability is the right one, it should be set the same in both places.

      If the capability should be "moodle/course:update" then this bug should be classified as a "minor security issue".

        Activity

        Hide
        Jean-Philippe Gaudreau added a comment -

        I would offer to fix the issue if I have some feedbacks from the community on which capability would be the right one.

        On my part, I think the more coherent (less risky) is to set "moodle/course:update" check in the page. But setting "moodle/grade:manage" in navigation would offer better flexibility in delegation of roles within a course (grade manager only role in a course without any other accesses in courses administration).

        Choice is yours!

        Show
        Jean-Philippe Gaudreau added a comment - I would offer to fix the issue if I have some feedbacks from the community on which capability would be the right one. On my part, I think the more coherent (less risky) is to set "moodle/course:update" check in the page. But setting "moodle/grade:manage" in navigation would offer better flexibility in delegation of roles within a course (grade manager only role in a course without any other accesses in courses administration). Choice is yours!
        Hide
        Michael de Raadt added a comment -

        I'm not sure of the specifics, but I suppose people might want to manage the grade settings without necessarily updating them.

        Andrew: you've worked in this area a lot, what are your thoughts?

        Show
        Michael de Raadt added a comment - I'm not sure of the specifics, but I suppose people might want to manage the grade settings without necessarily updating them. Andrew: you've worked in this area a lot, what are your thoughts?
        Hide
        Andrew Davis added a comment - - edited

        It's an interesting question. I'm not sure there is a single correct answer but that's not terribly helpful.

        There are three relevant capabilities.

        moodle/grade:edit - edit grades
        moodle/grade:manage - manage grade items
        moodle/course:update - update course settings

        These form a hierarchy. Someone who can grade student work, someone who can alter grade items and someone who can alter course settings. moodle/grade:edit is just for people doing grading and we can ignore it here.

        On to the other two. Looking at the settings page, the settings available are all related to what is shown on the grade reports and how they are displayed. I feel like it would be a bit odd to allow someone to manage grade items but not to allow them to control their display on the grade reports. That said, they are course settings.

        Overall, I believe moodle/grade:manage is probably the best choice. As Jean-Philippe mentioned, that would allow for someone to essentially be put in charge of the gradebook while having no ability to alter the course outside of it.

        Alternatively, we may even want to demand both capabilities ie you must be able to alter course settings and manage grade items before we allow you to alter grade related course settings. If I'm not mistaken that would effectively match the current behaviour. Currently you need one capability to see the settings page in the navigation and the second capability to actually use the page.

        Show
        Andrew Davis added a comment - - edited It's an interesting question. I'm not sure there is a single correct answer but that's not terribly helpful. There are three relevant capabilities. moodle/grade:edit - edit grades moodle/grade:manage - manage grade items moodle/course:update - update course settings These form a hierarchy. Someone who can grade student work, someone who can alter grade items and someone who can alter course settings. moodle/grade:edit is just for people doing grading and we can ignore it here. On to the other two. Looking at the settings page, the settings available are all related to what is shown on the grade reports and how they are displayed. I feel like it would be a bit odd to allow someone to manage grade items but not to allow them to control their display on the grade reports. That said, they are course settings. Overall, I believe moodle/grade:manage is probably the best choice. As Jean-Philippe mentioned, that would allow for someone to essentially be put in charge of the gradebook while having no ability to alter the course outside of it. Alternatively, we may even want to demand both capabilities ie you must be able to alter course settings and manage grade items before we allow you to alter grade related course settings. If I'm not mistaken that would effectively match the current behaviour. Currently you need one capability to see the settings page in the navigation and the second capability to actually use the page.
        Hide
        Damyon Wiese added a comment -

        +1 for moodle/grade:manage only.

        It seems more useful to allow someone to update the grade settings but not the course settings than it is to allow someone to update the course settings but not the grade settings.

        Current default roles:

          moodle/grade:manage moodle/course:update
        Teacher Y Y
        Course creator not set not set
        Manager Y Y
        Non editing teacher not set not set

        So has no effect for default installs.

        Show
        Damyon Wiese added a comment - +1 for moodle/grade:manage only. It seems more useful to allow someone to update the grade settings but not the course settings than it is to allow someone to update the course settings but not the grade settings. Current default roles:   moodle/grade:manage moodle/course:update Teacher Y Y Course creator not set not set Manager Y Y Non editing teacher not set not set So has no effect for default installs.
        Hide
        Michael de Raadt added a comment -

        I think we have consensus (and are all more informed). The upgrade path forward also looks clear, which is nice.

        If you would like to work on this, Jean-Philippe, feel free to assign this to yourself.

        I've declassified this issue.

        Show
        Michael de Raadt added a comment - I think we have consensus (and are all more informed). The upgrade path forward also looks clear, which is nice. If you would like to work on this, Jean-Philippe, feel free to assign this to yourself. I've declassified this issue.
        Hide
        Jean-Philippe Gaudreau added a comment -

        Good! I was hoping you chose this solution. I'm working on it. Won't be too hard

        Show
        Jean-Philippe Gaudreau added a comment - Good! I was hoping you chose this solution. I'm working on it. Won't be too hard
        Hide
        Andrew Davis added a comment -

        [Y] Syntax
        [NA] Output
        [Y] Whitespace
        [NA] Language
        [NA] Databases
        [Y] Testing
        [Y] Security
        [NA] Documentation
        [Y] Git
        [Y] Sanity check

        Looks good. Submit for integration whenever you're ready.

        Show
        Andrew Davis added a comment - [Y] Syntax [NA] Output [Y] Whitespace [NA] Language [NA] Databases [Y] Testing [Y] Security [NA] Documentation [Y] Git [Y] Sanity check Looks good. Submit for integration whenever you're ready.
        Hide
        Jean-Philippe Gaudreau added a comment -

        Sorry, I believe I don't have the right to submit for integration or I don't know how to do it... Can you help with that ?

        Show
        Jean-Philippe Gaudreau added a comment - Sorry, I believe I don't have the right to submit for integration or I don't know how to do it... Can you help with that ?
        Hide
        Michael de Raadt added a comment -

        Hi, Jean-Philippe.

        You are correct. Andrew should probably have pushed this to integration for you. I will do that now.

        Show
        Michael de Raadt added a comment - Hi, Jean-Philippe. You are correct. Andrew should probably have pushed this to integration for you. I will do that now.
        Hide
        Dan Poltawski added a comment -

        The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

        TIA and ciao

        Show
        Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
        Hide
        Damyon Wiese added a comment -

        This issue has been integrated to 23, 24 and master.

        Thanks for working on this Jean-Philippe.

        Show
        Damyon Wiese added a comment - This issue has been integrated to 23, 24 and master. Thanks for working on this Jean-Philippe.
        Hide
        Rossiani Wijaya added a comment -

        Hi Jean-Philippe,

        Could you clarify the testing instruction for step 2 and 3?

        2. As a teacher, go in a course and click on Grades section
        3. TEST : Make sure that you see "Course grade settings" in the navigation bar and "Course > Settings" in the dropdown list of the page (top left).
        

        Within the course, grades section could only be access after clicking on the grade link under the course admin > grades

        If I select the grades link, it will direct me to the grading report page where the 'course grade setting' link is available under settings > grade admin section, instead of under the navigation section. The drop down list doesn't contain course >settings, it has settings > course.

        Also for step 1, the prevent option is no longer existed, it used a checkbox for allow.

        Could you confirm the above issue?

        Thanks
        Rosie

        Thanks

        Show
        Rossiani Wijaya added a comment - Hi Jean-Philippe, Could you clarify the testing instruction for step 2 and 3? 2. As a teacher, go in a course and click on Grades section 3. TEST : Make sure that you see "Course grade settings" in the navigation bar and "Course > Settings" in the dropdown list of the page (top left). Within the course, grades section could only be access after clicking on the grade link under the course admin > grades If I select the grades link, it will direct me to the grading report page where the 'course grade setting' link is available under settings > grade admin section, instead of under the navigation section. The drop down list doesn't contain course >settings, it has settings > course. Also for step 1, the prevent option is no longer existed, it used a checkbox for allow. Could you confirm the above issue? Thanks Rosie Thanks
        Hide
        Jean-Philippe Gaudreau added a comment -

        Hi Rosie,

        Sorry you're right I wasn't clear enough. So here it is :
        Step 2: I was actually talking about clicking on Grades in the Course administration.
        Step 3: Assert that you see the "Course grade settings" link in the Grade administration. And yes, assert that the drop down list contain "Settings > Course" not "course >settings".

        I'll change the testing instructions and modify step 1 to unchecked the checkbox instead of prevent option.

        Thx!

        Show
        Jean-Philippe Gaudreau added a comment - Hi Rosie, Sorry you're right I wasn't clear enough. So here it is : Step 2: I was actually talking about clicking on Grades in the Course administration. Step 3: Assert that you see the "Course grade settings" link in the Grade administration. And yes, assert that the drop down list contain "Settings > Course" not "course >settings". I'll change the testing instructions and modify step 1 to unchecked the checkbox instead of prevent option. Thx!
        Hide
        Rossiani Wijaya added a comment -

        Thanks for clarifying the testing instruction.

        I re-tested and it works as expected.

        Tested for 2.3, 2.4 and master.

        Test passed.

        Show
        Rossiani Wijaya added a comment - Thanks for clarifying the testing instruction. I re-tested and it works as expected. Tested for 2.3, 2.4 and master. Test passed.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        I feel myself really alone tonight! So was time to push your fixes upstream!

        "Lest we forget. We will remember them."

        Thanks and ciao!

        Show
        Eloy Lafuente (stronk7) added a comment - I feel myself really alone tonight! So was time to push your fixes upstream! "Lest we forget. We will remember them." Thanks and ciao!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: