Skip to content

Commit

Permalink
Prevent unauthorized users managing others' device
Browse files Browse the repository at this point in the history
This adds a permissions check on the device list and delete views. When
a user has the 'change_user' permission, they are allowed to view
and delete other users' 2FA devices. If they don't have the
permission, they are only allowed to manage their own devices.
  • Loading branch information
nnist authored and mvantellingen committed Feb 10, 2020
1 parent ad8d77a commit ac23550
Show file tree
Hide file tree
Showing 2 changed files with 94 additions and 2 deletions.
21 changes: 21 additions & 0 deletions src/wagtail_2fa/views.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
from django.core.exceptions import PermissionDenied
import qrcode
import qrcode.image.svg
from django.conf import settings
Expand Down Expand Up @@ -75,6 +76,15 @@ def get_context_data(self, **kwargs):
context['user_id'] = int(self.kwargs['user_id'])
return context

def dispatch(self, request, *args, **kwargs):
if (int(self.kwargs["user_id"]) == request.user.pk or
request.user.has_perm("user.change_user")):
if not self.user_allowed(request.user):
return self.handle_no_permission(request)

return super(OtpRequiredMixin, self).dispatch(request, *args, **kwargs)
raise PermissionDenied


class DeviceCreateView(OtpRequiredMixin, FormView):
form_class = forms.DeviceForm
Expand Down Expand Up @@ -134,6 +144,17 @@ def get_queryset(self):
def get_success_url(self):
return reverse('wagtail_2fa_device_list', kwargs={'user_id': self.request.POST.get('user_id')})

def dispatch(self, request, *args, **kwargs):
device = TOTPDevice.objects.get(**self.kwargs)

if device.user.pk == request.user.pk or request.user.has_perm("user.change_user"):
if not self.user_allowed(request.user):
return self.handle_no_permission(request)

return super(OtpRequiredMixin, self).dispatch(request, *args, **kwargs)

raise PermissionDenied


class DeviceQRCodeView(OtpRequiredMixin, View):
# require OTP if configured
Expand Down
75 changes: 73 additions & 2 deletions tests/test_views.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
import pytest
from django.core.exceptions import PermissionDenied
from django.contrib.auth.models import Permission
from django.contrib.contenttypes.models import ContentType
from django.test import override_settings
from unittest.mock import patch
from django.http.response import Http404

from django.contrib.auth import get_user_model
from django.urls import reverse
from django_otp import DEVICE_ID_SESSION_KEY
from django_otp.plugins.otp_totp.models import TOTPDevice
from wagtail_2fa.views import DeviceListView

from wagtail_2fa.views import DeviceListView, DeviceDeleteView, DeviceUpdateView

def test_device_list_view(admin_client, admin_user, django_assert_num_queries):
with override_settings(WAGTAIL_2FA_REQUIRED=True):
Expand Down Expand Up @@ -129,3 +133,70 @@ def test_delete_user_device_unauthorized(client, user, monkeypatch):
})
assert response.status_code == 302
assert TOTPDevice.objects.all().count() == 1


class TestViewsChangeUserPermission:
"""Test suite which ensures that:
- users without the change_user permission cannot manage other users' 2FA devices
- users can manage their own devices
"""

def test_verified_user_has_no_change_user_perm(self, verified_user):
"""Sanity check."""
assert not verified_user.has_perm("user.change_user")

def test_device_list_view_for_own_user_returns_200(self, verified_user, rf):
with override_settings(WAGTAIL_2FA_REQUIRED=True):
request = rf.get('foo')
request.user = verified_user

response = DeviceListView.as_view()(request, user_id=verified_user.id)
assert response.status_code == 200

def test_device_list_view_for_other_user_raises_error(self, user, verified_user, rf):
with override_settings(WAGTAIL_2FA_REQUIRED=True):
request = rf.get('foo')
request.user = verified_user

with pytest.raises(PermissionDenied):
response = DeviceListView.as_view()(request, user_id=user.id)

def test_device_delete_view_for_own_user_returns_200(self, verified_user, rf):
with override_settings(WAGTAIL_2FA_REQUIRED=True):
device = TOTPDevice.objects.devices_for_user(verified_user, confirmed=True).first()
request = rf.get('foo')
request.user = verified_user

response = DeviceDeleteView.as_view()(request, pk=device.id)
assert response.status_code == 200

def test_device_delete_view_for_other_user_raises_error(self, user, verified_user, rf):
with override_settings(WAGTAIL_2FA_REQUIRED=True):
other_device = TOTPDevice.objects.create(name='Initial', user=user, confirmed=True)

device = TOTPDevice.objects.devices_for_user(verified_user, confirmed=True).first()
request = rf.get('foo')
request.user = verified_user

with pytest.raises(PermissionDenied):
response = DeviceDeleteView.as_view()(request, pk=other_device.id)

def test_device_update_view_for_own_user_returns_200(self, verified_user, rf):
with override_settings(WAGTAIL_2FA_REQUIRED=True):
device = TOTPDevice.objects.devices_for_user(verified_user, confirmed=True).first()
request = rf.get('foo')
request.user = verified_user

response = DeviceUpdateView.as_view()(request, pk=device.id)
assert response.status_code == 200

def test_device_update_view_for_other_user_raises_error(self, user, verified_user, rf):
with override_settings(WAGTAIL_2FA_REQUIRED=True):
other_device = TOTPDevice.objects.create(name='Initial', user=user, confirmed=True)

device = TOTPDevice.objects.devices_for_user(verified_user, confirmed=True).first()
request = rf.get('foo')
request.user = verified_user

with pytest.raises(Http404):
response = DeviceUpdateView.as_view()(request, pk=other_device.id)

0 comments on commit ac23550

Please sign in to comment.