The code bit used to determine the bitmask for rev_delete when querying contributions is duplicated in three different API modules: ApiQueryAllRevisions, ApiQueryRevisions, and ApiQueryUserContribs.
Hence, it should be factored out of the API and moved somewhere else, probably somewhere in the Revision namespace as a static method.
| Subject | Repo | Branch | Lines +/- | |
|---|---|---|---|---|
| Make PermissionManager not depend on RevisionLookup. | mediawiki/core | master | +36 -92 |
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Open | None | T291115 Check user permissions when computing page_recent_contributors | |||
| Open | None | T233222 Factor rev_deleted logic out of the API modules |
This also affects the new core REST API project. Since this logic doesn't currently have a common place, it keeps being duplicated in new REST API handlers as well.
Since this logic is pretty much an inverse of RevisionRecord::userCanBitfield, it might belong in RevisionRecord as well, like 'getUserCanBitfield'. However, that's not a very DI-friendly way, and userCanBitfield method already has existing TODO comments about avoiding the global state.
Other natural place to put it is RevisionStore. However the method requires a PermissionManager, and injecting a PM into RevisionStore will create a cyclic dependency.
Putting both methods into PermissionManager doesn't quite solve the problem either. getUserCanBitfield is intended to be used for querying revisions, so it will need to be used inside the RevisionStore, which would create a cyclic dependency yet again.
this logic should be folded into RevisionStore::getQueryInfo() or an associated method. RevisionStore provides encapsulation for the schema of the revision table, so that's where this logic should live.
Other natural place to put it is RevisionStore. However the method requires a PermissionManager, and injecting a PM into RevisionStore will create a cyclic dependency.
This is correct, and it's already the case, via RevisionRecord::userCanBitfield(). There are two things that can and shoule be done to address this:
Change 549123 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/core@master] Make PermissionManager not depend on RevisionLookup.
Now that we have Authority, we can implement a static method on RevisionRecord and use it.
Something like
/**
* Get a bitmask to apply to `rev_deleted` column appropriate for permissions
* given $performer has.
*
* @since 1.36
* @param Authority $performer user on who's behalf the revision is viewed
* @return int
*/
public static function getRevDeletedBitmask( Authority $performer ): int {
if ( !$performer->isAllowed( 'deletedhistory' ) ) {
$bitmask = RevisionRecord::DELETED_USER;
} elseif ( !$performer->isAllowedAny( 'suppressrevision', 'viewsuppressed' ) ) {
$bitmask = RevisionRecord::DELETED_USER | RevisionRecord::DELETED_RESTRICTED;
} else {
$bitmask = 0;
}
return $bitmask;
}Change 549123 abandoned by Ppchelko:
[mediawiki/core@master] Make PermissionManager not depend on RevisionLookup.
Reason:
I just realized that everything from this task also applies to the logic behind recentchanges.rc_deleted (relevant code, duplicated e.g. here). In fact, I think the logic would be pretty much the same.
Removing inactive assignee (Platform Engineering: Please unassign tasks of previous team members.)