From 71c3b2b001bf9b25e337691c4a3dcc92c9e96d95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gregor=20M=C3=BCllegger?= Date: Thu, 30 Jul 2015 18:12:48 +0200 Subject: [PATCH 1/6] Add django-dynamic-fixture for programmatic test fixture creation --- requirements/pip.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/requirements/pip.txt b/requirements/pip.txt index b947fb7c410..fe10dd76ef3 100644 --- a/requirements/pip.txt +++ b/requirements/pip.txt @@ -53,6 +53,7 @@ stripe==1.20.2 factory-boy==2.4.1 django-copyright==1.0.0 django-formtools==1.0 +django-dynamic-fixture==1.8.5 # Docs sphinx-http-domain==0.2 From 0e93ce41ff14bf1383a095abcb47ca88cec5babf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gregor=20M=C3=BCllegger?= Date: Thu, 30 Jul 2015 18:15:41 +0200 Subject: [PATCH 2/6] Validate user as admin of subproject when adding it to project It was previously possible to add subprojects without beeing admin of them. It should be required to be a admin. Otherwise people end up getting incorporated into a project without knowing it or beeing asked. Related to #1122 --- readthedocs/projects/forms.py | 14 +++-- readthedocs/projects/models.py | 3 + readthedocs/projects/views/private.py | 20 ++++--- .../rtd_tests/tests/test_subproject.py | 56 +++++++++++++++++++ 4 files changed, 82 insertions(+), 11 deletions(-) create mode 100644 readthedocs/rtd_tests/tests/test_subproject.py diff --git a/readthedocs/projects/forms.py b/readthedocs/projects/forms.py index c242216b43c..ecf3adc3982 100644 --- a/readthedocs/projects/forms.py +++ b/readthedocs/projects/forms.py @@ -309,7 +309,8 @@ class SubprojectForm(forms.Form): subproject = forms.CharField() def __init__(self, *args, **kwargs): - self.parent = kwargs.pop('parent', None) + self.user = kwargs.pop('user') + self.parent = kwargs.pop('parent') super(SubprojectForm, self).__init__(*args, **kwargs) def clean_subproject(self): @@ -318,11 +319,16 @@ def clean_subproject(self): if not subproject_qs.exists(): raise forms.ValidationError((_("Project %(name)s does not exist") % {'name': subproject_name})) - self.subproject = subproject_qs[0] - return subproject_name + subproject = subproject_qs.first() + if not subproject.user_is_admin(self.user): + raise forms.ValidationError(_( + 'You need to be admin of {name} in order to add it as ' + 'a subproject.'.format(name=subproject_name))) + return subproject def save(self): - relationship = self.parent.add_subproject(self.subproject) + relationship = self.parent.add_subproject( + self.cleaned_data['subproject']) return relationship diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index 5d195753082..91e13d89562 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -253,6 +253,9 @@ def sync_supported_versions(self): verbose_name__in=supported).update(supported=False) self.versions.filter(verbose_name=LATEST_VERBOSE_NAME).update(supported=True) + def user_is_admin(self, user): + return user in self.users.all() + def save(self, *args, **kwargs): first_save = self.pk is None if not self.slug: diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py index a1ad15363b7..91f395e3425 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -400,13 +400,19 @@ def project_subprojects(request, project_slug): project = get_object_or_404(Project.objects.for_admin_user(request.user), slug=project_slug) - form = SubprojectForm(data=request.POST or None, parent=project) - - if request.method == 'POST' and form.is_valid(): - form.save() - project_dashboard = reverse( - 'projects_subprojects', args=[project.slug]) - return HttpResponseRedirect(project_dashboard) + form_kwargs = { + 'parent': project, + 'user': request.user, + } + if request.method == 'POST': + form = SubprojectForm(request.POST, **form_kwargs) + if form.is_valid(): + form.save() + project_dashboard = reverse( + 'projects_subprojects', args=[project.slug]) + return HttpResponseRedirect(project_dashboard) + else: + form = SubprojectForm(**form_kwargs) subprojects = project.subprojects.all() diff --git a/readthedocs/rtd_tests/tests/test_subproject.py b/readthedocs/rtd_tests/tests/test_subproject.py new file mode 100644 index 00000000000..34fd1d15a1c --- /dev/null +++ b/readthedocs/rtd_tests/tests/test_subproject.py @@ -0,0 +1,56 @@ +from django.contrib.auth.models import User +from django.test import TestCase +from projects.forms import SubprojectForm +from django_dynamic_fixture import G + +from projects.models import Project + + +class SubprojectFormTests(TestCase): + def test_name_validation(self): + user = G(User) + project = G(Project, slug='mainproject') + + form = SubprojectForm({}, + parent=project, user=user) + form.full_clean() + self.assertTrue('subproject' in form.errors) + + form = SubprojectForm({'name': 'not-existent'}, + parent=project, user=user) + form.full_clean() + self.assertTrue('subproject' in form.errors) + + def test_adding_subproject_fails_when_user_is_not_admin(self): + # Make sure that a user cannot add a subproject that he is not the + # admin of. + + user = G(User) + project = G(Project, slug='mainproject') + project.users.add(user) + subproject = G(Project, slug='subproject') + + form = SubprojectForm({'subproject': subproject.slug}, + parent=project, user=user) + # Fails because user does not own subproject. + form.full_clean() + self.assertTrue('subproject' in form.errors) + + def test_admin_of_subproject_can_add_it(self): + user = G(User) + project = G(Project, slug='mainproject') + project.users.add(user) + subproject = G(Project, slug='subproject') + subproject.users.add(user) + + # Works now as user is admin of subproject. + form = SubprojectForm({'subproject': subproject.slug}, + parent=project, user=user) + # Fails because user does not own subproject. + form.full_clean() + self.assertTrue(form.is_valid()) + form.save() + + self.assertEqual( + [r.child for r in project.subprojects.all()], + [subproject]) From dcb5049f057c09587646f567198964c9f4bd0e53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gregor=20M=C3=BCllegger?= Date: Thu, 30 Jul 2015 18:46:28 +0200 Subject: [PATCH 3/6] Allow removing subprojects for non-subproject admins Also adding tests on removing subprojects --- readthedocs/projects/views/private.py | 2 +- readthedocs/rtd_tests/tests/test_views.py | 36 +++++++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py index 91f395e3425..d623e4f5e96 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -426,7 +426,7 @@ def project_subprojects(request, project_slug): @login_required def project_subprojects_delete(request, project_slug, child_slug): parent = get_object_or_404(Project.objects.for_admin_user(request.user), slug=project_slug) - child = get_object_or_404(Project.objects.for_admin_user(request.user), slug=child_slug) + child = get_object_or_404(Project.objects.all(), slug=child_slug) parent.remove_subproject(child) return HttpResponseRedirect(reverse('projects_subprojects', args=[parent.slug])) diff --git a/readthedocs/rtd_tests/tests/test_views.py b/readthedocs/rtd_tests/tests/test_views.py index 1bdc7c5842a..cfce8497910 100644 --- a/readthedocs/rtd_tests/tests/test_views.py +++ b/readthedocs/rtd_tests/tests/test_views.py @@ -2,6 +2,7 @@ from django.core.urlresolvers import reverse from django.test import TestCase from django.utils.six.moves.urllib.parse import urlsplit +from django_dynamic_fixture import G, N from readthedocs.builds.constants import LATEST from readthedocs.projects.models import Project @@ -168,3 +169,38 @@ def test_project_redirects(self): def test_project_redirects_delete(self): response = self.client.get('/dashboard/pip/redirects/delete/') self.assertRedirectToLogin(response) + + +class SubprojectViewTests(TestCase): + def setUp(self): + self.user = N(User, username='test') + self.user.set_password('test') + self.user.save() + + self.project = G(Project, slug='my-mainproject') + self.subproject = G(Project, slug='my-subproject') + self.project.add_subproject(self.subproject) + + self.client.login(username='test', password='test') + + def test_deny_delete_for_non_project_admins(self): + response = self.client.get('/dashboard/my-mainproject/subprojects/delete/my-subproject/') + self.assertEqual(response.status_code, 404) + + self.assertTrue(self.subproject in [r.child for r in self.project.subprojects.all()]) + + def test_admins_can_delete_subprojects(self): + self.project.users.add(self.user) + self.subproject.users.add(self.user) + + response = self.client.get('/dashboard/my-mainproject/subprojects/delete/my-subproject/') + self.assertEqual(response.status_code, 302) + self.assertTrue(self.subproject not in [r.child for r in self.project.subprojects.all()]) + + def test_project_admins_can_delete_subprojects_that_they_are_not_admin_of(self): + self.project.users.add(self.user) + self.assertFalse(self.subproject.user_is_admin(self.user)) + + response = self.client.get('/dashboard/my-mainproject/subprojects/delete/my-subproject/') + self.assertEqual(response.status_code, 302) + self.assertTrue(self.subproject not in [r.child for r in self.project.subprojects.all()]) From 43da77890a57ca38c0a50222ee9be48e4ca9ce4f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gregor=20M=C3=BCllegger?= Date: Fri, 31 Jul 2015 08:42:29 +0200 Subject: [PATCH 4/6] Adjust #1503 for readthedocs package namespacing introduced in #1496 --- readthedocs/rtd_tests/tests/test_subproject.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_subproject.py b/readthedocs/rtd_tests/tests/test_subproject.py index 34fd1d15a1c..deeca553fb2 100644 --- a/readthedocs/rtd_tests/tests/test_subproject.py +++ b/readthedocs/rtd_tests/tests/test_subproject.py @@ -1,9 +1,9 @@ from django.contrib.auth.models import User from django.test import TestCase -from projects.forms import SubprojectForm from django_dynamic_fixture import G -from projects.models import Project +from readthedocs.projects.forms import SubprojectForm +from readthedocs.projects.models import Project class SubprojectFormTests(TestCase): From 74a9c3a46f2c4d50d4e705bbabfba410841344fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gregor=20M=C3=BCllegger?= Date: Fri, 31 Jul 2015 08:43:20 +0200 Subject: [PATCH 5/6] Use django_dynamic_fixture's new/get instead of N/G Based on feedback: https://github.com/rtfd/readthedocs.org/pull/1503#discussion_r35933836 --- readthedocs/rtd_tests/tests/test_subproject.py | 18 +++++++++--------- readthedocs/rtd_tests/tests/test_views.py | 9 +++++---- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_subproject.py b/readthedocs/rtd_tests/tests/test_subproject.py index deeca553fb2..7e0e1c24298 100644 --- a/readthedocs/rtd_tests/tests/test_subproject.py +++ b/readthedocs/rtd_tests/tests/test_subproject.py @@ -1,6 +1,6 @@ from django.contrib.auth.models import User from django.test import TestCase -from django_dynamic_fixture import G +from django_dynamic_fixture import get from readthedocs.projects.forms import SubprojectForm from readthedocs.projects.models import Project @@ -8,8 +8,8 @@ class SubprojectFormTests(TestCase): def test_name_validation(self): - user = G(User) - project = G(Project, slug='mainproject') + user = get(User) + project = get(Project, slug='mainproject') form = SubprojectForm({}, parent=project, user=user) @@ -25,10 +25,10 @@ def test_adding_subproject_fails_when_user_is_not_admin(self): # Make sure that a user cannot add a subproject that he is not the # admin of. - user = G(User) - project = G(Project, slug='mainproject') + user = get(User) + project = get(Project, slug='mainproject') project.users.add(user) - subproject = G(Project, slug='subproject') + subproject = get(Project, slug='subproject') form = SubprojectForm({'subproject': subproject.slug}, parent=project, user=user) @@ -37,10 +37,10 @@ def test_adding_subproject_fails_when_user_is_not_admin(self): self.assertTrue('subproject' in form.errors) def test_admin_of_subproject_can_add_it(self): - user = G(User) - project = G(Project, slug='mainproject') + user = get(User) + project = get(Project, slug='mainproject') project.users.add(user) - subproject = G(Project, slug='subproject') + subproject = get(Project, slug='subproject') subproject.users.add(user) # Works now as user is admin of subproject. diff --git a/readthedocs/rtd_tests/tests/test_views.py b/readthedocs/rtd_tests/tests/test_views.py index cfce8497910..7fada7736a9 100644 --- a/readthedocs/rtd_tests/tests/test_views.py +++ b/readthedocs/rtd_tests/tests/test_views.py @@ -2,7 +2,8 @@ from django.core.urlresolvers import reverse from django.test import TestCase from django.utils.six.moves.urllib.parse import urlsplit -from django_dynamic_fixture import G, N +from django_dynamic_fixture import get +from django_dynamic_fixture import new from readthedocs.builds.constants import LATEST from readthedocs.projects.models import Project @@ -173,12 +174,12 @@ def test_project_redirects_delete(self): class SubprojectViewTests(TestCase): def setUp(self): - self.user = N(User, username='test') + self.user = new(User, username='test') self.user.set_password('test') self.user.save() - self.project = G(Project, slug='my-mainproject') - self.subproject = G(Project, slug='my-subproject') + self.project = get(Project, slug='my-mainproject') + self.subproject = get(Project, slug='my-subproject') self.project.add_subproject(self.subproject) self.client.login(username='test', password='test') From c9550d785547047ce7179af0e1ce1c762fccb663 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gregor=20M=C3=BCllegger?= Date: Fri, 31 Jul 2015 08:49:02 +0200 Subject: [PATCH 6/6] Use privacy.backend.AdminPermission.is_admin for testing project admins Based on feedback: https://github.com/rtfd/readthedocs.org/pull/1503#discussion_r35898932 --- readthedocs/projects/forms.py | 3 ++- readthedocs/projects/models.py | 3 --- readthedocs/rtd_tests/tests/test_views.py | 3 ++- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/readthedocs/projects/forms.py b/readthedocs/projects/forms.py index ecf3adc3982..4b5140ce846 100644 --- a/readthedocs/projects/forms.py +++ b/readthedocs/projects/forms.py @@ -14,6 +14,7 @@ from readthedocs.redirects.models import Redirect from readthedocs.projects import constants from readthedocs.projects.models import Project, EmailHook, WebHook +from readthedocs.privacy.loader import AdminPermission class ProjectForm(forms.ModelForm): @@ -320,7 +321,7 @@ def clean_subproject(self): raise forms.ValidationError((_("Project %(name)s does not exist") % {'name': subproject_name})) subproject = subproject_qs.first() - if not subproject.user_is_admin(self.user): + if not AdminPermission.is_admin(self.user, subproject): raise forms.ValidationError(_( 'You need to be admin of {name} in order to add it as ' 'a subproject.'.format(name=subproject_name))) diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index 91e13d89562..5d195753082 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -253,9 +253,6 @@ def sync_supported_versions(self): verbose_name__in=supported).update(supported=False) self.versions.filter(verbose_name=LATEST_VERBOSE_NAME).update(supported=True) - def user_is_admin(self, user): - return user in self.users.all() - def save(self, *args, **kwargs): first_save = self.pk is None if not self.slug: diff --git a/readthedocs/rtd_tests/tests/test_views.py b/readthedocs/rtd_tests/tests/test_views.py index 7fada7736a9..58f77e48a2e 100644 --- a/readthedocs/rtd_tests/tests/test_views.py +++ b/readthedocs/rtd_tests/tests/test_views.py @@ -8,6 +8,7 @@ from readthedocs.builds.constants import LATEST from readthedocs.projects.models import Project from readthedocs.projects.forms import UpdateProjectForm +from readthedocs.privacy.loader import AdminPermission class Testmaker(TestCase): @@ -200,7 +201,7 @@ def test_admins_can_delete_subprojects(self): def test_project_admins_can_delete_subprojects_that_they_are_not_admin_of(self): self.project.users.add(self.user) - self.assertFalse(self.subproject.user_is_admin(self.user)) + self.assertFalse(AdminPermission.is_admin(self.user, self.subproject)) response = self.client.get('/dashboard/my-mainproject/subprojects/delete/my-subproject/') self.assertEqual(response.status_code, 302)