Moodle
  1. Moodle
  2. MDL-32631

calendar_event::update() should not do capability checks

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Won't Fix
    • Affects Version/s: 2.2.2
    • Fix Version/s: None
    • Component/s: Calendar
    • Labels:
    • Affected Branches:
      MOODLE_22_STABLE
    • Rank:
      39560

      Description

      This is a tricky issue that was spotted while working on MDL-32630. The calendar_event::update() by default calls calendar_add_event_allowed(). While this is appropriate when dealing with manually created events, doing such check is wrong for events created/updated by module themselves. Imagine the following scenario:

      • the user has capability to edit the assignment settings
      • but the same user does not have the permissions to modify the course calendar (moodle/calendar:manageentries)

      What should happen now when the user modifies the assignment due date in the settings form? The common sense says that the event should be updated in the calendar as it is actually the assignment updating the event, not the user. And having different date in the assignment and in the calendar would definitely be wrong for students' experience.

      Sooo... This is a proposal to move the permission checks completely from the calendar_event API to the caller's scope where appropriate (that is call calendar_add_event_allowed() bascially only when dealing with manually created events.

      Not convienced yet? Here are some arguments:

      • As a general rule, libraries should not check for permissions, it is the caller's duty
      • While calendar_event::update() checks for the capabilities, calendar_event::delete() ignores them completely anyway so even the current behaviour is not ideal
      • The checks in calendar_add_event_allowed() are based on $event->eventtype but that field can have literally any value for events created by modules themselves. So calling that functions from {module}

        _add_instance() is potentially buggy anyway.

        Issue Links

          Activity

          David Mudrak created issue -
          Tim Hunt made changes -
          Field Original Value New Value
          Summary calendar_event::update() checks for permissions Remove capability checks from calendar_event::update()
          David Mudrak made changes -
          Link This issue will help resolve MDL-32630 [ MDL-32630 ]
          Tim Hunt made changes -
          Summary Remove capability checks from calendar_event::update() calendar_event::update() should not do capability checks
          Michael de Raadt made changes -
          Fix Version/s STABLE backlog [ 10463 ]
          Labels triaged
          Affects Version/s 2.2.2 [ 11552 ]
          Affects Version/s 2.3 [ 10657 ]
          Hide
          Michael de Raadt added a comment - - edited

          I thought I would ask a question for whoever deals with this in future: Is there a way of distinguishing the origin of the call? Perhaps an argument could be added so that user changes can be distinguished and still properly checked.

          Show
          Michael de Raadt added a comment - - edited I thought I would ask a question for whoever deals with this in future: Is there a way of distinguishing the origin of the call? Perhaps an argument could be added so that user changes can be distinguished and still properly checked.
          Andrew Nicols made changes -
          Link This issue blocks CONTRIB-3625 [ CONTRIB-3625 ]
          Andrew Nicols made changes -
          Link This issue blocks CONTRIB-3625 [ CONTRIB-3625 ]
          Andrew Nicols made changes -
          Link This issue caused a regression CONTRIB-3625 [ CONTRIB-3625 ]
          Hide
          Michael de Raadt added a comment -

          Thanks for reporting this issue.

          We have detected that this issue has been inactive for over a year. It was reported as affecting versions that are no longer supported.

          If you believe that this issue is still relevant to current versions (2.5 and beyond), please comment on the issue. Issues left inactive for a further month will be closed.

          Michael d.

          TW9vZGxlDQo=

          Show
          Michael de Raadt added a comment - Thanks for reporting this issue. We have detected that this issue has been inactive for over a year. It was reported as affecting versions that are no longer supported. If you believe that this issue is still relevant to current versions (2.5 and beyond), please comment on the issue. Issues left inactive for a further month will be closed. Michael d. TW9vZGxlDQo=
          Hide
          Michael de Raadt added a comment -

          I'm closing this issue as it has been inactive for over a year has been recorded as affecting versions that are no longer supported.

          This is being done as part of a bulk annual clean-up of issues.

          If you still believe this is an issue in supported versions, please create a new issue.

          Show
          Michael de Raadt added a comment - I'm closing this issue as it has been inactive for over a year has been recorded as affecting versions that are no longer supported. This is being done as part of a bulk annual clean-up of issues. If you still believe this is an issue in supported versions, please create a new issue.
          Michael de Raadt made changes -
          Status Open [ 1 ] Closed [ 6 ]
          Assignee moodle.com [ moodle.com ]
          Resolution Won't Fix [ 2 ]
          Fix Version/s STABLE backlog [ 10463 ]

            People

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

              Dates

              • Created:
                Updated:
                Resolved: