Page MenuHomePhabricator

Replace UserIsHidden with GetUserBlock in CentralAuth
Closed, ResolvedPublic

Description

Problem
Before UserIsHidden is deprecated in T228948, CentralAuth will need to be updated.

Proposed Solution
Replace the use of UserIsHidden with GetUserBlock (which replaces GetBlockedStatus)

Event Timeline

Tchanders renamed this task from Replace UserIsHidden with GetBlockedStatus in CentralAuth to Replace UserIsHidden with GetUserBlock in CentralAuth.Aug 21 2019, 4:51 PM
Tchanders claimed this task.
Tchanders updated the task description. (Show Details)

Change 527251 had a related patch set uploaded (by Tchanders; owner: Tchanders):
[mediawiki/extensions/CentralAuth@master] Use the GetUserBlock hook instead of UserIsHidden

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

Change 527251 merged by jenkins-bot:
[mediawiki/extensions/CentralAuth@master] Use the GetUserBlock hook instead of UserIsHidden

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

If a user is hidden via Special:CentralAuth, any blocks they have (unless they are hidden blocks) will be overridden by a system block.

System blocks don't block Special:EmailUser and nor does CentralAuth[1]. If the user has a non-hidden block applying to email, it will no longer be effective (and user will be able to email).

Normally when Special:CentralAuth hides a user it also automatically creates hidden database blocks locally on each connected wiki. Therefore, the above is not a problem.

But, there may be cases where these blocks aren't created automatically, for example where there is a preexisting database block on that user. Or if someone decides to replace the automatically created block (I don't know why).

@Niharika Is this a concern? Perhaps I should just raise a bug on CentralAuth not blocking Special:EmailUser (I have been meaning to do this for a while).

(also if @Tchanders could confirm what I said above. I'm pretty sure it's right.)

P.S. some things that list users or user info (e.g. Special:ListUsers) get info about a user's hidden status directly from the database. Therefore, only hidden database blocks are effective, not system blocks. So, for example, a globally hidden user can still be seen in Special:ListUsers, unless they have a local hidden block. But, this is no different to what it was before.

Notes

  1. Special:EmailUser does not use PermissionManager, which would eventually trigger one of CentralAuth's hooks to block the action (this is how CentralAuth applies blocking to Special:CreateAccount). Nor does CentralAuth have a hook for Special:EmailUser. I am not sure where the bug lies.

It sounds like the following would be a better approach:

  • If the user is unblocked, block them with the hidden SystemBlock
  • If the user is blocked, add the hidden SystemBlock to any existing blocks as a CompositeBlock (instead of overriding)

Special:EmailUser does not use PermissionManager [...]

some things that list users or user info (e.g. Special:ListUsers) get info about a user's hidden status directly from the database [...]

These sound worth filing and fixing to me.

Thanks for the detailed explanation, @dom_walden!

It sounds like the following would be a better approach:

  • If the user is unblocked, block them with the hidden SystemBlock
  • If the user is blocked, add the hidden SystemBlock to any existing blocks as a CompositeBlock (instead of overriding)

This makes sense to me as well.

Special:EmailUser does not use PermissionManager [...]

some things that list users or user info (e.g. Special:ListUsers) get info about a user's hidden status directly from the database [...]

These sound worth filing and fixing to me.

Me too. @dom_walden It'd be great if you can file these. :)

Change 533910 had a related patch set uploaded (by Tchanders; owner: Tchanders):
[mediawiki/extensions/CentralAuth@master] Add to existing blocks if a blocked user is centrally hidden

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

Change 533910 merged by jenkins-bot:
[mediawiki/extensions/CentralAuth@master] Add to existing blocks if a blocked user is centrally hidden

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

Change 534849 had a related patch set uploaded (by Dbarratt; owner: Dbarratt):
[mediawiki/extensions/CentralAuth@master] Allow users to be hidden from lists, without being blocked

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

Ping @dbarratt @Tchanders - is there anything blocking this ticket?

@Niharika there is an open question from @Tchanders, I think we talked about it but didn't come to a resolution:

With this change, the account is no longer hidden when an admin chooses "Account is hidden from the global users list" in Special:CentralAuth.

If we wanted to allow a user to be hidden but not blocked, we'd need to reintroduce something like the UserIsHidden hook, and set the mHideName flag directly on the User, without setting a block.

I don't think we should do that - the fact that a user cannot now be hidden without a block is not a regression, but an intended consequence of deprecating the UserIsHidden hook (explained in T228948).

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CentralAuth/+/534849#message-4ea3c3988f99592353e78565ede94973db385139

I think that option only effects that single page, so we can either remove the option completely, or fix the patch.

@Niharika there is an open question from @Tchanders, I think we talked about it but didn't come to a resolution:

With this change, the account is no longer hidden when an admin chooses "Account is hidden from the global users list" in Special:CentralAuth.

If we wanted to allow a user to be hidden but not blocked, we'd need to reintroduce something like the UserIsHidden hook, and set the mHideName flag directly on the User, without setting a block.

I don't think we should do that - the fact that a user cannot now be hidden without a block is not a regression, but an intended consequence of deprecating the UserIsHidden hook (explained in T228948).

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CentralAuth/+/534849#message-4ea3c3988f99592353e78565ede94973db385139

I think that option only effects that single page, so we can either remove the option completely, or fix the patch.

Hmmm, without any data about how often it is used, it's hard to make a judgement call on this. I'm fine with Thalia's suggestion to remove the option.
Can we find out if that option is also used elsewhere?

@Tchanders & @Niharika when the UI says: "global users list" is that just Special:GlobalUsers ? If so, it looks like GlobalUsersPager::getQueryInfo to ensure that only users who are not hidden in any way are shown:

$conds = [ 'gu_hidden' => CentralAuthUser::HIDDEN_NONE ];

or are we talking about some other page?

Perhaps hiding users on non-global pages was an unintended consequence of the previous hook? Or rather... it looks like what the UI says and what the code was actually doing is not the same thing?

@Tchanders & @Niharika when the UI says: "global users list" is that just Special:GlobalUsers ? If so, it looks like GlobalUsersPager::getQueryInfo to ensure that only users who are not hidden in any way are shown:

$conds = [ 'gu_hidden' => CentralAuthUser::HIDDEN_NONE ];

or are we talking about some other page?

Based on the wording, I would assume it just means Special:GlobalUsers. Let's work with that assumption.

Perhaps hiding users on non-global pages was an unintended consequence of the previous hook? Or rather... it looks like what the UI says and what the code was actually doing is not the same thing?

I don't think I understand this fully. Are you saying that the option was not working as expected and nobody noticed?

I don't think I understand this fully. Are you saying that the option was not working as expected and nobody noticed?

I'm saying the previous behavior was that the user would be removed from local user lists (like Special:ListUsers) but not have a block. I guess I'm saying it didn't work as the UI said it would, but I'm not sure what the users actually expect now.

I think the behavior should match the UI. We can either restore the previous behavior and change the UI, or fix the behavior to match the UI, but right now it's inconsistent and has been for (I assume) some time.

I don't think I understand this fully. Are you saying that the option was not working as expected and nobody noticed?

I'm saying the previous behavior was that the user would be removed from local user lists (like Special:ListUsers) but not have a block. I guess I'm saying it didn't work as the UI said it would, but I'm not sure what the users actually expect now.

I think the behavior should match the UI. We can either restore the previous behavior and change the UI, or fix the behavior to match the UI, but right now it's inconsistent and has been for (I assume) some time.

The behavior should definitely match the UI. I am inclined to go with @Tchanders' suggestion to remove the option completely for now. We have no data about whether it was used and what the use case was. And that nobody (to our knowledge) reported the behavior/UI mismatch tells me that it was probably not very popular. If we hear that someone misses that option, we can reconsider what we want to do with it but let's get rid of it until then. I believe that's also the least amount of work for us from all the options, right?

The behavior should definitely match the UI. I am inclined to go with @Tchanders' suggestion to remove the option completely for now. We have no data about whether it was used and what the use case was. And that nobody (to our knowledge) reported the behavior/UI mismatch tells me that it was probably not very popular. If we hear that someone misses that option, we can reconsider what we want to do with it but let's get rid of it until then. I believe that's also the least amount of work for us from all the options, right?

I believe the least amount of work would be making the option consistent with the UI, as that is already done with this patch https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CentralAuth/+/534849 :)

Change 534849 merged by jenkins-bot:
[mediawiki/extensions/CentralAuth@master] Allow users to be hidden from lists, without being blocked

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

The behavior should definitely match the UI. I am inclined to go with @Tchanders' suggestion to remove the option completely for now. We have no data about whether it was used and what the use case was. And that nobody (to our knowledge) reported the behavior/UI mismatch tells me that it was probably not very popular. If we hear that someone misses that option, we can reconsider what we want to do with it but let's get rid of it until then. I believe that's also the least amount of work for us from all the options, right?

I believe the least amount of work would be making the option consistent with the UI, as that is already done with this patch https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CentralAuth/+/534849 :)

Sorry for a late reply. That would be fine by me and I believe that's what we ended up with doing?

A user who is "Account is hidden from the global users list" does not appear in Special:GlobalUsers, but the user is not blocked from doing anything.

A user who is "Account is hidden completely" does not appear in Special:GlobalUsers, has local sitewide blocks created on connected wikis, and is blocked with a system block.

Tested on my local vagrant environment 1.35.0-alpha (869164a) 17:18, 27 October 2019 (as I don't have CentralAuth permissions on beta).