Moodle
  1. Moodle
  2. MDL-17499

Horrible hack in can_delete_course in course/lib.php

    Details

    • Type: Bug Bug
    • Status: Development in progress
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 1.8.7, 1.9.3, 2.0, 2.2, 2.3
    • Fix Version/s: None
    • Component/s: Libraries, Roles / Access
    • Labels:
      None
    • Testing Instructions:
      Hide

      Make sure your test user has 'moodle/course:create' but does NOT have 'moodle/course:delete'.

      Create a course and check that you can not delete it.

      Give that user 'moodle/course:delete' and check that they can now delete the course.

      Show
      Make sure your test user has 'moodle/course:create' but does NOT have 'moodle/course:delete'. Create a course and check that you can not delete it. Give that user 'moodle/course:delete' and check that they can now delete the course.
    • Affected Branches:
      MOODLE_18_STABLE, MOODLE_19_STABLE, MOODLE_20_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-17499_course_delete
    • Rank:
      2257

      Description

      This is all the fault of MDL-7796. That was supposed to be about restoring features that had gone away in the 1.7 roles upgrade. However, following this comment http://tracker.moodle.org/browse/MDL-7796?focusedCommentId=24645#action_24645, Vy implemented the new feature that course creators can delete 'their own' courses in the only way possible, via a nasty hack, since there is no way of knowing who created which course.

      I have checked and this was not the case in Moodle 1.6. Before roles, deleting courses was protected by a big fat isadmin() check.

      I would really like to change back to just using has_capability('moodle/course:delete');

        Issue Links

          Activity

          Hide
          Dan Poltawski added a comment -

          yuk, this is still here!

          Show
          Dan Poltawski added a comment - yuk, this is still here!
          Hide
          Martin Dougiamas added a comment -
          /**
           * Can the current user delete this course?
           * Course creators have exception,
           * 1 day after the creation they can sill delete the course.
           * @param int $courseid
           * @return boolean
           */
          function can_delete_course($courseid) {
              global $USER, $DB;
          
              $context = get_context_instance(CONTEXT_COURSE, $courseid);
          
              if (has_capability('moodle/course:delete', $context)) {
                  return true;
              }
          
              // hack: now try to find out if creator created this course recently (1 day)
              if (!has_capability('moodle/course:create', $context)) {
                  return false;
              }
          
              $since = time() - 60*60*24;
          
              $params = array('userid'=>$USER->id, 'url'=>"view.php?id=$courseid", 'since'=>$since);
              $select = "module = 'course' AND action = 'new' AND userid = :userid AND url = :url AND time > :since";
          
              return $DB->record_exists_select('log', $select, $params);
          }
          
          Show
          Martin Dougiamas added a comment - /** * Can the current user delete this course? * Course creators have exception, * 1 day after the creation they can sill delete the course. * @param int $courseid * @ return boolean */ function can_delete_course($courseid) { global $USER, $DB; $context = get_context_instance(CONTEXT_COURSE, $courseid); if (has_capability('moodle/course:delete', $context)) { return true ; } // hack: now try to find out if creator created this course recently (1 day) if (!has_capability('moodle/course:create', $context)) { return false ; } $since = time() - 60*60*24; $params = array('userid'=>$USER->id, 'url'=> "view.php?id=$courseid" , 'since'=>$since); $select = "module = 'course' AND action = ' new ' AND userid = :userid AND url = :url AND time > :since" ; return $DB->record_exists_select('log', $select, $params); }
          Hide
          Martin Dougiamas added a comment -

          Yeah, bit wierd. I guess it should go.

          Show
          Martin Dougiamas added a comment - Yeah, bit wierd. I guess it should go.
          Hide
          Dan Poltawski added a comment -

          Just came across is again whilst reviewing MDL-13130

          Show
          Dan Poltawski added a comment - Just came across is again whilst reviewing MDL-13130
          Hide
          Andrew Davis added a comment -

          Here is the simplest possible fix. You can either delete courses or you can't.

          Its possible we could be smarter about identifying the course creator but I'm a little uncomfortable with the whole idea. I can see why being able to delete a course you just created regardless makes some sense to users but side stepping capabilities seems... unwise.

          Show
          Andrew Davis added a comment - Here is the simplest possible fix. You can either delete courses or you can't. Its possible we could be smarter about identifying the course creator but I'm a little uncomfortable with the whole idea. I can see why being able to delete a course you just created regardless makes some sense to users but side stepping capabilities seems... unwise.
          Hide
          CiBoT added a comment -
          Show
          CiBoT added a comment - Results for MDL-17499 Remote repository: https://github.com/andyjdavis/moodle.git Remote branch MDL-17499 _course_delete to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2825 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/2825/artifact/work/smurf.html
          Hide
          Ankit Agarwal added a comment - - edited

          Hi Andrew,
          I recently worked on this method to remove the call to log table. It was discussed in backend SCRUM earlier, and the use case for this was:-
          Normally teachers don't have delete capability for courses where as they have create capability. So if they create something accidentally, they are not able to get rid of it, without contacting admins.

          Anyway this definitely should be discussed in forums imo before proceeding. Also if it is decided to remove this, a few things about the code:-

          1. This needs a mention in upgrade.txt
          2. The if statement is not required, just return has_capability('moodle/course:delete', $context); will do.

          Cheers

          Show
          Ankit Agarwal added a comment - - edited Hi Andrew, I recently worked on this method to remove the call to log table. It was discussed in backend SCRUM earlier, and the use case for this was:- Normally teachers don't have delete capability for courses where as they have create capability. So if they create something accidentally, they are not able to get rid of it, without contacting admins. Anyway this definitely should be discussed in forums imo before proceeding. Also if it is decided to remove this, a few things about the code:- This needs a mention in upgrade.txt The if statement is not required, just return has_capability('moodle/course:delete', $context); will do. Cheers

            People

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

              Dates

              • Created:
                Updated: