Page MenuHomePhabricator

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

Description

<vvv> Reedy: probably someone removed check for page creation permission from
upload module
<vvv> Was it rewritten in 1.17?
<Reedy> I think so
<snip>
<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
create-protected

Seems to have caused bug 27470


Version: unspecified
Severity: major

Details

Reference
bz28166

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).
Reedy created this task.Mar 21 2011, 8:58 PM
vvv added a comment.Mar 21 2011, 9:14 PM

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

Reedy added a comment.Mar 21 2011, 9:27 PM

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

vvv added a comment.Mar 21 2011, 9:30 PM

(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.

Reedy added a comment.Mar 21 2011, 9:31 PM
  • Bug 28169 has been marked as a duplicate of this bug. ***
Reedy added a comment.Mar 21 2011, 9:31 PM

Well, getUserPermissionsErrors is private for a start ;)

vvv added a comment.Mar 21 2011, 9:48 PM

(In reply to comment #5)

Well, getUserPermissionsErrors is private for a start ;)

Is it?

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

Reedy added a comment.Mar 21 2011, 9:50 PM

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 }

vvv added a comment.Mar 22 2011, 8:58 AM

(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

Gilles moved this task from Untriaged to Done on the Multimedia board.Dec 4 2014, 10:41 AM