Moodle
  1. Moodle
  2. MDL-19039

get_users_by_capability sometimes load all users from DB when in the system context

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 1.9.4
    • Fix Version/s: 2.0
    • Component/s: Roles / Access
    • Labels:
      None
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      31734

      Description

      Sam reported in MDL-19038:

      Tim - the function get_u_b_c doesn't actually return all half-million users - but when you make query at CONTEXT_SYSTEM level like this, it really DOES retrieve all half-million users into php, then it filters them. This is about line 4670 in accesslib. Code below.

      I think your lazy generation patch is correct (although it would be nice if somebody could make get_u_b_c faster for this case, but that's likely to be an awful lot harder to figure out).

          if ($context->contextlevel == CONTEXT_SYSTEM
              || $isfrontpage
              || $defaultroleinteresting) {
      
              // Handle system / sitecourse / defaultrole-with-perhaps-neg-overrides
              // with a SELECT FROM user LEFT OUTER JOIN against ra -
              // This is expensive on the SQL and PHP sides -
              // moves a ton of data across the wire.
              $ss = "SELECT u.id as userid, ra.roleid,
                            ctx.depth
                     FROM {$CFG->prefix}user u
                     LEFT OUTER JOIN {$CFG->prefix}role_assignments ra 
                       ON (ra.userid = u.id
                           AND ra.contextid IN ($ctxids)
                           AND ra.roleid IN (".implode(',',$roleids) .")
                           $condhiddenra)
                     LEFT OUTER JOIN {$CFG->prefix}context ctx
                       ON ra.contextid=ctx.id
                     WHERE u.deleted=0";
      

      Clearly this is silly. The CONTEXT_SYSTEM case must be easy to do in pure SQL.

      Note that there are quite extensive unit tests for get_users_by_capability in HEAD. If you plan to work on this, you probably should use them to reduce the chance of regressions.

        Activity

        Hide
        Tim Hunt added a comment -

        Adding a handful of watchers.

        Show
        Tim Hunt added a comment - Adding a handful of watchers.
        Hide
        Petr Škoda added a comment -

        hmm, I already proposed simplified capability evaluation rules that eliminate this and similar problems completely...

        Show
        Petr Škoda added a comment - hmm, I already proposed simplified capability evaluation rules that eliminate this and similar problems completely...
        Hide
        Petr Škoda added a comment -

        postponing, sorry - changes in this area will require longer testing and review

        Show
        Petr Škoda added a comment - postponing, sorry - changes in this area will require longer testing and review
        Hide
        Martin Dougiamas added a comment -

        Anything we can do in 1.9.6? Can you update the status please?

        Show
        Martin Dougiamas added a comment - Anything we can do in 1.9.6? Can you update the status please?
        Hide
        Petr Škoda added a comment -

        sure, going to commit later today

        Show
        Petr Škoda added a comment - sure, going to commit later today
        Hide
        Tim Hunt added a comment -

        What are you going to commit? I am happy to review patches.

        Show
        Tim Hunt added a comment - What are you going to commit? I am happy to review patches.
        Hide
        Petr Škoda added a comment -

        grrrrrrrrrrrrrrrrrrrr, wrong bug! I was working on MDL-19004,
        sorry!

        Show
        Petr Škoda added a comment - grrrrrrrrrrrrrrrrrrrr, wrong bug! I was working on MDL-19004 , sorry!
        Hide
        Petr Škoda added a comment -

        The problem is caused by negative permissions in authenticated users role (negative perm in frontpage role does something similar) - that is not recommended at present and will not work much any way. Please do not use any negative capabilities in the definition of roles at all, in fact do not use it anywhere if possible.

        Theoretically it should be possible to write such SQL at the system level, but I think it is a waste of time because the workaround is simple and in 2.0 the permissions where negative caps are involved will be probably different.

        Changing fix version for now, sorry.

        Show
        Petr Škoda added a comment - The problem is caused by negative permissions in authenticated users role (negative perm in frontpage role does something similar) - that is not recommended at present and will not work much any way. Please do not use any negative capabilities in the definition of roles at all, in fact do not use it anywhere if possible. Theoretically it should be possible to write such SQL at the system level, but I think it is a waste of time because the workaround is simple and in 2.0 the permissions where negative caps are involved will be probably different. Changing fix version for now, sorry.
        Hide
        Petr Škoda added a comment -

        this should be finally fixed after the recent commit enrolment related changes, please note the activity code may need to be updated
        thanks for the report

        Petr

        Show
        Petr Škoda added a comment - this should be finally fixed after the recent commit enrolment related changes, please note the activity code may need to be updated thanks for the report Petr

          People

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

            Dates

            • Created:
              Updated:
              Resolved: