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

Wrong capability check for role assign button in course?

    Details

      Description

      In /course/lib.php, line 3177, there is the output for the "assign role" button which will be shown in a course in editing mode near every activity:

          if (has_capability('moodle/course:managegroups', $modcontext)){
              $context = get_context_instance(CONTEXT_MODULE, $mod->id);
              $assign = '<a class="editing_assign" title="'.$str->assign.'" href="'.$CFG->wwwroot.'/'.$CFG->admin.'/roles/assign.php?contextid='.
                  $context->id.'"><img src="'.$OUTPUT->pix_url('i/roles') . '" alt="'.$str->assign.'" class="iconsmall"/></a>';
          } else {
              $assign = '';
          }

      I think the capability check is wrong as this button has nothing to do with groups. Shouldn't it be:

          if (has_capability('moodle/role:assign', $modcontext)){
              $context = get_context_instance(CONTEXT_MODULE, $mod->id);
              $assign = '<a class="editing_assign" title="'.$str->assign.'" href="'.$CFG->wwwroot.'/'.$CFG->admin.'/roles/assign.php?contextid='.
                  $context->id.'"><img src="'.$OUTPUT->pix_url('i/roles') . '" alt="'.$str->assign.'" class="iconsmall"/></a>';
          } else {
              $assign = '';
          }

      ?

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for reporting that. I think you are correct.

            Also, the managegroups capability check should be being applied to the group mode toggle icon immediately before the roles icon in that file.

            Show
            salvetore Michael de Raadt added a comment - Thanks for reporting that. I think you are correct. Also, the managegroups capability check should be being applied to the group mode toggle icon immediately before the roles icon in that file.
            Hide
            skodak Petr Skoda added a comment -

            Hello,

            I am lowering the severity and removing security level because the button itself does not give users rights to modify the roles - worst case scenario is that they get button pointing to page they can not access.

            The second issue with groups is a bit more tricky because the standard groupmode switch and ajax script use different access control, I am going to create a new issue for that and link it from here.

            Show
            skodak Petr Skoda added a comment - Hello, I am lowering the severity and removing security level because the button itself does not give users rights to modify the roles - worst case scenario is that they get button pointing to page they can not access. The second issue with groups is a bit more tricky because the standard groupmode switch and ajax script use different access control, I am going to create a new issue for that and link it from here.
            Hide
            skodak Petr Skoda added a comment -

            Thanks a lot for the report and proposed solution.

            Petr

            Show
            skodak Petr Skoda added a comment - Thanks a lot for the report and proposed solution. Petr
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks Petr - this has been integrated now

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Petr - this has been integrated now
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Tested during integration review. Passed.

            Show
            samhemelryk Sam Hemelryk added a comment - Tested during integration review. Passed.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Whoever decided one week was worth 14 days had really one bad idea. Anyway, the nightmare is over, so thanks for your, once again, amazing contributions. Many, many thanks!

            Now... disconnect, relax and enjoy the next days, yay!

            Closing...ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Whoever decided one week was worth 14 days had really one bad idea. Anyway, the nightmare is over, so thanks for your, once again, amazing contributions. Many, many thanks! Now... disconnect, relax and enjoy the next days, yay! Closing...ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  9/Jan/12