Assuming this was moved here because we decided to do:

check for now that the most recent IP is being correctly down on Special:Log, and keep this task open for when the migration is complete

I've tested and think this is the case, so I'll pass to QA to confirm.

In T326393#8630551, @Tchanders wrote:

Assuming this was moved here because we decided to do:

check for now that the most recent IP is being correctly down on Special:Log, and keep this task open for when the migration is complete

I've tested and think this is the case, so I'll pass to QA to confirm.

@Tchanders Have we made any code changes especially for this task? Or are we relying on current functionality that has been tested elsewhere?

I have done some basic testing of the button on Special:Log. It just makes a request to /w/rest.php/checkuser/v0/temporaryaccount/<username>?limit=1.

I don't think there is anything more to test here at the moment. When we say we want to keep this task open, should we put it on the backlog?

Assuming number 2 has been selected, then it makes sense to add T330158 as a subtask as that task once completed will mean that wikis will have log events written to the cu_log_event table.

Removing this as a subtask of the epic, since it's not blocking whether we consider it done.

Showing log-related IP addresses is an enhancement that will be made possible by the cool work being done in the subtasks of this, not a blocker.

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

[mediawiki/extensions/CheckUser@master] WIP Use cu_log_event table to look up temporary account IPs for log entries

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

Testing notes

Config

• Set $wgAutoCreateTempUser['enabled'] = true;
• Set $wgGroupPermissions['sysop']['checkuser-temporary-account'] = true;
• Set $wgCheckUserEventTablesMigrationStage to something that contains SCHEMA_COMPAT_READ_NEW (see https://gerrit.wikimedia.org/g/mediawiki/core/+/65dacd5748324612493af6f2e049e83983f50a07/includes/Defines.php#250 )

Setup

• As a temporary user, upload some files (or do some other logged action) from different IP addresses
• As an admin, check that IP Reveal is enabled in Special:Preferences

Testing steps

• As an admin, visit Special:Log and click on a reveal button for the temporary user who performed the logged actions
• Before this task, the most recent IPs were revealed next to each log line. After this task, the IPs used for each upload (or action) are revealed next to each log line

Regression testing

(Just some ideas)

• Make some edits to an article from the same temporary account, using different IPs
• As an admin, visit the history page from the article to ensure no regressions
• As an admin, visit Special:Contributions for the temporary user to ensure no regressions
• As an admin, visit Special:RecentChanges and check the IP addresses of both the revisions and log entries for the temporary user

When I test by uploading, then visiting Special:Log, I see this:

The problem is that the page creation log lines all have a log ID, but they don't have log entries in cu_log_event. We could get round it by making further API calls to the endpoint that just returns the most recent IP, if nothing is found by the log endpoint. But I'm wondering if it's a temporary problem to do with the DB migration, or whether it's likely to persist.

@Dreamy_Jazz are there any plans to add page creations to the cu_log_event table?

In T326393#9093732, @Tchanders wrote:

When I test by uploading, then visiting Special:Log, I see this:

The problem is that the page creation log lines all have a log ID, but they don't have log entries in cu_log_event. We could get round it by making further API calls to the endpoint that just returns the most recent IP, if nothing is found by the log endpoint. But I'm wondering if it's a temporary problem to do with the DB migration, or whether it's likely to persist.

@Dreamy_Jazz are there any plans to add page creations to the cu_log_event table?

Summary of chat with @Dreamy_Jazz :

• Not all log events make it to CheckUser, so others might be affected
• CheckUser stores events that are saved as recent changes (Hooks::onRecentChange_save)

I don't think there's an easy way to tell whether a given log entry displayed on Special:Log has an entry in cu_log_event. This table is populated by a handler for the onRecentChange_save hook. For log entries, this hook is run when ManualLogEntry::publish or RecentChange::notifyLog are called, and these are called from many places in MW core and extensions. There's no way to tell from the HTML whether a recent change was published for a given log line.

We could leave (unavailable), since it's technically true: we didn't store the IP for that logged event. Or we could do a follow-up API call for recent IPs if one if unavailable. I've spun out that decision into another task (T345068), so this one is ready for review again.

In T326393#9122994, @Tchanders wrote:

I don't think there's an easy way to tell whether a given log entry displayed on Special:Log has an entry in cu_log_event. This table is populated by a handler for the onRecentChange_save hook. For log entries, this hook is run when ManualLogEntry::publish or RecentChange::notifyLog are called, and these are called from many places in MW core and extensions. There's no way to tell from the HTML whether a recent change was published for a given log line.

Agreed, as I can see no reliable way to determine this either.

We could leave (unavailable), since it's technically true: we didn't store the IP for that logged event. Or we could do a follow-up API call for recent IPs if one if unavailable. I've spun out that decision into another task (T345068), so this one is ready for review again.

I would be concerned about giving the IP without telling the user that this isn't the IP associated with this exact log ID. I will note this at that task.

In T326393#9122994, @Tchanders wrote:

..., so this one is ready for review again.

Just in case you haven't seen, I did leave a -1 on that patch.

In T326393#9124071, @Dreamy_Jazz wrote:

In T326393#9122994, @Tchanders wrote:

..., so this one is ready for review again.

Just in case you haven't seen, I did leave a -1 on that patch.

Thanks - had missed that.

Change 948627 merged by jenkins-bot:

[mediawiki/extensions/CheckUser@master] Use cu_log_event table to look up temporary account IPs for logs

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

@Tchanders I feel like I have asked this before for the /revisions/ endpoint, but just in case. Should we do any permissions checking for the /logs/ endpoint? For example, checking that the user has permissions to see the performer of the logged action?

Also, I see that we pass a limit parameter in the URL, but it does not seem to be used by the API. Changing it makes no difference to the response. I don't know if it matters. The limit seems to be how long a URL the server will accept.

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

[mediawiki/extensions/CheckUser@master] ipReveal.js: Don't pass limit param to revisions and logs APIs

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

@dom_walden Thanks for raising these!

@Tchanders I feel like I have asked this before for the /revisions/ endpoint, but just in case. Should we do any permissions checking for the /logs/ endpoint? For example, checking that the user has permissions to see the performer of the logged action?

You're right, looks like we did discuss this on T324603, but we didn't decide whether IPs should be visible if the revision is not. My thoughts:

• As long as the performer is visible, the IP address should be revealable
• If the performer is removed (via Special:RevisionDelete, since it's not possible via Special:Block), the IP should possibly technically be hidden too... However, I think the risk here is small since (1) I doubt a temporary user name would get hidden since it's just autogenerated, (2) if the user name is hidden, the reveal button won't get added in the UI, and (3) although the user could query the API directly, they'd need to send the user name as a request parameter, which they shouldn't be able to do if it was hidden, since they wouldn't know it.

I think we can leave this as is, unless @Niharika disagrees.

Also, I see that we pass a limit parameter in the URL, but it does not seem to be used by the API. Changing it makes no difference to the response. I don't know if it matters. The limit seems to be how long a URL the server will accept.

Looks as though this was just added by accident then propagated. I've removed it in the latest patch.

Change 955353 merged by jenkins-bot:

[mediawiki/extensions/CheckUser@master] ipReveal.js: Don't pass limit param to revisions and logs APIs

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

Moved back to QA following the merge of the above change.

Thanks. The limit is not longer passed to the API endpoint. I will move this to Done.