<div dir="ltr"> <div class="gmail_quote"><div dir="ltr">On Thu, May 17, 2018 at 7:08 PM Fam Zheng <<a href="mailto:famz@redhat.com">famz@redhat.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">On Thu, 05/17 18:49, Shubham Jain wrote: > The test check for: > - user that is not a maintainer of any project should not result in any message being imported > - user that is not a maintainer of a project, but is in the importer groups, should result in the message being imported to all recognized projects > - user that is a maintainer of a project and is not in the importer group, should result in the message being imported to recognized & maintained projects The commit message is misleading. You want to mention the code change as well. </blockquote><div>I'll try to write better commit message next time. </div><div><div class="gmail-adm"><div id="gmail-q_667" class="gmail-ajR gmail-h4"></div></div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"> > --- > api/rest.py | 19 +++++++++++++++---- > tests/test_rest.py | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++---- > 2 files changed, 65 insertions(+), 8 deletions(-) > > diff --git a/api/rest.py b/api/rest.py > index 94b3162..3499250 100644 > --- a/api/rest.py > +++ b/api/rest.py > @@ -48,6 +48,12 @@ class PatchewPermission(permissions.BasePermission): > def has_project_permission(self, request, view, obj): > return obj.maintained_by(request.user) > > + def has_maintainer_permission(self, request, view): > + for p in Project.objects.all(): > + if p.maintained_by(request.user): > + return True > + return False > + > def has_message_permission(self, request, view, obj): > return obj.project.maintained_by(request.user) > > @@ -60,7 +66,8 @@ class PatchewPermission(permissions.BasePermission): > def has_generic_permission(self, request, view): > return (request.method in permissions.SAFE_METHODS) or \ > self.is_superuser(request) or \ > - self.has_group_permission(request, view) > + self.has_group_permission(request, view) or \ > + self.has_maintainer_permission(request, view) Does this do what we want? If the user maintains ANY project, it gets the permission, regardless of the project in request, which may not be the same one. </blockquote><div>Changing it to just IsAuthenticated class as Paolo suggested </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"> > > def has_permission(self, request, view): > return self.has_generic_permission(request, view) or \ > @@ -394,9 +401,13 @@ class MessagesViewSet(BaseMessageViewSet): > parser_classes = (JSONParser, MessagePlainTextParser, ) > > def create(self, request, *args, **kwargs): > - projects = [p for p in Project.objects.all() if p.recognizes(MboxMessage(self.request.data['mbox']))] > - if 'importers' not in self.request.user.groups.all(): > - projects = set(projects) & set([p for p in Project.objects.all() if p.maintained_by(self.request.user)]) > + m = MboxMessage(self.request.data['mbox']) > + user_groups = request.user.groups.all() > + user_group_names = [<a href="http://grp.name" rel="noreferrer" target="_blank">grp.name</a> for grp in user_groups] > + if request.user.is_superuser or 'importers' in user_group_names: > + projects = [p for p in Project.objects.all() if p.recognizes(m)] > + else: > + projects = [p for p in Project.objects.all() if p.maintained_by(self.request.user)] I think the else branch should still union the p.recognizes() filtering. </blockquote><div>As you have mentioned in the test case below "test_maintainer_of_project_x_import_a_patch_to_project_y()", does this mean even if user is maintainer of one project, but result in message is simply imported to all the recognised project. If yes, why are we just not using only recognised condition? </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"> > results = [] > for project in projects: > serializer = self.get_serializer(data=request.data) > diff --git a/tests/test_rest.py b/tests/test_rest.py > index 2da5459..af21c7b 100755 > --- a/tests/test_rest.py > +++ b/tests/test_rest.py > @@ -274,7 +274,7 @@ class RestTest(PatchewTestCase): > self.assertEqual(resp_after.status_code, 404) > self.assertEqual(resp_reply_after.status_code, 404) > > - def test_create_message(self): > + def test_json_create_project_message(self): Why the renames? Should they belong to a separate patch? > dp = self.get_data_path("0022-another-simple-patch.json.gz") > with open(dp, "r") as f: > data = f.read() > @@ -286,7 +286,7 @@ class RestTest(PatchewTestCase): > self.assertEqual(resp.data['subject'], "[Qemu-devel] [PATCH v2 10/27] imx_fec: Reserve full 4K " > "page for the register file") > > - def test_create_text_message(self): > + def test_text_create_project_message(self): > dp = self.get_data_path("0004-multiple-patch-reviewed.mbox.gz") > with open(dp, "r") as f: > data = f.read() > @@ -297,7 +297,7 @@ class RestTest(PatchewTestCase): > self.assertEqual(resp_get.status_code, 200) > self.assertEqual(resp.data['subject'], "[Qemu-devel] [PATCH v4 0/2] Report format specific info for LUKS block driver") > > - def test_create_message_without_project_pk(self): > + def test_json_create_message(self): > dp = self.get_data_path("0024-multiple-project-patch.json.gz") > with open(dp, "r") as f: > data = f.read() > @@ -311,7 +311,7 @@ class RestTest(PatchewTestCase): > resp_get2 = self.api_client.get(self.PROJECT_BASE_2 + "messages/<a href="http://20180223132311.26555-2-marcandre.lureau@redhat.com/" rel="noreferrer" target="_blank">20180223132311.26555-2-marcandre.lureau@redhat.com/</a>") > self.assertEqual(resp_get2.status_code, 200) > > - def test_create_text_message_without_project_pk(self): > + def test_text_create_message(self): > dp = self.get_data_path("0023-multiple-project-patch.mbox.gz") > with open(dp, "r") as f: > data = f.read() > @@ -325,6 +325,52 @@ class RestTest(PatchewTestCase): > resp_get2 = self.api_client.get(self.PROJECT_BASE_2 + "messages/<a href="http://20180223132311.26555-2-marcandre.lureau@redhat.com/" rel="noreferrer" target="_blank">20180223132311.26555-2-marcandre.lureau@redhat.com/</a>") > self.assertEqual(resp_get2.status_code, 200) > > + def test_without_login_create_message(self): > + dp = self.get_data_path("0022-another-simple-patch.json.gz") > + with open(dp, "r") as f: > + data = f.read() > + resp = self.api_client.post(self.PROJECT_BASE + "messages/", data, content_type='message/rfc822') > + self.assertEqual(resp.status_code, 403) > + > + def test_non_maintainer_create_message(self): > + self.create_user(username="test", password="userpass") > + self.api_client.login(username="test", password="userpass") > + dp = self.get_data_path("0023-multiple-project-patch.mbox.gz") > + with open(dp, "r") as f: > + data = f.read() > + resp = self.api_client.post(self.REST_BASE + "messages/", data, content_type='message/rfc822') > + self.assertEqual(resp.status_code, 403) > + > + def test_maintainer_create_message(self): > + test = self.create_user(username="test", password="userpass") > + self.api_client.login(username="test", password="userpass") > + self.p.maintainers = (test, ) > + dp = self.get_data_path("0023-multiple-project-patch.mbox.gz") > + with open(dp, "r") as f: > + data = f.read() > + resp = self.api_client.post(self.REST_BASE + "messages/", data, content_type='message/rfc822') > + self.assertEqual(resp.status_code, 201) > + self.assertEqual(resp.data['count'], 1) > + resp_get = self.api_client.get(self.PROJECT_BASE + "messages/<a href="http://20180223132311.26555-2-marcandre.lureau@redhat.com/" rel="noreferrer" target="_blank">20180223132311.26555-2-marcandre.lureau@redhat.com/</a>") > + self.assertEqual(resp_get.status_code, 200) > + resp_get2 = self.api_client.get(self.PROJECT_BASE_2 + "messages/<a href="http://20180223132311.26555-2-marcandre.lureau@redhat.com/" rel="noreferrer" target="_blank">20180223132311.26555-2-marcandre.lureau@redhat.com/</a>") > + self.assertEqual(resp_get2.status_code, 404) > + > + def test_importer_create_message(self): > + dp = self.get_data_path("0023-multiple-project-patch.mbox.gz") > + with open(dp, "r") as f: > + data = f.read() > + test = self.create_user(username="test", password="userpass", groups=['importers']) > + self.api_client.login(username="test", password="userpass") > + resp = self.api_client.post(self.REST_BASE + "messages/", data, content_type='message/rfc822') > + self.assertEqual(resp.status_code, 201) > + self.assertEqual(resp.data['count'], 2) > + resp_get = self.api_client.get(self.PROJECT_BASE + "messages/<a href="http://20180223132311.26555-2-marcandre.lureau@redhat.com/" rel="noreferrer" target="_blank">20180223132311.26555-2-marcandre.lureau@redhat.com/</a>") > + self.assertEqual(resp_get.status_code, 200) > + self.assertEqual(resp_get.data['subject'], "[Qemu-devel] [PATCH 1/7] SecurityPkg/Tcg2Pei: drop Tcg2PhysicalPresenceLib dependency") > + resp_get2 = self.api_client.get(self.PROJECT_BASE_2 + "messages/<a href="http://20180223132311.26555-2-marcandre.lureau@redhat.com/" rel="noreferrer" target="_blank">20180223132311.26555-2-marcandre.lureau@redhat.com/</a>") > + self.assertEqual(resp_get2.status_code, 200) Please also add a "test_maintainer_of_project_x_import_a_patch_to_project_y()" case. Fam > + > def test_message(self): > series = self.apply_and_retrieve('0001-simple-patch.mbox.gz', > <a href="http://self.p.id" rel="noreferrer" target="_blank">self.p.id</a>, '<a href="mailto:20160628014747.20971-1-famz@redhat.com" target="_blank">20160628014747.20971-1-famz@redhat.com</a>') > -- > 2.14.3 (Apple Git-98) > > _______________________________________________ > Patchew-devel mailing list > <a href="mailto:Patchew-devel@redhat.com" target="_blank">Patchew-devel@redhat.com</a> > <a href="https://www.redhat.com/mailman/listinfo/patchew-devel" rel="noreferrer" target="_blank">https://www.redhat.com/mailman/listinfo/patchew-devel</a> </blockquote></div></div>