On Mon, Aug 11, 2008 at 7:50 AM, brion@svn.wikimedia.org wrote:
- Clean up horrible horrible permission checks which are 1000 times more complicated than they should be. We have User::isAllowed() for a reason, kids :)
I've since fixed up the getUserPermissionsErrors function to allow an array of errors to be ignored to be passed.
It seems like a good idea to prevent blocked users from globally blocking, and to allow extensions / other code to prevent doing action X on title Y. This is why I prefer to use getUserPermissionsErrors rather than isAllowed, since isAllowed only checks if the user has the appropriate permission (this is the point of getUserPermissionsErrors - to be a one-stop shop for checking permissions).
Also, the wfReadOnly checks are done in getUserPermissionsErrors, so we need to add them separately if we are to use User::isAllowed.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Andrew Garrett wrote:
On Mon, Aug 11, 2008 at 7:50 AM, brion@svn.wikimedia.org wrote:
- Clean up horrible horrible permission checks which are 1000 times more complicated than they should be. We have User::isAllowed() for a reason, kids :)
I've since fixed up the getUserPermissionsErrors function to allow an array of errors to be ignored to be passed.
Why should it ignore any errors? This makes no sense to me...
It seems like a good idea to prevent blocked users from globally blocking, and to allow extensions / other code to prevent doing action X on title Y. This is why I prefer to use getUserPermissionsErrors rather than isAllowed, since isAllowed only checks if the user has the appropriate permission (this is the point of getUserPermissionsErrors
- to be a one-stop shop for checking permissions).
These aren't page actions that you do "to" Special:GlobalBlockList, they're general actions that you perform in the system.
Also, the wfReadOnly checks are done in getUserPermissionsErrors, so we need to add them separately if we are to use User::isAllowed.
Read-only isn't a permission issue, it's a "the system can't handle this right now" issue.
- -- brion
I don't, on the whole, have a big problem with using isAllowed instead of getUserPermissionsErrors - either provides adequate security, and it is certainly simpler to run isAllowed than getUserPermissionsErrors. I've changed my mind partially because of your comments about the idea of doing an action "to" a page (although ideally we could think about it as being "done" to the range), and partially because I've realised that most of the issues I was worried about (not being able to simply block user X from doing action Y in an extension) could be done with other hooks (such as UserGetRights).
Nevertheless, I've made a few points below about the current permissions system brought up by this change.
Generally speaking, the permissions code does need to be smoothed over. It's currently a mishmash of older code which uses the old sequence of checking read-only, blocks, rights individually, some newer code which does the same, code which does some subset of the above, and code which uses getUserPermissionsErrors, with various hacks and so on to make it work (such as the count() stuff you removed). Certainly, if I decided today to make the changes to permissions code that I made back last year, I would have done them differently (and perhaps heeded Simetrical's advice to move the checks away from being methods of the Title object to being methods of the User object, with the title becoming optional).
On Tue, Aug 12, 2008 at 2:50 AM, Brion Vibber brion@wikimedia.org wrote:
Andrew Garrett wrote:
I've since fixed up the getUserPermissionsErrors function to allow an array of errors to be ignored to be passed.
Why should it ignore any errors? This makes no sense to me...
getUserPermissionsErrors, for compatibility with the older userCan interface, returns a permissions error on Special-page titles. This error obviously needs to be ignored in this case.
Also, the wfReadOnly checks are done in getUserPermissionsErrors, so we need to add them separately if we are to use User::isAllowed.
Read-only isn't a permission issue, it's a "the system can't handle this right now" issue.
This is true, but it's always checked along with permissions (and was, IIRC, in the original userCan method before I touched it, so backwards-compatibility is relevant), so it makes sense to put it in the getUserPermissionsErrors method, so a separate check doesn't have to be done every time we check permissions.
On Tue, Aug 12, 2008 at 8:34 AM, Andrew Garrett andrew@epstone.net wrote:
Generally speaking, the permissions code does need to be smoothed over. It's currently a mishmash of older code which uses the old sequence of checking read-only, blocks, rights individually, some newer code which does the same, code which does some subset of the above, and code which uses getUserPermissionsErrors, with various hacks and so on to make it work (such as the count() stuff you removed). Certainly, if I decided today to make the changes to permissions code that I made back last year, I would have done them differently (and perhaps heeded Simetrical's advice to move the checks away from being methods of the Title object to being methods of the User object, with the title becoming optional).
Another ugly thing is the errors format, with arrays of arrays. Tim has pointed out that a much cleaner interface would be an associative array of key => params. Then we could use if( isset( $errors['foo'] ) ) instead of some ungodly mess like $condition = false; foreach( $errors as $error ) if( $error[0] == 'foo' ) { $condition = true; break; } if( $condition ). Similarly we can use array_merge to merge errors instead of having to roll our own function, it's easier to unset errors, etc.
Alternatively, we might create a custom error class, but I'm not sure what the added utility of that would be in any immediate sense. What do other software projects do for reporting when multiple errors occur? Other than just reporting the first, of course, or giving a generic error message. For that matter, what kind of return format do they use for permissions checks altogether?
Maybe we should consider introducing another set of functions and using those. The current interface is remarkably terrible for the amount of discussion it received.
getUserPermissionsErrors, for compatibility with the older userCan interface, returns a permissions error on Special-page titles.
Which makes no sense, generally speaking. We should probably change this so that it only returns errors for known bad operations like edit or move. We don't need to preserve old interfaces if they're stupid. :) Of course, someone somewhere might be relying on the fact that attempting 'squizzle' on special pages will fail, but I doubt it. At worst it should fail when the operation is actually attempted, if it really makes no sense. There's not much you *can* do to a special page, since it has no presence in the database and that's the only thing that's changeable persistently across requests.
If another set of functions is introduced, though, it might be a good idea to *not* try to rewrite the current functions as wrappers for it. That way we avoid crufty nonsense like this. Build a good interface first, and only then consider whether and how to rewrite the old stuff. Maybe I'll try it this time -- I just rewrote another crufty old interface. :) (Maybe sometime I'll get back to writing features and fixing bugs, too . . .)
This is true, but it's always checked along with permissions (and was, IIRC, in the original userCan method before I touched it, so backwards-compatibility is relevant), so it makes sense to put it in the getUserPermissionsErrors method, so a separate check doesn't have to be done every time we check permissions.
It's not always checked with permissions. If a user doesn't have permission to do something, that means that for policy reasons, they are not allowed to do it. If the wiki is read-only, the user *does* have permission to do the action, just it's impossible at the moment (but should become possible shortly). For examples of where this makes a difference, consider links like "edit" or "block". We should and do display those if the database is read-only, but not if the user doesn't have permission. Similarly, if a user tries to save an edit, a permissions error results in a simple error page -- a read-only error returns a preview page with a message at the top asking the user to resubmit. It makes no sense to lump the two types of errors together.
wikitech-l@lists.wikimedia.org