[Patchew-devel] [PATCH] test: more testcases around authorization
Shubham Jain
shubhamjain7495 at gmail.com
Thu May 17 17:25:48 UTC 2018
On Thu, May 17, 2018 at 7:08 PM Fam Zheng <famz at redhat.com> wrote:
> 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.
>
> I'll try to write better commit message next time.
> > ---
> > 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.
>
> Changing it to just IsAuthenticated class as Paolo suggested
> >
> > 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 = [grp.name 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.
>
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?
>
> > 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/
> 20180223132311.26555-2-marcandre.lureau at redhat.com/")
> > 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/
> 20180223132311.26555-2-marcandre.lureau at redhat.com/")
> > 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/
> 20180223132311.26555-2-marcandre.lureau at redhat.com/")
> > + self.assertEqual(resp_get.status_code, 200)
> > + resp_get2 = self.api_client.get(self.PROJECT_BASE_2 + "messages/
> 20180223132311.26555-2-marcandre.lureau at redhat.com/")
> > + 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/
> 20180223132311.26555-2-marcandre.lureau at redhat.com/")
> > + 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/
> 20180223132311.26555-2-marcandre.lureau at redhat.com/")
> > + 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',
> > self.p.id, '
> 20160628014747.20971-1-famz at redhat.com')
> > --
> > 2.14.3 (Apple Git-98)
> >
> > _______________________________________________
> > Patchew-devel mailing list
> > Patchew-devel at redhat.com
> > https://www.redhat.com/mailman/listinfo/patchew-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/patchew-devel/attachments/20180517/2364fedb/attachment.htm>
More information about the Patchew-devel
mailing list