Page MenuHomePhabricator

Partial Blocks are enforced as Sitewide blocks in Wikibase entities
Closed, ResolvedPublic

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 ...)

Details

Related Gerrit Patches:
mediawiki/extensions/Wikibase : masterMake SpecialWikibasePage only respect site-wide block
mediawiki/extensions/WikibaseLexeme : masterAdd site wide block check to checkBlocked
mediawiki/extensions/Wikibase : masterAdd namespace-based block check on SpecialNewEntity
mediawiki/extensions/WikibaseLexeme : masterAdd namespace-based block check to SpecialNewLexeme

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Addshore triaged this task as Low priority.Mar 19 2019, 12:15 PM
Addshore added a project: Wikidata-Campsite.
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 discussion or investigation to ready to go on the Wikidata board.
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.

Addshore moved this task from ready to go to in progress on the Wikidata board.Jun 21 2019, 11:31 PM

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. :)

@dbarratt @Niharika I added you both to the admin group on test wikidata

@dbarratt @Niharika have you tested this? is it good?

@dbarratt @Niharika have you tested this? is it good?

It works beautifully for me! I will allow @dom_walden to take a look if he wants too, otherwise I think this is good to enable on production. :)

dom_walden added a comment.EditedAug 14 2019, 2:54 PM

I can access and submit the Special:NewItem and Special:NewProperty forms unless I am partially blocked from the Main or Property namespace (resp.)

I setup partial blocks for a user and their IP (i.e. a composite block), which used page restrictions to block the user from specific wikidata items.

I performed the following special page actions on items they were and were not blocked from, and was allowed or not allowed as appropriate.

  • Special:MergeItems
  • Special:RedirectEntity
  • Special:SetSiteLink
  • Special:SetAliases
  • Special:SetDescription
  • Special:SetLabel
  • Special:SetLabelDescriptionAliases
  • Using the "Add links" on client wikidata wikis

I could also edit items (e.g. adding statements, links, etc.) unless I was blocked from them specifically.

I note that some of the block messages were a bit uninformative (e.g. some just said "Permission denied.", no extra info.) I assume this is a preexisting bug.

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...

I did not test the API, apart from the API calls that appear to happen when editing an item. Although, I see there are some phpunit checks that simulate performing certain API requests while blocked.

I did all this testing on my local Vagrant environment, with the wikidata role enabled.

  • MediaWiki 1.34.0-alpha (dbdbeb7) 06:23, 14 August 2019
  • WikibaseRepository (dfadb52) 07:16, 14 August 2019

I was not able to test Special:NewLexeme. It is not included in the vagrant role nor on https://commons.wikimedia.beta.wmflabs.org/wiki/. It is on https://wikidata.beta.wmflabs.org/wiki/, but I am not an administrator there. @Addshore, may I be added to the admin group on wikidata.beta, if you think this matters? (My user is "Dom walden")

@dom_walden I've just added you to admin group on https://wikidata.beta.wmflabs.org (rights expire in 24 hours, let me know if this is a silly restriction, so I will make you unexpirying admin)

WMDE-leszek removed Ladsgroup as the assignee of this task.Aug 15 2019, 12:38 PM

Thanks @dom_walden. We'll fix this. Also I've updated your admin rights on beta wikidata so they wouldn't expire.

noarave claimed this task.Aug 20 2019, 8:11 AM
Restricted Application added a project: User-Noarave. Β· View Herald TranscriptAug 20 2019, 8:11 AM

Change 531347 had a related patch set uploaded (by Noa wmde; owner: Noa wmde):
[mediawiki/extensions/WikibaseLexeme@master] Add site wide block check to checkBlocked

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

Change 531347 merged by jenkins-bot:
[mediawiki/extensions/WikibaseLexeme@master] Add site wide block check to checkBlocked

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

Oversight on my part, it looks like the creation code (in both patches) is using AbstractBlock::isSitewide() instead of AbstractBlock::appliesToNamespace() (as is outlined in the task description). Using the former means that someone who is blocked from a namespace will be allowed to create an item in that namespace, using the latter will ensure they are blocked properly from that namespace.

dbarratt updated the task description. (Show Details)Aug 21 2019, 6:23 PM

Oversight on my part, it looks like the creation code (in both patches) is using AbstractBlock::isSitewide() instead of AbstractBlock::appliesToNamespace() (as is outlined in the task description). Using the former means that someone who is blocked from a namespace will be allowed to create an item in that namespace, using the latter will ensure they are blocked properly from that namespace.

As I understand it, the namespace blocking for Lexemes was done in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/WikibaseLexeme/+/520200. But please confirm this.

Testing on beta, I cannot create a new Lexeme (either via Special:NewLexeme or api.php?action=wbeditentity) if I am partially blocked from the "Lexeme" namespace or if I am sitewide blocked.

I can if I am partially blocked but not from the Lexeme namespace.

Oversight on my part, it looks like the creation code (in both patches) is using AbstractBlock::isSitewide() instead of AbstractBlock::appliesToNamespace() (as is outlined in the task description). Using the former means that someone who is blocked from a namespace will be allowed to create an item in that namespace, using the latter will ensure they are blocked properly from that namespace.

I just want to mention those parts are for basic access to the special page, the real action is being handled by PermissionManager so nothing is broken here.

I just want to mention those parts are for basic access to the special page, the real action is being handled by PermissionManager so nothing is broken here.

Ah! perfect. thanks!

Oversight on my part, it looks like the creation code (in both patches) is using AbstractBlock::isSitewide() instead of AbstractBlock::appliesToNamespace() (as is outlined in the task description). Using the former means that someone who is blocked from a namespace will be allowed to create an item in that namespace, using the latter will ensure they are blocked properly from that namespace.

As I understand it, the namespace blocking for Lexemes was done in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/WikibaseLexeme/+/520200. But please confirm this.
Testing on beta, I cannot create a new Lexeme (either via Special:NewLexeme or api.php?action=wbeditentity) if I am partially blocked from the "Lexeme" namespace or if I am sitewide blocked.
I can if I am partially blocked but not from the Lexeme namespace.

thanks @dom_walden ! So are we fine to conclude this is now resolved?

thanks @dom_walden ! So are we fine to conclude this is now resolved?

If everyone else is happy, then I'm happy :)

WMDE-leszek closed this task as Resolved.Aug 26 2019, 4:21 PM

thanks @dom_walden, thanks everyone - looks it's done!