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(a)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.
--
Andrew Garrett