Closed Bug 200174 Opened 22 years ago Closed 19 years ago

ConfirmGroup should be run when groups change, not all the time

Categories

(Bugzilla :: Bugzilla-General, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: myk, Unassigned)

Details

(Keywords: perf)

The groups rewrite includes a function, ConfirmGroup, which gets called every
time a non-anonymous user accesses Bugzilla.  It runs a query to determine if
groups have changed since the last time the user accessed a bug and rederives
group permissions if so.

This is a waste of resources, since it never has to happen unless groups change,
and then it has to happen for everyone.  Even on installations where most user
accounts are unused, it'll still end up taking more time to check if group
permissions need to be rederived every time in the long run than in just
rederiving them when the groups change.  The only exception to this is an
installation where groups change regularly and most accounts are unused, which
seems rare to nonexistent.

There's a second problem with the current approach: it makes the lives of
developers harder, because any time someone adds code to Bugzilla that needs to
find out if a user can see a bug (f.e. bug 179510), they have to remember to
call ConfirmGroup before calling CanSeeBug.  This dependency is not documented
in CanSeeBug, and leaving it out is a security risk, since code that makes it
into the tree without calling ConfirmGroup when necessary creates a security hole.

It's also unnecessary, since the code is perfectly capable of rederiving groups
when groups change.  Thus educating all new developers about this behavior is a
waste of time and energy.  If it weren't for the fact that the current behavior
spends resources unnecessarily, I would say that the solution is for CanSeeBug
itself to call ConfirmGroup when it detects that the user in question isn't the
current user.

Given the potential for security holes to be introduced into new code, however,
the solution is for groups to be rederived when groups change.

Relevant comments from the groups rewrite bug:

http://bugzilla.mozilla.org/show_bug.cgi?id=157756#c9
http://bugzilla.mozilla.org/show_bug.cgi?id=157756#c13
Note that User.pm dos this as part of the |new| call, so you can forget it, in
effect.

The issue is the time that it would take - we have to do a regexp match on every
single user, in perl (since we use perl regexps, not mysql ones). And its not
even that simple, since we may hve to remove/add groups based on that, so we'd
have to do the full derivation. Thats likely to be real slow - see numbers in
the bug you mentioned.

If it wasn't for the regexp thing, this would be simple, but.... (In postgres,
we could write a PL/Perl function to do this, and that would be faster by
avoiding the db<->perl overhead, I guess, although probably not enugh to notice)
It'd take an hour to run for every user on the system when groups change, and
the groups table would be READ locked the entire time, effectively taking down
the system.  That's why it's done one user at a time, the next time permissions
need to be checked for that user.

Note that not only does it have to be redone when groups change, but it has to
be redone when the user gets edited as well.
Adding Joel, since he wrote it.  Joel, please comment :)
If it takes a long time, then make it a background process whose progress can be
followed on the "edit groups" page.  Why would the groups table need to be READ
locked the whole time?
Very funny... an April 1 bug :-)

The problem is that it takes prohibitively long to update 40000 users when one
group changes and many of the SQL servers are rather crippled with respect to
regular expressions.  Therefore, before a user is given any privileges, we need
to know if the groups have changed since the user was upodated.  This should be
a really quick query.

Ideally, we would look at the change and run down all the users who would be
impacted as the edit is completed.  Unfortumnately, that would require having
the SQL server process an arbitrary regexp.   MySQL can do this, but some others
cannot.
>The problem is that it takes prohibitively long to update 40000 users when one
>group changes

I'd be surprised if there weren't optimizations available.

>and many of the SQL servers are rather crippled with respect to 
>regular expressions.

Argh.  So now we're going to make the app worse so that it works with more SQL
servers?  We should provide emulation code for the ones that don't support it.

>Therefore, before a user is given any privileges, we need
>to know if the groups have changed since the user was upodated.  This should be
>a really quick query.

Quick or not, it's unnecessary and wasteful.  My objection is not just
hypothetical, either; b.m.o has suffered in the past from these types of queries.
Perhaps a solution to this problem is a simpler query in ConfirmGroup()...

Add a column to the profiles table (calc_groups tinyint default 1). Whenever a
group change is made, run "UPDATE profiles SET calc_groups = 1". Then change the
query in ConfirmGroup() to be "SELECT calc_group FROM profiles WHERE userid =
$userid AND calc_groups = 1". This will eliminate the date calculation and the
table join.

This would, of course, be conceeding that the calc needs to happen when the user
first accesses Bugzilla not when the change is made. It would also still be
doing a query on every access, just a simpler one.
OK, Building on what Jake says....  if it's in the profile, just load it with
the rest of the user's record during the query we're already running to load
that user when we get their logincookie.
myk: One of those sql servers starts with 'My' :)

More seriously, we chose perl regexps, so unless the db supports them (and I
don't know any which do; postgres was making noices about using pcre, but I
don't know if that happened) we can't really switch.

Is the checking overhead really noticable at all? You could add an index to
last_used, and that may help, but there are only arround 10 groups anyway, so
the index probably won't end up being used anyway.

The UPDATE overhead for the cookie stuff probably takes more time (and I should
really get arround to fixing that at some point, along with the check which myk
has removed from bmo. Its on the TODO list...)
I don't know how long the query takes, but I know it takes longer than necessary
and that this method creates the potential for security holes.  If there's no
way around doing what we're doing (and I still think there is), then the least
we can do is run ConfirmGroup in CanSeeBug whenever necessary so the call
doesn't have to be inserted into every piece of code that wants to use CanSeeBug
to check a user's permissions.
WRT comment 10, CanSeeBug should only need to confirm if the user has not
already been confirmed.  This is done automatically when login is confirmed for
the logged-in user.  It also only needs to be done once per user for
notifications.  So, it should be possible to cache (in a hash) which users are
known to be up-to-date and, possibly, cache (in a variable) the date groups were
last updated to simplify the query.
new Bugzilla::User will do this for you, as soon as I finish off the patch.

I really doubt that this wuery shows up anywhere on a profile. I'm willing to be
proved wrong, though, and an index on teh last_updated column wouldn't hurt.
See bug 179510, comment 23 for more problems with the current way of doing things.
Reassigning bugs that I'm not actively working on to the default component owner
in order to try to make some sanity out of my personal buglist.  This doesn't
mean the bug isn't being dealt with, just that I'm not the one doing it.  If you
are dealing with this bug, please assign it to yourself.
Assignee: justdave → general
QA Contact: mattyt-bugzilla → default-qa
ConfirmGroup is now part of User::_create() and DeriveGroup is
User::derive_groups().
The underlying issue here seems to have been fixed during the Bugzilla::User
rewrite in the 2.17.7 timeframe.  This bug was an alternate way to fix it, which
is no longer necessary.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.