Require admin status in project to add it as a subproject

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):
@@ -309,7 +310,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 +320,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 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)))
+        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/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()
 
@@ -420,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]))

readthedocs/rtd_tests/tests/test_subproject.py

@@ -0,0 +1,56 @@
+from django.contrib.auth.models import User
+from django.test import TestCase
+from django_dynamic_fixture import get
+
+from readthedocs.projects.forms import SubprojectForm
+from readthedocs.projects.models import Project
+
+
+class SubprojectFormTests(TestCase):
+    def test_name_validation(self):
+        user = get(User)
+        project = get(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 = get(User)
+        project = get(Project, slug='mainproject')
+        project.users.add(user)
+        subproject = get(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 = get(User)
+        project = get(Project, slug='mainproject')
+        project.users.add(user)
+        subproject = get(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])

readthedocs/rtd_tests/tests/test_views.py

@@ -2,10 +2,13 @@
 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 get
+from django_dynamic_fixture import new
 
 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):
@@ -168,3 +171,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 = new(User, username='test')
+        self.user.set_password('test')
+        self.user.save()
+
+        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')
+
+    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(AdminPermission.is_admin(self.user, self.subproject))
+
+        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()])

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