Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Authenticating with backup token should remove token step from final form list #709

Open
gherceg opened this issue Feb 24, 2024 · 1 comment

Comments

@gherceg
Copy link
Contributor

gherceg commented Feb 24, 2024

Expected Behavior

Once a valid backup token is submitted and saved in validated_step_data, the token step/form should be filtered out of the form_list for the LoginView. I could be wrong here, but based on what I've observed so far, that is my read of the code.

When render_done is called after submitting a backup token, it attempts to validate all forms in the final form list, including the form for the token step since the condition (defined in LoginView.has_token_step) evaluates to True. However, I'd assume that if a valid backup token is used, the token step should not be included in the final form list.

For reference, the condition for including the backup step already performs similar logic where it checks if a valid token step has already been completed, and if so, evaluates to False (see LoginView.has_backup_step).

Current Behavior

Currently, render_done will grab the token form object, which has idempotent set to False meaning it won't attempt to revalidate anyway, but notably the form is not valid at this point. There could be a reason to still include this form that I'm unaware of, but to me it doesn't seem necessary.

I think this only presents an issue once custom registry methods are being used, because the render_done method will fetch the form for the token step, triggering this logic (found here):

if (step or self.steps.current) == self.TOKEN_STEP:
    # Set form class dynamically depending on user device.
    method = registry.method_from_device(self.get_device())
    self.form_list[self.TOKEN_STEP] = method.get_token_form_class()

For a backup token, self.get_device() will return a StaticDevice, but my custom generator method does not recognize this device type, and in my opinion it shouldn't. If no registered method recognizes the StaticDevice type, then method_from_device defaults to the built-in GeneratorMethod. My code does not anticipate this and attempts to send custom arguments that I've setup for my custom forms to the two_factor.forms.TOTPDeviceForm which results in a TypeError.

Possible Solution

I think this is the most promising solution which I can either make on my end in the view that inherits from LoginView, or in the two_factor LoginView directly. We can mimic the logic performed in has_backup_step to ensure the token step is filtered out of the form list once a valid backup token has been submitted.

def has_token_step(self):
    return (
        default_device(self.get_user()) and
        self.BACKUP_STEP not in self.storage.validated_step_data and  # this line is new
        not self.remember_agent
    )

Another solution I've verified though do not like is to update my custom generator method's recognize_device method to recognize devices of type StaticDevice. Unless there is something I'm missing, the generator method should never need to recognize a StaticDevice, but this enables the custom token form to be included in the final form list and avoids trying to pass custom arguments into built-in two_factor forms.

Steps to Reproduce (for bugs)

  1. Implement a custom generator method and form that accepts custom arguments in its init.
class CustomGeneratorMethod(GeneratorMethod):
   ...
    def get_token_form_class(self):
        return CustomAuthenticationTokenForm

    def recognize_device(self, device):
        return isinstance(device, TOTPDevice) or isinstance(device, StaticDevice)

class CustomAuthenticationTokenForm(AuthenticationTokenForm):
    ...
    def __init__(self, user, initiali_device, custom_arg, **kwargs):
        super().__init__(user, initial_device, **kwargs)
        self.custom_arg = custom_arg
  1. Make sure this method is registered instead of the built-in generator method and the LoginView is aware of passing custom_arg into it.
  2. Attempt to login with backup token
  3. Should encounter TypeError: __init__() got an unexpected keyword argument 'custom_arg'

Context

I've explained this in the Current Behavior section.

Your Environment

  • Browser and version: Chrome 121.0.6167.160
  • Python version: 3.9.18
  • Django version: 3.2.23
  • django-otp version: 1.1.3
  • django-two-factor-auth version: 1.15.5
  • Link to your project:
@claudep
Copy link
Contributor

claudep commented Feb 26, 2024

Feel free to cook a PR with your privileged proposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants