Page MenuHomePhabricator
Create Task
Maniphest T360621

Test and make improvements to the fixGlobalBlockWhitelist.php script
Closed, ResolvedPublic2 Estimated Story Points
Actions

Description

The fixGlobalBlockWhitelist.php maintenance script can be used to fix rows in the global_block_whitelist table by fixing the gbw_id value as well as deleting any orphaned rows (where there is no associated gb_id in the globalblocks table).

This script needs several improvements:

  1. The code needs to use the SelectQueryBuilder (T312329)
  2. Selecting rows from global_block_whitelist is not batched
  3. The code will fail if multiple broken whitelist entries exist for the same target when a global block for that target exists.
  4. There are no tests for the code and there should be if it is being automatically run weekly on production
Acceptance criteria
  • The fixGlobalBlockWhitelist.php is well tested and uses the SelectQueryBuilder over calls to ::select.
  • The maintenance script properly handles batching and duplicate broken whitelist rows

Details

SubjectRepoBranchLines +/-
Update gbw_expiry and gbw_target_central_id on whitelist fixmediawiki/extensions/GlobalBlockingmaster+18 -4
Handle duplicate broken whitelist entries in fixGlobalBlockWhitelistmediawiki/extensions/GlobalBlockingmaster+195 -5
Use SelectQueryBuilder and improve batching in fixGlobalBlockWhitelistmediawiki/extensions/GlobalBlockingmaster+138 -80
FixGlobalBlockWhitelistTest: Add assertions to check DB was updatedmediawiki/extensions/GlobalBlockingmaster+24 -0
Fully test fixGlobalBlockWhitelist.phpmediawiki/extensions/GlobalBlockingmaster+148 -0
Customize query in gerrit

Related Objects
Search...

View Standalone Graph
This task is connected to more than 200 other tasks. Only direct parents and subtasks are shown here. Use View Standalone Graph to show more of the graph.
StatusSubtypeAssignedTask
· · ·
ResolvedTchandersT360516 Periodically remove orphaned global_block_whitelist entries
ResolvedUmherirrenderT312329 Migrate usage of Database::select to SelectQueryBuilder in GlobalBlocking
ResolvedDreamy_JazzT360621 Test and make improvements to the fixGlobalBlockWhitelist.php script
· · ·

Event Timeline

Dreamy_Jazz created this task.Mar 21 2024, 12:10 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 21 2024, 12:10 PM
Dreamy_Jazz added a parent task: T360516: Periodically remove orphaned global_block_whitelist entries.Mar 21 2024, 12:10 PM
Dreamy_Jazz claimed this task.Mar 21 2024, 6:18 PM
Dreamy_Jazz moved this task from Priority Backlog to In Progress on the Trust and Safety Product Sprint (Sprint Gangan (11th - 22nd March)) board.
Dreamy_Jazz set the point value for this task to 2.
gerritbot added a comment.Mar 22 2024, 12:13 AM

Change #1013426 had a related patch set uploaded (by Dreamy Jazz; author: Dreamy Jazz):

[mediawiki/extensions/GlobalBlocking@master] Fully test fixGlobalBlockWhitelist.php

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

gerritbot added a project: Patch-For-Review.Mar 22 2024, 12:13 AM

Change #1013427 had a related patch set uploaded (by Dreamy Jazz; author: Dreamy Jazz):

[mediawiki/extensions/GlobalBlocking@master] Use SelectQueryBuilder and improve batching in fixGlobalBlockWhitelist

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

gerritbot added a comment.Mar 22 2024, 9:24 AM

Change #1013502 had a related patch set uploaded (by Tchanders; author: Tchanders):

[mediawiki/extensions/GlobalBlocking@master] FixGlobalBlockWhitelistTest: Add assertions to check DB was updated

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

gerritbot added a comment.Mar 22 2024, 9:28 AM

Change #1013426 merged by jenkins-bot:

[mediawiki/extensions/GlobalBlocking@master] Fully test fixGlobalBlockWhitelist.php

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

gerritbot added a comment.Mar 22 2024, 9:43 AM

Change #1013502 merged by jenkins-bot:

[mediawiki/extensions/GlobalBlocking@master] FixGlobalBlockWhitelistTest: Add assertions to check DB was updated

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

ReleaseTaggerBot added a project: MW-1.42-notes (1.42.0-wmf.24; 2024-03-26).Mar 22 2024, 10:00 AM
Dreamy_Jazz updated the task description. (Show Details)Mar 25 2024, 4:13 PM
Dreamy_Jazz updated the task description. (Show Details)
gerritbot added a comment.Mar 25 2024, 5:06 PM

Change #1014045 had a related patch set uploaded (by Dreamy Jazz; author: Dreamy Jazz):

[mediawiki/extensions/GlobalBlocking@master] Handle duplicate broken whitelist entries in fixGlobalBlockWhitelist

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

Dreamy_Jazz moved this task from In Progress to Needs review on the Trust and Safety Product Sprint (Sprint Gangan (11th - 22nd March)) board.Mar 25 2024, 5:21 PM
Dreamy_Jazz added a parent task: T356924: Deploy global account blocks to WMF wikis.Mar 27 2024, 9:41 AM
Dreamy_Jazz removed a parent task: T356924: Deploy global account blocks to WMF wikis.
Dreamy_Jazz added a parent task: T312329: Migrate usage of Database::select to SelectQueryBuilder in GlobalBlocking.Mar 27 2024, 9:44 AM
Dreamy_Jazz edited projects, added Trust and Safety Product Sprint (Sprint Tabla (1st - 14th April)); removed Trust and Safety Product Sprint (Sprint Gangan (11th - 22nd March)).Mon, Apr 1, 4:24 PM
Dreamy_Jazz moved this task from Priority Backlog to Needs Review on the Trust and Safety Product Sprint (Sprint Tabla (1st - 14th April)) board.
gerritbot added a comment.Tue, Apr 2, 5:34 PM

Change #1013427 merged by jenkins-bot:

[mediawiki/extensions/GlobalBlocking@master] Use SelectQueryBuilder and improve batching in fixGlobalBlockWhitelist

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

gerritbot added a comment.Tue, Apr 2, 5:35 PM

Change #1014045 merged by jenkins-bot:

[mediawiki/extensions/GlobalBlocking@master] Handle duplicate broken whitelist entries in fixGlobalBlockWhitelist

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

Dreamy_Jazz moved this task from Needs Review to Needs QA on the Trust and Safety Product Sprint (Sprint Tabla (1st - 14th April)) board.Tue, Apr 2, 5:53 PM
gerritbot added a comment.Tue, Apr 2, 6:19 PM

Change #1016416 had a related patch set uploaded (by Dreamy Jazz; author: Dreamy Jazz):

[mediawiki/extensions/GlobalBlocking@master] Update gbw_expiry and gbw_target_central_id on whitelist fix

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

Dreamy_Jazz moved this task from Needs QA to Needs Review on the Trust and Safety Product Sprint (Sprint Tabla (1st - 14th April)) board.Tue, Apr 2, 6:19 PM
ReleaseTaggerBot edited projects, added MW-1.42-notes (1.42.0-wmf.26; 2024-04-09); removed MW-1.42-notes (1.42.0-wmf.24; 2024-03-26).Wed, Apr 3, 2:02 PM
gerritbot added a comment.Mon, Apr 8, 1:27 PM

Change #1016416 merged by jenkins-bot:

[mediawiki/extensions/GlobalBlocking@master] Update gbw_expiry and gbw_target_central_id on whitelist fix

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

Tchanders moved this task from Needs Review to Needs QA on the Trust and Safety Product Sprint (Sprint Tabla (1st - 14th April)) board.Mon, Apr 8, 4:18 PM
Dreamy_Jazz removed a project: Patch-For-Review.Mon, Apr 8, 5:17 PM
Dreamy_Jazz added a comment.EditedMon, Apr 8, 5:38 PM

Suggested QA steps for a local wiki:

  1. Perform a infinite global block on an IP address
  2. Perform a infinite global block on an account
  3. Locally disable the global blocks performed in steps 1 and 2
  4. Remove the global blocks performed in steps 1 and 2
  5. Re-block the same IP address as you blocked in step 1 with a global block that is not indefinite
  6. Run the fixGlobalBlockWhitelist.php script with the --delete option specified
  7. Open Special:GlobalBlockList and verify that the global block on the IP address is marked as disabled
  8. Open the database for the local wiki
  9. Run SELECT gbw_expiry from global_block_whitelist where gbw_address = <IP>; replacing <IP> with the IP address you blocked in step 5. Verify that returned value is not indefinite and also matches the expiry you specified when you performed the global block in step 5
  10. Run SELECT COUNT(*) from global_block_whitelist where gbw_address = <username>; replacing <username> with the username of the account you blocked in step 2 and verify that the result is 0.
dom_walden moved this task from Needs QA to Done on the Trust and Safety Product Sprint (Sprint Tabla (1st - 14th April)) board.Mon, Apr 15, 1:24 PM
dom_walden subscribed.

A few observations:

  • If there are duplicate broken entries in global_block_whitelist for the same user/IP, one will be fixed and the rest will be deleted (even if you do not pass --delete flag). For example:

globalblocks:

gb_idgb_address
3Foobar

global_block_whitelist:

gbw_idgbw_address
1Foobar
2Foobar

After running script (even without --delete flag):
global_block_whitelist:

gbw_idgbw_address
3Foobar
  • However, if a non-broken entry also exists alongside the broken entries for the same globally blocked user, nothing will happen (i.e. duplicates won't be deleted, even with --delete flag). For example:

globalblocks:

gb_idgb_address
3Foobar

global_block_whitelist:

gbw_idgbw_address
2Foobar
3Foobar

After running script (even with --delete flag):
global_block_whitelist:

gbw_idgbw_address
2Foobar
3Foobar
  • Rows in global_block_whitelist with no associated globally blocked user will be deleted only if you pass --delete.
  • If a broken entry exists in global_block_whitelist for an expired global block, it will not be deleted but it will be fixed.
  • When an entry is fixed, gbw_by and gbw_by_text are not updated and will keep their current value. Perhaps this is OK as the script does not know what to update them to.
Dreamy_Jazz added a comment.Tue, Apr 16, 5:09 PM
In T360621#9713336, @dom_walden wrote:
  • However, if a non-broken entry also exists alongside the broken entries for the same globally blocked user, nothing will happen (i.e. duplicates won't be deleted, even with --delete flag). For example:

globalblocks:

gb_idgb_address
3Foobar

global_block_whitelist:

gbw_idgbw_address
2Foobar
3Foobar

After running script (even with --delete flag):
global_block_whitelist:

gbw_idgbw_address
2Foobar
3Foobar

This should probably be okay, but we could file a task for this if desired.

  • When an entry is fixed, gbw_by and gbw_by_text are not updated and will keep their current value. Perhaps this is OK as the script does not know what to update them to.

Yeah, it is probably okay because there is no definitive way to determine who should actually be marked as the performer. However, keeping the current value is probably the better than blanking it.

Dreamy_Jazz closed this task as Resolved.Tue, Apr 23, 10:54 AM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL