Uploaded image for project: '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 Master Branch:
      MDL-39084-master

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

        Gliffy Diagrams

          Activity

          Hide
          gaudreaj 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
          gaudreaj 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
          salvetore 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
          salvetore 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
          andyjdavis 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
          andyjdavis 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 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 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
          salvetore 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
          salvetore 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
          gaudreaj 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
          gaudreaj 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
          andyjdavis 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
          andyjdavis 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
          gaudreaj 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
          gaudreaj 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
          salvetore 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
          salvetore 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
          poltawski 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
          poltawski 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 Damyon Wiese added a comment -

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

          Thanks for working on this Jean-Philippe.

          Show
          damyon Damyon Wiese added a comment - This issue has been integrated to 23, 24 and master. Thanks for working on this Jean-Philippe.
          Hide
          rwijaya 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
          rwijaya 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
          gaudreaj 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
          gaudreaj 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
          rwijaya 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
          rwijaya 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
          stronk7 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
          stronk7 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:
                Fix Release Date:
                13/May/13