Fix state propragation of the backup codes provider #10465
Conversation
ChristophWurst
added
bug
3. to review
|
Remark: I couldn't execute the I'm now crossing my fingers and hope that it works on CI 🤞 |
|
Forgot to import a class, so pushed a fixup.
|
| use OCP\IUser; | ||
| use Symfony\Component\EventDispatcher\Event; | ||
|
|
||
| class CodesGenerated extends Event { |
There was a problem hiding this comment.
I would prefer to use the GenericEvent instead, because that allows other apps to also listen to the event. Otherwise they would need to cast to this specific class of another app to get the user info.
new GenericEvent($user)
And access with $event->getSubject()
There was a problem hiding this comment.
First, my intention is not to share this event with other apps. It's meant to be used internally only. Second, I like this custom event class better because it offers a type-safe way of transporting the user/subject. The GenericEvent is, well, generic 😉.
These are minor issues. If you insist on the change, I'll happily do so. I just wanted to give you my point of view.
|
|
||
| use Symfony\Component\EventDispatcher\Event; | ||
|
|
||
| interface IListener { |
There was a problem hiding this comment.
since the interface can not be used by other apps drop it?
There was a problem hiding this comment.
I agree that it's not necessary to have an interface here. But having an interface is not only helpful when types are shared across apps. They make sure that the listeners implement a certain interface. This is purely used to keep the code clean and maintainable.
Again, I can remove it if you prefer to have it simpler. We're back at duck typing then, though.
Starting with Nextcloud 14, the server knows the enabled/disabled state of 2fa providers. While it will query that information if it's unknown (on first use), it won't notice any changes. Thus, providers have to propagate that information themselves. Ref nextcloud/twofactor_totp#263 Ref nextcloud/twofactor_u2f#210 Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
ChristophWurst
force-pushed
the
fix/backup-codes-state-propagation
branch
from
3a4b53f to
f8fe748
Compare
|
Commits squashed and rebased onto latest master 🚀 |
|
@nickvergessen please see my comments. Is this okay to integrate as is or do you want me to change that? We should have this in beta2 ✌️ |
Starting with Nextcloud 14, the server knows the enabled/disabled
state of 2fa providers. While it will query that information if it's
unknown (on first use), it won't notice any changes. Thus, providers
have to propagate that information themselves.
Ref nextcloud/twofactor_totp#263
Ref nextcloud/twofactor_u2f#210
Ref https://github.com/orgs/nextcloud/projects/4#card-10826964
Test steps
Expected behavior
See regular 2FA provider and backup codes option.
Actual behavior
See regular 2FA provider, but miss backup codes option.
To see the expected behavior on this branch, you'll have to re-generate the backup codes.