Page MenuHomePhabricator
Create Task
Maniphest T237296

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

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

SubjectRepoBranchLines +/-
Add PreliminaryCheckService and PSR-4 to extensionmediawiki/extensions/CheckUsermaster+311 -0
Customize query in gerrit

Related Objects
Search...

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.
• Niharika moved this task from Untriaged to Triage/To be Estimated on the Anti-Harassment board.Nov 4 2019, 10:24 PM
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
• Niharika moved this task from Triage/To be Estimated to Cards ready for development on the Anti-Harassment board.
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
• Niharika moved this task from Cards ready for development to The Letter Song on the Anti-Harassment board.Nov 7 2019, 10:15 PM
• Niharika edited projects, added Anti-Harassment (The Letter Song); removed Anti-Harassment.
dbarratt added a parent task: T237298: CheckUser 2.0: Display the data fetched on special page (preliminary check).Nov 7 2019, 10:18 PM
dmaza claimed this task.Nov 8 2019, 2:08 PM
dmaza moved this task from Ready 🎬 (ONLY IF YOU HAVE NO MORE CODE TO REVIEW) 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?

dbarratt subscribed.Nov 13 2019, 4:05 PM
In T237296#5658378, @dmaza wrote:

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. :)

• aezell added a comment.Nov 13 2019, 4:08 PM

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
In T237296#5660633, @aezell wrote:

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) :)

• aezell added a comment.Nov 13 2019, 4:10 PM

@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
In T237296#5660626, @dbarratt wrote:
In T237296#5658378, @dmaza wrote:

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

• aezell added a comment.Nov 13 2019, 4:13 PM

That works for me.

dbarratt added a comment.Nov 13 2019, 4:15 PM

@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 added a comment.Nov 15 2019, 4:00 PM
In T237296#5665416, @dmaza wrote:

@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.

gerritbot added a comment.Nov 15 2019, 7:25 PM

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

gerritbot added a project: Patch-For-Review.Nov 15 2019, 7:26 PM
dbarratt updated the task description. (Show Details)Nov 18 2019, 3:38 PM
gerritbot added a comment.Nov 20 2019, 8:08 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

dmaza moved this task from In Progress 💪 to Code Review 🔍 on the Anti-Harassment (The Letter Song) board.Nov 20 2019, 9:22 PM
dbarratt moved this task from Code Review 🔍 to QA/Testing 🐞 on the Anti-Harassment (The Letter Song) board.Nov 22 2019, 1:33 AM
gerritbot added a comment.Nov 22 2019, 1:45 AM

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

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

ReleaseTaggerBot added a project: MW-1.35-notes (1.35.0-wmf.8; 2019-11-26).Nov 22 2019, 2:00 AM
dom_walden subscribed.Nov 27 2019, 2:29 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.

dom_walden added a comment.Nov 27 2019, 2:35 PM

@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
In T237296#5697186, @dom_walden wrote:

@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

dbarratt added a subtask: T239682: CU 2.0: Namespace Service Name.Dec 3 2019, 4:16 AM
dbarratt closed subtask T239682: CU 2.0: Namespace Service Name as Resolved.Dec 3 2019, 3:35 PM
dom_walden mentioned this in T240585: "Undefined variable: globalUser" submitting Special:Investigate form without CentralAuth.Dec 12 2019, 4:38 PM
dom_walden mentioned this in T240586: Should hidden blocks be shown in Preliminary Check?.Dec 12 2019, 4:40 PM
dom_walden moved this task from QA/Testing 🐞 to Done (2019-20: Q2) on the Anti-Harassment (The Letter Song) board.Dec 12 2019, 5:46 PM

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.

• Niharika added a comment.Dec 17 2019, 10:10 PM
In T237296#5736940, @dom_walden wrote:

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
dbarratt added a comment.Jan 2 2020, 7:55 PM
In T237296#5749259, @Niharika wrote:

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).

Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL