Potential fix:

Adding @jsn.sherman, @Scardenasmolinar and @Novem_Linguae could you guys take a look and provide a review :)

Hi @Soda
Thanks for your work!

I have one suggestion to move the blocked check before the role check so that it accounts for anyone who is blocked even if they are autoPatrolled or patroller. How it is coded It will never run the is blocked check for those users. Is there a specific reason for this? If that's the case then we should add a test case for people who are blocked even if they are autopatroller and patroller.

if (
        $patrolPermissionStatus->isBlocked() ||
       $autopatrolledPermissionStatus->isBlocked()
) {
       $this->dieBlocked( $patrolPermissionStatus->getBlock() );
        return false;
}

 if ( $isPatroller && $isAutopatrolled ) {
         return true;
 }

nit: unnecessary extra space at the end of $pageId = $this->makeDraft( 'Test ' ); (after the word Test) in two places

In T366991#9886099, @Kgraessle wrote:

Hi @Soda
Thanks for your work!

I have one suggestion to move the blocked check before the role check so that it accounts for anyone who is blocked even if they are autoPatrolled or patroller. How it is coded It will never run the is blocked check for those users. Is there a specific reason for this? If that's the case then we should add a test case for people who are blocked even if they are autopatroller and patroller.

if (
        $patrolPermissionStatus->isBlocked() ||
       $autopatrolledPermissionStatus->isBlocked()
) {
       $this->dieBlocked( $patrolPermissionStatus->getBlock() );
        return false;
}

 if ( $isPatroller && $isAutopatrolled ) {
         return true;
 }

I think we should be fine since $this->getAuthority()->definitelyCan(...) also evaluates blocks and will never allow

if ( $isPatroller && $isAutopatrolled ) {
        return true;
}

to execute and return with true. The added integration test also tests for this case since it uses sysop which has both autopatrolled and patrolled rights on a default instance.

However, that being said, the code doesn't make sense from a semantic/readability POV and I've changed it.

In T366991#9886134, @Novem_Linguae wrote:

nit: unnecessary extra space at the end of $pageId = $this->makeDraft( 'Test ' ); (after the word Test) in two places

Fixed :)

In T366991#9886912, @Soda wrote:

In T366991#9886099, @Kgraessle wrote:

Hi @Soda
Thanks for your work!

I have one suggestion to move the blocked check before the role check so that it accounts for anyone who is blocked even if they are autoPatrolled or patroller. How it is coded It will never run the is blocked check for those users. Is there a specific reason for this? If that's the case then we should add a test case for people who are blocked even if they are autopatroller and patroller.

if (
        $patrolPermissionStatus->isBlocked() ||
       $autopatrolledPermissionStatus->isBlocked()
) {
       $this->dieBlocked( $patrolPermissionStatus->getBlock() );
        return false;
}

 if ( $isPatroller && $isAutopatrolled ) {
         return true;
 }

I think we should be fine since $this->getAuthority()->definitelyCan(...) also evaluates blocks and will never allow

if ( $isPatroller && $isAutopatrolled ) {
        return true;
}

to execute and return with true. The added integration test also tests for this case since it uses sysop which has both autopatrolled and patrolled rights on a default instance.

However, that being said, the code doesn't make sense from a semantic/readability POV and I've changed it.

In T366991#9886134, @Novem_Linguae wrote:

nit: unnecessary extra space at the end of $pageId = $this->makeDraft( 'Test ' ); (after the word Test) in two places

Fixed :)

0001-Prevent-blocked-users-from-being-able-to-review-unre.patch4 KBDownload

Hello Soda, thanks for your work, I took another look and it looks great!
I have one minor review comment:
The false is unreachable here after the dieBlocked function is called.

if ( $patrolPermissionStatus->isBlocked() ||$autopatrolledPermissionStatus->isBlocked() ) {
			$this->dieBlocked( $patrolPermissionStatus->getBlock() );
			return false;
}

Happy to help get this deployed to Wikimedia production, if we can come to consensus on the return false; issue above and upload a new patch.

In T366991#9894076, @Kgraessle wrote:

In T366991#9886912, @Soda wrote:

In T366991#9886099, @Kgraessle wrote:

Hi @Soda
Thanks for your work!

I have one suggestion to move the blocked check before the role check so that it accounts for anyone who is blocked even if they are autoPatrolled or patroller. How it is coded It will never run the is blocked check for those users. Is there a specific reason for this? If that's the case then we should add a test case for people who are blocked even if they are autopatroller and patroller.

if (
        $patrolPermissionStatus->isBlocked() ||
       $autopatrolledPermissionStatus->isBlocked()
) {
       $this->dieBlocked( $patrolPermissionStatus->getBlock() );
        return false;
}

 if ( $isPatroller && $isAutopatrolled ) {
         return true;
 }

I think we should be fine since $this->getAuthority()->definitelyCan(...) also evaluates blocks and will never allow

if ( $isPatroller && $isAutopatrolled ) {
        return true;
}

to execute and return with true. The added integration test also tests for this case since it uses sysop which has both autopatrolled and patrolled rights on a default instance.

However, that being said, the code doesn't make sense from a semantic/readability POV and I've changed it.

In T366991#9886134, @Novem_Linguae wrote:

nit: unnecessary extra space at the end of $pageId = $this->makeDraft( 'Test ' ); (after the word Test) in two places

Fixed :)

0001-Prevent-blocked-users-from-being-able-to-review-unre.patch4 KBDownload

Hello Soda, thanks for your work, I took another look and it looks great!
I have one minor review comment:
The false is unreachable here after the dieBlocked function is called.

if ( $patrolPermissionStatus->isBlocked() ||$autopatrolledPermissionStatus->isBlocked() ) {
			$this->dieBlocked( $patrolPermissionStatus->getBlock() );
			return false;
}

Sorry for the delay, I got stuck on some other things IRL. I've fixed the return false issue in this latest patch.

Hello Soda, thanks for your work, I tested this locally and it looks to be working great.
You have my +1. (and plus 2 if you need, but want to give others a chance to review as well).

Sounds like you tested it locally, so we can treat it as +2 to keep things moving :)

This looks good to me, too. I also tested and verified that an alert explaining the block now pops up as expected; CR +2!
@sbassett this is good to go.

In T366991#9926607, @Soda wrote:

In T366991#9894076, @Kgraessle wrote:

In T366991#9886912, @Soda wrote:

In T366991#9886099, @Kgraessle wrote:

Hi @Soda
Thanks for your work!

I have one suggestion to move the blocked check before the role check so that it accounts for anyone who is blocked even if they are autoPatrolled or patroller. How it is coded It will never run the is blocked check for those users. Is there a specific reason for this? If that's the case then we should add a test case for people who are blocked even if they are autopatroller and patroller.

if (
        $patrolPermissionStatus->isBlocked() ||
       $autopatrolledPermissionStatus->isBlocked()
) {
       $this->dieBlocked( $patrolPermissionStatus->getBlock() );
        return false;
}

 if ( $isPatroller && $isAutopatrolled ) {
         return true;
 }

I think we should be fine since $this->getAuthority()->definitelyCan(...) also evaluates blocks and will never allow

if ( $isPatroller && $isAutopatrolled ) {
        return true;
}

to execute and return with true. The added integration test also tests for this case since it uses sysop which has both autopatrolled and patrolled rights on a default instance.

However, that being said, the code doesn't make sense from a semantic/readability POV and I've changed it.

In T366991#9886134, @Novem_Linguae wrote:

nit: unnecessary extra space at the end of $pageId = $this->makeDraft( 'Test ' ); (after the word Test) in two places

Fixed :)

0001-Prevent-blocked-users-from-being-able-to-review-unre.patch4 KBDownload

Hello Soda, thanks for your work, I took another look and it looks great!
I have one minor review comment:
The false is unreachable here after the dieBlocked function is called.

if ( $patrolPermissionStatus->isBlocked() ||$autopatrolledPermissionStatus->isBlocked() ) {
			$this->dieBlocked( $patrolPermissionStatus->getBlock() );
			return false;
}

Sorry for the delay, I got stuck on some other things IRL. I've fixed the return false issue in this latest patch.

0001-Prevent-blocked-users-from-being-able-to-review-unre.patch4 KBDownload

deployed

I uploaded this patch to gerrit for the supplemental release announcement and it looks like there are some test failures. @Soda please take a look when you get a chance: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/PageTriage/+/1076839.

In T366991#10189732, @Mstyles wrote:

I uploaded this patch to gerrit for the supplemental release announcement and it looks like there are some test failures. @Soda please take a look when you get a chance: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/PageTriage/+/1076839.

Just FYI, I added a phan suppress comment in PS2 on that change set and it tests fine now. I don't really know what phan's problem is with this. The code tells me that getBlock returns a block and that dieBlocked takes one. I don't understand where phan is finding the type ?\MediaWiki\Block\Block|?\MediaWiki\DAO\WikiAwareEntity...

Change #1077081 had a related patch set uploaded (by Mstyles; author: Sohom Datta):

[mediawiki/extensions/PageTriage@REL1_42] Prevent blocked users from being able to review/unreview articles

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

@sbassett thank you!

Hey folks. It appears this patch no longer applies cleanly due to a conflict with https://gerrit.wikimedia.org/r/c/mediawiki/extensions/PageTriage/+/1081612 which merged a few hours ago. We'll need an updated patch from you all.

I went ahead and resolved the conflicts so our presync job doesn't fail tonight.

Updated patch:

In T366991#10248602, @dduvall wrote:

Hey folks. It appears this patch no longer applies cleanly due to a conflict with https://gerrit.wikimedia.org/r/c/mediawiki/extensions/PageTriage/+/1081612 which merged a few hours ago. We'll need an updated patch from you all.

Oh yikes, not sure what happened here. This should have been released with the most recent supplemental security release (T368628) but it looks like the backport to master was never merged? That's not good. We should get the new patch up to gerrit asap as this should no longer be an embargoed security patch.

In T366991#10248777, @dduvall wrote:

Updated patch:

0001-Prevent-blocked-users-from-being-able-to-review-unre.patch4 KBDownload

This doesn't seem to apply to current master for me in ext:PageTriage:

extensions/PageTriage [master] (⩾﹏⩽)> git apply --check ~/Downloads/01-T366991-rev2.patch 
error: patch failed: includes/Api/ApiPageTriageAction.php:14
error: includes/Api/ApiPageTriageAction.php: patch does not apply
error: patch failed: tests/phpunit/integration/ApiPageTriageActionTest.php:3
error: tests/phpunit/integration/ApiPageTriageActionTest.php: patch does not apply

Looks like there were a few use-related conflicts? I've fixed those and sent a new PS up to https://gerrit.wikimedia.org/r/1076839.

Change #1076839 merged by jenkins-bot:

[mediawiki/extensions/PageTriage@master] Prevent blocked users from being able to review/unreview articles

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

Change #1082243 had a related patch set uploaded (by SBassett; author: Sohom Datta):

[mediawiki/extensions/PageTriage@wmf/1.43.0-wmf.28] Prevent blocked users from being able to review/unreview articles

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

Change #1077081 merged by jenkins-bot:

[mediawiki/extensions/PageTriage@REL1_42] Prevent blocked users from being able to review/unreview articles

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

Change #1082243 merged by jenkins-bot:

[mediawiki/extensions/PageTriage@wmf/1.43.0-wmf.28] Prevent blocked users from being able to review/unreview articles

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

Mentioned in SAL (#wikimedia-operations) [2024-10-22T18:01:32Z] <dancy@deploy2002> Started scap sync-world: Backport for [[gerrit:1082243|Prevent blocked users from being able to review/unreview articles (T366991)]]

Mentioned in SAL (#wikimedia-operations) [2024-10-22T18:04:04Z] <dancy@deploy2002> dancy, sbassett: Backport for [[gerrit:1082243|Prevent blocked users from being able to review/unreview articles (T366991)]] synced to the testservers (https://wikitech.wikimedia.org/wiki/Mwdebug)

Mentioned in SAL (#wikimedia-operations) [2024-10-22T18:08:59Z] <dancy@deploy2002> Finished scap sync-world: Backport for [[gerrit:1082243|Prevent blocked users from being able to review/unreview articles (T366991)]] (duration: 07m 26s)