Moodle

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

Details

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

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 (skodak) added a comment -

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

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

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

Show
Petr Škoda (skodak) 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 (skodak) added a comment -

sure, going to commit later today

Show
Petr Škoda (skodak) 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 (skodak) added a comment -

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

Show
Petr Škoda (skodak) added a comment - grrrrrrrrrrrrrrrrrrrr, wrong bug! I was working on MDL-19004, sorry!
Hide
Petr Škoda (skodak) 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 (skodak) 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 (skodak) 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 (skodak) 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

Vote (0)
Watch (6)

Dates

  • Created:
    Updated:
    Resolved: