feat: Add SetupCheck to warn about missing second factor provider #57854
Conversation
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
come-nc
requested review from
a team,
ChristophWurst,
miaulalala and
nickvergessen
as code owners
come-nc
requested review from
Altahrim,
ArtificialOwl,
icewind1991 and
salmart-dev
and removed request for
a team
|
/backport to stable33 |
|
/backport to stable32 |
| if ($user === null) { | ||
| $allApps = $this->appManager->getEnabledApps(); | ||
| } else { |
There was a problem hiding this comment.
Feels weird, not sure why tho . least privilege maybe?
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
| public function run(): SetupResult { | ||
| $providers = $this->providerLoader->getProviders(); | ||
| if (count($providers) === 0) { | ||
| return SetupResult::warning($this->l10n->t('This instance has no second factor provider available.')); | ||
| } else { | ||
| return SetupResult::success( | ||
| $this->l10n->t( | ||
| 'Second factor providers are available: %s.', | ||
| [ | ||
| implode(', ', array_map( | ||
| fn ($p) => '"' . $p->getDisplayName() . '"', | ||
| $providers) |
There was a problem hiding this comment.
I wonder, shouldn't the test also check if 2FA is actually enabled for users or even enforced?
There was a problem hiding this comment.
We can (and should) not loop over all users in a setup check
There was a problem hiding this comment.
Yes but maybe we could check for the system config "twofactor_enforced": "true", here?
ChristophWurst
left a comment
ChristophWurst
left a comment
There was a problem hiding this comment.
Some comments. I don't think this should succeed when only backup codes are available. It needs at least one real 2FA provider.
| declare(strict_types=1); | ||
|
|
||
| /** | ||
| * SPDX-FileCopyrightText: 2023 Nextcloud GmbH and Nextcloud contributors |
| } | ||
|
|
||
| public function getName(): string { | ||
| return $this->l10n->t('Two factor configuration'); |
There was a problem hiding this comment.
nit: I think it should be either two-factor authentication or second factor configuration. Two factor configuration sounds wrong.
| } | ||
|
|
||
| public function run(): SetupResult { | ||
| $providers = $this->providerLoader->getProviders(); |
There was a problem hiding this comment.
This will give you one provider if only twofactor_backup is enabled. That provider alone is not usable as 2FA.
You could fetch the provider set from \OC\Authentication\TwoFactorAuth\Manager::getProviderSet and use \OC\Authentication\TwoFactorAuth\ProviderSet::getPrimaryProviders instead.
Downside is that getProviderSet is specific to a user.
Or you duplicate the logic here and exclude the backup codes provider.
Summary
Add warning if no two factor auth provider is found.
Checklist
3. to review, feature component)stable32)