Page MenuHomePhabricator

Partial Blocks are enforced as Sitewide blocks in Wikibase entities
Open, HighPublic

Description

Problem
When creating or modifying a Wikibase Entity, Partial Blocks are enforced as if they were sitewide blocks. Without fixing this problem, Partial Blocks do not work on Wikidata.

Solution
SpecialWikibasePage::checkBlocked needs to be updated to use User::isBlockedFrom() with the title of the page in order to determine if the user can create or edit the entity. The full title should be passed into that method (i.e. Q2 or Property:P18).

For entity creation, use User::getBlock()->appliesToNamespace() to check if the user is blocked from the namespace the creation is happening in.

As for edits through the API, these block checks are already handled via the edit actions an EditEntityAction -> Action.php in core. So nothing to do there (according to @Addshore ...)

Event Timeline

dbarratt created this task.Oct 24 2018, 9:29 PM
Restricted Application added a project: Wikidata. · View Herald TranscriptOct 24 2018, 9:29 PM
Restricted Application added subscribers: MGChecker, Aklapper. · View Herald Transcript
dbarratt updated the task description. (Show Details)Oct 24 2018, 9:38 PM
dbarratt updated the task description. (Show Details)
TBolliger moved this task from Untriaged to Backlog on the Anti-Harassment board.Oct 24 2018, 9:57 PM

Note that checkBlocked is also used in SpecialNewEntity, where we don’t have a title to check yet. What’s the correct way to check if a user is allowed to create a page?

Note that checkBlocked is also used in SpecialNewEntity, where we don’t have a title to check yet. What’s the correct way to check if a user is allowed to create a page?

I would:

For creation, use the reserved 0 for the entity (i.e. Q0 or Property:P0).

unless that's a bad idea? That should allow for namespace checking, etc.

Is the Anti-Harassment team going to work on this or is the expectation that it be picked up by Wikidata ?

TBolliger added a subscriber: TBolliger.EditedNov 5 2018, 6:03 PM

Is the Anti-Harassment team going to work on this or is the expectation that it be picked up by Wikidata ?

Great question — right now our team is focussed on getting Partial Blocks into releasable shape for a few early adopter Wikipedias. We'll only be bugfixing and building new functionality for them. Once we're satisfied with the level of functionality and quality, we'll release to more wikis, bugfixing as needed. Enabling this functionality will depend on when different communities are ready.

Of course, if the wikidata team wants to hop in early and make any wikidata specific fixes, AHT would be happy to help code review and provide input.

Small addendum: There will some some extensions/situations where it might be less effort and more straightforward for the owning team (or whomever is most familiar) to fix themselves. In these situations we'll discuss on Phab.

Note that checkBlocked is also used in SpecialNewEntity, where we don’t have a title to check yet. What’s the correct way to check if a user is allowed to create a page?

If it makes more sense, we could make a method on Block that would allow you to pass in a namespace and it would return a boolean based on whether the user is blocked from that namespace. Would that be helpful?

dbarratt updated the task description. (Show Details)Nov 29 2018, 9:11 PM

In T204991 we've added the method Block::appliesToNamespace() which can be used for new entity creation.

Addshore triaged this task as Low priority.Mar 19 2019, 12:15 PM
Addshore moved this task from Incoming to Ready to estimate on the Wikidata-Campsite board.

@WMDE-leszek can we sit down together and get this ready for pickup?

@WMDE-leszek can we sit down together and get this ready for pickup?

Hi @Lydia_Pintscher! Is there an update on this? We're getting ready to deploy partial blocks across all wikis.

I just had another quick look at this today.

In T204991 we've added the method Block::appliesToNamespace() which can be used for new entity creation.

So I guess we use BlockManager to get the blocks of a user?
And then check the blocks to see if they apply to the namespace we are creating in?

Blockmanager::getUserBlock only returns one block ("the most relevant one"), what happens in the situation when a user has 2 block for different namespaces?
Is there a way to get all blocks of a user? Or a way to see if any of a users blocks apply to a namespace?

Maybe we need a User::isBlockedFromNamespace, or rather PermissionManager::isBlockedFromNamespace() ?

@WMDE-leszek can we sit down together and get this ready for pickup?

The work that is needed for editing special pages is already outlined in the task description, although some refactoring will be needed there in order to get the Title to the method where we check blocks, or just do the block checking in each special page class itself.

Then we need to resolve my questions at the top of this comment about appliesToNamespace for entity creation.

As for edits through the API, as far as I can tell these block checks are already handled via the edit actions an EditEntityAction -> Action.php in core. So nothing to do there.

Going to move to waiting until the above comment has a response regarding the namespace checks.

So I guess we use BlockManager to get the blocks of a user?

You can use either User::getBlock() or BlockManager::getUserBlock(). They are slightly different, but for your purposes, it should always return the same result.

And then check the blocks to see if they apply to the namespace we are creating in?

Yes. :)

Blockmanager::getUserBlock only returns one block ("the most relevant one"), what happens in the situation when a user has 2 block for different namespaces?

You will get a CompositeBlock object which will extend from AbstractBlock class and have all of the same methods. MediaWiki has, up until this week, always returned a single, most-relevant block anyways. Starting soon, you'll get a CompositeBlock which can be used the same way.

Is there a way to get all blocks of a user? Or a way to see if any of a users blocks apply to a namespace?

We could probably build that into CompositeBlock if you really need them, but calling ::appliesToNamespace() on the AbstractBlock returned from User::getBlock() or BlockManager::getUserBlock() should be what you want.

Maybe we need a User::isBlockedFromNamespace, or rather PermissionManager::isBlockedFromNamespace() ?

I think the best way would be to determine if the AbstractBlock applies to the namespace with AbstractBlock::appliesToNamespace().

dbarratt updated the task description. (Show Details)Jun 12 2019, 12:22 AM

So I guess we use BlockManager to get the blocks of a user?

You can use either User::getBlock() or BlockManager::getUserBlock(). They are slightly different, but for your purposes, it should always return the same result.

You should use User::getBlock, since that will call BlockManager::getUserBlock and run the hooks too.

(BlockManager::getUserBlock should be treated as internal and only be called by User::getBlockedStatus (as documented). Eventually, the blocks logic should be taken out of User, as part of the wider effort to untangle such dependencies, at which point the way to get a block will be via the BlockManager, but we're not there yet.)

And then check the blocks to see if they apply to the namespace we are creating in?

Yes. :)

Yes, so somethig like:

$block = $user->getBlock();
if ( $block && $block->appliesToNamespace( $ns ) ) { ... }

Blockmanager::getUserBlock only returns one block ("the most relevant one"), what happens in the situation when a user has 2 block for different namespaces?

You will get a CompositeBlock object which will extend from AbstractBlock class and have all of the same methods. MediaWiki has, up until this week, always returned a single, most-relevant block anyways. Starting soon, you'll get a CompositeBlock which can be used the same way.

A few things to add here:

  • Two blocks can't be stored against exactly the same target, but a user can be affected by more than one block if there are blocks against their account and IP address, or against some ranges that cover their IP address
  • "the most relevant one" is a not-terribly-helpful way of saying the most "specific" one, meaning that a user account block is chosen over an IP block, which is chosen over an IP range block (and narrower IP range blocks are favoured over wider ones). Thanks for pointing this out, will improve the docs.
  • AHT should be merging this soon: https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/497668/ After this, if BlockManager::getUserBlock finds more than one block, then instead of choosing just one, it will return a composite block combining the strictest features of all the blocks. So if there are blocks that apply to different namespaces, this composite block will apply to all of them.

Is there a way to get all blocks of a user? Or a way to see if any of a users blocks apply to a namespace?

We could probably build that into CompositeBlock if you really need them, but calling ::appliesToNamespace() on the AbstractBlock returned from User::getBlock() or BlockManager::getUserBlock() should be what you want.

Maybe we need a User::isBlockedFromNamespace, or rather PermissionManager::isBlockedFromNamespace() ?

I think the best way would be to determine if the AbstractBlock applies to the namespace with AbstractBlock::appliesToNamespace().

After the above patch is merged, doing $user->getBlock() and $block->appliesToNamespace( $ns ) as shown above will do all of this.

Thanks for all of the details.
I believe that 1 bit of phpdoc, which sounds like it will now be altered, is what was throwing me a bit off.

@lydia I believe we have all of the details we need to write this up for pickup. :)
I'll try and get that done this week (unless someone wants to jump in before me) or if we just want to pick it up and have whoever implements it read through the comments.!

Niharika raised the priority of this task from Low to Normal.Jun 13 2019, 1:03 AM

Raising the priority on this as it is a blocker for partial blocks deployment.

@Niharika would you mind reminding us what is the planned timeline of the deployment of partial blocks? Just so that we know how urgently the fix is needed (I do anticipate that ASAP is preferred, but given you've "only" raised the priority up to "Normal" we would also only prioritize it accordingly)

@Niharika would you mind reminding us what is the planned timeline of the deployment of partial blocks? Just so that we know how urgently the fix is needed (I do anticipate that ASAP is preferred, but given you've "only" raised the priority up to "Normal" we would also only prioritize it accordingly)

Our planned rollout for Partial blocks is end of this month (June). I was actually going to raise the priority higher but I didn't want to step on anyone's toes (whoever prioritizes tickets at your end). ASAP is indeed preferred.

Lydia_Pintscher raised the priority of this task from Normal to High.Jun 13 2019, 7:26 PM

Hehe ok let's raise it then.
@WMDE-leszek let's push for it after the Commons page move bug then?

Addshore updated the task description. (Show Details)Jun 16 2019, 4:08 PM
Addshore moved this task from Needs Work to Ready to estimate on the Wikidata-Campsite board.

This is rather lots of work, SpecialWikibasePage is not the right place to keep the list, Making it per special page also is complex, like Special:SetLabel can be used on property and item and if the person is blocked on item namespace, there's no way to make sure it's applied until the user actually tries it (in which EntityStore should handle it properly using PermissionManager), so maybe we should just drop it? I would prefer if the special page still checks for site-wide bans.

Change 517442 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/extensions/Wikibase@master] Make SpecialWikibasePage only respect site-wide block

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

Change 517450 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/extensions/Wikibase@master] Add namespace-based block check on SpecialNewEntity

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

Restricted Application added a project: User-Ladsgroup. · View Herald TranscriptJun 17 2019, 4:06 PM

This is rather lots of work, SpecialWikibasePage is not the right place to keep the list, Making it per special page also is complex, like Special:SetLabel can be used on property and item and if the person is blocked on item namespace, there's no way to make sure it's applied until the user actually tries it (in which EntityStore should handle it properly using PermissionManager), so maybe we should just drop it? I would prefer if the special page still checks for site-wide bans.

It's fine if the user doesn't know until it's attempted. For instance, any edits made with the Action API this is the case. We try to notify the user before hand, but that's not always possible. For instance we added T194585 so a preflight can be created to determine if the user is blocked from a particular title. However, if it's not possible, I would throw a validation error to the user.

You can see an example of not knowing in MediaWiki by blocking a test user from a namespace (say MediaWiki) and then attempting to create a new page in that namespace.

Change 520200 had a related patch set uploaded (by Hoo man; owner: Hoo man):
[mediawiki/extensions/WikibaseLexeme@master] Add namespace-based block check to SpecialNewLexeme

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

Change 520200 merged by jenkins-bot:
[mediawiki/extensions/WikibaseLexeme@master] Add namespace-based block check to SpecialNewLexeme

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

Change 517442 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Make SpecialWikibasePage only respect site-wide block

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

Change 517450 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Add namespace-based block check on SpecialNewEntity

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

hoo added a subscriber: hoo.

I assume this is done now… feel free to move this back if anything is missing.

Thanks @hoo. What's the best way to test this?

Thanks @hoo. What's the best way to test this?

Enabling partial blocks on wikidata and commons in beta cluster would be a good point IMO

Thanks @hoo. What's the best way to test this?

Enabling partial blocks on wikidata and commons in beta cluster would be a good point IMO

Thanks @Ladsgroup!

It's also enabled in production at:
https://test.wikidata.org/wiki/Special:Block

@dbarratt Do you have rights on that site to give me and @dom_walden access to Special:Block?

@Niharika User:NKohli (WMF) doesn’t seem to exist on testwikidata yet, I think you’ll have to create the account there first before rights can be given to it. (@dbarratt exists, but I don’t have the rights to make him admin, apparently.)

It's also enabled in production at:
https://test.wikidata.org/wiki/Special:Block

@dbarratt Do you have rights on that site to give me and @dom_walden access to Special:Block?

I can access it myself, but not grant access.

But we can also enable on beta. :)