Last modified: 2013-11-16 00:58:04 UTC
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?
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. 2) 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. 3) 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 https://gerrit.wikimedia.org/r/95701
Change 95701 merged by jenkins-bot: Include implicit rights in UserIsEveryoneAllowed https://gerrit.wikimedia.org/r/95701
