_verifyUser can be extremely expensive for Anonymous

Bug #273680 reported by Paul Winkler
Affects Status Importance Assigned to Milestone
Zope PAS
Fix Released
Tres Seaver

Bug Description

r78370 fixed the problem with PAS._verifyUser() returning wrong users due to inexact matching.
But it introduced a new problem, because the subsequent if clause could never be false, and the rest of the code was always run. If the user is Anonymous, so user_id and login are both None, the criteria is just {'exact_match': True}.

This means that we're likely to run into the scenario described in IUserEnumerationPlugin.enumerateUsers():

        o Insufficiently-specified criteria may have catastrophic
          scaling issues for some implementations.

Membrane is a good example of such an implementation. If you call enumerateUsers without a user_id or login, membrane returns ALL users. And aside from the hideous expense, PAS._verifyUser() ends up returning the first (arbitrary) user! This is truly both Bad And Wrong.

Returning all users when given no criteria seems like a reasonable interpretation of the interface; I don't think the fault is Membrane's, I think we should not be getting that far - and before r78370, this would not happen.

I discovered this in the wild, wondering why our production site was ridiculously slow on some pages for anonymous users. We're using the PAS 1.5 branch with membrane trunk r71705.

Fixed in the attached patch against trunk, with a test that fails before the fix.

Revision history for this message
Paul Winkler (slinkp) wrote :
Revision history for this message
Tres Seaver (tseaver) wrote :
Changed in zope-pas:
assignee: nobody → tseaver
status: New → Fix Committed
Revision history for this message
Tres Seaver (tseaver) wrote :
Changed in zope-pas:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers