Page MenuHomePhabricator

Expose whether a user is blocked from editing a specific page via the API
Closed, ResolvedPublic3 Story Points

Description

Problem
There isn't a way to determine, via the API, if a particular user is blocked from editing a specific page.

MobileFrontend uses it's own logic to determine if a user can edit a particular page or not.

This means any implementations of UserIsBlockedFrom is ignored and the client will assume the user cannot edit a page they may be able to edit (i.e. the client will assume a site-wide block). While the user will be notified that they are blocked (and the client will prevent editing) the actual edit request will succeed.

Solution
Add to ApiQueryInfo an inprop to indicate whether the current user is blocked from the page. This should execute User::isBlockedFrom.

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
dbarratt updated the task description. (Show Details)May 14 2018, 1:53 PM
In T194585, @dbarratt wrote:

and then return the block that blocks the user from editing that page.

That goes beyond exposing User::isBlockedFrom(), and is not supported by the 'UserIsBlockedFrom' hook.

That goes beyond exposing User::isBlockedFrom(), and is not supported by the 'UserIsBlockedFrom' hook.

Returning a bool seemed kinda silly when User::isBlockedFrom() determines this from a single block and most (if not all?) clients will need to tell the user the info from the block. I think if the method returns true it should just return the result of User::isBlocked(). But maybe this is a good idea for a param. :)

Returning a bool seemed kinda silly when User::isBlockedFrom() determines this from a single block and most (if not all?) clients will need to tell the user the info from the block. I think if the method returns true it should just return the result of User::isBlocked().

Err, User::isBlocked() also returns a boolean. And, as I already pointed out to you, the 'UserIsBlockedFrom' hook does not return a block object at all.

Reworking any of that is probably more in scope of T190350: Epic: ⚡️ Partial blocks.

Err, User::isBlocked() also returns a boolean. And, as I already pointed out to you, the 'UserIsBlockedFrom' hook does not return a block object at all.

My apologies, I meant User::getBlock()

kaldari added a subscriber: kaldari.EditedMay 15 2018, 6:45 PM

I think Brad's first suggestion makes the most sense ("Add to ApiQueryInfo an inprop to indicate whether the current user is blocked from the page."). This would be similar to the existing readable prop. Although it would be nice to get the full block info back, I don't think this is strictly necessary. We could always output a generic message like "You are currently blocked from editing this page. Learn more." and have "Learn more" link to the user's block log or something like that. This would also let us work around T194530.

I think Brad's first suggestion makes the most sense ("Add to ApiQueryInfo an inprop to indicate whether the current user is blocked from the page.").

How would this work if the page doesn't exist yet? This would happen if I was blocked from a specific namespace. I mean the client could always go and get the block and figure out if the user is blocked, but I'd rather avoid having the same logic on the client and the server if that can be avoided.

How would this work if the page doesn't exist yet?

It would work well.

It would work well.

Oh. I didn't know you can query for nonexistant pages.
https://www.mediawiki.org/wiki/API:Query#Missing_and_invalid_titles
Well I learn something new everyday!

dbarratt updated the task description. (Show Details)May 16 2018, 12:25 PM
dbarratt renamed this task from Expose User::isBlockedFrom() as an API endpoint to Expose whether a user is blocked from editing a specific page via the API.
TBolliger moved this task from Untriaged to Backlog on the Anti-Harassment board.May 29 2018, 4:14 PM
dbarratt updated the task description. (Show Details)Jun 13 2018, 2:21 PM
dbarratt updated the task description. (Show Details)Jun 13 2018, 4:08 PM
Vvjjkkii renamed this task from Expose whether a user is blocked from editing a specific page via the API to 61caaaaaaa.Jul 1 2018, 1:10 AM
Vvjjkkii triaged this task as High priority.
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed a subscriber: Aklapper.
Scott renamed this task from 61caaaaaaa to Expose whether a user is blocked from editing a specific page via the API.Jul 1 2018, 4:56 PM
Scott lowered the priority of this task from High to Normal.
Scott updated the task description. (Show Details)
Scott added a subscriber: Aklapper.
TBolliger set the point value for this task to 3.
dbarratt claimed this task.

Change 461829 had a related patch set uploaded (by Dbarratt; owner: Dbarratt):
[mediawiki/core@master] Add a prop to ApiQueryInfo to indicate if user is blocked from current page.

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

Well fancy that! I'll ensure this works properly with the partial block changes, if it does (I don't see why it wouldn't looking at the code) then we can close this task! :)

The partial block changes have to add some checks to Title::userCan to support namespace or page depending user blocks in all places of mediawiki (like edit tabs in the navigation etc.)

Just for hint: The checks are more expensive than the IsProbablyEditable flag used by VisualEditor in the web ui (but that also includes create checks)

@Umherirrender I get a true value even if the user is blocked (on master), so this doesn't resolve the use case.

should User::isBlockedFrom be executed from Title::userCan? If not, then another property should be created, or I suppose this property should take both into consideration?

@Umherirrender I get a true value even if the user is blocked (on master), so this doesn't resolve the use case.

should User::isBlockedFrom be executed from Title::userCan? If not, then another property should be created, or I suppose this property should take both into consideration?

For me this reports an empty "actions" key in the json response as expected.
I am testing this as loggedin blocked user on current master.

User::isBlockedFrom is executed from Title::userCan in Title::checkUserBlock which is part for every check in Title::getUserPermissionsErrorsInternal

For me this reports an empty "actions" key in the json response as expected.
I am testing this as loggedin blocked user on current master.

User::isBlockedFrom is executed from Title::userCan in Title::checkUserBlock which is part for every check in Title::getUserPermissionsErrorsInternal

I was using formatversion=2 and getting true with a blocked and unblocked user, but I'll try again.

I double checked this, and it does work correctly.

Reference URL (for myself):
http://localhost:8888/api.php?action=query&titles=Gotham&prop=info&intestactions=edit&formatversion=2

dbarratt closed this task as Invalid.

Change 461829 abandoned by Dbarratt:
Add a prop to ApiQueryInfo to indicate if user is blocked from current page.

Reason:
Unnecessary

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

dbarratt reopened this task as Open.EditedSep 24 2018, 9:06 PM
dbarratt moved this task from Done to In progress on the Anti-Harassment (AHT Sprint 29) board.

The existing API doesn't actually resolve our use case. We need to not only know if you can edit a page or not, but also why you cannot edit the page.

Change 461829 restored by Dbarratt:
Add a prop to ApiQueryInfo to indicate if user is blocked from current page.

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

matmarex removed a subscriber: matmarex.Sep 25 2018, 5:59 AM

I'm not sure "you're blocked!" is all that useful of a "why". Perhaps the better solution would be to have intestactions (optionally?) return the message data from Title::getUserPermissionsErrors().

I'm not sure "you're blocked!" is all that useful of a "why". Perhaps the better solution would be to have intestactions (optionally?) return the message data from Title::getUserPermissionsErrors().

Well the client would have to lookup the block to get the block reason regardless.

dbarratt moved this task from Done to Review on the Anti-Harassment (AHT Sprint 29) board.

I'm not sure "you're blocked!" is all that useful of a "why". Perhaps the better solution would be to have intestactions (optionally?) return the message data from Title::getUserPermissionsErrors().

Implemented that in https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/463292. Using that with a query like /w/api.php?action=query&prop=info&titles=Test&intestactions=edit&intestactionsdetail=full, a user that is blocked would show as something like

"actions": {
    "edit": [
        {
            "code": "blocked",
            "text": "You have been blocked from editing.",
            "data": {
                "blockinfo": {
                    "blockid": 7,
                    "blockedby": "Anomie",
                    "blockedbyid": 1,
                    "blockreason": "testing",
                    "blockedtimestamp": "2018-09-27T15:10:36Z",
                    "blockexpiry": "2018-09-27T15:11:36Z"
                }
            }
        }
    ],
}

The "blocksitewide" flag from https://gerrit.wikimedia.org/r/c/mediawiki/core/+/442222/39/includes/api/ApiQueryUserInfo.php would also be included in there.

Implemented that in https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/463292. Using that with a query like /w/api.php?action=query&prop=info&titles=Test&intestactions=edit&intestactionsdetail=full, a user that is blocked would show as something like

I think you may have misunderstood me in T194585#4204705, because what you've done here is exactly what we are looking for. Ideally, the client (in this case, mobile web) should be able to 1) determine if the user is blocked on a per-page basis and 2) get the reason for the block (preferably on a single request). Your patch allows for that. :)

I'm not a huge fan of changing the param type, (from boolean to an object), but other than using another name for "actions" I don't have a solution (and that might make the params weirder anyways). I also wish the block didn't prefix block in it's params, but that's a separate issue.

I'll go ahead an abandon the patch I wrote since I think your solution is better (and what I was looking for in the first place).

In my earlier comments here, I forgot that intestactions existed. I re-found it during code review of your original patch and realized that that would be a better way to handle determining whether a page is editable or not.

because what you've done here is exactly what we are looking for.

I'm glad, that's what I was hoping for (:

I'm not a huge fan of changing the param type, (from boolean to an object),

No parameter types are changing in my patch. It does add a parameter to change the output field type, but in a backwards-compatible way since the change is gated behind a parameter.

but other than using another name for "actions" I don't have a solution (and that might make the params weirder anyways).

I agree it would be weirder to have intestactions and something else that does the same thing with a different output format. Unless we were going to deprecate intestactions entirely, but I think it still serves well for its original purpose with the boolean returns.

 I also wish the block didn't prefix block in it's params, but that's a separate issue.

It reuses code that displays permissions errors in other contexts, which in turn reuses the block-formatting code from ApiQueryUserInfo.

Change 461829 abandoned by Dbarratt:
Add a prop to ApiQueryInfo to indicate if user is blocked from current page.

Reason:
Better solution at https://gerrit.wikimedia.org/r/c/mediawiki/core/ /463292

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

Change 463292 had a related patch set uploaded (by Dbarratt; owner: Anomie):
[mediawiki/core@master] API: Allow prop=info intestactions to return reasons

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

@Anomie while I was testing your patch, I found this bug T205895, it's not related to this task explicitly, but partial blocks do not work properly with your patch until we fix it.

dbarratt added a comment.EditedOct 1 2018, 5:27 PM

@Anomie while I was testing your patch, I found this bug T205895, it's not related to this task explicitly, but partial blocks do not work properly with your patch until we fix it.

Correction... since prevents( 'edit' ) always returns true for partial blocks, then T205895 is not an issue for partial blocks.

Change 463292 merged by jenkins-bot:
[mediawiki/core@master] API: Allow prop=info intestactions to return reasons

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

dbarratt closed this task as Resolved.Oct 16 2018, 7:55 PM
dbarratt moved this task from Review to Done on the Anti-Harassment (AHT Sprint 31) board.