Skip to content

Events of TwoFactor provider and manager names are very confusing (at best) #39087

Description

@nickvergessen

The names of the typed events are in my opinion wrong and will lead to wrong assumptions:

Case 1

  • Dispatched when a user successfully passes a 2FA challange:
  • $dispatchEvent = new GenericEvent($user, ['provider' => $provider->getDisplayName()]);
    $this->legacyDispatcher->dispatch(IProvider::EVENT_SUCCESS, $dispatchEvent);
    $this->dispatcher->dispatchTyped(new TwoFactorProviderForUserEnabled($user, $provider));
    $this->publishEvent($user, 'twofactor_success', [
    'provider' => $provider->getDisplayName(),
    ]);
  • Legacy dispatcher: IProvider::EVENT_SUCCESS
  • Typed: OCP\Authentication\TwoFactorAuth\TwoFactorProviderForUserEnabled
    • 😕 ❓ Was the provider disabled for a user?!
    • 💥 No, it auth was passed and a better name would be OCP\Authentication\TwoFactorAuth\TwoFactorProviderChallangePassed
    • The old event should be deprecated immediately and both events be dispatched for the time being

Case 2

  • Dispatched when a user does not pass a 2FA challange:
  • $this->legacyDispatcher->dispatch(IProvider::EVENT_FAILED, $dispatchEvent);
    $this->dispatcher->dispatchTyped(new TwoFactorProviderForUserDisabled($user, $provider));
    $this->publishEvent($user, 'twofactor_failed', [
    'provider' => $provider->getDisplayName(),
    ]);
  • Legacy dispatcher: IProvider::EVENT_FAILED
  • Typed: OCP\Authentication\TwoFactorAuth\TwoFactorProviderForUserDisabled
    • 😕 ❓ Was the provider disabled for a user?!
    • 💥 No, it auth was failed and a better name would be OCP\Authentication\TwoFactorAuth\TwoFactorProviderChallangeFailed
    • The old event should be deprecated immediately and both events be dispatched for the time being

Case 3

  • Dispatched when a user is being deleted inside a listener for UserDeletedEvent
  • public function handle(Event $event): void {
    if (!($event instanceof UserDeletedEvent)) {
    return;
    }
    $this->registry->deleteUserData($event->getUser());
    }
  • public function deleteUserData(IUser $user): void {
    foreach ($this->assignmentDao->deleteByUser($user->getUID()) as $provider) {
    $event = new TwoFactorProviderDisabled($provider['provider_id']);
    $this->dispatcher->dispatchTyped($event);
    }
    }
  • Legacy dispatcher: none
  • Typed: OCP\Authentication\TwoFactorAuth\TwoFactorProviderDisabled
    • 😕 ❓ Was the provider disabled for everyone?!
    • 💥 No, all entries of a single user get deleted OCP\Authentication\TwoFactorAuth\TwoFactorProviderForUserDisabled
      • But that name is from case 2, so need a different one Maybe "TwoFactorProviderUnregisteredForUser"
    • The old event should be deprecated immediately and both events be dispatched for the time being

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

1. to developAccepted and waiting to be taken care ofbug

Type

No type

Projects

No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions