Page MenuHomePhabricator

[S] Investigate: Creating temp user on non-EditPage actions
Closed, ResolvedPublic

Description

Background

Currently, a temporary user is created if the first action performed by an anonymous user is edit (more specifically, if the action is performed via EditPage).

However, any other action performed first could cause an anonymous user to be created, stored in the actor table using the IP address as their name. If the action is a logged action, their IP address will still appear in the UI.

An example from core is uploading; an example from an extension is posting to a Flow board.

What should we do

We should specify the other actions in the configuration AutoCreateTempUser['actions'].

However, we also need to implement creating a temp user where these actions are performed. There is a TempUserCreator service which is called from EditPage, but EditPage also does a lot of other work, such as creating unsaved and placeholder temp users for permissions checks etc, for workflow prior to save (e.g. preview).

Questions:

  • What other actions should we create a temp user from? (Loggable actions should be flagged by searching for ManualLogEntry.)
  • Do the other actions need to do much more than calling TempUserCreator::create? Anything that we should factor out to somewhere common?
  • Do we need to re-implement temp user creation for each action, or is there somewhere common we should do this from?
  • Can we provide an API for creating temporary users, for JS-only features that need to create a new user?

Related Objects

Event Timeline

EditPage also does a lot of other work, such as creating unsaved and placeholder temp users for permissions checks etc, for workflow prior to save (e.g. preview).

Expanding a bit on what EditPage does:

  1. For permissions checks, creates a placeholder temporary user (UserFactor::newTempPlaceholder)
  2. For displaying a temporary user name in a preview, acquires a name (TempUserCreator::acquireAndStashName) and creates a temporary user without saving it (UserFactory::newUnsavedTempUser)
  3. On saving an edit, saves a temporary user (TempUserCreator::create, using the acquired name)
  4. Alters the post edit cookie, so that an explanatory popup can be added (mediawiki.action.view.postEdit)

Note that (1) is necessary if the following are set:

$wgGroupPermissions['*']['edit'] = false;
$wgGroupPermissions['temp']['edit'] = true;

...otherwise permissions checks will fail on the IP user before a temporary user is ever created.

Editing APIs

They will probably at least need to do (1), e.g. T342770: Can't edit any page via visual editor while not logged into an account or a temporary account was caused by VisualEditor not doing (1). As part of that task ApiEditPage was updated to do (1).

If there are any editing APIs that don't fall through to EditPage, they would also need to do (3).

All actions

All actions will probably need to do at least (1) and (3).

If there are associated previews that display a user name, they would need (2).

Depending on the feature, (4) may be required for a better user experience.


Is there anything we can do to centralise some of this?

  • EditPage and ApiEditPage currently create a new unsaved temp user to check permissions. Does this need repeating everywhere? PermissionManager::getPermissionErrorsInternal will create a placeholder temporary user for permissions checks of actions that cause temp user autocreation, but only currently for RIGOR_QUICK.
  • The temp user popup is implemented in mediawiki.action.view.postEdit.js - maybe we should put it somewhere reusable.

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

[mediawiki/core@master] DNM Demo: Creating a temporary user for 'upload' action

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

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

[mediawiki/core@master] DNM Demo: Creating a temporary user for 'upload' action

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

This is an example of what I think is the minimum that would need to be done to auto-create a temporary user for a non-edit action (using the example of upload). Note that:

  • This assumes that permissions checks are centralised (see T336187#9140953)
  • This doesn't display any indication after the upload has finished that a temporary account was created

Testing this example:

  1. Set config:
$wgAutoCreateTempUser['enabled'] = true;
$wgAutoCreateTempUser['actions'][] = 'upload';
$wgEnableUploads = true;
$wgGroupPermissions['temp']['upload'] = true;
$wgGroupPermissions['*']['upload'] = true;
  1. Log out
  2. Upload a file, e.g. via Special:Upload (note that UploadWizard will fail with UploadStashNotLoggedInException)
  3. Observe that a new temporary user is created

Is there anything we can do to centralise some of this?

  • EditPage and ApiEditPage currently create a new unsaved temp user to check permissions. Does this need repeating everywhere? PermissionManager::getPermissionErrorsInternal will create a placeholder temporary user for permissions checks of actions that cause temp user autocreation, but only currently for RIGOR_QUICK.

Permissions checks are made complicated by this situation:

$wgAutoCreateTempUser['actions'][] = 'someaction';
$wgGroupPermissions['*']['someaction'] = false;
$wgGroupPermissions['temp']['someaction'] = true;

Currently, if someaction is in the list $wgAutoCreateTempUser['actions'], then an anonymous user (i.e. in group * but not temp) is allowed to do it. However, when doing full checks via the PermissionManager, we check the anonymous user's rights. EditPage must therefore create a placeholder temporary user for permissions checks, to pass to the PermissionManager for full checks, and so must the other actions too. This seems like a fairly complicated step to ask each action to take.

We could do away with the placeholder temporary user for permission checks with either of the following:

  • Insist that $wgGroupPermissions['*']['someaction'] be set to true, otherwise a temporary account can't be created by performing that action
  • Have the PermissionManager always check against a temporary user for actions in the list $wgAutoCreateTempUser['actions'] (when checking for an anon user)

@Niharika @tstarling Would you have concerns about either of those approaches, or a preference for either?

We could do away with the placeholder temporary user for permission checks with either of the following:

  • Insist that $wgGroupPermissions['*']['someaction'] be set to true, otherwise a temporary account can't be created by performing that action
  • Have the PermissionManager always check against a temporary user for actions in the list $wgAutoCreateTempUser['actions'] (when checking for an anon user)

@Niharika @tstarling Would you have concerns about either of those approaches, or a preference for either?

Spoke with @Niharika, who is happy with either option, with a preference for the second one.

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

[mediawiki/core@master] PermissionManager: Check autocreate actions against temporary user

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

Tchanders renamed this task from Create temp user on non-edit actions to Investigate: Creating temp user on non-EditPage actions.Oct 17 2023, 5:17 PM
Tchanders updated the task description. (Show Details)
  • What other actions should we create a temp user from? (Loggable actions should be flagged by searching for ManualLogEntry.)

Filed as a subtask: T349219: [M] Investigate: Which workflows create an IP actor?

Do the other actions need to do much more than calling TempUserCreator::create? Anything that we should factor out to somewhere common?

Do we need to re-implement temp user creation for each action, or is there somewhere common we should do this from?
Can we provide an API for creating temporary users, for JS-only features that need to create a new user?

As discussed in T340540#9201679, each workflow should create a temporary account. This should be done via the PHP handler that's performing the update.

If we discover any JS-only workflows that handle the logic of doing whatever database update the action performs, and creating the actor, etc, then they might need a temporary account creator API. @matmarex Does such a workflow sound likely/possible?

If we discover any JS-only workflows that handle the logic of doing whatever database update the action performs, and creating the actor, etc, then they might need a temporary account creator API. @matmarex Does such a workflow sound likely/possible?

Sounds possible but unlikely.

This can almost be closed (or stalled pending the tasks in T336187#9262568).

I think the only outstanding thing to figure out is what to do about checking permissions for an anon attempting to perform an action that would make them a temporary account (described in T336187#9140953). This is being discussed on https://gerrit.wikimedia.org/r/c/mediawiki/core/+/955979/

Tchanders renamed this task from Investigate: Creating temp user on non-EditPage actions to [S] Investigate: Creating temp user on non-EditPage actions.Oct 30 2023, 4:26 PM

Following further discussion, here's what we'll do:

  1. To allow features that need to create temporary accounts to keep their existing permissions checks and rights config, we''ll do this (from T336187#9140953):

Insist that $wgGroupPermissions['*']['someaction'] be set to true, otherwise a temporary account can't be created by performing that action

  1. To guard against accidentally creating an IP actor (because IP actors will now pass permissions checks), do the following:
    • Ensure an IP actor can't be created if temp accounts are enabled (T345578)
    • Proactively look for workflows that create an IP actor (T349219)
    • Look for workflows that save the IP address other than through the actor table (e.g. like Flow: T345578#9290889) (T352914)
  2. Allow importing to create an IP actor, as a special case, for old edits assigned to IP actors from before temporary accounts were enabled (T350155#9341746)

We can close this investigation. Follow-up work is filed (or referenced and about to be filed) in the links in the comments above.

Change 955979 abandoned by Tchanders:

[mediawiki/core@master] PermissionManager: Check autocreate actions against temporary user

Reason:

We're going in the opposite direction: T336187#9341749, and don't want to check anon permissions against temporary accounts at higher rigor levels

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