Moodle
  1. Moodle
  2. MDL-21782 Major 2.0 enrolments rewrite META
  3. MDL-23232

enrol UI ajax must explicitly ask each enrol plugin instance if manual enrol allowed

    Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0
    • Component/s: Enrolments
    • Labels:
      None
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      32817

      Description

      At present the enrol ajax behind the UI uses $plugin->enrol_user($instance, $user->id, $roleid, $timestart, $timeend);

      This is very wrong, you must explicitly ask the plugin if it allows manual enrolments. At minimum you would not be allowed to unenrol, but it could create severe other problems or break synchronisation.

        Activity

        Hide
        Petr Škoda added a comment -

        OH! there is no access control check when enrolling users in enrol/ajax.php

        Show
        Petr Škoda added a comment - OH! there is no access control check when enrolling users in enrol/ajax.php
        Hide
        Petr Škoda added a comment -

        found more places with missing access control there, fixing......

        Show
        Petr Škoda added a comment - found more places with missing access control there, fixing......
        Hide
        Petr Škoda added a comment -

        hey! you have removed all role access control and validation even from enrol/users!
        you must validate that the role is actually assignable by that user in that context before doing anything with it

        Show
        Petr Škoda added a comment - hey! you have removed all role access control and validation even from enrol/users! you must validate that the role is actually assignable by that user in that context before doing anything with it
        Hide
        Petr Škoda added a comment -

        I have added a allow_enrol() method into the base class and patched enrol/manual.

        problems:
        1/ add roles validation and access control into both ajax.php and users.php to both assign and unassign
        2/ add enrol access control and plugin validation - again both
        3/ edit_enrolment uses incorrect capability, should be manage

        I would personally prefer to have the basic access control outside of manager class because it makes reviews many times easier. It would not matter if it was duplicated somewhere down deep such as the role:assign cap

        Show
        Petr Škoda added a comment - I have added a allow_enrol() method into the base class and patched enrol/manual. problems: 1/ add roles validation and access control into both ajax.php and users.php to both assign and unassign 2/ add enrol access control and plugin validation - again both 3/ edit_enrolment uses incorrect capability, should be manage I would personally prefer to have the basic access control outside of manager class because it makes reviews many times easier. It would not matter if it was duplicated somewhere down deep such as the role:assign cap
        Hide
        Sam Hemelryk added a comment -

        Hi Petr,

        I've just commit a series of changes to locallib, users, and ajax that add the missing capability and plugin checks and also copy the checks back into users and ajax.
        Could you please have a look at the changes I've made and the files in general and let me know if I have missed anything.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Petr, I've just commit a series of changes to locallib, users, and ajax that add the missing capability and plugin checks and also copy the checks back into users and ajax. Could you please have a look at the changes I've made and the files in general and let me know if I have missed anything. Cheers Sam

          People

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

            Dates

            • Created:
              Updated:
              Resolved: