Fix login redirection if only one 2FA provider is active #10588
Conversation
Fixes #10500. Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
ChristophWurst
added
bug
3. to review
ChristophWurst
requested review from
rullzer,
nickvergessen and
blizzz
and removed request for
rullzer
| @@ -25,6 +25,8 @@ | |||
|
|
|||
| namespace OC\Authentication\TwoFactorAuth; | |||
|
|
|||
| use function array_filter; | |||
| use OCA\TwoFactorBackupCodes\Provider\BackupCodesProvider; | |||
There was a problem hiding this comment.
🤔 mixing in App space into core space?
There was a problem hiding this comment.
its a force enabled shipped app, so should be fine
There was a problem hiding this comment.
True, it's not a clean approach though. I could be moved to public space, if it makes sense, or private space otherwise. Missing private class in app namespace is also not nice, but better than vice versa. It's tech debt we stack. There already 2, 3 corners like that.
There was a problem hiding this comment.
I agree that it's not the cleanest approach.
A possible solution would be adding something like a IProvidesBackupCodes extends IProvider (tagging interface) to OCP. But we'd then have to support that for x years and it would mean someone could be tempted to use that interface as well.
There's also the possibility to have this interface in OC, but then the app isn't clean anymore because it uses core private API …
A agree with @nickvergessen here, having the app namespace of an always-enabled app here is fine IMO.
There was a problem hiding this comment.
This anyways is something we won't change so close before a release. I vote to just get it in and discuss in a new issue for 15. 😉
There was a problem hiding this comment.
Another approach (but def. no solution) would be to just check the ID of the provider. But then you still kind of depend on this specific implementation, it just gets a bit blurred.
I vote for keeping it as it is now. We can always improve later on 👍
|
😛
I like the Please though, we should add it to all method names. It is much more polite! |
I like that! |
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
| * @return IProvider[] | ||
| */ | ||
| public function getPrimaryProviders(): array { | ||
| return array_filter($this->providers, function(IProvider $provider) { |
There was a problem hiding this comment.
Can we be 100% sure that providers is always properly set at this point? Extra check?
There was a problem hiding this comment.
Aaah never mind. Wrong file in mind.
blizzz
added
4. to release
Fixes #10500.
get3rdPartyProvidersisn't a particularly beautiful method name, butgetProvidersButNotTheBackupCodesProviderPleasewas a bit too long. Suggestions welcome 😉