feat: only load available commands in maintenance mode

[kesselb]

• Resolves: #

Summary

Nextcloud is in maintenance mode, hence the database isn't accessible.
Cannot perform any command except 'maintenance:mode --off'

In maintenance mode, the above message is printed for every command.
A customer brought to our attention that the message is misleading.

"isn't accessible" is a simplified statement.
Technically, the database is accessible, but it's probably unsafe to run commands that modify the system.

server/lib/base.php

Lines 1032 to 1053 in bb4b34f

	// Always load authentication apps
	OC_App::loadApps(['authentication']);
	OC_App::loadApps(['extended_authentication']);
	// Load minimum set of apps
	if (!\OCP\Util::needUpgrade()
	&& !((bool) $systemConfig->getValue('maintenance', false))) {
	// For logged-in users: Load everything
	if (Server::get(IUserSession::class)->isLoggedIn()) {
	OC_App::loadApps();
	} else {
	// For guests: Load only filesystem and logging
	OC_App::loadApps(['filesystem', 'logging']);
	// Don't try to login when a client is trying to get a OAuth token.
	// OAuth needs to support basic auth too, so the login is not valid
	// inside Nextcloud and the Login exception would ruin it.
	if ($request->getRawPathInfo() !== '/apps/oauth2/api/v1/token') {
	self::handleLogin($request);
	}
	}
	}

server/lib/private/Console/Application.php

Lines 116 to 121 in 426c034

	if (\OCP\Util::needUpgrade()) {
	throw new NeedsUpdateException();
	} elseif ($this->config->getSystemValueBool('maintenance')) {
	$this->writeMaintenanceModeInfo($input, $output);
	} else {
	OC_App::loadApps();

In maintenance mode, only a minimum set of apps is loaded.

The commands available in maintenance mode are mainly related to system operations.
It's fine and for some even recommended to run those commands in maintenance mode.

However, some commands are difficult:

• user:delete will emit BeforeUserDeletedEvent and UserRemovedEvent. We have a couple of apps listening for those events: https://github.com/search?q=org%3Anextcloud+%28UserRemovedEvent+OR+BeforeUserDeleted%29&type=code

• user:add

• user:resetpassword

• group:add

• group:delete

• group:adduser

• group:removeuser

Just to name some examples.

My thought is, if we already know that a command is potentially unsafe, we should prevent the execution.

TODO

• Make interface public
• Rename IAvailableInMaintenanceMode to IUnavailableInMaintenanceMode (and flip the logic)

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• Tests (unit, integration, api and/or acceptance) are included
• Screenshots before/after for front-end changes
• Documentation (manuals or wiki) has been updated or is not required
• Backports requested where applicable (ex: critical bugfixes)

[nickvergessen]

Why not? Talk has commands that could be run while maintenance is on?
Similar for files and others, maybe an admin restored a folder from a backup and wants to run file scan before sync clients interfere?
support:system-report, transfer-ownership, user-migration and others are also good examples for commands that could (maybe even should) be run in maintenance-mode.
As mentioned in #37365 also all the db:* commands are recommended to run in maintenance mode.

[nickvergessen]

I would even argue to default to "is available in maintenance mode" and have commands opt out of it.
But at the same time I would not know which command I would opt-out of it?

[kesselb]

Thanks!

I updated the pull request description to add some more context.

To opt out is indeed much better than opt in here. Most commands in register_commands.php are okay to run in maintenance mode.

[nickvergessen]

Should definitely be in OCP for better usage.

[nickvergessen]

Suggested change

	if ($this->maintenanceMode && in_array(IUnavailableInMaintenanceMode::class, class_implements($command, false))) {
	if ($this->maintenanceMode && $command instanceof IUnavailableInMaintenanceMode) {

???

[kesselb]

I understand your puzzlement ;)

class_implements is a leftover from an experiment. My idea was to change Command to Command|string and use the dependency injection to build the class when a string is given to clean up register_command.php a bit.

Will use instanceof for now and submit another pr sometime for the dependency injection feature.

[nickvergessen]

Can't find any changes apart from s/$application/$this/ correct?
The new interface is not being added anywhere (totally okay, just asking)?

[kesselb]

Yep.

[nickvergessen]

In maintenance mode, only a minimum set of apps is loaded.

So if that is the reason, I would change the other half and also make sure to load all apps in CLI?
I would find it quite disturbing to not be able to prepopulate and manage an instance during maintenance mode.

Since we are approaching the next deadline in light speed maybe we can merge the interface and Application change in a single PR and discuss the implementing this quite restrictive behaviour in another one.

[kesselb]

So if that is the reason, I would change the other half and also make sure to load all apps in CLI?

As reference:

owncloud/core#20939
owncloud/core#21076

I would find it quite disturbing to not be able to prepopulate and manage an instance during maintenance mode.

You put your instance in maintenance mode to delete a user 🤔

Since we are approaching the next deadline in light speed maybe we can merge the interface and Application change in a single PR and discuss the implementing this quite restrictive behaviour in another one.

I see your concern, we don't need to rush. Let's continue the work on this feature when 27 is done.

My next task is to update the documentation: nextcloud/documentation#10063