Page MenuHomePhabricator

Access level changes are being logged spuriously
Closed, ResolvedPublicBUG REPORT

Description

Problem

Following T292842: Log when the user's access level changes, logs are sometimes generated when a user's access level hasn't changed.

Steps to reproduce:

  1. Log in as a user who has rights to view IPInfo data and IPInfo logs, and who has enabled IPInfo, including accepting the user agreement
  2. Open Special:Log/ipinfo
  3. Open Special:Contributions/<some ip> and view the IP information (expand the infobox if necessary)
  4. Refresh Special:Log

Expected: No new log lines saying that the user enabled/disabled access (though maybe 1 log line saying that they viewed the data)
Actual: Several new log lines saying something like: Admin disabled their own access to IPInfo

This is only one example of how to reproduce; there may be others.

Cause

In PreferencesHandler::onSaveUserOptions we check whether to log:

$isEnabled = isset( $originalOptions[ 'ipinfo-enable' ] ) &&
	isset( $originalOptions[ 'ipinfo-use-agreement' ] ) &&
	$originalOptions[ 'ipinfo-enable' ] &&
	$originalOptions[ 'ipinfo-use-agreement' ];
$willEnable = isset( $modifiedOptions[ 'ipinfo-enable' ] ) &&
	isset( $modifiedOptions[ 'ipinfo-use-agreement' ] ) &&
	$modifiedOptions[ 'ipinfo-enable' ] &&
	$modifiedOptions[ 'ipinfo-use-agreement' ];
if ( $isEnabled !== $willEnable ) {
	$logger = $this->loggerFactory->getLogger();
	if ( $willEnable ) {
		$logger->logAccessEnabled( $user );
	} else {
		$logger->logAccessDisabled( $user );
	}
}

If either of $modifiedOptions[ 'ipinfo-enable' ] or $modifiedOptions[ 'ipinfo-use-agreement' ] are not set, the user is not disabling the feature, but $willEnable will be false, so $isEnabled !== $willEnable will pass and a log will be made.

Solution

We need to check explicitly for the user disabling IPInfo, rather than assuming that not enabling implies disabling. We already do something like this further up the function: https://gerrit.wikimedia.org/g/mediawiki/extensions/IPInfo/+/d13f6e60beaca0656e522a5a73a69143d954ba47/src/HookHandler/PreferencesHandler.php#125

Event Timeline

Restricted Application added a subscriber: Aklapper. ยท View Herald TranscriptMar 4 2022, 5:06 PM
Tchanders added a subscriber: Niharika.

@Niharika This seems important enough to block testwiki deployment, so working on it now

Change 768135 had a related patch set uploaded (by Tchanders; author: Tchanders):

[mediawiki/extensions/IPInfo@master] Fix logging of changes to a user's IPInfo access

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

Change 768135 merged by jenkins-bot:

[mediawiki/extensions/IPInfo@master] Fix logging of changes to a user's IPInfo access

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

Niharika changed the subtype of this task from "Task" to "Bug Report".Mar 8 2022, 4:54 PM
dom_walden added a subscriber: dom_walden.

I cannot reproduce the original bug.

I was trying to find circumstances where a user's access level is changed without it being logged.

I found a couple of circumstances, but I don't think they are important:

  • If a user has agreed to the user agreement (ipinfo-use-agreement) and then gets their ipinfo rights removed
    • This will not be logged as a "disable" access level change until the user saves Special:Preferences
    • If they do not save Special:Preferences and they have ipinfo rights granted again, nothing will be logged. Their ipinfo options will remain as they were before they had the right removed (i.e. enabled).
  • If a user agrees to the user agreement and then disables the IPInfo beta feature globally, this will not be logged as a "disable" access level change
    • Until the user saves Special:Preferences
  • They can make a particular API request (see note [1] below) which will be logged as an "enable" access level change, even though technically they have not enabled their access

Testing included:

  • Testing what is logged when we transition from every combination of IPInfo user options to every other combination
    • We only log an "enable" access level change when we set ipinfo-use-agreement to 1 in the database
    • This testing could only be done locally and did not include global preferences
    • FYI resetting the user's options via the API or UI will be logged as a "disable" access level change (if they previously had IPInfo enable)
  • Testing what happens when you have various connected wikis with various combinations of options and you enable/disable the beta feature globally
  • Testing auto-enroll option on the beta features tab
  • Testing sending invalid options to the API
    • e.g. trying to enable IPInfo when you don't have the right

Test environments: various versions of beta and local docker

Notes:

  1. If they enable the IPInfo beta feature and then send this API request:
{
	"action": "options",
	"change": "ipinfo-beta-feature-enable|ipinfo-enable=1|ipinfo-use-agreement=1",
        ...
}

This is logged as an "enable" access level change. However, in the database ipinfo-enable and ipinfo-use-agreement are set to 1 and ipinfo-beta-feature-enable will be cleared. Therefore, they will not have access to IPInfo.

Thanks for the thorough testing, Dom!