Page MenuHomePhabricator

OAuth cache suppression impact
Closed, ResolvedPublic


For OAuth, we added a hook to User::isEveryoneAllowed() to remove rights on OAuth wikis, since "everyone" (including an api request by an OAuth consumer) may not be able to do everything.

However, lots of places in the code, we use User::isEveryoneAllowed( 'read' ) to decide if this is a private wiki, and if so, disable caching (ApiMain, RawAction) or do more extensive checks (Title).

I'm worried enabling OAuth may have larger performance impact in general. Should onUserIsEveryoneAllowed return true for 'read' if the wiki is public?

Version: master
Severity: blocker



Event Timeline

bzimport raised the priority of this task from to High.Nov 22 2014, 2:34 AM
bzimport set Reference to bz56975.

Define "public", isn't that just "'*' is allowed 'read'"?

I originally suggested that we include a way to specify rights that all OAuth consumers get to have automatically for this sort of situation.

Yeah, "public" seems to have always been defined as "'*' is allowed 'read'". I just didn't fully understand the impact of not having it. Looking more at it yesterday, I think the performance hit would be pretty bad if we flipped it on as is.

It seems like there are a couple ways to fix it, but Brad, since you did a lot of that work I want to make sure it sounds sane to you.

All uses of isEveryoneAllowed() in core and extensions that I could find, are to check 'read', basically to decide if the wiki is public or not. So we could either:

  1. Change those back to checking if '*' has read directly.
  1. Change the OAuth hook to only return false if the right isn't one of the basic rights, since we mostly assume that will always be available.
  1. Remove the hook from OAuth, under the reasoning that if * is allowed a right, then the OAuth app can make an anonymous call just as easily.

I actually like having the hook-- it solves some of the issues that a lot of the access control extensions have struggled with, and I think it's useful. For the second two options, 3 seems like it would simplify the system overall, but maybe there are some rights (other than read) we would want to pull out?

Option 2 seems like the best to me, although then I'd really like to see every consumer being required to have that basic rights group.

I think the reason we only see isEveryoneAllowed used with 'read' is because read has the combination of being almost-always allowed along with having to be checked on basically every page view. Other rights don't have that combination, so short-circuiting based on them isn't as much of a win.

Change 95701 had a related patch set uploaded by CSteipp:
Include useoauth in UserIsEveryoneAllowed rights

Change 95701 merged by jenkins-bot:
Include implicit rights in UserIsEveryoneAllowed