Page MenuHomePhabricator

Admin account created by the installer isn't made global by CentralAuth
Closed, ResolvedPublic

Description

Soon after adding Translate as a CI dependency of the CampaignEvents extension, lots of selenium and api-testing tests started failing. All the failures have one thing in common: the user performing a certain action (like creating an event) is reportedly not allowed to do so. To provide a bit of context, the CampaignEvents extension mostly only works with global accounts (via core's CentralIdLookup); having a global account is a prerequisite for creating events. Looking at the CI logs, it was immediately clear that the addition of Translate as a dependency brought in a total of 38 new dependencies, among which is CentralAuth.

I still haven't found the courage to install CentralAuth locally, so I ran a few tests in this patch. All the failing selenium and api-testing tests use the default user account to perform actions; this account is created by install.php. However, it looks like this account is just a local account, as there's no sign of it in the globaluser and localuser tables, which are in fact empty.

If I create a new account via the API, CentralAuth makes it global and I can find it in its tables; still, no sign of the default account. I'm not sure if this is an intentional design choice, but it would be nice to change that, as it makes testing much easier. I also don't know whether this is just a CI thing, or if it actually happens for every new wiki.

I also wanted to see if CentralAuth had a solution for this in its own selenium tests, but I think you already know what I found (or didn't). This also means that perhaps nobody ever noticed (it's maybe not too common for extensions to work with central accounts only).

Finally, I'm not sure what tags to use here. I don't know if this is a bug (feature?) in CentralAuth itself, in how the installer creates the default account, or in how CI is configured; as such, I'm tagging all these for the time being.

Event Timeline

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

Extensions aren't enabled when the installer runs and creates the admin account. Vagrant migrates the admin user manually; other environments could do that, or I guess we could create some sort of post-install hook that the installer calls as a final step.

Extensions aren't enabled when the installer runs and creates the admin account.

Oh, I see. I'm not sure if I like it: extensions can change how user accounts are created (CentralAuth being just an example); having the initial admin account bypass all extension code that we already know is going to be run for every other user account feels off. I'm sure there are reasons for it (maybe wanting to avoid side effects from extension in early phases, or maybe just trying to guarantee that the admin will be created without anything funky being done to it), but it still seems wrong somehow.

Vagrant migrates the admin user manually; other environments could do that, or I guess we could create some sort of post-install hook that the installer calls as a final step.

Could it be done in the LoadExtensionSchemaUpdates hook handler? IIRC, there should be at least a hacky-ish way to detect if the handler is run from the installer vs the updater; it could scan the user table and migrate any account(s) it finds.

Could it be done in the LoadExtensionSchemaUpdates hook handler?

If you can reliably differentiate between install and update then yes. Otherwise, you'd have to be very careful not to violate any expectations as CentralAuth still supports various partially-unified modes.

One thing to consider might be for the CampaignEvents extension, whos tests are incompatible with the glboal state provisioned by CentralAuth, to turn off CentralAuth.

We do this from time to time in PHPUnit tests using MediaWikiIntegrationTestCase->clearHook() and the like. If you restore the default CentralIdLookup service, would that make CampaignEvents tests' behave more consistently and thus pass in local and CI alike regardless of whether CA is installed?

Could it be done in the LoadExtensionSchemaUpdates hook handler?

If you can reliably differentiate between install and update then yes. Otherwise, you'd have to be very careful not to violate any expectations as CentralAuth still supports various partially-unified modes.

I don't think it's officially supported. I do remember of a hacky way (possibly some config values being unset), but that's not reliable.

One thing to consider might be for the CampaignEvents extension, whos tests are incompatible with the glboal state provisioned by CentralAuth, to turn off CentralAuth.

Unfortunately only selenium and api-testing tests are affected, and AFAIK there's pretty much no way to change the MW config in those. A workaround would be to create a new bot account and give it +sysop, but that seems too much overhead for us to do it in every test.

Could it be done in the LoadExtensionSchemaUpdates hook handler?

If you can reliably differentiate between install and update then yes. Otherwise, you'd have to be very careful not to violate any expectations as CentralAuth still supports various partially-unified modes.

I don't think it's officially supported. I do remember of a hacky way (possibly some config values being unset), but that's not reliable.

I think we agree that this is basically the way we want to do this, and the question becomes how to make it non-hacky.

I browsed around the installed code, and it seems that checking for the MEDIAWIKI_INSTALL constant (defined if we're in the installer) is a reliable way. We could just document somewhere that this is supported (there is only one unrelated mention of it on mw.org).

You can add custom callbacks from the LoadExtensionSchemaUpdates hook handler using $updater->addExtensionUpdate( [ [ __CLASS__, 'callback' ] ] ) (example).

Extension updates run at the end of the installation, after the initial user has been created, so this is it – CentralAuth could check MEDIAWIKI_INSTALL and if so, add a callback to globalize any users it finds.

@Daimona Do you want to have a go at implementing that?


We don't need any core changes (unless I got something wrong above), but do we want any core changes? (I think this is out of scope for this task, but if any of these ideas seem good, we should file follow-ups.)

Checking a constant is icky and bad for tests. We could add a getter method to $updater instead (DatabaseUpdater) to return whether we're in the installer, and have the installer set it.

LoadExtensionSchemaUpdates hook would ideally just do schema updates, not arbitrary callbacks. Instead, we could add a new hook in Installer::getInstallSteps – or an extension.json property rather than a hook, since the steps are declarative-ish – and declare arbitrary callback to run that way. We'd need to add a bit of code in the installer to run this hook (it only runs LoadExtensionSchemaUpdates right now), or read the property. A part of this system already exists, but it's only used to have custom steps for specific database types in core: https://codesearch.wmcloud.org/search/?q=addInstallStep

Or, we approach this from another angle, and have extensions declare in extension.json which of their hooks support being executed in the installer, and then have the installer register these hooks at the beginning before doing the rest of the installation. For example, CentralAuth could declare that its LocalUserCreated hook handler should be executed in the installer, which would magically make the default user become a global user.

I think we agree that this is basically the way we want to do this, and the question becomes how to make it non-hacky.

At least in the short term, yes. But for the longer term, I really wonder if we can postpone creating the admin account until after extensions have been installed.

@Daimona Do you want to have a go at implementing that?

Hmmm I don't currently have CentralAuth set up locally and wouldn't be able to test this (I'm working to change that though).

Daimona renamed this task from Admin account created by the installer isn't made global by CentralAuth (at least in CI) to Admin account created by the installer isn't made global by CentralAuth.Mar 8 2024, 12:23 AM

Well, the good news is that it's a problem in the installer, so having it set up wouldn't really help ;) I would mostly rely on CI to test it for me, testing the installation process is annoying enough already.

I could give it a try too, but I won't have the time in the next two weeks or so.

I started playing with the approach I described in T358985#9603935: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CentralAuth/+/1015140, and I ran into a bit of a chicken-and-egg problem – during installation, to globalize an account, we need to have the CentralAuth tables, and for that we need the CentralAuth database, and for that we need to define $wgCentralAuthDatabase, which we can't do until the installer created LocalSettings.php.

Maybe we just need to have reasonable defaults enabled normally, instead of gated by defined( 'MW_QUIBBLE_CI' ): https://codesearch.wmcloud.org/search/?q=MW_QUIBBLE_CI&repos=Extension%3ACentralAuth

Or maybe T348486: Migrate CentralAuth to use a virtual database domain will make this easier (I need to catch up with that work to make sure what it enables and what it doesn't)

As a side note, I got CentralAuth working on Patch Demo (not as a real wiki farm, just installed on a single wiki): https://gitlab.wikimedia.org/repos/ci-tools/patchdemo/-/merge_requests/610 and got a demo wiki demonstrating this bug: https://patchdemo.wmflabs.org/wikis/1a27a5e689/wiki/Special:CentralAuth/Patch_Demo. Hopefully this will end with a demo wiki demonstrating the fix :)

Now that T348486 is (almost) resolved I had a look again. The changes there do help – the installer now creates CentralAuth tables in the local wiki's database, so it should be possible to record the default users in them.

However, my initial idea of just using the existing maintenance scripts won't work. I tried, and found that the installer doesn't load service wiring files that the scripts depend on; and when I tried to fix that (https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1036775), I found that it doesn't load the default config options that the services depend on; I haven't tried fixing that, but at this point we may as well just load the whole extension, and I don't really want to get into that.

But it would be possible to write a new maintenance script, that doesn't rely on anything, and run it. It would need to assume that all configs are default and maybe check that this is really a newly installed wiki, without other wikis in the farm, and re-implement the database insertions needed to migrate all existing users. That shouldn't be too difficult, I'll give it a try.

Change #1015140 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/CentralAuth@master] Globalize accounts created by the installer during wiki installation

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

Change #1015140 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@master] Globalize accounts created by the installer during wiki installation

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