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] 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])