Disabling 2FA should also work with backup tokens.

[valberg]

Fixes #152

Since this uses the OTPAuthenticationFormMixin the form field name has been changed from token to otp_token - which is backwards incompatible. Maybe we could do something to mitigate this?

[akx]

Since this uses the OTPAuthenticationFormMixin the form field name has been changed from token to otp_token - which is backwards incompatible. Maybe we could do something to mitigate this?

Have the form's ctor (or validation) check whether self.fields includes token, and raise an ImproperlyConfigured that tells the user to make their subclass compatible?

[codecov-commenter]

Codecov Report

Merging #155 (247c42b) into main (aec9e78) will increase coverage by 2.58%. The diff coverage is 94.73%.

@@            Coverage Diff             @@
##             main     #155      +/-   ##
==========================================
+ Coverage   89.47%   92.06%   +2.58%     
==========================================
  Files          14       14              
  Lines         475      504      +29     
==========================================
+ Hits          425      464      +39     
+ Misses         50       40      -10     

Impacted Files	Coverage Δ
allauth_2fa/forms.py	93.44% <86.95%> (+11.96%)	:arrow_up:
tests/test_allauth_2fa.py	100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[valberg]

Since this uses the OTPAuthenticationFormMixin the form field name has been changed from token to otp_token - which is backwards incompatible. Maybe we could do something to mitigate this?

Have the form's ctor (or validation) check whether self.fields includes token, and raise an ImproperlyConfigured that tells the user to make their subclass compatible?

Yes, or do

        if "token" in self.cleaned_data:
            self.cleaned_data["otp_token"] = self.cleaned_data["token"]

in clean and adding a deprecation warning?

But I think that depends on whether our next release is a 1.0.0 or not?

[akx]

... adding a deprecation warning?

But I think that depends on whether our next release is a 1.0.0 or not?

I think we'll want to fail hard nevertheless, because otherwise a subclass with token = ... will result in a form that has two token fields?

[valberg]

... adding a deprecation warning? But I think that depends on whether our next release is a 1.0.0 or not?

I think we'll want to fail hard nevertheless, because otherwise a subclass with token = ... will result in a form that has two token fields?

No, OTPAuthenticationFormMixin (https://github.com/django-otp/django-otp/blob/master/src/django_otp/forms.py#L11) does not include any fields, only methods to do the cleaning.

[akx]

No, OTPAuthenticationFormMixin (https://github.com/django-otp/django-otp/blob/master/src/django_otp/forms.py#L11) does not include any fields, only methods to do the cleaning.

Yeah, sorry, I meant an user subclassing allauth_2fa.TOTPDeviceRemoveForm in their own app, à la

class MyTOTPDeviceRemoveForm(TOTPDeviceRemoveForm):
    token = forms...

to e.g. override a label. Then they'd get the otp_token from this impl and token from their own; previously their token would've overridden this.

[valberg]

But what I'm mostly concerned about is templates using the form. If someone has styled the form with {{ form.token }} they are going to have a bad day.

[valberg]

No, OTPAuthenticationFormMixin (https://github.com/django-otp/django-otp/blob/master/src/django_otp/forms.py#L11) does not include any fields, only methods to do the cleaning.

Yeah, sorry, I meant an user subclassing allauth_2fa.TOTPDeviceRemoveForm in their own app, à la

class MyTOTPDeviceRemoveForm(TOTPDeviceRemoveForm):
    token = forms...

to e.g. override a label. Then they'd get the otp_token from this impl and token from their own; previously their token would've overridden this.

Yes, true true - I also got a bit confused (there are three layers here, django-otp, django-allauth-2fa and then someone using django-allauth-2fa) :laughing:

I'm inclined to say that it is an backwards incompatible issue that we write about in the release notes. Not sure how to do an ImproperlyConfigured exception for something in a template, other than checking if kwargs.get("data") includes token and not otp_token.

[valberg]

@akx I've added a suggestion on how to handle it.

[akx]

But what I'm mostly concerned about is templates using the form. If someone has styled the form with {{ form.token }} they are going to have a bad day. [...] Not sure how to do an ImproperlyConfigured exception for something in a template...

Technically we could do a @property token that also yells at the user...

I'm inclined to say that it is an backwards incompatible issue that we write about in the release notes.

Definitely. For consistency, we should also look at all of the other places where we could use the mixins and/or rename fields to otp_* anyway...

[akx]

@valberg Codecov seems to say we're not testing TOTPDeviceForm.clean_otp_token() at all..? 🤔

[valberg]

@valberg Codecov seems to say we're not testing TOTPDeviceForm.clean_otp_token() at all..? thinking

Hmm, yes, guess we'll have to test that.

[valberg]

@akx I think this is ready now :smiley: