api/rest.py | 19 +++++++++++++++---- tests/test_rest.py | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 65 insertions(+), 8 deletions(-)
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
---
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)
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)]
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):
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@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@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@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@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@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@redhat.com/")
+ self.assertEqual(resp_get2.status_code, 200)
+
def test_message(self):
series = self.apply_and_retrieve('0001-simple-patch.mbox.gz',
self.p.id, '20160628014747.20971-1-famz@redhat.com')
--
2.14.3 (Apple Git-98)
_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel
On 17/05/2018 15:19, 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
> ---
> 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
This can use request.user.project_set.exists() or something like that,
but see below...
> +
> 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)
>
> def has_permission(self, request, view):
> return self.has_generic_permission(request, view) or \
If you go this way, the extra check should be added only to
MessagesViewSet, perhaps by subclassing ImportPermission.
But there is another possibility: just use IsAuthenticatedOrReadOnly for
the MessagesViewSet. If a user is authenticated but not a maintainer,
simply the create() message will not recognize them, and it will
"import" the message into no project at all.
> @@ -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)]
You should keep both tests: maintainer _and_ recognized, exactly as in
the code that is there now. But actually I realized now that this
change is not needed, because superusers are maintainers of all
projects! So it's already working as Fam intended, and it's also being
tested already.
Paolo
> 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):
> 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@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@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@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@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@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@redhat.com/")
> + self.assertEqual(resp_get2.status_code, 200)
> +
> def test_message(self):
> series = self.apply_and_retrieve('0001-simple-patch.mbox.gz',
> self.p.id, '20160628014747.20971-1-famz@redhat.com')
>
_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel
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.
> ---
> 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.
>
> 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.
> 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@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@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@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@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@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@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@redhat.com')
> --
> 2.14.3 (Apple Git-98)
>
> _______________________________________________
> Patchew-devel mailing list
> Patchew-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/patchew-devel
_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel
On Thu, May 17, 2018 at 7:08 PM Fam Zheng <famz@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@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@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@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@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@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@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@redhat.com')
> > --
> > 2.14.3 (Apple Git-98)
> >
> > _______________________________________________
> > Patchew-devel mailing list
> > Patchew-devel@redhat.com
> > https://www.redhat.com/mailman/listinfo/patchew-devel
>
_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel
On 17/05/2018 19:25, Shubham Jain wrote: > > + 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? The idea is that if a user wants to import to a particular project he maintains, he uses /projects/.../messages. If a user wants to recognize the projects based on the recipients, he uses /messages. (In practice the former happens rarely, but it was already complicated enough and it makes sense to have it as part of the REST API!) Does it make sense? Paolo _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel
Yes. Could you please also explain how maintainers and importers play different role? On Thu, May 17, 2018 at 11:00 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > On 17/05/2018 19:25, Shubham Jain wrote: > > > + 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? > > The idea is that if a user wants to import to a particular project he > maintains, he uses /projects/.../messages. If a user wants to recognize > the projects based on the recipients, he uses /messages. > > (In practice the former happens rarely, but it was already complicated > enough and it makes sense to have it as part of the REST API!) > > Does it make sense? > > Paolo > _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel
On 17/05/2018 20:06, Shubham Jain wrote: > Yes. Could you please also explain how maintainers and importers play > different role? Maintainers import the messages manually using patchew-cli (for whatever reason). Importer users set up a background job that processes the messages for multiple projects from a single inbox, for example an IMAP account. Paolo > On Thu, May 17, 2018 at 11:00 PM Paolo Bonzini <pbonzini@redhat.com > <mailto:pbonzini@redhat.com>> wrote: > > On 17/05/2018 19:25, Shubham Jain wrote: > > > + 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? > > The idea is that if a user wants to import to a particular project he > maintains, he uses /projects/.../messages. If a user wants to recognize > the projects based on the recipients, he uses /messages. > > (In practice the former happens rarely, but it was already complicated > enough and it makes sense to have it as part of the REST API!) > > Does it make sense? > > Paolo > _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel
On Thu, 05/17 19:30, Paolo Bonzini wrote: > On 17/05/2018 19:25, Shubham Jain wrote: > > > + 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? Here is how maintainership and recoginsed condition work together. Usually, all patches in all projects are imported by "importers", it's a "bot". Maintainers are humans. He could "override" importers in the scope where he maintains a project, e.g. by importing additional patches or removing unwanted patches. But a maintainer of project X shouldn't be allowed to make such changes to project Y. On the other hand, user A could be a maintainer for both projects A & B, but not for C. When he submits a new Message M, it could end up in project A or project B, or both, depending on the p.recognizes() test, but M cannot be added to project C because he doesn't maintain it, even if he attempts to, or project C does recognize the message. On the other hand, AND'ing p.maintained_by() and p.recognizes() makes it easy for the user since he only needs to submit for once, leaving the project list to the server. In this case the message shouldn't be added to _all_ maintained projects. > > The idea is that if a user wants to import to a particular project he > maintains, he uses /projects/.../messages. If a user wants to recognize > the projects based on the recipients, he uses /messages. > > (In practice the former happens rarely, but it was already complicated > enough and it makes sense to have it as part of the REST API!) Yes, the latter is more important. Fam _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel
On Thu, 05/17 22:55, Shubham Jain wrote: > > > + 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. Wrong term. I meant "intersect". > > > As you have mentioned in the test case below > "test_maintainer_of_project_x_import_a_patch_to_project_y()", does this And this is actually "test_maintainer_of_project_x_cannot_import_a_patch_to_project_y(). Fam _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel
Thanks, now things make more sense than earlier. On Fri, May 18, 2018 at 7:34 AM Fam Zheng <famz@redhat.com> wrote: > On Thu, 05/17 22:55, Shubham Jain wrote: > > > > + 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. > > Wrong term. I meant "intersect". > > > > > > As you have mentioned in the test case below > > "test_maintainer_of_project_x_import_a_patch_to_project_y()", does this > > And this is actually > "test_maintainer_of_project_x_cannot_import_a_patch_to_project_y(). This is covered in test_maintainer_create_message() itself where the two projects recognised the mbox but it is imported to the one where the user was the maintainer. Shubham > > _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel
On Fri, 05/18 08:04, Shubham Jain wrote: > Thanks, now things make more sense than earlier. > On Fri, May 18, 2018 at 7:34 AM Fam Zheng <famz@redhat.com> wrote: > > > On Thu, 05/17 22:55, Shubham Jain wrote: > > > > > + 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. > > > > Wrong term. I meant "intersect". > > > > > > > > > As you have mentioned in the test case below > > > "test_maintainer_of_project_x_import_a_patch_to_project_y()", does this > > > > And this is actually > > "test_maintainer_of_project_x_cannot_import_a_patch_to_project_y(). > > This is covered in test_maintainer_create_message() itself where the two > projects recognised the mbox but it is imported to the one where the user > was the maintainer. That's right, I missed it. Thanks. Fam _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel
© 2016 - 2024 Red Hat, Inc.