Page MenuHomePhabricator

Throw an error from UserGroupManager::addUserToGroup if called on a temporary user
Closed, ResolvedPublic3 Estimated Story Points

Description

This is for T330816: [Epic] Temporary users should not be assigned to user groups. See also T330816#8958447.

We're disabling this feature, not because it can't be done, but because we don't want it for the MVP of IP masking (see parent task).

What needs doing

  • Throw an error from UserGroupManager::addUserToGroup if called on a temporary user
  • Don't re-use the existing error for IP users, since it states that the user needs an ID, which a temporary user does have
  • Ensure that nowhere in MediaWiki core is calling UserGroupManager::addUserToGroup with a temporary user

Related Objects

StatusSubtypeAssignedTask
Resolvedkostajh
DeclinedNone
In ProgressNiharika
OpenNone
OpenTchanders
OpenNone
ResolvedSTran
ResolvedUmherirrender
Resolved AGueyte
Duplicate AGueyte
ResolvedSTran
ResolvedDreamy_Jazz
ResolvedSTran
ResolvedSTran
ResolvedSTran
ResolvedSTran
ResolvedSTran
Resolvedkostajh
ResolvedSTran
ResolvedTchanders
ResolvedSTran
ResolvedSTran
ResolvedSTran
Opensgrabarczuk
ResolvedSTran
ResolvedBUG REPORTDreamy_Jazz
ResolvedSTran
Resolved TThoabala
Resolved TThoabala
Resolved TThoabala
Resolved AGueyte
ResolvedBUG REPORT AGueyte
ResolvedBUG REPORT AGueyte
Resolved AGueyte
Resolved AGueyte
ResolvedCyndymediawiksim
DuplicateNone
OpenNone
ResolvedTchanders
ResolvedReleasedancy
ResolvedPRODUCTION ERRORZabe
Resolved AGueyte

Event Timeline

Change 932820 had a related patch set uploaded (by AGueyte; author: AGueyte):

[mediawiki/core@master] Throw an error from UserGroupManager::addUserToGroup if called on a temporary user

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

Change 932820 merged by jenkins-bot:

[mediawiki/core@master] Throw an error from UserGroupManager::addUserToGroup if called on a temporary user

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

dom_walden subscribed.

When trying to view Special:UserRights for a temporary user you see a message Temporary users cannot be added into groups.

specialuserrights_temp.png (415×697 px, 27 KB)

When trying to add or remove a group from a user via the API (e.g. https://de.wikipedia.beta.wmflabs.org/wiki/Spezial:ApiSandbox#action=userrights&format=json&user=*Unregistered%201062&remove=autoreview):

{
    "error": {
        "code": "userrights-no-tempuser",
        "info": "Temporary users cannot be added into groups.",
        ...
}

As the change is to throw an exception in the addUserToGroup function, which already throws exceptions in other circumstances, I assume that other places which call this function are already setup to handle exceptions being thrown from this function.

Test environment: https://de.wikipedia.beta.wmflabs.org MediaWiki 1.41.0-alpha (9a384e7) 13:21, 3 July 2023.

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

[mediawiki/core@master] Revert "Throw an error from UserGroupManager::addUserToGroup if called on a temporary user"

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

As the change is to throw an exception in the addUserToGroup function, which already throws exceptions in other circumstances, I assume that other places which call this function are already setup to handle exceptions being thrown from this function.

Thanks for pointing this out. It seems not to be the case, e.g. I got this error when attempting to assign a group to a temporary user from Special:UserRights.

I've reverted the commit for this reason.

We should keep working on this and make sure that we address this, from the acceptance criteria:

  • Ensure that nowhere in MediaWiki core is calling UserGroupManager::addUserToGroup with a temporary user

Change 935106 merged by jenkins-bot:

[mediawiki/core@master] Revert "Throw an error from UserGroupManager::addUserToGroup if called on a temporary user"

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

Change 935143 had a related patch set uploaded (by AGueyte; author: AGueyte):

[mediawiki/core@master] Prevent Special:UserRights to add temp users into groups

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

What has been done to complete this task:

  • Remove groups form when viewing a Temp Users from Special:UserRights When requesting to see rights for a temporary user, only a list of the actual rights of the user will be shown, but no longer the form with the possibility to add groups.
  • Add a fatal error when trying to save groups for a temp user from Special:UserRights The previous point prevents the case of adding groups to a temp user, but for safety reasons, an error will be thrown if a group is added to a temporary user from Special:UserRights
  • The API for special user rights also depends on the functions mentioned above, and a task to prevent these functions to be called from the API has been covered in T340458
  • Creating an exemption from UserGroupManager when attempting to save groups to a temp user The original function adding groups to a user from UserGroupManager is now throwing an exception if trying to add groups to a temp user.

To avoid throwing an exception, I have searched the codebase for this function UserGroupManager::addUserToGroup

Where is UserGroupManager::addUserToGroup called from

SpecialUserRights.php is calling UserGroupManager::addUserToGroup

Work done as explained above.

includes/installer/Installer.php

From this file, the function is called to create the first user account, and grant it sysop, bureaucrat, and interface-admin rights. Therefore, I believe no changes are needed.

includes/user/User.php

User::addGroup calls UserGroupManager::addUserToGroup but has been deprecated.
It is now replaced with UserRightsProxy::addGroup which isn't being used on core.
Therefore, I believe no changes are needed.

The following files are maintenance files that use the UserGroupManager::addUserToGroup to add the bot group to a system user, Therefore, I believe no changes are needed.

maintenance/cleanupSpam.php
maintenance/deleteDefaultMessages.php
maintenance/deleteEqualMessages.php
tests/phpunit/includes/api/ApiStashEditTest.php

The following files are test files that use the UserGroupManager::addUserToGroup. They test the functions that actually add groups to users. They do not save in the database.
Therefore and with the work done previously to prevent temp users to save groups and use certain functionality, I believe no changes are needed.

tests/phpunit/includes/upload/UploadFromUrlTest.php
tests/phpunit/includes/user/UserGroupManagerTest.php
tests/phpunit/integration/includes/user/UserRightsProxyTest.php

Change 935143 merged by jenkins-bot:

[mediawiki/core@master] Prevent saving groups to a Temp User

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

@AGueyte Tested the following and I cannot add/remove rights on temp users as seen in the screenshots below, which all resulted in "Temporary users do not have groups.". I will move this to Done, thanks for all your work!

Userrights added/removed: bot, bureaucrat, checkuser, checksuer-temporary-account, electionadmin, interface-admin, no-ipinfo, steward, suppress, sysop, upwizcampeditors
Environment: Local
Old patch- 1bb83857973c5bc9cf0fdb0841baa9c45e1e1384
Current patch- 6853d8b390c3b12aa554e5f6dcbf65b3adc492e7

Special:UserRights -w/temp user

OldPatchCurrenPatch
T340468_IPMasking_TempUserRights_Old.png (988×3 px, 267 KB)
T340468_IPMasking_TempUserRights_Current.png (610×3 px, 157 KB)

APISandBox -w/temp user
Result

T340468_IPMasking_TempUserRights_CurrentConfigResult.png (771×3 px, 247 KB)

Sample Config
T340468_IPMasking_TempUserRights_CurrentConfig.png (1×3 px, 272 KB)

Change 940043 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Zabe):

[mediawiki/core@master] SpecialUserRights: Check for username to be temporary

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

Change 940126 had a related patch set uploaded (by Dreamy Jazz; author: Zabe):

[mediawiki/core@wmf/1.41.0-wmf.18] SpecialUserRights: Check for username to be temporary

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

Change 940043 merged by jenkins-bot:

[mediawiki/core@master] SpecialUserRights: Check for username to be temporary

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

Change 940126 merged by jenkins-bot:

[mediawiki/core@wmf/1.41.0-wmf.18] SpecialUserRights: Check for username to be temporary

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

Mentioned in SAL (#wikimedia-operations) [2023-07-20T12:12:13Z] <zabe@deploy1002> Started scap: Backport for [[gerrit:940126|SpecialUserRights: Check for username to be temporary (T340468 T342322)]]

Mentioned in SAL (#wikimedia-operations) [2023-07-20T12:13:49Z] <zabe@deploy1002> zabe and dreamyjazz: Backport for [[gerrit:940126|SpecialUserRights: Check for username to be temporary (T340468 T342322)]] synced to the testservers mwdebug2002.codfw.wmnet, mwdebug2001.codfw.wmnet, mwdebug1001.eqiad.wmnet, mwdebug1002.eqiad.wmnet, and mw-debug kubernetes deployment (accessible via k8s-experimental XWD option)

Mentioned in SAL (#wikimedia-operations) [2023-07-20T12:20:35Z] <zabe@deploy1002> Finished scap: Backport for [[gerrit:940126|SpecialUserRights: Check for username to be temporary (T340468 T342322)]] (duration: 08m 22s)