Ideally, no OTP entered (see also T150903: Alert sre/security on many 2FA failures). Not sure how we would do that though.

Change 469878 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/OATHAuth@master] Add some logging of OATHAuth actions

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

Some of our security-related logging adds whether the targeted user is privileged (is an admin, steward, admin at another wiki etc), other (like these) doesn't. Not much of a problem in the short term, as unprivileged users cannot currently use OATHAuth, but eventually it should be improved IMO. Either we should move the logic into User::getPrivilegedGroups (with some way for auth extensions like CentralAuth to hook into it), or the isPrivileged and privilegedGroups fields should be added in a custom log processor added in our config, so they are present in all log events.

Any chance someone can review my patch please?

I know a few bits of it are a bit clunky

Any objections to making this task public? There is nothing sensitive about logging, and private patches tend to linger forever without getting review / getting deployed.

This task already is public.

I

In T151010#4751687, @Bawolff wrote:

This task already is public.

And the patch is in gerrit

Eh, sorry. This new task type got me all confused.

Change 469878 merged by jenkins-bot:
[mediawiki/extensions/OATHAuth@master] Add some logging of OATHAuth actions

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

Where does that is supposed to log to? Logstash? On-wiki log that is accessible to functionaries (if we want to do T210919 and T209749)? If so, then the messages shall be localizable and not be hardcoded in code.

This is for server side logging

I checked out the patch to mwdebug1002 and had no problem adding, removing or using a 2FA key on Wikitech. Also the error seems not at all related to the patch. I'm not sure what to do here, short of re-deploying and live-debugging if the problems reoccur.

In T151010#4829718, @Tgr wrote:

I checked out the patch to mwdebug1002 and had no problem adding, removing or using a 2FA key on Wikitech. Also the error seems not at all related to the patch. I'm not sure what to do here, short of re-deploying and live-debugging if the problems reoccur.

Seems sane to me. Could be something specific to the host(s) running wikitech

Change 480274 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/OATHAuth@master] Re-instate "Add some logging of OATHAuth actions"

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

OTOH it does not seem it logged anything, either. Maybe I messed up. I'll retry the test later today.

So it does not log anything because X-Wikimedia-Debug does not work on wikitech. Duhh. On checkuserwiki when I try to login with the patch checked out on the debug host I get Call to a member function info() on a non-object (null) for the $this->logger->info() call. WTF?

In CLI mode the config seems OK:

tgr@mwdebug1002:~$ PHP=php7.0 mwscript shell.php --wiki=checkuserwiki
>>> $k = OATHAuthKey::newFromRandom()                                                                                                                                                                       => OATHAuthKey {#2452}
>>> sudo $k->logger->name
=> "authentication"
>>> sudo $k->logger->handlers
=> [
     Monolog\Handler\WhatFailureGroupHandler {#168},
   ]
>>> sudo $k->logger->handlers[0]->handlers
=> [
     MediaWiki\Logger\Monolog\LegacyHandler {#169},
     MediaWiki\Logger\Monolog\SyslogHandler {#170},
   ]
>>> sudo $k->logger->info( 'test', [ 'test-foo' => 'bar' ] )
=> true

...and that last test does show up in Logstash.

The null error is deterministic. (actually it's not, now I get the Argument must be an instance of array, null given error instead) When I replace $this with \MediaWiki\Logger\LoggerFactory::getInstance( 'authentication' ), (which is the value it should be set to in the constructor) things work (and logs show up in Logstash).

See T210696#4830215.

In hindsight that should have been obvious: if a property is null even though absolutely no code path leaves it null, chances are it disappears during serialization. I got too invested into an alternative theory (that there is some non-fatal error PHP recovers from by exiting the method without returning anything, and since this happens during logger creation logging gets messed up and the error is not logged) to think of that.

Change 480274 merged by jenkins-bot:
[mediawiki/extensions/OATHAuth@master] Re-instate "Add some logging of OATHAuth actions"

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