Change 592287 had a related patch set uploaded (by Urbanecm; owner: Urbanecm):
[mediawiki/core@master] [WIP] Introduce BlockValidator service for validating block targets

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

Change 592287 merged by jenkins-bot:
[mediawiki/core@master] Introduce BlockPermissionChecker service for validating block targets

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

It doesn't look like Platform Engineering is currently actively involved in code review for this task. If code review is needed, please feel free to re-tag with Platform Engineering.

Change 628448 had a related patch set uploaded (by Urbanecm; owner: Urbanecm):
[mediawiki/core@master] [tests] SpecialBlockTest: Make testProcessFormRestrictionsChange not to save an empty block

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

Change 628448 merged by jenkins-bot:
[mediawiki/core@master] [tests] SpecialBlockTest: Make testProcessFormRestrictionsChange not to save an empty block

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

Change 628465 had a related patch set uploaded (by Urbanecm; owner: Urbanecm):
[mediawiki/core@master] [tests] SpecialBlockTest: Make testProcessFormUserTalkEditFlag not to save an empty block

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

Change 628465 merged by jenkins-bot:
[mediawiki/core@master] [tests] SpecialBlockTest: Do not save an empty block in testProcessFormUserTalkEditFlag

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

Change 628377 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Add tests for ApiBlock return values when blocking by userid

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

Change 628377 merged by jenkins-bot:
[mediawiki/core@master] Add tests for ApiBlock return values when blocking by userid

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

Change 588104 merged by jenkins-bot:
[mediawiki/core@master] Introduce backend class for blocking users

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

Bug found:

On mediawiki.org, via API attempt to block a user (in this case DannyS712) with:
allowusertalk enabled (1)
partial enabled (1)
no page or namespace restrictions set

Result: ipb-empty-block
Beta cluster: block is processed as sitewide block

Additionally, a sub case of this would be blocking with a partial block, no restrictions, and not allowing talk page access.
Result on mediawiki.org: ipb-prevent-user-talk-edit
Beta cluster: block is processed as sitewide block with user talk disabled

Change 629381 had a related patch set uploaded (by Tchanders; owner: Tchanders):
[mediawiki/core@master] Pass correct parameters to BlockUser from ApiBlock and SpecialBlock

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

Thanks @DannyS712 - should be fixed by the patch above.

Change 629381 merged by jenkins-bot:
[mediawiki/core@master] Pass correct parameters to BlockUser from ApiBlock and SpecialBlock

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

In T189073#6487413, @Tchanders wrote:

Thanks @DannyS712 - should be fixed by the patch above.

Indeed, the blocks aren't processed and error out properly

Change 629418 had a related patch set uploaded (by Urbanecm; owner: Urbanecm):
[mediawiki/core@master] [tests] Add ApiBlockTest::testEmptyBlock

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

@Urbanecm @Tchanders When attempting to block an IP for the second time:

[81f0537a6316d82474210e8b] /wiki/Special:Block Error from line 513 of /vagrant/mediawiki/includes/block/BlockUser.php: Call to a member function getUserPage() on string

Backtrace:

#0 /vagrant/mediawiki/includes/block/BlockUser.php(467): MediaWiki\Block\BlockUser->placeBlockInternal(boolean)
#1 /vagrant/mediawiki/includes/block/BlockUser.php(401): MediaWiki\Block\BlockUser->placeBlockUnsafe(boolean)
#2 /vagrant/mediawiki/includes/specials/SpecialBlock.php(873): MediaWiki\Block\BlockUser->placeBlock(boolean)
#3 /vagrant/mediawiki/includes/specials/SpecialBlock.php(982): SpecialBlock::processForm(array, RequestContext)
#4 /vagrant/mediawiki/includes/htmlform/HTMLForm.php(707): SpecialBlock->onSubmit(array, OOUIHTMLForm)
#5 /vagrant/mediawiki/includes/htmlform/HTMLForm.php(597): HTMLForm->trySubmit()
#6 /vagrant/mediawiki/includes/htmlform/HTMLForm.php(613): HTMLForm->tryAuthorizedSubmit()
#7 /vagrant/mediawiki/includes/specialpage/FormSpecialPage.php(187): HTMLForm->show()
#8 /vagrant/mediawiki/includes/specialpage/SpecialPage.php(600): FormSpecialPage->execute(NULL)
#9 /vagrant/mediawiki/includes/specialpage/SpecialPageFactory.php(709): SpecialPage->run(NULL)
#10 /vagrant/mediawiki/includes/MediaWiki.php(307): MediaWiki\SpecialPage\SpecialPageFactory->executePath(Title, RequestContext)
#11 /vagrant/mediawiki/includes/MediaWiki.php(940): MediaWiki->performRequest()
#12 /vagrant/mediawiki/includes/MediaWiki.php(543): MediaWiki->main()
#13 /vagrant/mediawiki/index.php(53): MediaWiki->run()
#14 /vagrant/mediawiki/index.php(46): wfIndexMain()
#15 /var/www/w/index.php(5): require(string)
#16 {main}

Similarly, when trying to reblock an IP with the same block parameters, except for line 505 of BlockUser.php.

In T189073#6494158, @dom_walden wrote:

@Urbanecm @Tchanders When attempting to block an IP for the second time:

[81f0537a6316d82474210e8b] /wiki/Special:Block Error from line 513 of /vagrant/mediawiki/includes/block/BlockUser.php: Call to a member function getUserPage() on string

Prior code used $block->getTarget() as the parameter for ipb_already_blocked, new code used $this->target->getUserPage() but the target could be a string

Change 630227 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] BlockUser: Avoid getUserPage() on string

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

Thanks @dom_walden - this is fixed by @DannyS712's patch.

Change 630227 merged by jenkins-bot:
[mediawiki/core@master] BlockUser: Avoid getUserPage() on string

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

@Urbanecm trying to reblock someone with the same parameters as the current block results in a new block log entry. I think this used to result in an error instead? See https://www.mediawiki.org/w/index.php?title=Special:Log&type=block&user=&page=User:DannyS712+test

In T189073#6496974, @DannyS712 wrote:

@Urbanecm trying to reblock someone with the same parameters as the current block results in a new block log entry. I think this used to result in an error instead? See https://www.mediawiki.org/w/index.php?title=Special:Log&type=block&user=&page=User:DannyS712+test

[note] AFAICS, BlockUser will land with wmf.11, hence, this is not caused by BlockUser class. Perhaps fill a different task?

In T189073#6496974, @DannyS712 wrote:

@Urbanecm trying to reblock someone with the same parameters as the current block results in a new block log entry. I think this used to result in an error instead? See https://www.mediawiki.org/w/index.php?title=Special:Log&type=block&user=&page=User:DannyS712+test

@DannyS712 I can only reproduce this if I change the reason for the block, as you appear to have done here. I cannot reproduce if I keep the reason the same. Either via Special:Block or the API. Please confirm.

This behaviour seems reasonable/desirable to me. An admin may need to change the reason on a block without changing any of the other parameters.

@Urbanecm @Tchanders If I submit a block via the API with either pagerestrictions or namespacerestrictions (or both) set but I don't explicitly set partial, it is treated as a partial block.

Previously, it would be treated as a sitewide block (the page/namespace restrictions would be ignored).

E.g., this request:

{
	"action": "block",
	"user": <user>,
	...
	"pagerestrictions": "Foobar",
}

creates a partial block against the page "Foobar".

Similarly, on Special:Block, if I have pages or namespaces selected but I have "sitewide" toggled, e.g.:

It will still create a partial block:

If I want to edit an existing partial block to make it sitewide, I first have to remove all the page and namespace restrictions before selecting "sitewide". If I don't remove any, the new block is treated as equal to the existing block (and won't submit).

Arguably the API behaviour is reasonable, but the Special:Block behaviour might confuse or lead to the wrong block parameters being submitted unknowingly.

Change 630637 had a related patch set uploaded (by Tchanders; owner: Tchanders):
[mediawiki/core@master] Ignore block restrictions if block is not specified as partial

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

In T189073#6498507, @dom_walden wrote:

Arguably the API behaviour is reasonable, but the Special:Block behaviour might confuse or lead to the wrong block parameters being submitted unknowingly.

I agree that we shouldn't change the behaviour of Special:Block here: e.g. if upgrading a partial block to sitewide, the admin would now have to delete each restriction.

The above patch restores the previous behaviour of the API and the special page, by making sure the restrictions aren't passed on to BlockUser unless the 'partial' options are explicitly chosen. (We may want to change that behaviour in the future, in which case we should raise it separately.)

Change 630637 merged by jenkins-bot:
[mediawiki/core@master] Ignore block restrictions if block is not specified as partial

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

In T189073#6499337, @Tchanders wrote:

The above patch restores the previous behaviour of the API and the special page, by making sure the restrictions aren't passed on to BlockUser unless the 'partial' options are explicitly chosen. (We may want to change that behaviour in the future, in which case we should raise it separately.)

Thanks!

@Tchanders @Urbanecm I notice that in the log_params of the logging table we are now storing namespaces as integers. Previously they were strings.

For example, now: ... "namespaces";a:1:{i:0;i:3;} ....
Previously: ... "namespaces";a:1:{i:0;s:1:"3";} ....

I am not sure if this matters at all. The only difference I have noticed so far is in the logevents API they no longer have quotation marks (e.g. https://en.wikipedia.beta.wmflabs.org/w/api.php?action=query&list=logevents&lelimit=10&format=json&letype=block).

In T189073#6502159, @dom_walden wrote:

@Tchanders @Urbanecm I notice that in the log_params of the logging table we are now storing namespaces as integers. Previously they were strings.

For example, now: ... "namespaces";a:1:{i:0;i:3;} ....
Previously: ... "namespaces";a:1:{i:0;s:1:"3";} ....

I am not sure if this matters at all. The only difference I have noticed so far is in the logevents API they no longer have quotation marks (e.g. https://en.wikipedia.beta.wmflabs.org/w/api.php?action=query&list=logevents&lelimit=10&format=json&letype=block).

@dom_walden Thanks for flagging this.

I think these log params are mostly used by the BlockLogFormatter, and the namespaces are cast to int or string as needed, so I don't think it matters too much; if anything int seems more correct, since the namespace constants are ints.

The only thing I can think of that might break could be analytics, if analytics are using the logging table and looking for strings (I've no idea if analytics looks at this table, but flagging up for @jwang just in case).

@dbarratt Do you think it's OK to switch to ints for storing blocked namespaces in the logging table?

In T189073#6512598, @Tchanders wrote:

@dbarratt Do you think it's OK to switch to ints for storing blocked namespaces in the logging table?

I think so... arguably it's more correct. If it doesn't appear to be causing any problems I would say roll with it.

@Tchanders , thanks for the heads-up. I will double check the impact on existing dashboard and report.

Change 631992 had a related patch set uploaded (by Urbanecm; owner: Urbanecm):
[mediawiki/core@master] Hard deprecate SpecialBlock::validateTarget

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

I have compared validation of the Block API and Special:Block with the previous version. Some differences to note:

• If the target is an IP, we ignore the suppress/hide parameter. Previously, we might return a validation error depending on other parameters.
• In some cases the equality testing is stricter: Previously, setting a page or namespace restriction but not setting the block as partial would count the block as not equal to the previous block, allowing a user to be reblocked with exactly the same settings. This is no longer true.

I tested that the correct block params where stored in the database (including block restrictions) and appear on Special:BlockList.

I tested that the logs are correctly recorded in the database and appear on Special:Log, including tags (when using the API). Checked that hidden blocks get recorded with log_type = suppress, so they are not shown publicly.

Bugs in T189073#6494158 and T189073#6498507 no longer reproducible.

I did some basic permissions testing (e.g. you need sysop rights to create a block, oversight rights to create a hidden block). However, it appears most of this is dealt with by the BlockPermissionChecker class, which was previously tested in T251861.

I briefly tested the onBlockIpComplete hook, fired from the new UserBlock class, triggered the appropriate hook handler in PageTriage.

I did not test:

• Unblocking (this is a different part of the code)
• Application of blocks (apart from very briefly)
• System and composite blocks (not really relevant to this change as it is only about creating individual Database blocks)
• Hooks
  • LdapAuthentication (I don't know how this works or how to use it)
  • EventBus (I don't know how this works or how to use it)

In T189073#6515734, @gerritbot wrote:

Change 631992 had a related patch set uploaded (by Urbanecm; owner: Urbanecm):
[mediawiki/core@master] Hard deprecate SpecialBlock::validateTarget

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

@Tchanders Should this be in a separate ticket, or should we keep this ticket open?

In T189073#6520614, @dom_walden wrote:

In T189073#6515734, @gerritbot wrote:

Change 631992 had a related patch set uploaded (by Urbanecm; owner: Urbanecm):
[mediawiki/core@master] Hard deprecate SpecialBlock::validateTarget

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

@Tchanders Should this be in a separate ticket, or should we keep this ticket open?

It would be nice to close this task, but there are still some subtasks, and some unchecked items in the task description. I suspect most of those could become parent tasks, since really they depend on the introduction of this service... Will leave that to @Urbanecm's judgement though.

@Urbanecm Do you think it would be better to tag the above patch against T263189 since that's where validateTarget is moved?

In T189073#6521336, @Tchanders wrote:

In T189073#6520614, @dom_walden wrote:

In T189073#6515734, @gerritbot wrote:

Change 631992 had a related patch set uploaded (by Urbanecm; owner: Urbanecm):
[mediawiki/core@master] Hard deprecate SpecialBlock::validateTarget

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

@Tchanders Should this be in a separate ticket, or should we keep this ticket open?

It would be nice to close this task, but there are still some subtasks, and some unchecked items in the task description. I suspect most of those could become parent tasks, since really they depend on the introduction of this service... Will leave that to @Urbanecm's judgement though.

I think we can change that.

@Urbanecm Do you think it would be better to tag the above patch against T263189 since that's where validateTarget is moved?

Sure, just done.

Change 633233 had a related patch set uploaded (by Urbanecm; owner: Urbanecm):
[mediawiki/core@master] Hard deprecate SpecialBlock::canBlockEmail

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

Change 633233 merged by jenkins-bot:
[mediawiki/core@master] Hard deprecate SpecialBlock::canBlockEmail

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