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

Horrible hack in can_delete_course in course/lib.php

    Details

    • Type: Bug
    • Status: Open
    • Priority: 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

      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');

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            poltawski Dan Poltawski added a comment -

            yuk, this is still here!

            Show
            poltawski Dan Poltawski added a comment - yuk, this is still here!
            Hide
            dougiamas 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
            dougiamas 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
            dougiamas Martin Dougiamas added a comment -

            Yeah, bit wierd. I guess it should go.

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

            Just came across is again whilst reviewing MDL-13130

            Show
            poltawski Dan Poltawski added a comment - Just came across is again whilst reviewing MDL-13130
            Hide
            andyjdavis 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
            andyjdavis 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 CiBoT added a comment -
            Show
            cibot 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_frenz 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_frenz 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
            Hide
            andyjdavis Andrew Davis added a comment -

            Unassigning myself as I am unable to work on this in the short term.

            Show
            andyjdavis Andrew Davis added a comment - Unassigning myself as I am unable to work on this in the short term.

              People

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

                Dates

                • Created:
                  Updated: