Change 955398 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/core@master] RevisionArchiveRecord: Also check for permission to view deleted pages

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

Sounds like a good idea. I can't think of a situation where the current behavior would be required. The check can still be bypassed using RevisionRecord::RAW if needed.

In T264765#9137884, @matmarex wrote:

Should RevisionArchiveRecord::userCan() also check the page deletion permissions?

We should still consider whether RevisionArchiveRecord::userCan() should check that the user can view deleted pages, in addition to the currently existing checks that the user can view revision-deleted revisions.

Arguments for:

• As evidenced by this bug, forgetting it is an easy mistake to make
• RevisionArchiveRecord extends RevisionRecord, so your code could receive a RevisionArchiveRecord without being aware of it and leak stuff (however, there are currently separate methods for getting a RevisionRecord and a RevisionArchiveRecord)

Arguments against:

• It would break CheckUser until something is done about T268147
• These methods are only called when showing revision-deletable parts of a revision, but not when showing e.g. its timestamp or the related title, so additional permission checks are necessary anyway

I'm not really convinced either way. Since it would require at least some refactoring in CheckUser, I think we should leave this unchanged for now.

After a bit more research, both of my arguments against turned out to be wrong:

• CheckUser is not affected by the change, as it's not using this method to check permissions
• The other metadata of revisions of deleted pages are allowed to be public, per T232389

In T345777#9151353, @matmarex wrote:

• CheckUser is not affected by the change, as it's not using this method to check permissions

I was even wronger: this change actually fixes the CheckUser issue in T268147. I just did it wrong the first time.

I looked at gerrit 955853 (commenting here instead of Gerrit because of better formatting support). It seems there will be three significant differences in the returned revision: the page ID (which might change in the process of restoring a page - RevisionArchiveRecord uses the historical page ID, RevisionStoreRecord the actual page ID of the restored revision), the parent ID (which can change due to history merge) and rev_deleted (when undeletion is performed with the unsuppress option). Maybe the user if used with a slightly broken DB (a not unusual consequence of actor migration) but that's not worth worrying about.

Internally, the new revision will be passed to WikiPage::updateRevisionOn() and DerivedPageDataUpdater, which is obviously more correct. Externally, it will be passed to the RevisionUndeleted and PageUndeleteComplete hooks, which per the documentation of those hooks is more correct. In actual usage, it will affect:

• WikibaseMediaInfo - used to store data about the new revision, seems like a clear improvement
• FlaggedRevs - uses it to update page ID in the flaggedrevs table, so I think the change fixes a bug here on history splits/merges
• EventBus - will send data to the mediawiki.page_change.v1 stream including revision.rev_parent_id and revision.is_*_visible which will now slightly change. This seems like a bugfix (for the visibility fields at least) but probably worth a heads-up.

Change 955398 merged by jenkins-bot:

[mediawiki/core@master] RevisionArchiveRecord: Also check for permission to view deleted pages

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

In T345777#9186037, @Tgr wrote:

• EventBus - will send data to the mediawiki.page_change.v1 stream including revision.rev_parent_id and revision.is_*_visible which will now slightly change.

If I read the code correctly (didn't test these specific scenarios):

• Before the patch rev_parent_id would be the parent revision at the time of deletion and after the patch it would be the parent revision after undeletion. These can differ if a subset of all deleted revisions were selectively undeleted, and the parent revision was left deleted.
• Before the patch, when a page was undeleted with the "unsuppress" option (which makes all properties of all undeleted revisions public), the is_*_visible fields reflected the state of the revision before undeletion (e.g. if the revision had its comment hidden before it was deleted, is_comment_visible would be false). After the patch, they are always true in that situation.

@Ottomata FYI.