Page MenuHomePhabricator

Update ConfirmAccount to use AuthManager
Closed, ResolvedPublic

Description

The following items should be looked at:

  1. The 'UserCreateForm' and 'UserLoginForm' hooks are deprecated. To just inject text, you might use the 'AuthChangeFormFields' hook instead (like this).
  2. The 'AbortNewAccount' hook is deprecated. Implement a PreAuthenticationProvider (or other AuthenticationProvider) instead.
  3. $wgAuth is deprecated, and uses of it should be replaced.
  4. Adding users directly to the database is probably not going to reliably work anymore in all cases. Ideally users would be created through the normal Special:CreateAccount page, and assigned a group (e.g. using the 'LocalUserCreated' hook) that would cause a PreAuthenticationProvider to prevent any login. "Confirming" the account would then remove this group.
  5. Deletion of users is a tricky business when other extensions might still have record of the user and might even try to auto-create them if they're missing. If you really need this sort of thing, you'll likely need to add a hook on deletion and make sure every other extension that tracks users implements it.

Details

Related Gerrit Patches:
mediawiki/extensions/ConfirmAccount : REL1_27Handle AuthManager introduction to core
mediawiki/extensions/ConfirmAccount : masterHandle AuthManager introduction to core

Related Objects

Event Timeline

Tgr created this task.Aug 27 2015, 2:18 AM
Tgr raised the priority of this task from to Needs Triage.
Tgr updated the task description. (Show Details)
Tgr added subscribers: Liuxinyu970226, Krenair, Florian and 2 others.
Paladox set Security to None.Sep 11 2015, 4:55 PM
Paladox added a subscriber: aaron.
Anomie updated the task description. (Show Details)May 13 2016, 7:54 PM
aaron added a comment.Jul 10 2016, 2:12 PM

Letting users create accounts could lead to inappropriate name spam and probably leaks with read-only wikis. There should be some other way to be able to defer the creation until approval.

Paladox triaged this task as Unbreak Now! priority.Jul 10 2016, 4:01 PM
Restricted Application added subscribers: Luke081515, TerraCodes. · View Herald TranscriptJul 10 2016, 4:01 PM
Anomie added a subscriber: Anomie.Jul 10 2016, 7:29 PM

The problem is that the account creation process is interactive, so you can't just create the account without interaction anymore. As I see it, your options include:

  • Create the account with a group (or lack of a group) that causes it to have no rights. The approval would adjust the group membership to allow normal access. There shouldn't be any leaks there, although there could be name-spam.
  • Instead of creating the account as it does now, approval would email the requester a link to Special:CreateAccount with a pre-filled username and a token in the URL which a PreAuthenticationProvider would validate in testForAccountCreation(). The requester would thus be able to create only the approved account and no other.
    • See WikimediaIncubator or Campaigns for similar use of parameters passed into Special:CreateAccount, although those are using a SecondaryAuthenticationProvider to do stuff after creation.
    • You could probably manage the "pre-filled username" bit using the 'AuthChangeFormFields' hook to replace the standard username field with a hidden field when your token is present.
Kghbln added a subscriber: Kghbln.Jul 20 2016, 1:15 PM
greg lowered the priority of this task from Unbreak Now! to High.Jul 21 2016, 7:01 AM
greg added a subscriber: greg.

This is not UBN! given the 'unmaintained' status on https://www.mediawiki.org/wiki/Extension:ConfirmAccount and the fact that to do this properly requires a large investment of time (most likely).

Just to be clear as well: we don't need two UBN! tasks in the ConfirmAccount project that basically are the same thing (this and T137843).

This is not UBN! given the 'unmaintained' status on https://www.mediawiki.org/wiki/Extension:ConfirmAccount

@greg So if change back the status to stable it will be possible to get the UBN status again??? I do not think you subscribe to this yourself. I changed the status of the extension that anybody dealing with one of the 4000+ wikis using this extension is aware of it not supporting the new account creation method of MW 1.27+. I am sure you understand. Anyway ...

I think that everyone here at least realises that the situation with this extension is très désagréable. I guess this is as good as UBN.

Tgr added a comment.Jul 21 2016, 8:11 PM

@Kghbln see T140207 for context. It makes it harder to respond to emergencies if tasks are stuck as UBN forever because no one is actually working on them. Bugzilla had separate severity and priority fields; in that terminology this task is high severity, low priority. UBN is for high priority bugs (or at least we seem to be moving towards that consensus).

robkam added a subscriber: robkam.Aug 1 2016, 1:34 PM
Tgr added a comment.Aug 12 2016, 8:56 PM

One possible approach:

  • create a PreAuthenticationProvider to prevent unconfirmed account creation
  • check in testForAccountCreation for some sort of token; if it's not there, store whatever details you care about (username, etc) in the extension's own DB table and tell the user what's happening
  • have testUserForCreation check that table to prevent to users reserving the same username
  • once an admin reviews the application create a token and send the user an account creation link with the token in it
  • if the token is present, pre-fill the form; probably the least hacky way to do that is to use the AuthChangeFormFields hook and set the default key for whatever form elements you care about.

Some shortcomings (which seem acceptable):

  • due to the flexibility of AuthManager, there is no guarantee any other field than username will be present
  • since any extension can add form fields, you have no way of knowing what fields should be stored (storing a captcha value is not all that helpful; some fields might have password-like security properties and should not be kept in plaintext). This problem existed before AuthManager, though. It's probably best to use some fieldname whitelist.
  • you have no access to fields displayed in later steps of a multistep form
Tuxxic added a subscriber: Tuxxic.Aug 14 2016, 10:05 AM

Change 305830 had a related patch set uploaded (by Aaron Schulz):
[WIP] Handle AuthManager introduction to core

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

Change 305830 had a related patch set uploaded (by Aaron Schulz):
[WIP] Handle AuthManager introduction to core
https://gerrit.wikimedia.org/r/305830

This patch seems to work, but I'll want to iterate on it to actually use preauth providers instead of the legacy hooks at some point.

@aaron hi, thanks for working on this :)

Change 305830 merged by jenkins-bot:
Handle AuthManager introduction to core

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

Change 305847 had a related patch set uploaded (by Aaron Schulz):
Handle AuthManager introduction to core

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

Change 305847 merged by jenkins-bot:
Handle AuthManager introduction to core

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

aaron closed this task as Resolved.Aug 21 2016, 7:31 AM
aaron claimed this task.

Some request cleanup could be done, but that's lower priority.