Repository: jazzband/django-two-factor-auth
Issue category: Security Vulnerability

Issue:
DisableView doesn't require request user to be verified
`DisableView` only checks that the request user *has* a valid, not that the user is verified.

It is possible that if a site has been misconfigured and allows users to bypass django-two-factor-auth's login view (e.g. the admin site hasn't been configured correctly and admins can login without 2fa) and then disable that user's two-factor device(s). At this point an attacker could go set up a new two-factor and gain access to views protected by `otp_required`.

This bug could also be exploited if the user has multiple sessions open before enabling two-factor auth. Any other session could then disable two-factor auth without having access to the device.

## Expected Behavior

`DisableView` should check if the current user has two factor auth enabled on their account *and* has used it to log in on this session.

## Current Behavior

`DisableView` only checks that the user has two factor auth enabled on their account.

## Possible Solution

Swap `login_required` for `otp_required`. Write some tests too.

## Steps to Reproduce (for bugs)

```
>>> from two_factor.views.profile import DisableView                                                                                                                                                   
>>> from django.contrib.auth import get_user_model
>>> from django.http import HttpRequest
>>> user = get_user_model().objects.first()
>>> user.staticdevice_set.get_or_create(name='backup')
>>> print(getattr(user, "is_verified", "user has no is_verified method"))
user has no is_verified method
>>> request = HttpRequest()
>>> request.user = user
>>> request.method = "GET"
>>> DisableView.as_view()(request)
<TemplateResponse status_code=200, "text/html; charset=utf-8">
>>> request.method = "POST"
>>> request.POST = {"understand": "on"}
>>> DisableView.as_view()(request)
<HttpResponseRedirect status_code=302, "text/html; charset=utf-8", url="/user/home/">
>>> print(user.staticdevice_set.count())
0
```

It should also be possible to reproduce this bug by logging in from two different browsers, enabling two factor auth on one session and then disabling it from the other (I've not tried it due to time constraints) 

## Context

<https://github.com/Bouke/django-two-factor-auth/issues/253#issuecomment-701881736> prompted me to look at the source code.


## Your Environment
<!--- Include as many relevant details about the environment you experienced the bug in -->
* Browser and version: n/a, this was done via the shell
* Python version: 3.6.12
* Django version: 2.2.15
* django-otp version: 0.9.4
* django-two-factor-auth version: 1.12.1
* Link to your project: https://github.com/Bouke/django-two-factor-auth

DisableView doesn't require request user to be verified
`DisableView` only checks that the request user *has* a valid, not that the user is verified.

It is possible that if a site has been misconfigured and allows users to bypass django-two-factor-auth's login view (e.g. the admin site hasn't been configured correctly and admins can login without 2fa) and then disable that user's two-factor device(s). At this point an attacker could go set up a new two-factor and gain access to views protected by `otp_required`.

This bug could also be exploited if the user has multiple sessions open before enabling two-factor auth. Any other session could then disable two-factor auth without having access to the device.

## Expected Behavior

`DisableView` should check if the current user has two factor auth enabled on their account *and* has used it to log in on this session.

## Current Behavior

`DisableView` only checks that the user has two factor auth enabled on their account.

## Possible Solution

Swap `login_required` for `otp_required`. Write some tests too.

## Steps to Reproduce (for bugs)

```
>>> from two_factor.views.profile import DisableView                                                                                                                                                   
>>> from django.contrib.auth import get_user_model
>>> from django.http import HttpRequest
>>> user = get_user_model().objects.first()
>>> user.staticdevice_set.get_or_create(name='backup')
>>> print(getattr(user, "is_verified", "user has no is_verified method"))
user has no is_verified method
>>> request = HttpRequest()
>>> request.user = user
>>> request.method = "GET"
>>> DisableView.as_view()(request)
<TemplateResponse status_code=200, "text/html; charset=utf-8">
>>> request.method = "POST"
>>> request.POST = {"understand": "on"}
>>> DisableView.as_view()(request)
<HttpResponseRedirect status_code=302, "text/html; charset=utf-8", url="/user/home/">
>>> print(user.staticdevice_set.count())
0
```

It should also be possible to reproduce this bug by logging in from two different browsers, enabling two factor auth on one session and then disabling it from the other (I've not tried it due to time constraints) 

## Context

<https://github.com/Bouke/django-two-factor-auth/issues/253#issuecomment-701881736> prompted me to look at the source code.


## Your Environment
<!--- Include as many relevant details about the environment you experienced the bug in -->
* Browser and version: n/a, this was done via the shell
* Python version: 3.6.12
* Django version: 2.2.15
* django-otp version: 0.9.4
* django-two-factor-auth version: 1.12.1
* Link to your project: https://github.com/Bouke/django-two-factor-auth



Hints:
I think this is a fairly minor security issue as it relies on things beyond our control going wrong first and no secrets are exposed. I should have PR ready by the end of the day.
I think this is a fairly minor security issue as it relies on things beyond our control going wrong first and no secrets are exposed. I should have PR ready by the end of the day.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index d242f7369..45b1cc581 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,8 @@
 
 ### Changed
 - The templates are now based on Bootstrap 4.
+- `DisableView` now checks user has verified before disabling two-factor on
+  their account
 
 ## 1.12 - 2020-07-08
 ### Added
diff --git a/two_factor/views/profile.py b/two_factor/views/profile.py
index 50802a0a3..e21ea9705 100644
--- a/two_factor/views/profile.py
+++ b/two_factor/views/profile.py
@@ -1,9 +1,11 @@
 from django.conf import settings
 from django.contrib.auth.decorators import login_required
 from django.shortcuts import redirect, resolve_url
+from django.utils.functional import lazy
 from django.views.decorators.cache import never_cache
 from django.views.generic import FormView, TemplateView
-from django_otp import devices_for_user, user_has_device
+from django_otp import devices_for_user
+from django_otp.decorators import otp_required
 
 from ..forms import DisableForm
 from ..models import get_available_phone_methods
@@ -39,21 +41,22 @@ def get_context_data(self, **kwargs):
 
 
 @class_view_decorator(never_cache)
-@class_view_decorator(login_required)
 class DisableView(FormView):
     """
     View for disabling two-factor for a user's account.
     """
     template_name = 'two_factor/profile/disable.html'
-    success_url = None
+    success_url = lazy(resolve_url, str)(settings.LOGIN_REDIRECT_URL)
     form_class = DisableForm
 
-    def get(self, request, *args, **kwargs):
-        if not user_has_device(self.request.user):
-            return redirect(self.success_url or resolve_url(settings.LOGIN_REDIRECT_URL))
-        return super().get(request, *args, **kwargs)
+    def dispatch(self, *args, **kwargs):
+        # We call otp_required here because we want to use self.success_url as
+        # the login_url. Using it as a class decorator would make it difficult
+        # for users who wish to override this property
+        fn = otp_required(super().dispatch, login_url=self.success_url, redirect_field_name=None)
+        return fn(*args, **kwargs)
 
     def form_valid(self, form):
         for device in devices_for_user(self.request.user):
             device.delete()
-        return redirect(self.success_url or resolve_url(settings.LOGIN_REDIRECT_URL))
+        return redirect(self.success_url)


Test patch:
diff --git a/tests/test_views_disable.py b/tests/test_views_disable.py
index 69a74a8ff..74b963f91 100644
--- a/tests/test_views_disable.py
+++ b/tests/test_views_disable.py
@@ -2,7 +2,7 @@
 from django.shortcuts import resolve_url
 from django.test import TestCase
 from django.urls import reverse
-from django_otp import devices_for_user
+from django_otp import DEVICE_ID_SESSION_KEY, devices_for_user
 
 from .utils import UserMixin
 
@@ -14,19 +14,30 @@ def setUp(self):
         self.enable_otp()
         self.login_user()
 
-    def test(self):
+    def test_get(self):
         response = self.client.get(reverse('two_factor:disable'))
         self.assertContains(response, 'Yes, I am sure')
 
+    def test_post_no_data(self):
         response = self.client.post(reverse('two_factor:disable'))
         self.assertEqual(response.context_data['form'].errors,
                          {'understand': ['This field is required.']})
 
+    def test_post_success(self):
         response = self.client.post(reverse('two_factor:disable'),
                                     {'understand': '1'})
         self.assertRedirects(response, resolve_url(settings.LOGIN_REDIRECT_URL))
         self.assertEqual(list(devices_for_user(self.user)), [])
 
-        # cannot disable twice
+    def test_cannot_disable_twice(self):
+        [i.delete() for i in devices_for_user(self.user)]
+        response = self.client.get(reverse('two_factor:disable'))
+        self.assertRedirects(response, resolve_url(settings.LOGIN_REDIRECT_URL))
+
+    def test_cannot_disable_without_verified(self):
+        # remove OTP data from session
+        session = self.client.session
+        del session[DEVICE_ID_SESSION_KEY]
+        session.save()
         response = self.client.get(reverse('two_factor:disable'))
         self.assertRedirects(response, resolve_url(settings.LOGIN_REDIRECT_URL))