Page MenuHomePhabricator

Wikitech->Gerrit account block and unblock has stopped working
Closed, ResolvedPublic

Description

The debug log from wmf-config/wikitech.php for WikitechGerritBan shows:

Gerrit unblock of fomafix failed with status 404

As well as various entries like:

Gerrit block of *** failed with status 404
Gerrit block of *** failed with status 404
Gerrit block of *** failed with status 404
Gerrit block of *** failed with status 404

https://logstash.wikimedia.org/goto/8341ab7923ae0588624182d340c2e569

The code in question (source of wmf-config/wikitech.php, thanks @Reedy):

// the specified HTTP method (PUT to enable, DELETE to disable)
function wmfGerritSetActive(
	string $gerritUsername,
	string $gerritPassword,
	string $username,
	string $httpMethod
) {
	$gerritUrl = 'https://gerrit.wikimedia.org';
	$ch = curl_init(
		"{$gerritUrl}/r/a/accounts/" . urlencode( $username ) . '/active'
	);
	curl_setopt( $ch, CURLOPT_RETURNTRANSFER, true );
	curl_setopt( $ch, CURLOPT_CUSTOMREQUEST, $httpMethod );
	curl_setopt(
		$ch, CURLOPT_USERPWD,
		"{$gerritUsername}:{$gerritPassword}"
	);

Event Timeline

At glance, it seems /r/a/accounts might be a mistake that shoudl be /r/accounts, given the examples and documentation:

https://gerrit.wikimedia.org/r/Documentation/rest-api-accounts.html#set-active

In particular since the simple examples for anonymous read queries such as https://gerrit.wikimedia.org/r/accounts/?q=name:Krinkle work as-is. Upon closer inspection, I understand that /a/ is an optional prefix that puts the API in authenticated mode, where HTTP authentication username/password is accepted. This seems to check out. My personal account doesn't have the rights to enable/disable accounts, but at least the API appears to exist at this address since it yields HTTP 401 and not 404:

$ curl -X PUT 'https://gerrit.wikimedia.org/r/a/accounts/fomafix/active' -i -u 'krinkle:***'
HTTP/1.1 401 Unauthorized

I tried doing the same with the production credentials from wikitech/labweb's private settings, using the dedicated bot account for the purpose (managed by @thcipriani it seems). And this indeed yields an HTTP 404:

$ curl -X PUT 'https://gerrit.wikimedia.org/r/a/accounts/fomafix/active' -i -u '***'
HTTP/1.1 404 Not Found

But, not because the API doesn't exist. Rather the reason is in the body, and it has to do with the input given:

HTTP/1.1 404 Not Found

Account 'fomafix' only matches inactive accounts.
To use an inactive account, retry with one of the following exact account IDs:
 562: Fomafix <fomafix@googlemail.com>

I tried the following:

$ curl PUT /r/a/accounts/fomafix/active
HTTP 404
Account 'fomafix' only matches inactive accounts.

$ curl PUT /r/a/accounts/Fomafix/active
HTTP 404
Account 'Fomafix' only matches inactive accounts.

$ curl PUT /r/a/accounts/fomafix@googlemail.com/active
HTTP 404
Account 'fomafix@googlemail.com' only matches inactive accounts.

$ curl /r/a/accounts/Fomafix+<fomafix@googlemail.com>/active
HTTP 404
Account 'Fomafix <fomafix@googlemail.com>' only matches inactive accounts.

$ curl /r/a/accounts/562:+Fomafix+<fomafix@googlemail.com>/active
HTTP 404
Account '562: Fomafix <fomafix@googlemail.com>' only matches inactive accounts.

$ curl /r/a/accounts/562/active
HTTP/1.1 201 Created
""

So, it appears it currently only works via Gerrit's own account number (which afaik is not known to LDAP).

The wmfGerritSetActive() function is invoked by MediaWiki hooks BlockIPComplete and UnblockUserComplete. The hooks are being passed the MediaWiki username normalized with strtolower.

As Timo found out, is confused by the all lower case id fomafix and hint at using a more specific one:

Account 'fomafix' only matches inactive accounts.
To use an inactive account, retry with one of the following exact account IDs:
562: Fomafix <fomafix@googlemail.com>

It is essentially expecting Fomafix.

I think it is an aftermath of us switching Gerrit LDAP system to use lower case (ldap.localUsernameToLowerCase):

If set, it must be ensured that the local usernames for all existing accounts are converted to lower case, otherwise a user that has a local username that contains upper case characters will not be able to login anymore.
The local usernames for the existing accounts can be converted to lower case by running the server program LocalUsernamesToLowerCase.
Please be aware that the conversion of the local usernames to lower case can’t be undone.
For newly created accounts the local username will be directly stored in lower case.

Which caused a bit of havoc at the time (T197083, T216605 and child tasks, upstream https://bugs.chromium.org/p/gerrit/issues/detail?id=9256 ).

When looking at the All-Users.git repository in refs/meta/external-id, there are two local accounts:

$ git grep -i gerrit:fomafix
07/9fa52b1cb259788fe8c2549b62ae444faaf324:[externalId "gerrit:Fomafix"]
e6/525681d15714c1b8fe6dcfe96d4ade27c9ee9d:[externalId "gerrit:fomafix"]

Accoss all accounts there are 199 with an upper case letter:

$ git grep 'gerrit:.*[A-Z]'|wc -l
199

Which I guess would end up facing a similar problem.

If I remember properly we have to delete the account the local accounts (gerrit: prefixed) which have upper case letters when they have an equivalent in all lower case. Maybe the LocalUsernamesToLowerCase script takes care of that. It says:

The program will automatically remove duplicates where the username differs only in case but all other attributes are identical.

From /a/accounts/Fomafix/external.ids

{
  "identity": "mailto:fomafix@googlemail.com",
  "email_address": "fomafix@googlemail.com",
  "trusted": true,
  "can_delete": true
},
{
  "identity": "username:fomafix",
  "trusted": true
},
{
  "identity": "gerrit:Fomafix",
  "trusted": true,
  "can_delete": true
},
{
  "identity": "gerrit:fomafix",
  "email_address": "fomafix@googlemail.com",
  "trusted": true,
  "can_delete": true
}

My understanding is gerrit:Fomafix should be deleted.

We should had all accounts fixed via T216605 which used the following script:

1#!/usr/bin/env bash
2#
3# Gerrit Username To Lowercase
4# =====================
5#
6# Script for converting gerrit:-scheme usernames to lowercase directly
7# in a checkout of refs/meta/external-ids of the All-Users git repo.
8#
9# Copyright: Tyler Cipriani <tcipriani@wikimedia.org> 2019
10# License: GPLv3+
11
12# Utility logging methods
13info() {
14 printf '[INFO] %s' "$@"
15}
16println() {
17 printf '%s\n' "$@"
18}
19
20# Convert username with gerrit: schema to lowercase
21#
22# accepts a username as a mixed case string without a schema by:
23#
24# 1. Convert username mixed case to username lowercase
25# 2. Compute sha1sum of lowercase schema for use as new file name
26# 3. Find the old file name using git grep
27# 4. Git move the old file to the new file path (computed with sha1sum)
28# 5. Replace the username mixed-case in the new file with username lowercase
29#
30# param username: string, mixed case
31usernameToLower() {
32 local username username_lower shasum new_file old_file
33
34 username="$1"
35 username_lower="${username,,}"
36
37 shasum=$(printf "gerrit:%s" "${username_lower}" | shasum -a 1)
38
39 new_file=$(printf '%s/%s\n' "${shasum:0:2}" "${shasum:2:38}")
40 old_file=$(git grep --full-name --files-with-matches "\"gerrit:${username}\"")
41
42 if [ -f "$new_file" ]; then
43 println "The new file '${new_file}' exists!!!! for '${username}'. Aborting!"
44 exit 1
45 fi
46
47 git mv "$old_file" "$new_file"
48
49 # Change username to lowercase in new file
50 sed -i "s/gerrit:${username}/gerrit:${username_lower}/" "$new_file"
51}
52
53# Find any gerrit:-schema users with capital letters
54# Look to see if there is a lowercase version
55# If not, convert user to lowercase
56main() {
57 while read -r user; do
58 # Grep for lowercase user
59 if git grep "\\[externalId \"gerrit:${user,,}\"\\]" &>/dev/null; then
60 continue
61 fi
62 info "Converting ${user}..."
63 usernameToLower "$user"
64 println "DONE!"
65 done < <(git grep -P 'gerrit:.*[A-Z]+.*' | sed -e 's/.*:\[externalId "gerrit:\(.*\)"]/\1/')
66}
67
68main "$@"

It finds any local username (gerrit:XXX) which have an upper case letter and convert it to all lower case IF there is not an existing lower case user already. The script should be adjusted to support deleting the dupe mixed case account.

Looking at my own account, via https://gerrit.wikimedia.org/r/settings/#Identities I see gerrit:marcoaurelio the only identity (including other emails I have used for patch attribution). Since this (WikitechGerritBan) is still erroring, is there any unresolved issues that need to be addressed before fixing this Task?

hashar claimed this task.

@MarcoAurelio yes it is fixed per my earlier comment T306297#7867248

The issue was due to the mixed case, most cases got fixed by T216605 and the remaining one (including Fomafix from this task) got fixed as part of the Gerrit 3.5 upgrade with T323097. I guess I forgot to mark this one resolved ;)