Page MenuHomePhabricator

CheckUser 2.0: Create service and methods for new CU extension
Closed, ResolvedPublic3 Estimate Story Points

Description

This new service should:

Notes;

  • We could do this through the JobQueue if the queries prove to be expensive
  • Create the query and see what it looks like
  • Examine getting the data (or some of it) from the CentralAuth API. This may need a fall back for 3rd-party wikis or any WMF wiki without CentralAuth
    • CentralAuth's meta=globaluserinfo only supports retrieving a single user at a time. This would need to be updated to support more than one user at a time and with pagination, etc. (see T237505). However, it seems unreasonable to pass hundreds(?) of users to CentralAuth.
  • Pagination and filtering will be done in a later ticket

Details

Related Gerrit Patches:
mediawiki/extensions/CheckUser : masterAdd PreliminaryCheckService and PSR-4 to extension

Related Objects

Event Timeline

aezell created this task.Nov 4 2019, 6:34 PM
Restricted Application added subscribers: MGChecker, Aklapper. · View Herald TranscriptNov 4 2019, 6:34 PM
Niharika triaged this task as Medium priority.Nov 4 2019, 10:22 PM
Niharika removed a project: Epic.
dbarratt renamed this task from Create class and methods for new CU extension to Create service and methods for new CU extension.Nov 5 2019, 7:45 PM
dbarratt updated the task description. (Show Details)
Niharika set the point value for this task to 3.Nov 5 2019, 7:50 PM
dbarratt updated the task description. (Show Details)Nov 5 2019, 8:04 PM
dbarratt updated the task description. (Show Details)
Niharika renamed this task from Create service and methods for new CU extension to CheckUser 2.0: Create service and methods for new CU extension.Nov 5 2019, 8:06 PM
Niharika updated the task description. (Show Details)Nov 5 2019, 8:21 PM
dmaza claimed this task.Nov 8 2019, 2:08 PM
dmaza moved this task from Ready to In Progress on the Anti-Harassment (The Letter Song) board.
dmaza added a comment.Nov 12 2019, 9:35 PM

Considering that the use of internal API requests is discouraged I assume that we are mentioning CentralAuth API only if we can't run the db queries and instead we would load whatever we need from the client side using ajax. Am I wrong?

Considering that the use of internal API requests is discouraged I assume that we are mentioning CentralAuth API only if we can't run the db queries and instead we would load whatever we need from the client side using ajax. Am I wrong?

Right, if we called the API "internally" we would do it form the client with JavaScript.

Although (and I tried this with GraphQL extension and it works well), you can construct a FauxRequest and pass it to an API module directly... that would allow you to "call the API" without actually opening up another process in Apache. Though, MediaWiki doesn't support "sub requests" like that very well, so not sure how acceptable it would be. Then again, this is an extension, so do what you want. :)

I would prefer if we called from the client with JavaScript as opposed to the internal FauxRequest stuff.

dbarratt added a comment.EditedNov 13 2019, 4:09 PM

I would prefer if we called from the client with JavaScript as opposed to the internal FauxRequest stuff.

Based on what we found, I don't think we are going to do either. (see task description) :)

@dbarratt I see. That makes sense to not try to overload the API that way.

dmaza added a comment.Nov 13 2019, 4:11 PM

Although (and I tried this with GraphQL extension and it works well), you can construct a FauxRequest and pass it to an API module directly... that would allow you to "call the API" without actually opening up another process in Apache. Though, MediaWiki doesn't support "sub requests" like that very well, so not sure how acceptable it would be. Then again, this is an extension, so do what you want. :)

Based on FauxRequest docs it should not be used outside of testing.

We might get away with querying the db. Even from the client it could potentially spawn multiple api calls which might not be optimal either. Let's see where the queries gets us and worse case we can ask CheckUsers to limit their search to a smaller subset of potential users

That works for me.

@dmaza yeah I don't think the existing API helps anyways. I feel like we are going to end up making our own queries for the page / new API.

dmaza added a comment.Nov 14 2019, 9:53 PM

@Niharika What is the acceptance criteria for this task? Should ip users also be supported as part of this or is it supporting only registered accounts enough?

@Niharika What is the acceptance criteria for this task? Should ip users also be supported as part of this or is it supporting only registered accounts enough?

Only registered accounts as this is for the Preliminary check tab and we aren't showing any data for IP users in that tab.

Change 550944 had a related patch set uploaded (by Dbarratt; owner: Dmaza):
[mediawiki/extensions/CheckUser@master] WIP - CheckUser services

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

dbarratt updated the task description. (Show Details)Nov 18 2019, 3:38 PM

Change 550944 had a related patch set uploaded (by Dmaza; owner: Dmaza):
[mediawiki/extensions/CheckUser@master] Add CheckUserService and PSR-4 to extension

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

Change 550944 merged by jenkins-bot:
[mediawiki/extensions/CheckUser@master] Add PreliminaryCheckService and PSR-4 to extension

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

@dmaza Submitting the Special:Investigate form on a wiki without CentralAuth, I see in the logs:

[error] [f7d4d69c1c92ecf5b9e28a69] /core/index.php/Special:Investigate   ErrorException from line 83 of /var/www/html/core/extensions/CheckUser/src/PreliminaryCheckService.php: PHP Notice: Undefined variable: globalUser
#0 /var/www/html/core/extensions/CheckUser/src/PreliminaryCheckService.php(83): MWExceptionHandler::handleError(integer, string, string, integer, array)
#1 /var/www/html/core/extensions/CheckUser/src/PreliminaryCheckService.php(61): MediaWiki\CheckUser\PreliminaryCheckService->getGlobalUser(User)
#2 /var/www/html/core/extensions/CheckUser/src/PreliminaryCheckService.php(44): MediaWiki\CheckUser\PreliminaryCheckService->getAttachedWikis(User)
#3 /var/www/html/core/extensions/CheckUser/includes/specials/pagers/PreliminaryCheckPager.php(59): MediaWiki\CheckUser\PreliminaryCheckService->getPreliminaryData(array)
#4 /var/www/html/core/includes/pager/IndexPager.php(616): PreliminaryCheckPager->doQuery()
#5 /var/www/html/core/extensions/CheckUser/includes/specials/SpecialInvestigate.php(120): IndexPager->getNumRows()
#6 /var/www/html/core/includes/htmlform/HTMLForm.php(694): SpecialInvestigate->onSubmit(array, OOUIHTMLForm)
#7 /var/www/html/core/includes/htmlform/HTMLForm.php(586): HTMLForm->trySubmit()
#8 /var/www/html/core/includes/htmlform/HTMLForm.php(601): HTMLForm->tryAuthorizedSubmit()
#9 /var/www/html/core/extensions/CheckUser/includes/specials/SpecialInvestigate.php(41): HTMLForm->show()
#10 /var/www/html/core/includes/specialpage/SpecialPage.php(575): SpecialInvestigate->execute(NULL)
#11 /var/www/html/core/includes/specialpage/SpecialPageFactory.php(607): SpecialPage->run(NULL)
#12 /var/www/html/core/includes/MediaWiki.php(298): MediaWiki\Special\SpecialPageFactory->executePath(Title, RequestContext)
#13 /var/www/html/core/includes/MediaWiki.php(967): MediaWiki->performRequest()
#14 /var/www/html/core/includes/MediaWiki.php(530): MediaWiki->main()
#15 /var/www/html/core/index.php(46): MediaWiki->run()
#16 {main}

I don't know if this is something we need to support.

@Niharika At the moment, a user will be reported as blocked even if the block is hidden (and the check user does not have rights to see hidden blocks).

If the Preliminary Check is only meant to be public data (as suggested in T237039), should this be changed?

dmaza added a comment.Nov 27 2019, 5:01 PM

@dmaza Submitting the Special:Investigate form on a wiki without CentralAuth, I see in the logs:

[error] [f7d4d69c1c92ecf5b9e28a69] /core/index.php/Special:Investigate   ErrorException from line 83 of /var/www/html/core/extensions/CheckUser/src/PreliminaryCheckService.php: PHP Notice: Undefined variable: globalUser
#0 /var/www/html/core/extensions/CheckUser/src/PreliminaryCheckService.php(83): MWExceptionHandler::handleError(integer, string, string, integer, array)
#1 /var/www/html/core/extensions/CheckUser/src/PreliminaryCheckService.php(61): MediaWiki\CheckUser\PreliminaryCheckService->getGlobalUser(User)
#2 /var/www/html/core/extensions/CheckUser/src/PreliminaryCheckService.php(44): MediaWiki\CheckUser\PreliminaryCheckService->getAttachedWikis(User)
#3 /var/www/html/core/extensions/CheckUser/includes/specials/pagers/PreliminaryCheckPager.php(59): MediaWiki\CheckUser\PreliminaryCheckService->getPreliminaryData(array)
#4 /var/www/html/core/includes/pager/IndexPager.php(616): PreliminaryCheckPager->doQuery()
#5 /var/www/html/core/extensions/CheckUser/includes/specials/SpecialInvestigate.php(120): IndexPager->getNumRows()
#6 /var/www/html/core/includes/htmlform/HTMLForm.php(694): SpecialInvestigate->onSubmit(array, OOUIHTMLForm)
#7 /var/www/html/core/includes/htmlform/HTMLForm.php(586): HTMLForm->trySubmit()
#8 /var/www/html/core/includes/htmlform/HTMLForm.php(601): HTMLForm->tryAuthorizedSubmit()
#9 /var/www/html/core/extensions/CheckUser/includes/specials/SpecialInvestigate.php(41): HTMLForm->show()
#10 /var/www/html/core/includes/specialpage/SpecialPage.php(575): SpecialInvestigate->execute(NULL)
#11 /var/www/html/core/includes/specialpage/SpecialPageFactory.php(607): SpecialPage->run(NULL)
#12 /var/www/html/core/includes/MediaWiki.php(298): MediaWiki\Special\SpecialPageFactory->executePath(Title, RequestContext)
#13 /var/www/html/core/includes/MediaWiki.php(967): MediaWiki->performRequest()
#14 /var/www/html/core/includes/MediaWiki.php(530): MediaWiki->main()
#15 /var/www/html/core/index.php(46): MediaWiki->run()
#16 {main}

I don't know if this is something we need to support.

Yes, we should fix this. I'll send a patch asap. Thank you

The information you get in the Preliminary Check on Special:Investigate is pretty much the same as in Special:CentralAuth. I compared against the latter to check that the information was correct.

The only difference I noticed is if you have several local accounts with the same username which are not merged. On Special:Investigate, it just shows information for the username on the current wiki. On Special:CentralAuth, we show all accounts with that username. I don't know how likely this is to happen in real life.

I could only test this on my local version because the code to display the results is not merged yet. I had to cherry pick the patch from T237298.

Testing environment: Vagrant; MediaWiki 1.35.0-alpha (5564296); CheckUser 2.5 (da63908) 17:00, 22 November 2019.

I raised T237296#5697205 as T240586 and T237296#5697186 as T240585.

The information you get in the Preliminary Check on Special:Investigate is pretty much the same as in Special:CentralAuth. I compared against the latter to check that the information was correct.
The only difference I noticed is if you have several local accounts with the same username which are not merged. On Special:Investigate, it just shows information for the username on the current wiki. On Special:CentralAuth, we show all accounts with that username. I don't know how likely this is to happen in real life.

Good to know. I am surprised that there is a possibility for having multiple accounts with the same username. I'm not sure what the right behavior should be here. Hopefully it won't cause any problems.

I could only test this on my local version because the code to display the results is not merged yet. I had to cherry pick the patch from T237298.
Testing environment: Vagrant; MediaWiki 1.35.0-alpha (5564296); CheckUser 2.5 (da63908) 17:00, 22 November 2019.
I raised T237296#5697205 as T240586 and T237296#5697186 as T240585.

Awesome!

Niharika closed this task as Resolved.Dec 17 2019, 10:11 PM

I am surprised that there is a possibility for having multiple accounts with the same username.

Yeah I found out that if you have multiple wikis in a farm with different users then you install CentralAuth, you could have multiple users with the same user name. This is why CentralAuth provides a way to "attach" them together. (the user has to provide their password on each of the local wikis).