Page MenuHomePhabricator

Adapt to changes in post-login/signup hooks after switching to a central login wiki
Closed, ResolvedPublic

Description

Currently when using CentralAuth with a central login wiki, login or signup still happens locally (the "central login wiki" is really more of a central session wiki), so hooks mostly fire the same way they would on baseline MediaWiki. After T348388: SUL3: Use a dedicated domain for login and account creation this won't necessarily be the case, so we need to adjust how this hooks are invoked, or update the logic of callers of these hook which are expected to be used together with CentralAuth (and maybe provide alternatives like CentralAuth currently does for PostLoginRedirect).

Related Objects

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

This is a first guess but we'll have to audit hook usage:

  • Hooks that do or display things after a successful login (UserLoginComplete, PostLoginRedirect / CentralAuthPostLoginRedirect, LocalUserCreated with autocreate=true; also not-quite-hook callbacks like the postAuthentication callback of an AuthManager authentication provider) should keep working since the the local wiki still sees central login / signup as a login event. We might need to make sure that (some of) these hooks do not run on the central login wiki, though. (And in popup mode, we might want to provide their return values via a Javascript callback so the UI that initiated async login / signup can decide how to handle it, instead of trying to display them in the popup - this is handled in T364939: Provide a JavaScript library for logging in / signing up to MediaWiki via a popup.)
  • Hooks that do or display things after a successful signup (BeforeWelcomeCreation, LocalUserCreated with autocreate=false; also not-quite-hook callbacks like the finishAccountCreation callback of an AuthManager authentication provider) will not run locally, since the local wiki will only see an autocreation (the actual account creation will happen on the central login wiki). Especially for LocalUserCreated, this will probably disrupt a lot of assumptions in current code. We might want a CentralUserCreated or similar hook that runs in the wiki which initiated the account creation.
  • Hooks that run during authentication and have no visible output (AuthManagerLoginAuthenticateAudit, ExemptFromAccountCreationThrottle) will run on the central login wiki instead of the local wiki, but this probably won't make any difference.
  • SpecialCreateAccountBenefits will run on the central wiki, which means both that it needs access to the starting wiki (currently it just assumes that's the wiki it's running on), and that allowing customization via on-wiki i18n messages, community configuration etc. is nontrivial.
  • LoginFormValidErrorMessages is used by extensions to add their own error messages, that might be tricky if the extension is not installed on the central login wiki.

Logout and temp user creation (UserLogout, UserLogoutComplete, TempUserCreatedRedirect) are not affected by the changes.

Tgr changed the task status from Open to Stalled.Jul 31 2024, 10:53 PM
Tgr moved this task from Ready to Blocked / External on the SUL3 board.

Hopefully we can avoid this entire problem cluster via T363695: Create a Wikimedia login domain that can be served by any wiki.

Most of it we can; but we'll probably still have to special-case the situation where an extension wants to redirect away at the end of signup (but now signup happens on the central domain, mid-flow, where a redirect would break login; and the final step in the signup process is, from the local wiki's point of view, a login, so it doesn't trigger the right hooks).

More generally, we should audit how LocalUserCreated gets used - it's probably the hook that's affected the most. We should also look at post-login hooks like PostLoginRedirect - CentralAuth should prevent other hooks handlers from running, but there might be ordering issues.

Tgr changed the task status from Stalled to Open.Jan 25 2025, 8:38 PM
Tgr claimed this task.

Change #1115491 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/extensions/CentralAuth@master] Fire CentralAuthPostLoginRedirect on SUL3 login

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

Change #1115491 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@master] Fire CentralAuthPostLoginRedirect on SUL3 login

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

Change #1117927 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/extensions/CentralAuth@wmf/1.44.0-wmf.15] Fire CentralAuthPostLoginRedirect on SUL3 login

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

Change #1118143 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/extensions/CentralAuth@master] Do not preserve 'sul3-action' when restarting authentication

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

Change #1118143 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@master] Do not preserve 'sul3-action' when restarting authentication

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

Change #1118234 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/extensions/CentralAuth@master] Use the token store for passing "is signup" to local in SUL3 flow

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

Change #1117927 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@wmf/1.44.0-wmf.15] Fire CentralAuthPostLoginRedirect on SUL3 login

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

Mentioned in SAL (#wikimedia-operations) [2025-02-10T14:59:33Z] <tgr@deploy2002> Started scap sync-world: Backport for [[gerrit:1117927|Fire CentralAuthPostLoginRedirect on SUL3 login (T364866)]], [[gerrit:1118515|Preserve 'campaign' parameter during authentication]], [[gerrit:1118517|Call AuthPreserveQueryParams hook when redirecting to SUL3 domain]]

Mentioned in SAL (#wikimedia-operations) [2025-02-10T15:03:11Z] <tgr@deploy2002> tgr: Backport for [[gerrit:1117927|Fire CentralAuthPostLoginRedirect on SUL3 login (T364866)]], [[gerrit:1118515|Preserve 'campaign' parameter during authentication]], [[gerrit:1118517|Call AuthPreserveQueryParams hook when redirecting to SUL3 domain]] synced to the testservers (https://wikitech.wikimedia.org/wiki/Mwdebug)

Mentioned in SAL (#wikimedia-operations) [2025-02-10T15:14:21Z] <tgr@deploy2002> Finished scap sync-world: Backport for [[gerrit:1117927|Fire CentralAuthPostLoginRedirect on SUL3 login (T364866)]], [[gerrit:1118515|Preserve 'campaign' parameter during authentication]], [[gerrit:1118517|Call AuthPreserveQueryParams hook when redirecting to SUL3 domain]] (duration: 14m 47s)

Change #1118234 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@master] Use the token store for passing "is signup" to local in SUL3 flow

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

Change #1119531 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/extensions/CentralAuth@wmf/1.44.0-wmf.16] Do not preserve 'sul3-action' when restarting authentication

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

  • SpecialCreateAccountBenefits will run on the central wiki, which means both that it needs access to the starting wiki (currently it just assumes that's the wiki it's running on), and that allowing customization via on-wiki i18n messages, community configuration etc. is nontrivial.
  • LoginFormValidErrorMessages is used by extensions to add their own error messages, that might be tricky if the extension is not installed on the central login wiki.

With T363695: Create a Wikimedia login domain that can be served by any wiki, these now behave identically to before. With SpecialCreateAccountBenefits, there's a risk of parser cache pollution due to the different path configuration on the shared domain, we'll have to test that. The hook's only implementation is in GrowthExperiments.

(Also AuthenticationAttemptThrottled, I missed that earlier.)

Actually, they run on both domains, which is definitely a problem for AuthManagerLoginAuthenticateAudit (though fine for ExemptFromAccountCreationThrottle and AuthenticationAttemptThrottled). Usage:

  • LoginNotify to send failed login / unknown device login emails
  • CheckUser to record IP/UA data; to set the "get client hints via JS" flag on error; to store limited client hint data on success.
  • WikimediaEvents for analytics
  • mediawiki-config for logging security-relevant logins

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CentralAuth/+/1118249 should fix all of those. A nasty hack though, we should find a nicer way of dealing with this eventually.

  • Hooks that do or display things after a successful signup (BeforeWelcomeCreation, LocalUserCreated with autocreate=false; also not-quite-hook callbacks like the finishAccountCreation callback of an AuthManager authentication provider) will not run locally, since the local wiki will only see an autocreation (the actual account creation will happen on the central login wiki). Especially for LocalUserCreated, this will probably disrupt a lot of assumptions in current code. We might want a CentralUserCreated or similar hook that runs in the wiki which initiated the account creation.

With the shared domain approach, this plays out much more nicely - both hooks run on the shared domain, but still in the right wiki context. The two potential problem I can think of are doing something in BeforeWelcomeCreation that prevent the followup authentication in the local domain (e.g. a redirect), but this would already break SUL2 central login; or generating or caching output and causing problems because of the different URL path of the shared domain.

Just in case, reviewed usage. Nothing in codesearch uses BeforeWelcomeCreation. LocalUserCreated usage in production:

  • GrowthExperiments (1, 2, 3, 4, 5) to set user options and other user-specific data
  • AntiSpoof to set user-specific data
  • CentralAuth to trigger remote autocreation and set user-specific data
  • CheckUser to record private data
  • Echo to trigger "welcome" event and set user-specific data
  • LoginNotify to set cookies and record user-specific data
  • NewUserMessage to create a talk page welcome message.
  • PageTriage to set user options
  • Thanks to set user options
  • Wikibase to set user options
  • WikimediaEvents for analytics
  • Vector to set user preferences

The only potentially problematic one here is NewUserMessage (which sometimes uses a job, but on other cases, a deferred - the latter would inherit the shared domain configuration overrides). We'll have to test if that causes any issues with cached HTML, and probably move it to always use a job.

Same for finishAccountCreation / postAccountCreation and such - these will run on the central domain, should be unproblematic. Reviewed existing examples just in case, seemed fine.

  • Hooks that do or display things after a successful login (UserLoginComplete, PostLoginRedirect / CentralAuthPostLoginRedirect, LocalUserCreated with autocreate=true; also not-quite-hook callbacks like the postAuthentication callback of an AuthManager authentication provider) should keep working since the the local wiki still sees central login / signup as a login event. We might need to make sure that (some of) these hooks do not run on the central login wiki, though.

(Also UserLoggedIn, I missed that earlier.)

With the "central wiki" and the "local wiki" being the same wiki, it should be fine that they run on the central domain, except the redirect ones which are specifically needed at the end of authentication; but PostLoginRedirect already needs to be suppressed so that SUL2 central login can work, and the firing of CentralAuthPostLoginRedirect was fixed in rECAUbbf811d96960: Fire CentralAuthPostLoginRedirect on SUL3 login.

Just in case, reviewed UserLoginComplete / UserLoggedIn. (Why are those separate hooks?) UserLoginComplere is only used by CentralAuth, in SUL2 mode. UserLoggedIn in only used by VisualEditor to set user options. Seems fine. (See above for LocalUserCreate and auth provider methods.)

I missed CentralAuthLoginRedirectData and CentralAuthSilentLoginRedirect earlier, but they are not affected - they are just there so MobileFrontend can inject mobile URL information in the redirect process, and SUL3 handles that directly.

tl;dr: apart from the two things we already fixed (AuthManagerLoginAuthenticateAudit firing twice; CentralAuthPostLoginRedirect not called after login), the only potential concern I can see here is a hook (SpecialCreateAccountBenefits, LocalUserCreated in NewUserMessage, maybe something else?) causing wikitext -> HTML rendering and that resulting in parser cache or message cache pollution because the path prefixes are different + the rules for when to use relative vs. absolute URLs are different. I can't work out in my head if that can happen, we'll just have to test.

Change #1119531 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@wmf/1.44.0-wmf.16] Do not preserve 'sul3-action' when restarting authentication

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

Mentioned in SAL (#wikimedia-operations) [2025-02-13T21:50:52Z] <tgr@deploy2002> Started scap sync-world: Backport for [[gerrit:1119516|auth: Use POST trxProfiler expectations during return/reauth (T385566)]], [[gerrit:1119530|Track the number of started / finished SUL3 flows (T377261)]], [[gerrit:1119531|Do not preserve 'sul3-action' when restarting authentication (T364866)]]

Mentioned in SAL (#wikimedia-operations) [2025-02-13T21:53:33Z] <tgr@deploy2002> tgr: Backport for [[gerrit:1119516|auth: Use POST trxProfiler expectations during return/reauth (T385566)]], [[gerrit:1119530|Track the number of started / finished SUL3 flows (T377261)]], [[gerrit:1119531|Do not preserve 'sul3-action' when restarting authentication (T364866)]] synced to the testservers (https://wikitech.wikimedia.org/wiki/Mwdebug)

Mentioned in SAL (#wikimedia-operations) [2025-02-13T22:05:56Z] <tgr@deploy2002> Finished scap sync-world: Backport for [[gerrit:1119516|auth: Use POST trxProfiler expectations during return/reauth (T385566)]], [[gerrit:1119530|Track the number of started / finished SUL3 flows (T377261)]], [[gerrit:1119531|Do not preserve 'sul3-action' when restarting authentication (T364866)]] (duration: 15m 03s)

the only potential concern I can see here is a hook causing wikitext -> HTML rendering and that resulting in parser cache or message cache pollution because the path prefixes are different + the rules for when to use relative vs. absolute URLs are different

Since that's not really actionable for now, let's track it in the QA task and close this.

If it does turn out to be a problem, we can use the ParserOptionsRegister hook to split parser cache on whether we are on the shared authentication domain. That's not great performance-wise, but not much parsing is shared between the login/signup pages and the rest of the site, mostly just skin stuff, so I don't think it would be catastrophic, either.