django-two-factor-auth icon indicating copy to clipboard operation
django-two-factor-auth copied to clipboard

How to subclass AuthenticationTokenView and use it in a subclassed LoginView

Open isparks-tg opened this issue 2 months ago • 2 comments

I have a subclassed LoginView which defines a set of forms:

class UserLogin(LoginView):
    form_list = (
        (LoginView.AUTH_STEP, MyAuthenticationForm),
        (LoginView.TOKEN_STEP, MyAuthenticationTokenForm),
        (LoginView.BACKUP_STEP, MyBackupTokenForm),
    )

To my surprise the TOKEN_STEP form was being switched to AuthenticationTokenForm, I finally tracked it down to code in the LoginView class

    def get_form(self, step=None, **kwargs):
        """
        Returns the form for the step
        """
        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() #<-- returns AuthenticationTokenForm from registry but in other setups would be other forms

It is not clear what the best approach is here. I overrode get_form() in my LoginView subclass to bypass this:

    def get_form(self, step=None, **kwargs):
        """
        Returns the form for the step
        """

        # This is a copy of code from core.py but it does not override the self.TOKEN_STEP which LoginView does

        form = super(LoginView, self).get_form(step=step, **kwargs)
        if self.show_timeout_error:
            form.cleaned_data = getattr(form, 'cleaned_data', {})
            form.add_error(None, ValidationError(_('Your session has timed out. Please login again.')))
        return form

but possibly I should be doing something different/better maybe with registry?

It feels like a gap in the docs regarding what you do if you want to specify a fixed class in the token step and don't want it dynamically switching on you.

Expected Behavior

For my use case either it should respect the token step form defined in forms_list or block setting that value because it will be ignored, i.e.

    form_list = (
        (LoginView.AUTH_STEP, forms.MyAuthenticationForm),
        (LoginView.TOKEN_STEP, forms.MyAuthenticationTokenForm), <-- This should be a runtime error, don't do that?
        (LoginView.BACKUP_STEP, forms.MyBackupTokenForm),
    )

Current Behavior

Quietly switches classes on you behind your back without warning you that your TOKEN_STEP value is going to be overridden from the registry which is very confusing.

isparks-tg avatar Apr 11 '24 13:04 isparks-tg

That is surprising and a little bit confusing too! I'm almost tempted to call this a bug as we populate form_list and then just go and ignore it - I feel like we should do something different to make it clearer that we're going to call the registry for that particular form.

moggers87 avatar Apr 11 '24 14:04 moggers87

I have the same issues, my token auth form is getting replaced and it took me a lot to figure out why,

I hope this can be fixed ASAP

aarighi avatar May 02 '24 14:05 aarighi