Page MenuHomePhabricator

UploadBase assumes that 'edit' and 'upload' rights are not per page restrictions
Closed, ResolvedPublic


<vvv> Reedy: probably someone removed check for page creation permission from
upload module
<vvv> Was it rewritten in 1.17?
<Reedy> I think so
<vvv> Reedy: it looks like UploadBase assumes that 'edit', 'upload', etc are
not per-page restrictions
<vvv> Reedy: oh, and it also allows to upload images even when they are

Seems to have caused bug 27470

Version: unspecified
Severity: major



Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 11:28 PM
bzimport set Reference to bz28166.
bzimport added a subscriber: Unknown Object (MLST).

Just to clarify: the UploadBase should use proper Title::getUserPermissionsErrors call for checking create, edit and upload rights instead of checking them itself.

I suppose it's Title::newFromWhatever( $idontcare )->userCan( 'action' )

(In reply to comment #2)

I suppose it's Title::newFromWhatever( $idontcare )->userCan( 'action' )

No, do NOT use userCan. If you use it, you cannot output the meaningful error message, and you certainly should.

  • Bug 28169 has been marked as a duplicate of this bug. ***

Well, getUserPermissionsErrors is private for a start ;)

(In reply to comment #5)

Well, getUserPermissionsErrors is private for a start ;)

Is it?

public function getUserPermissionsErrors( $action, $user, $doExpensiveQueries = true, $ignoreErrors = array() )

Looks like I'm being blind. Never mind. :)

Bryan.TongMinh wrote:

public function verifyPermissions( $user ) {
403 $permErrors = $nt->getUserPermissionsErrors( 'edit', $user );
404 $permErrorsUpload = $nt->getUserPermissionsErrors( 'upload', $user );
405 if ( $nt->exists() ) {
406 $permErrorsCreate = $nt->getUserPermissionsErrors( 'createpage', $user );
407 } else {
408 $permErrorsCreate = array();
409 }

(In reply to comment #8)

405 if ( $nt->exists() ) {
406 $permErrorsCreate = $nt->getUserPermissionsErrors( 'createpage', $user

I think it should be !$nt->exists().

I'd concur, also based on CR on r65898

I've committed it in r84573, but it hasn't fix the issue as of yet

Fixed with r84573 and r84575

However, seems I broke some error handling in r83979, will investigate