jazzband / django-two-factor-auth

Complete Two-Factor Authentication for Django providing the easiest integration into most Django projects.
MIT License
1.71k stars 448 forks source link

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

Open gherceg opened 9 months ago

gherceg commented 9 months ago

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


2. Make sure this method is registered instead of the built-in generator method and the LoginView is aware of passing custom_arg into it. 
3. Attempt to login with backup token
4. Should encounter `TypeError: __init__() got an unexpected keyword argument 'custom_arg'`

## Context
<!--- How has this issue affected you? What are you trying to accomplish? -->
<!--- Providing context helps us come up with a solution that is most useful in the real world -->
I've explained this in the Current Behavior section.
## Your Environment
<!--- Include as many relevant details about the environment you experienced the bug in -->
* 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 commented 9 months ago

Feel free to cook a PR with your privileged proposal.