Please correct the description as needed.

@nettrom_WMF will follow up with Marshall on how we need to support on the Product Analytics side

This is something that we will work on in the coming weeks. It is not fully specified yet, but I'm moving it to the sprint board to prioritize it getting into Ready for Development.

The control flow for normal IP blocks and autoblocks is SpecialCreateAccount::checkPermissions() -> AuthManager::checkAccountCreatePermissions() -> User::authorizeWrite() -> PermissionManager::checkUserBlock(). For global blocks, it's the same, except the last step is PermissionManager::checkPermissionHooks() which calls the GlobalBlocking extension's getUserPermissionsErrorsExpensive hook handler.

So one option is to add some logging code to SpecialCreateAccount::checkPermissions(). Adding Wikimedia-specific code to core is not great but it's going to be temporary so not a big deal. That method receives a PermissionStatus from AuthManager; the block information can be retrieved from PermissionStatus. Global block information cannot, but we can just match against a list of error messages (wikimedia-globalblocking-ipblocked, wikimedia-globalblocking-ipblocked-range, wikimedia-globalblocking-ipblocked-xff) and then use GlobalBlocking::getUserBlock() to retrieve the block information.

The other option would be to use WikimediaEvents and find appropriate hooks. I don't see a reasonable hook though, we'd probably have to add one (maybe an audit hook in PermissionManager::getPermissionErrorsInternal() which can then filter by action=createaccount and rigor=RIGOR_SECURE, which is probably a good approximation of someone actually attempting account creation (this would need to be verified), and check the error message against a list of known block-related error messages. It wouldn't have the block object available, but we can just call User::getBlock() (and possibly copy the IP-block-related trickery done in PermissionManager::checkUserBlock()); and it wouldn't be able to differentiate Special:CreateAccount from the API but that might actually be a good thing. I would probably go with this one as it seems to be a generically useful capability to have (and maybe also useful for the editing half of this metrics project), and we can avoid the awkwardness of referencing EventGate and GlobalBlocking in core code (even if temporarily).

We would probably want to report the block event via EventGate as that is more useful for analysts than Logstash; also report it to Logstash with a stack trace so we can QA and ensure we are not overcounting (e.g. because MediaWiki makes a permission check on some non-login page to determine whether to show a login link); and maybe also send something to statsd for convenient graphing.

To what extent are we blocking bot account generation? (e.g. frequent block encounters by same IP & user agent)

Note that 1) most bots probably never trigger the block instrumentation because they use the API, not the login page (although maybe we should instrument the API as well); 2) for bots which do load the login page for some reason, it's going to be hard to differentiate a bot with a legitimate-looking user agent from real users, many of whom share the same user agent, and some of the IPs in question are shared between many users (that's why they get blocked).

In T306018#7916919, @Tgr wrote:

Note that 1) most bots probably never trigger the block instrumentation because they use the API, not the login page (although maybe we should instrument the API as well)

That's a good point! I think we should instrument the API too because as far as I know that's what the apps use. I'll be updating the description to reflect that we'd like to be able to determine what platform was used (API, desktop/mobile if Special:CreateAccount).

2. for bots which do load the login page for some reason, it's going to be hard to differentiate a bot with a legitimate-looking user agent from real users, many of whom share the same user agent, and some of the IPs in question are shared between many users (that's why they get blocked).

Yeah, I see that point, and I hope we'll be able to know more about how that shows up once we have data. @MMiller_WMF mentioned on Slack the interest of learning to what extent a user refreshes to try again, or comes back later to try again, which means that we have various temporal patterns that we're looking for. With regards to the notion of a "bot", I think we might want to change it to "automated" similar to how page views are labelled. There might be things in the User-Agent that allows us to label them as bots, but in practice we're probably trying to determine if the traffic is significant enough to label it "automated".

@Tgr : I have one question about the ability to determine block parameters. If we're logging the block ID and the block is temporary, is there a way to retrieve the block parameters once the block has expired? I tried searching through the logging table, but it doesn't seem like it logs the block ID. I'm asking because we might want to add block parameters to the logging in a way similar to what the mediawiki_user_blocks_change stream does.

In T306018#7918131, @nettrom_WMF wrote:

@Tgr : I have one question about the ability to determine block parameters. If we're logging the block ID and the block is temporary, is there a way to retrieve the block parameters once the block has expired? I tried searching through the logging table, but it doesn't seem like it logs the block ID. I'm asking because we might want to add block parameters to the logging in a way similar to what the mediawiki_user_blocks_change stream does.

Log records should be possible to look up with something like SELECT log_params FROM logging JOIN log_search ON ls_log_id = log_id AND ls_field = 'ipb_id' WHERE log_type = block AND log_action IN ('block', 'reblock') AND ls_value = {id}. (The one thing I'm unsure about is how reblocks work - do they change the block ID? If not, selecting the right log event will be trickier.) We could add block parameters to the event anyway if that's more convenient.

Global blocks don't seem to record the block ID in the log entry though. We should probably fix that.

@Tgr briefed me on the action plan for this task, I can start the implementation and ask further details if needed.

Change 802483 had a related patch set uploaded (by Sergio Gimeno; author: Sergio Gimeno):

[mediawiki/core@master] WIP: Add account creation blocked attempt

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

Change 821711 had a related patch set uploaded (by Sergio Gimeno; author: Sergio Gimeno):

[schemas/event/secondary@master] [WIP] Instrument blocked account registration

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

Change 821700 had a related patch set uploaded (by Sergio Gimeno; author: Sergio Gimeno):

[mediawiki/extensions/WikimediaEvents@master] WIP: log createaccount attempts with blocked account

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

In the event details mentioned in the description what are the expected values for:

• block type: I see there are 5 types in the Block.php interface. Are these the expected values for this field? Should they map to a meaningful string?
• user ip: Is using $user->getRequest()->getIP() as in PermissionManager::checkUserBlock() reliable to obtain the user IP?
• api or special page: Can we use x-requested-with as a reliable method for the distinction?

What do you think @Tgr ?

• user ip: Is using $user->getRequest()->getIP() as in PermissionManager::checkUserBlock() reliable to obtain the user IP?

Out of curiosity, is there a technical reason to have user_ip as a field and manually populating it instead of adding - $ref: /fragment/http/client_ip/1.0.0# to the schema (like in analytics/mediawiki/welcomesurvey/interaction) to have EventGate automatically populate http.client_ip (doc)?

Oh it just occurred to me that the http.client_ip fragment thing perhaps doesn't work as expected for server-side events?

In T306018#8142988, @mpopov wrote:

• user ip: Is using $user->getRequest()->getIP() as in PermissionManager::checkUserBlock() reliable to obtain the user IP?

Out of curiosity, is there a technical reason to have user_ip as a field and manually populating it instead of adding - $ref: /fragment/http/client_ip/1.0.0# to the schema (like in analytics/mediawiki/welcomesurvey/interaction) to have EventGate automatically populate http.client_ip (doc)?

Thanks for pointing this, I wasn't aware of the existing fragments and auto-populated fields. I was asking the question because @Tgr mentioned specifically the IP-block-related trickery done in PermissionManager::checkUserBlock() and I'm not sure if the scenario in this task requires more fine grained IP retrieval.

In T306018#8143015, @mpopov wrote:

Oh it just occurred to me that the http.client_ip fragment thing perhaps doesn't work as expected for server-side events?

Not sure what do you mean by server-side events here. I think in this task we are mostly looking at the requests coming from the web and rest api so I think http.client_id should be fine.

In T306018#8143015, @mpopov wrote:

Oh it just occurred to me that the http.client_ip fragment thing perhaps doesn't work as expected for server-side events?

It should work just fine in both cases, we fixed a server-side issue in T288853.

I'll take a closer look at the proposed schema for the current task later today.

In T306018#8142946, @Sgs wrote:

In the event details mentioned in the description what are the expected values for:

• block type: I see there are 5 types in the Block.php interface. Are these the expected values for this field? Should they map to a meaningful string?

In the interest of having consistency across schemas to simplify working with them, I think there's a little bit of coordination needed here between us and the Editing team's folks working on T310390. From digging into the discussion across both tasks as well as the parent task, I think the key information we need about the block would be:

1. What type of block it is: global or local.
2. Whether the block is indefinite or has an expiry.
3. The block ID.

The first one enables us to determine where the main impact comes from, we often point towards the global VPN blocks as a possible issue. The second one enables us to understand if users are encountering temporary or permanent blocks, and here there might be key differences between local and global blocks. The third enables us to look up more information about the block if needed. (although @Tgr mentions in T306018#7919267 that global blocks apparently don't log their ID in the logging table? I agree that we might want to fix that)

I'm unsure how the types defined in Block.php maps to these concepts, and I'm happy to learn more about it! And @MNeisler should feel free to chime in here if there's something I've forgotten!

In T306018#8142946, @Sgs wrote:

• block type: I see there are 5 types in the Block.php interface. Are these the expected values for this field? Should they map to a meaningful string?

Those are the expected values (although I'm not sure TYPE_ID is a real thing, but no harm in handling it), plus there will also be global blocks, which is a separate set of classes. Yes, id, range etc would be nicer than numeric IDs.

• user ip: Is using $user->getRequest()->getIP() as in PermissionManager::checkUserBlock() reliable to obtain the user IP?

Yes, it's reliable.

• api or special page: Can we use x-requested-with as a reliable method for the distinction?

No, that's set by jQuery and similar JS libraries; in general it's not guaranteed to be present on API requests. You can check instead whether the MW_API and MW_REST_API constants are defined.

In T306018#8142988, @mpopov wrote:

Out of curiosity, is there a technical reason to have user_ip as a field and manually populating it instead of adding - $ref: /fragment/http/client_ip/1.0.0# to the schema (like in analytics/mediawiki/welcomesurvey/interaction) to have EventGate automatically populate http.client_ip (doc)?

I'm not sure if it's the same thing. Does it use the trusted XFF list? Might be somewhat relevant to this task, as that feature is specifically for differentiating between users behind NAT to avoid collateral block damage.

In T306018#8144192, @nettrom_WMF wrote:

I'm unsure how the types defined in Block.php maps to these concepts

Those are various types of local blocks. TYPE_USER affects a specific user regardless of their IP (not really relevant for registration, although see below), TYPE_IP and TYPE_RANGE are single-IP / IP range blocks respectively, TYPE_AUTO ("autoblock") is a "child block" created from a TYPE_USER block when the user tries to do something and the block prevents it - the idea is once you see what IP they are editing from, you block that IP for a short while (one day), to prevent them from evading the block by logging out. This is an optional feature that can be enabled or not when the TYPE_USER block is created.

We might want to also log whether the account was created by an anonymous user, or a logged-in user (e.g. event organizers sometimes create accounts for event participants; sometimes this can be used to work around range blocks, or other restrictions such as throttling).

In T306018#7916919, @Tgr wrote:

The control flow for normal IP blocks and autoblocks is SpecialCreateAccount::checkPermissions() -> AuthManager::checkAccountCreatePermissions() -> User::authorizeWrite() -> PermissionManager::checkUserBlock(). For global blocks, it's the same, except the last step is PermissionManager::checkPermissionHooks() which calls the GlobalBlocking extension's getUserPermissionsErrorsExpensive hook handler.

That was a bit incomplete. There are three avenues in which a signup can be interrupted by a block:

• when POSTing the login page (edge case; normally the check above would result in an error and the login form not showing): SpecialCreateAccount::checkPermissions() -> AuthManager::authorizeCreateAccount() -> Authority::authorizeWrite('createaccount',...) -> PermissionManager::getPermissionErrorsInternal('createaccount',...,RIGOR_SECURE)
• when using the action=query&list=users&usprop=cancreate API: ApiQueryUsers::execute() -> AuthManager::canCreateAccount() -> CheckBlocksSecondaryAuthenticationProvider::testUserForCreation() -> User::isBlockedFromCreateAccount() (no permission check, it looks up the block record directly)
• when using the action=createaccount API: ApiAMCreateAccount::execute() -> AuthManager::beginAccountCreation() -> AuthManager::::authorizeCreateAccount() -> Authority::authorizeWrite('createaccount',...) -> PermissionManager::getPermissionErrorsInternal('createaccount',...,RIGOR_SECURE)

The web UI uses the cancreate API flag (for AJAX validation of the loginname field), and the Android app also seems to do so, so we need to do something about that not going through the permission system. User::isBlockedFromCreateAccount() says it is deprecated in favor of a createaccount permission check, so it should be easy.

I was very sure we also check the blocks when displaying the login page on GET, and don't even show it if the user is blocked, but after testing, that's not actually the case. The control flow there is SpecialCreateAccount::checkPermissions() -> AuthManager::probablyCanCreateAccount() -> Authority::probablyCan('createaccount',...) -> PermissionManager::getPermissionErrorsInternal('createaccount',...,RIGOR_QUICK) and block checks are skipped on RIGOR_QUICK.

Also, I would have expected action=query&meta=authmanagerinfo&amirequestsfor=create to do a permission check somewhere but it doesn't.

Change 822686 had a related patch set uploaded (by Sergio Gimeno; author: Sergio Gimeno):

[operations/mediawiki-config@master] Declare mediawiki.createaccount_blocked_user schema

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

I have left open patches for this task ( core 802483, WikimediaEvents 821700 (WIP), event-secondary 821711 and config 822686 ).

The WikimediaEvents hook handler will log local and global blocks via web interface or API. However the action=query&list=users&usprop=cancreate case is not yet handled since I did not know which would be the best way to do it. Calling the PermissionErrorsAudit hook from User::isBlockedFromCreateAccount() didn't seem conventional. Also the information extraction from Block/GlobalBlock objects have some nuances that can be improved.

In regards the schema definition maybe it would be good to consolidate some of the naming between (gerrit 820908 and gerrit 821711 ). For instance block_type has a different meaning on each of these.

I'm out of the office for the next two weeks, happy to follow-up when I'm back. Also comfortable if any engineer wants to pick it up from here since this task is marked as high priority.

Per this codesearch, only two methods do a createaccount permission check:

• AuthManager::probablyCanCreateAccount() - triggers the hook with RIGOR_QUICK (which does not actually trigger a block check in PermissionManager). Only caller is Special:CreateAccount, on GET.
• AuthManager::authorizeCreateAccount() - triggers the hook with RIGOR_SECURE. Only caller is Special:CreateAccount, on POST.

No extension/skin usage as far as I could tell (a bunch of things call Authority::isAllowed() but that only checks user group permissions and won't trigger the hook). So there shouldn't be any false positives.

Wrt the CheckBlocksSecondaryAuthenticationProvider issue, I can think of three approaches:

• Make CheckBlocksSecondaryAuthenticationProvider do a createaccount permission check instead of calling User::isBlockedFromCreateAccount() (which is deprecated and says such a permission check should be done instead). After thinking about it, this doesn't sit well with me. Something called CheckBlocks shouldn't do all kinds of arbitrary checks that are included in the permission check.
• Accept that the list=users API will block account creation without triggering the new hook, and detect it in a different way. APIQueryAfterExecute, I guess? Would be very hacky.
• Drop the block check from CheckBlocksSecondaryAuthenticationProvider::testUserForCreation() (or the whole method as that's all it does). I think this is my preferred solution. testUserForCreation() should check whether a given username is available for account creation. You can't register an account that already exist, and you can't block an account that doesn't exist (I guess it's technically possible but does not make sense), so there is no reason to check for blocks there. What the current code actually does is see whether the current user's IP is blocked, but it shouldn't do that - the response from that method should not depend on the current user at all.

Change 824142 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/core@master] [WIP] Do not check for IP blocks in testUserForCreation()

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

Change 824578 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/extensions/GlobalBlocking@master] Log block ID

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

Change 821711 merged by jenkins-bot:

[schemas/event/secondary@master] Instrument blocked account registration

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

Hi Security-Team, just a heads up that we intend to start merging these patches after branch cut today (so starting on August 30). tl;dr we're adding code to instrument when blocked users try to create an account, and in doing so, we made some bug fixes to block handling (see this patch). If you want for us to wait to merge that patch so that you have time to review it, please let Growth-Team know.

Change 822686 merged by jenkins-bot:

[operations/mediawiki-config@master] Declare mediawiki.accountcreation_block stream

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

Mentioned in SAL (#wikimedia-operations) [2022-08-30T07:17:22Z] <tgr@deploy1002> Synchronized wmf-config/InitialiseSettings.php: Config: [[gerrit:822686|Declare mediawiki.accountcreation_block stream (T306018)]] (duration: 04m 11s)

Change 802483 merged by jenkins-bot:

[mediawiki/core@master] PermissionManager: Add PermissionErrorAudit hook

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

Change 821700 merged by jenkins-bot:

[mediawiki/extensions/WikimediaEvents@master] WikimediaEventsHooks: log createaccount failed attempts due to a block

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

In T306018#8195330, @kostajh wrote:

Hi Security-Team, just a heads up that we intend to start merging these patches after branch cut today (so starting on August 30). tl;dr we're adding code to instrument when blocked users try to create an account, and in doing so, we made some bug fixes to block handling (see this patch). If you want for us to wait to merge that patch so that you have time to review it, please let Growth-Team know.

@kostajh et al -

Thanks for the heads up. @mmartorana is going to catch up on this bug and c824142 and attempt to qualify the risks related to merging the change set. In general, the Security-Team wouldn't block on a merge like this, but rather describe and qualify associated risks for developers and teams to either mitigate or accept when merging new/refactored code.

In T306018#8199717, @sbassett wrote:

In T306018#8195330, @kostajh wrote:

Hi Security-Team, just a heads up that we intend to start merging these patches after branch cut today (so starting on August 30). tl;dr we're adding code to instrument when blocked users try to create an account, and in doing so, we made some bug fixes to block handling (see this patch). If you want for us to wait to merge that patch so that you have time to review it, please let Growth-Team know.

@kostajh et al -

Thanks for the heads up. @mmartorana is going to catch up on this bug and c824142 and attempt to qualify the risks related to merging the change set. In general, the Security-Team wouldn't block on a merge like this, but rather describe and qualify associated risks for developers and teams to either mitigate or accept when merging new/refactored code.

Thanks, we will wait for your review before merging this patch.

In T306018#8195330, @kostajh wrote:

Hi Security-Team, just a heads up that we intend to start merging these patches after branch cut today (so starting on August 30). tl;dr we're adding code to instrument when blocked users try to create an account, and in doing so, we made some bug fixes to block handling (see this patch). If you want for us to wait to merge that patch so that you have time to review it, please let Growth-Team know.

Hi @kostajh and @Tgr - we have reviewed this patch, and we need some clarification on these points:

• Partial blocks are ignored for authentication, account creation and auto creation.

I don't see associated risk with removing account creation checks for IP-blocked accounts for auto creation in testUserForCreation(). low risk

• On $wgBlockDisablesLogin wikis IP blocks won't prevent login anymore.

I may have some concerns here. My understanding is that $wgBlockDisablesLogin was mainly meant for private wikis which need a mechanism to revoke reading privileges once a user is not active anymore. Removing these checks implies that all the locked out accounts will get login access back, allowing them to re-gain their privileges (read, edit, delete, etc), because sysops/admins may have applied a $wgBlockDisablesLogin kind of block to those accounts rather than removing privileges. medium risk

• On $wgBlockDisablesLogin wikis, blocks will now prevent account autocreation even if they are not configured to prevent account creation. The assumption is that on such wikis account creation is restricted via some means. This probably isn't necessary as blocks should also prevent the conditions needed for autocreation (e.g. log the user out centrally), but can serve as defense in depth. Along with the special-casing of auto creation, this means on such wikis any IP block will prevent auto creation, which is not great but seems not worth even more code complexity to avoid.

Since the impact of this change is rather low and the affected Wikis should be few, I would assign a risk of low.

• The action=query&list=users&usprop=cancreate API won't take blocks into account anymore.

Do you plan on sending out an announcement to wikitech-l related to these changes? This would also be beneficial to mitigate some risk.

In T306018#8222758, @mmartorana wrote:

• Partial blocks are ignored for authentication, account creation and auto creation.

I don't see associated risk with removing account creation checks for IP-blocked accounts for auto creation in testUserForCreation(). low risk

Note this is specific to partial blocks, ie. blocking users from editing specific pages and not the entire site. Sitewide IP blocks will continue to prevent autocreation (and also account creation, although that won't be done in this class anymore).

• On $wgBlockDisablesLogin wikis IP blocks won't prevent login anymore.

I may have some concerns here. My understanding is that $wgBlockDisablesLogin was mainly meant for private wikis which need a mechanism to revoke reading privileges once a user is not active anymore. Removing these checks implies that all the locked out accounts will get login access back, allowing them to re-gain their privileges (read, edit, delete, etc), because sysops/admins may have applied a $wgBlockDisablesLogin kind of block to those accounts rather than removing privileges. medium risk

Account blocks will continue to prevent login when $wgBlockDisablesLogin is set, the change is limited to IP blocks. I don't think IP blocks make sense as a mechanism to prevent login.

• The action=query&list=users&usprop=cancreate API won't take blocks into account anymore.

Do you plan on sending out an announcement to wikitech-l related to these changes? This would also be beneficial to mitigate some risk.

I wasn't planning to as I don't consider the API change a B/C break (the syntax of the API does not change, it still tells you whether the account can be created (for a slightly different definition of "can be created"), and the possibility of some error being returned by action=createaccount but not by list=users was always there, so clients should already be prepared to handle unexpected errors) and I don't think the other changes are significant. I can make an announcement though if you feel that's necessary.

Hi @Tgr, thank you for your clarifications.

Since we don't have any further concerns, feel free to deploy this patch.

Let us know if you need anything else.

Change 824142 merged by jenkins-bot:

[mediawiki/core@master] Fix block handling in CheckBlocksSecondaryAuthenticationProvider

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

This can move to QA, correct?

I suspect I noticed an error in the patch while working on related instrumentation, which might turn up in your QA: it has auto in its block_type enum, and I think that should be autoblock instead.

Change 832203 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[schemas/event/secondary@master] accountcreation: Adjust block type value from 'auto' to 'autoblock'

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

Change 832206 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[mediawiki/extensions/WikimediaEvents@master] BlockMetrics: Update to new event schema version

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

In T306018#8235044, @DLynch wrote:

I suspect I noticed an error in the patch while working on related instrumentation, which might turn up in your QA: it has auto in its block_type enum, and I think that should be autoblock instead.

Thanks for catching that, I made https://gerrit.wikimedia.org/r/832203 and https://gerrit.wikimedia.org/r/832206 to address the issue.

Change 832203 merged by jenkins-bot:

[schemas/event/secondary@master] accountcreation: Adjust block type value from 'auto' to 'autoblock'

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

Change 831969 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[mediawiki/extensions/WikimediaEvents@wmf/1.40.0-wmf.1] BlockMetrics: Update to new event schema version

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

Change 832206 merged by jenkins-bot:

[mediawiki/extensions/WikimediaEvents@master] BlockMetrics: Update to new event schema version

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

Change 831969 merged by jenkins-bot:

[mediawiki/extensions/WikimediaEvents@wmf/1.40.0-wmf.1] BlockMetrics: Update to new event schema version

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

Mentioned in SAL (#wikimedia-operations) [2022-09-14T10:24:39Z] <kharlan@deploy1002> Synchronized php-1.40.0-wmf.1/extensions/WikimediaEvents/includes/BlockMetrics/BlockMetricsHooks.php: Backport: [[gerrit:831969|BlockMetrics: Update to new event schema version (T306018)]] (duration: 03m 48s)