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

function cohort_get_invisible_contexts incorrectly checks system level cohorts

    XMLWordPrintable

Details

    • MOODLE_33_STABLE, MOODLE_34_STABLE, MOODLE_35_STABLE
    • MOODLE_33_STABLE, MOODLE_34_STABLE
    • MDL-61005-master
    • Hide

      Setup

      1. Login as an admin.
      2. Create some system level cohorts.
      3. Access to "Site administration | Users | Permissions | Define roles" and edit the "Teacher" role:
        1. Add "System" to the "Context types where this role may be assigned".
        2. Check the site-level capability "moodle/cohort:view" is checked.
      4. Access to "Site administration | Users | Permissions | Assign system roles" and assign role "Teacher" in System to a user.

       Testing scenario 1

      1. Login as admin.
      2. Go to Site administration > users > accounts > cohorts > "All Cohorts" tab
      3. Search for some cohorts
        • Make sure the search works as expected.

      Testing scenario 2

      1. Login as the user you've assigned the "Teacher" role in system.
      2. Go to Site administration > users > accounts > cohorts > "All Cohorts" tab (you'll need to write the URL http://yourmoodleserver/cohort/index.php?contextid=1&showall=0).
      3. Search for some cohorts
        • Make sure the search works and all site-level cohorts created show as available.

      Testing scenario 3

      (Nothing to do, CiBoT will tell us): Execute the phpunit for the testcases defined in "cohort/tests/externallib_test.php"

      Show
      Setup Login as an admin. Create some system level cohorts. Access to "Site administration | Users | Permissions | Define roles" and edit the "Teacher" role: Add "System" to the "Context types where this role may be assigned". Check the site-level capability "moodle/cohort:view" is checked. Access to "Site administration | Users | Permissions | Assign system roles" and assign role "Teacher" in System to a user.  Testing scenario 1 Login as admin. Go to Site administration > users > accounts > cohorts > "All Cohorts" tab Search for some cohorts Make sure the search works as expected. Testing scenario 2 Login as the user you've assigned the "Teacher" role in system. Go to Site administration > users > accounts > cohorts > "All Cohorts" tab (you'll need to write the URL http://yourmoodleserver/cohort/index.php?contextid=1&showall=0 ). Search for some cohorts Make sure the search works and all site-level cohorts created show as available. Testing scenario 3 (Nothing to do, CiBoT will tell us): Execute the phpunit for the testcases defined in "cohort/tests/externallib_test.php"

    Description

      cohort/lib.php cohort_get_all_cohorts() states:

      The function assumes that user capability to view/manage cohorts on system level

      • has already been verified. This function only checks if such capabilities have been
      • revoked in child (categories) contexts.

      then it calls: 
      $excludedcontexts = cohort_get_invisible_contexts())

      which finds the contexts that all cohorts might be in and then checks:
      if (!has_any_capability(array('moodle/cohort:manage', 'moodle/cohort:view'), context::instance_by_id($ctx->id)))

      using the context of the cohorts. 

       

      Problem is that it also includes the system level context in the loop and then checks to see if the user has cohort:manage or cohort:view at the system context and if not, excludes the system level cohorts from the list.

      system level cohorts are "special" and permission can be provided at the course-level using the capability "moodle/cohort:view" so it should not be checking the system level context in that loop inside cohort_get_invisible_cohorts.

      hard to reproduce in core, but I'm looking at a separate improvement - re-using the admin tool_lp/cohort searchable cohort selector in the enrolment cohort sync plugin which discovered this particular issue.

      I'll try to provide a pull request to deal with this at some point.

       

      Attachments

        1. admin.PNG
          admin.PNG
          113 kB
        2. teacher.PNG
          teacher.PNG
          111 kB

        Issue Links

          Activity

            People

              danmarsden Dan Marsden
              danmarsden Dan Marsden
              Sara Arjona (@sarjona) Sara Arjona (@sarjona)
              Eloy Lafuente (stronk7) Eloy Lafuente (stronk7)
              Janelle Barcega Janelle Barcega
              Amaia Anabitarte, Carlos Escobedo, Ferran Recio, Ilya Tregubov, Laurent David, Sara Arjona (@sarjona)
              Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:
                19/Mar/18

                Time Tracking

                  Estimated:
                  Original Estimate - 0 minutes
                  0m
                  Remaining:
                  Remaining Estimate - 0 minutes
                  0m
                  Logged:
                  Time Spent - 34 minutes
                  34m