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

Author: gregmuellegger

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
 
 

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:

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

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