don't allow enforcing 2FA when no provider is enabled #13735
Conversation
rummatee
added
3. to review
rummatee
requested review from
icewind1991,
ChristophWurst,
schiessle,
blizzz and
MariusBluem
|
thanks for the PR, I think it makes sense. Please also adjust and extend the failing tests. |
| @@ -30,7 +30,10 @@ | |||
|
|
|||
| <div id="two-factor-auth" class="section"> | |||
| <h2><?php p($l->t('Two-Factor Authentication'));?></h2> | |||
| <?php unset($_['twoFactorProviderData']['backup_codes']); if (!empty($_['twoFactorProviderData'])) { ?> | |||
There was a problem hiding this comment.
Please see the original ticket. We must not hide the full UI, but just add a warning ;) There are use cases where enforcing 2FA with user that don't have it yet is still desirable, for example when they get enrolled afterwards.
|
|
||
| foreach ($allApps as $appId) { | ||
| $info = $this->appManager->getAppInfo($appId); | ||
| if (isset($info['two-factor-providers'])) { |
There was a problem hiding this comment.
this should be cleaned up a bit as it duplicates a lot of the logic from the other method (I can help you with that)
rummatee
force-pushed
the
issue12267
branch
2 times, most recently
from
414501f to
02a1db9
Compare
| $('#two-factor-warning').toggleClass('hidden', !this.state.enforced); | ||
| } | ||
|
|
||
| saveChanges(); |
There was a problem hiding this comment.
I don't actually understand why it makes sense to save the changes immediately, but since this was the behavior before, I left it this way.
There was a problem hiding this comment.
FYI this has changed in the meantime. Saving is now explicit.
|
So, does this new approach, that works with the .vue files rather than with the php templates solve your objections about the code duplication and about warning the admin instead of disabling? @ChristophWurst |
There are conflicts because the webpack bundle has been touched by other PRs as well. You can resolve this by rebasing the branch onto the latest master. When you see conflicts in one of the files above don't try to manually resolve them but just rebuild the js 😉 |
ChristophWurst
added
1. to develop
|
I will try to tackle this for Nextcloud 17. We should do more accurate queries whether providers are enabled in general and in the case of the admin. This needs a bit of work. |
|
Sorry for the late reply, I had university exams. I am not completely sure if I understand your comment correctly. I think the second case would be issue #12269 rather than #12267 and I think that it would be also solved by allowing to set up 2FA on login as described in issue #12268. However I can also work on providing 2 different warnings. |
Yes, that is correct. Yes, for users it will be possible to set up 2FA on login in the future. But when admins enforce it, I would prefer to already warn them on enforcing if they haven't set up a provider. It sounds a bit too scary for me to hope the admin will successfully set it up afterwards. I would rather have that ensured beforehand. |
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Florian Schunk <florian.schunk@rwth-aachen.de>
|
Sorry, I didn't mean to put all these commits from master in here. How did that happen? In any case, now it should show different warnings in the case of no provider enabled at all and no provider enabled for the admin.
|
Signed-off-by: Florian Schunk <florian.schunk@rwth-aachen.de>
|
Rebased and resolved the issues here in #16463 |
Fixes issue #12267
I decided to pass a complete list of enabled 2FA providers to the template, so that if we decide to do something with this information (for example show a list of the names) this is now easy to change.