Page MenuHomePhabricator

Remove fallback to `$wgUser` in LocalRepo::findFiles(), FileRepo::findFiles(), and FileRepo::findFileFromKey()
Closed, ResolvedPublic

Description

Remove fallback to $wgUser in LocalRepo::findFiles(), FileRepo::findFiles(), and FileRepo::findFileFromKey().

These were added in 5e703cdf669f85e2924dee76c5a8831633259ce0 as a hacky backport patch to fix T263014 and properly deprecate a missing hard-deprecation in File::canUser. But it should be fixed properly in 1.36.

Event Timeline

DannyS712 triaged this task as Medium priority.Sep 16 2020, 3:13 PM
DannyS712 removed a project: Commons.
cscott renamed this task from Remove fallback to `$wgUser` in LocalRepo::findFiles() to Remove fallback to `$wgUser` in LocalRepo::findFiles(), FileRepo::findFiles(), and FileRepo::findFileFromKey().Sep 16 2020, 4:30 PM
cscott updated the task description. (Show Details)

Note: callers refers to deployed callers only. Didn't check everywhere

Fallbacks exist in:

FileRepo::findFile if $options['private'] is not empty and not a user object
FileRepo::findFileFromKey if $options['private'] is not empty and not a user object
LocalRepo::findFiles if $items is not an array with $items['user'] being a user object

Tackling findFileFromKey first, since that seems easiest.

findFileFromKey

Callers:
FlaggedRevs - $options['private'] is not set, fallback not used

Core:
RepoGroup::findFileFromKey - $options is whatever the method was called with; method has the same name, so callers of it are included in this list already
Parser::fetchFileNoRegister - $options is whatever the method was called with.

Callers of Parser::fetchFileNoRegister:
Protected method, only callers are in the parser class:
Parser::fetchFileAndTitle - $options is whatever the method was called with
Parser::renderImageGallery - $options is an empty array that can be modified by extensions using the BeforeParserFetchFileAndTitle hook. The only deployed extension to use the hook, FlaggedRevs, does not set the private key. We should document that if it is set it must be a user object, but there are no other changes needed.

Callers of Parser::fetchFileAndTitle
WikidataPageBanner: no $options are set, fallback not used

Core:
TraditionalImageGallery::toHTML - $options same as Parser::renderImageGallery - empty and then modified by BeforeParserFetchFileAndTitle hook.
Parser::handleInternalLinks2 - same as above, BeforeParserFetchFileAndTitle
Parser::makeImage - same as above, BeforeParserFetchFileAndTitle

Solution:
no deployed code is using this fallback. To remove it:
Document that if the hook sets the private option, it must be set to a user object
Through an exception in findFileFromKey if its set and not a user

Change 628563 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Remove fallback to $wgUser in FileRepo::findFileFromKey

https://gerrit.wikimedia.org/r/628563

findFiles

Almost all callers pass an array of titles, rather than an array of arrays with private and title set. Outside of core, deployed extensions that would need to be updated include MachineVision, TimedMediaHandler, GlobalUsage, and parsoid. Rather than trying to rush through updating all of them, I suggest using RequestContext::getMain for now, and at some point in the future change the method signature so that you don't need to set a user for each file that should be retrieved.

Change 628564 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Remove fallback to $wgUser in LocalRepo::findFiles

https://gerrit.wikimedia.org/r/628564

findFile

Fallback if options['private'] is not empty and not a user object

Callers in core:
BadFileLookup::isBadFile - no options
EditPage::showIntro - no options
wfFindFile - whatever options are passed to it (see below)
Linker::makeThumbLink2 - no options
Linker::makeBrokenImageLinkObj - no options
Linker::makeMediaLinkObj - private is not set
MediaFileHandler::getFile - private is properly set to a user
Title::isAlwaysKnown - no options
InfoAction::pageInfo - no options
ApiImageRotate::execute - no options
ApiMove::execute (x2) - no options
FileRepo::findFiles - based on caller - as noted above, this usually does not have a user set. Suggesting falling back to RequestContext::getMain for now
RepoGroup::findFile (x2) - based on caller, callers already included in this list (same method name)
TraditionalImageGallery::toHTML - no options
ImagePage::loadFile - no options
WikiFilePage::loadFile - no options
CoreParserFunctions::filepath - no options
Parser::fetchFileNoRegister - based on caller, see above
RevisionSearchResultTrait::initFromTitle - no options
SearchNearMatcher::getNearMatchInternal - no options
FullSearchResultWidget::generateFileHtml - no options
SpecialFileDuplicateSearch::execute - no options
SpecialMovepage::doSubmit (x2) - no options
SpecialRedirect::dispatchFile - no options
SpecialWantedfiles::existenceCheck - no options
ImageListPager::formatValue - private is not set
UploadBase::checkOverwrite - private is not set
DeleteBatch::execute - private is not set
DumpUploads::outputItem - no options
ApiUploadTestCase::deleteFileByTitle - private is not set
UploadFromUrlTest::deleteFile - private is not set
thumb.php - no options

Callers in extensions:
(rather than listing everything, because there are a lot of callers, not listing those with hardcoded options that don't set private or don't include any options)

FlaggedRevs - all good
MachineVision - all good
PageImages - all good
TimedMediaHandler - all good
MobileFrontend - mostly good
ApiMobileView::findFile - whatever options are passed to it, since its the same method name callers already included in this list
Wikibase - all good
CirrusSearch - all good
GlobalUsage - all good
ProofreadPage - all good
vendor - not sure if its referring to the same method, but either way no second parameter is passed
3D - all good
AbuseFilter - RCVariableGenerator::addUploadVars - sets private to true, NEEDS FIXING
CommonsMetadata - all good
FileImporter - all good
GeoData - all good
ImageMap - all good
MediaModeration - all good
ParserFunctions - all good
Score - all good
Scribunto - all good
Translate - all good
UploadWizard - all good
VipsScaler - all good
WikibaseMediaInfo - all good
WikidataPageBanner - all good
WikimediaIncubator - all good
parsoid / deploy - not sure if its the same method, but either way no second parameter set
Nostalgia - all good
Timeless - all good

Deployed callers of wfFindFile:
Graph - all good, no options
Translate - not in MW 1.34+, anyway all good, no options

Summary:
AbuseFilter - RCVariableGenerator::addUploadVars - sets private to true, NEEDS FIXING
FileRepo::findFiles - based on caller - as noted above, this usually does not have a user set. Suggesting falling back to RequestContext::getMain for now

Change 628565 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/AbuseFilter@master] Pass a user to RCVariableGenerator::getVars

https://gerrit.wikimedia.org/r/628565

Change 628606 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Remove fallback to $wgUser in FileRepo::findFile

https://gerrit.wikimedia.org/r/628606

DannyS712 added a project: AbuseFilter.
DannyS712 added a subscriber: Daimona.

@Daimona can you please take a look at https://gerrit.wikimedia.org/r/628565 ? Injecting a user into RCVariableGenerator::getVars

Change 628565 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Inject a user into RCVariableGenerator

https://gerrit.wikimedia.org/r/628565

Change 628606 merged by jenkins-bot:
[mediawiki/core@master] Remove fallback to $wgUser in FileRepo::findFile

https://gerrit.wikimedia.org/r/628606

Change 628564 merged by jenkins-bot:
[mediawiki/core@master] Remove fallback to $wgUser in LocalRepo::findFiles

https://gerrit.wikimedia.org/r/628564

Change 628563 merged by jenkins-bot:
[mediawiki/core@master] Remove fallback to $wgUser in FileRepo::findFileFromKey

https://gerrit.wikimedia.org/r/628563

DannyS712 claimed this task.