Add permissions for users #1106

koradon · 2017-09-07T08:55:29Z

This closes #301
This also fixes #1123

codecov-io · 2017-09-07T09:08:42Z

Codecov Report

Merging #1106 into master will increase coverage by 4.24%.
The diff coverage is 95.02%.

@@            Coverage Diff             @@
##           master    #1106      +/-   ##
==========================================
+ Coverage   63.43%   67.68%   +4.24%     
==========================================
  Files         107      114       +7     
  Lines        5979     6140     +161     
  Branches      741      747       +6     
==========================================
+ Hits         3793     4156     +363     
+ Misses       2049     1812     -237     
- Partials      137      172      +35

Impacted Files	Coverage Δ
saleor/dashboard/customer/views.py	`100% <100%> (+50%)`	⬆️
saleor/dashboard/discount/views.py	`66.66% <100%> (+42.05%)`	⬆️
saleor/dashboard/category/views.py	`54.16% <100%> (+30.28%)`	⬆️
saleor/dashboard/group/views.py	`100% <100%> (ø)`
saleor/dashboard/group/urls.py	`100% <100%> (ø)`
saleor/dashboard/product/views.py	`48.07% <100%> (+29.45%)`	⬆️
saleor/userprofile/models.py	`87.01% <100%> (+0.17%)`	⬆️
saleor/dashboard/staff/views.py	`100% <100%> (ø)`
saleor/dashboard/sites/views.py	`100% <100%> (ø)`	⬆️
saleor/dashboard/group/forms.py	`100% <100%> (ø)`
... and 30 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4d0414...93ccbcd. Read the comment docs.

patrys

I'm reluctant to merge this as (1) there are no tests and (2) it does not support roles (called "groups" by Django) which I believe are the preferred way to group permissions.

maarcingebala · 2017-09-07T09:44:46Z

saleor/core/permissions.py

+    form_data = {}
+    permissions = Permission.objects.filter(user=user)
+    for permission in permissions:
+        if str(permission.content_type) not in form_data:


If we used defaultdict with form_data we wouldn't need this if-else structure.

maarcingebala · 2017-09-07T09:51:34Z

saleor/dashboard/staff/views.py

+from django.shortcuts import get_object_or_404
+from django.template.response import TemplateResponse
+
+from saleor.core.permissions import get_user_permissions, update_permissions


We use relative imports when importing local modules.

A ok, this was autogenerated when i refactor in PyCharm

maarcingebala · 2017-09-07T10:00:50Z

saleor/core/permissions.py

+MODELS_PERMISSIONS = (('view', 'View Products in Dashboard'),
+                      ('edit', 'Edit Product in Dashboard'))
+
+PERMISSIONS = set([permission[0] for permission in MODELS_PERMISSIONS])


Names MODELS_PERMISSIONS and PERMISSIONS are a bit confusing when you first read this, because it's hard to guess what is the purpose of having the two constants with similar names and the second being just the transformation of the first one. PERMISSIONS is used only once in update_permissions so maybe it should be a local variable. And if MODELS_PERMISSIONS is meant to be imported across the project models then this could be the only constant.

Yea i know this looks odd. I will change it to local variable.

By the way, set comprehension has been a thing for at least five years 😄

patrys · 2017-09-07T13:16:01Z

.gitignore

@@ -14,6 +14,7 @@
 *.log
 *.pot
 *.pyc
+.envrc


Could you add such files to your global gitignore instead? Otherwise we'll end up with a list covering all existing tools and editors. 😄

Ok sorry :)

patrys · 2017-09-07T13:17:22Z

saleor/dashboard/product/views.py

@@ -17,6 +18,7 @@


 @staff_member_required
+@permission_required('product.edit')


Instead of copy-pasting the same set of decorators we should have one for each unique set of permissions. Something like @product_manager_required.

This is a good idea when i implement Groups. I think.

patrys · 2017-09-07T13:19:43Z

saleor/product/models.py

@@ -47,6 +48,8 @@ class Meta:
        verbose_name = pgettext_lazy('Category model', 'category')
        verbose_name_plural = pgettext_lazy('Category model', 'categories')
        app_label = 'product'
+        permissions = (('view_category', 'Can View Category in Dashboard'),
+                       ('edit_category', 'Can Edit Category in Dashboard'))


What's the idea behind this title-like capitalization? Why have all permissions end in "in Dashboard"? Are we planning to introduce a second set of permissions for other use cases such as REST or GraphQL? If not the is_staff field is responsible for the "in dashboard" part.

As in Django documentation. This second value has to be "Human readible". I can simplyfi this to just "Edit"/"View" or "edit"/"view".

Sorry, I meant the Way It is Capitalized.

ok i will turn it off

patrys · 2017-09-12T14:53:24Z

saleor/dashboard/customer/views.py

@@ -17,7 +17,8 @@ def customer_list(request):
        .select_related('default_billing_address', 'default_shipping_address')
        .annotate(
            num_orders=Count('orders', distinct=True),
-            last_order=Max('orders', distinct=True)))
+            last_order=Max('orders', distinct=True))
+    )


As in commit message. Staff can be also a customer. Now we have a separate view with only staff members. If sb would like to see other user Order history, he will not have to search in two places.

Customers - list of all users

Staff members - list of users which are staff members

patrys · 2017-09-12T14:58:53Z

saleor/core/permissions.py

+        q |= Q(content_type__app_label=app, content_type__model__in=models)
+        q &= ~Q(content_type__app_label=app, codename__startswith='add_')
+        q &= ~Q(content_type__app_label=app, codename__startswith='change_')
+        q &= ~Q(content_type__app_label=app, codename__startswith='delete_')


This seems like a very inefficient way to fetch data. Why not match how has_perm works and query for a list of known appname+codename pairs?

This is used only to construct ModelForm when creating/editing a group.
This only excludes a standard django permissions which are created for every model.

I understand how it works. The point I'm trying to make is that you could change the GROUP_PERMISSIONS_MODELS setting to use the same syntax that has_perm uses and end up with a much simpler query that did not require costly __startswith lookups.

By the way, is there a reason for GROUP_PERMISSIONS_MODELS to live in settings? I don't think it's likely to change unless you add more views.

mirekm · 2017-09-12T15:25:35Z

Technical but important detail: please follow this guidelines for writing commit messages.

maarcingebala · 2017-09-14T13:15:41Z

saleor/core/permissions.py

+]
+
+
+def get_user_groups(user):


Do we need this function?

maarcingebala · 2017-09-14T13:21:37Z

saleor/dashboard/group/urls.py

+
+
+urlpatterns = [
+    url(r'^$', views.groups_list, name="group-list"),


Strings should be consistently enclosed in quotes or double quotes. In Saleor we tend to use single quotes in most cases.

old habbits

maarcingebala · 2017-09-14T13:22:14Z

saleor/dashboard/group/views.py

@@ -0,0 +1,59 @@
+from __future__ import unicode_literals
+
+from django.shortcuts import redirect, get_object_or_404


Imports should be sorted.

maarcingebala · 2017-09-14T13:23:45Z

saleor/dashboard/group/views.py

+@staff_member_required
+def groups_list(request):
+    groups = Group.objects.all()
+


This empty line is not necessary here. It's simple function and it doesn't need visual separation of logic.

maarcingebala · 2017-09-14T13:24:08Z

saleor/dashboard/group/views.py

+@staff_member_required
+def group_create(request):
+    form = GroupPermissionsForm(request.POST or None)
+


Again, I would remove these empty lines.

maarcingebala · 2017-09-14T13:47:05Z

templates/materializecssform/field.html

@@ -35,7 +35,7 @@
    {% endif %}
    {% for choice in field %}
      <p>
-        <input id="{{ choice.id_for_label }}" name="{{ choice.name }}" class="filled-in" type="checkbox" value="{{ choice.choice_value }}" {% if choice.choice_value in choice.value %} checked="checked" {% endif %}>
+        <input id="{{ choice.id_for_label }}" name="{{ choice.data.name }}" class="filled-in" type="checkbox" value="{{ choice.data.value }}" {% if choice.data.selected%} checked="checked" {% endif %}>


Missing space before '%' in {% if choice.data.selected%}

maarcingebala · 2017-09-14T13:48:17Z

tests/conftest.py

+def staff_client(staff_user):
+    """A Django test client logged in as an staff member"""
+    from django.test.client import Client
+    client = Client()


Instead of creating the client here you could use a fixture just as in authorized_client below.

maarcingebala · 2017-09-14T13:49:38Z

tests/dashboard/test_permissions.py

+from saleor.dashboard.staff.forms import UserGroupForm
+
+
+def test_superuser_permissions(admin_user):


I'm not sure that we need such tests. Superuser has all permission by default and this is granted by Django. We don't have test that Django works correctly:)

Yea, first test. Checking if this works as i expect and i forget about it. Deleted

maarcingebala · 2017-09-14T13:50:10Z

tests/dashboard/test_permissions.py

+    assert admin_user.has_perm("product.edit_product")
+
+
+def test_superuser(admin_user):


What is the purpose of this test?

Again just checking. Deleted

maarcingebala · 2017-09-14T13:51:10Z

tests/dashboard/test_permissions.py

+
+
+def test_staff_list_view(admin_client):
+    response = admin_client.get('/dashboard/staff/')


Again, you should always use reverse for getting URLs.

patrys · 2017-09-14T14:02:09Z

saleor/core/permissions.py

+    for group_permission in GROUP_PERMISSIONS_MODELS:
+        model_name = group_permission.split('.')[1]
+        codenames.append('view_%s' % model_name)
+        codenames.append('edit_%s' % model_name)


Why can't we list those explicitly in GROUP_PERMISSIONS?

We can but the idea was to edit only one place when add new permission to view/edit sth and to filter out those default from django.

You will still edit only one place. You'll just need to add two permissions there. What if we introduce a financial reporting feature that does not use an "edit" permission or we need to limit the ability to delete certain items?

This was consulted with @elwoodxblues. I think we should discus this together.

koradon · 2017-09-15T10:47:35Z

We have to wait for PR #966 to have more sens.

Deleted staff members from the customer list in dashboard. Added new templates for Staff section. Prepared siple form for future permission handling for staff members.

Permissions have to be unique for every model in django app.

Removed variable pk from update_permissions.

Test incomplete.

- Added new tests for accessing product list and product update. - Redesign get_permissions to be more readable. - Moved constant variable GROUP_PERMISSIONS_MODELS from settings.py to permissions.py. - Updated @permission_required for dashboard product views to match permissions from products models.

- Changed staff and group templates matching the rest of the site. - Add first tests using user with group.

- Add group delete functionality. View and templates ready. Tests in progress. - Change groups-list to group-list. - Change dashboard/groups to dashboard/group. - Add some tests of group and staff forms.

- Add tests for views. - Delete group description in group list.

Refactor permissions.py with better names of variables and simplyfied logic. List of permissions now have name convention as User.has_perm().

Order permissions in permissions.py MODEL_PERMISSIONS.

Update dashboard.product.views.py with correct required permissions. Update dashboard/product/detail.html with correct required permissions.

Update templates in dashboard.products to hide buttons if user has no permission for respective action.

Add missing migration merge with changes from read-only PR.

Add updates in dashboard/category templates with required permissions.

Add updates in dashboard templates with required permissions.

Add pgettext_lazy for permissions description in Meta class of objects with model permissions.

Add list of permissions in group list in dashboard. Permissions are listed as <p class='list-item-desc">.

- Add main views tests to tets_permissions. - Change some permissions in order views.

maarcingebala · 2017-09-19T11:38:53Z

saleor/dashboard/group/views.py

+@staff_member_required
+def group_list(request):
+    groups = [{'name': group, 'permissions': group.permissions.all()}
+              for group in Group.objects.all()]


Shouldn't we prefetch permissions when querying Groups? It looks like accessing group.permissions.all() will perform duplicated queries.

maarcingebala · 2017-09-19T11:40:27Z

saleor/dashboard/product/views.py

@@ -263,7 +279,23 @@ def stock_delete(request, product_pk, variant_pk, stock_pk):
        request, 'dashboard/product/stock/modal_confirm_delete.html', ctx)


+@require_http_methods(['POST'])
+@permission_required('product.edit_stock_location')
+def stock_bulk_delete(request, product_pk):


Is this view used? If I'm correct we have removed the ability for bulk deletions in PR with read-only views.

Yea, during rebase i didn't delete any of the views so there might be some old present.

maarcingebala · 2017-09-19T11:42:12Z

saleor/dashboard/product/views.py

@@ -405,6 +443,24 @@ def variant_delete(request, product_pk, variant_pk):


 @staff_member_required
+@require_http_methods(['POST'])
+@permission_required('product.edit_product')
+def variants_bulk_delete(request, product_pk):


Same as above, I think this is not used.

maarcingebala · 2017-09-19T11:48:10Z

saleor/order/migrations/0019_auto_20170912_0401.py

@@ -0,0 +1,19 @@
+# -*- coding: utf-8 -*-


This migration does the same what saleor/order/migrations/0018_auto_20170907_0909.py does. Every new migration file makes the process of applying all migrations a little bit longer, so we should try to add only necessary migrations. I would revert these two migrations, remove them and recreate. This will result only in one migration file.

maarcingebala · 2017-09-19T11:50:50Z

saleor/product/migrations/0042_auto_20170912_0401.py

@@ -0,0 +1,27 @@
+# -*- coding: utf-8 -*-


All product migrations from 0036 to this one could be replaced with one migration file.

maarcingebala · 2017-09-19T12:05:28Z

templates/dashboard/base.html

            <li class="side-nav-section">
              <p>
                {% trans "Configuration"  context "Dashboard configuration" %}
              </p>
              <ul>
+                <li class="{% block menu_staff_class %}{% endblock %}">


"Staff members" and "Groups" sections should probably be visible only to superusers.

Yea i have a question if we need some additional User model permissions for staff like the whole Configuration in Dashboard or do we just use superuser.

Decided to cover every view and template behind superuser check.

maarcingebala · 2017-09-19T12:09:52Z

Links to sections "Product types" and "Attributes" should also be hidden in template depending on permissions. "Site settings" should be visible only to superusers. I don't know about "Shipping methods", maybe we need separate permissions for them?

- Add superuser_require decorator to dashboard views.py - Add @superuser_required decorators to every view from Configuration side menu in dashboard. - Add superuser check in dashboard/base.html for Configuration side menu. - Corrected migrations.

Add tests for Configuration with superuser requirements.

- Order template has "Add note" hidden behind permission edit_order. - Order view add_note requires permission edit_order. - Correct padding in group template. - Delete information about orders and addresses from staff detail. - Add redirect to group-list after group update.

Correct tests with changes in dashboard orders permissions.

Fix textarea issue from #1123.

maarcingebala · 2017-09-20T11:07:31Z

@patrys I think that this PR is ready to merge. What do you think?

koradon · 2017-09-20T11:40:30Z

One thing i missed. We talk about updating populatedb script with at least one group.

Update populatedb script to create Group example "Products Manager" with permissions to view and edit products in dashboard. Script uses get_or_create to create Group only at the first time.

Delete blank lines in populatedb script.

Fix title trans context in group/list.html. Delete additional line break.

Fix query for Permission in permissions.py Fix query for staff_members in views.py Delete unused templatetags in modal_group_confirm_delete.

maarcingebala · 2017-09-25T10:29:17Z

🎉

koradon requested a review from maarcingebala September 7, 2017 08:55

patrys requested changes Sep 7, 2017

View reviewed changes

maarcingebala reviewed Sep 7, 2017

View reviewed changes

koradon added the in progress label Sep 7, 2017

patrys reviewed Sep 7, 2017

View reviewed changes

patrys requested changes Sep 12, 2017

View reviewed changes

maarcingebala reviewed Sep 14, 2017

View reviewed changes

patrys reviewed Sep 14, 2017

View reviewed changes

koradon force-pushed the permissions branch from 45f55f1 to f148dcf Compare September 18, 2017 11:25

koradon added 15 commits September 18, 2017 13:32

Added 'Staff' section in Dashboard with only staff members.

185fa55

Deleted staff members from the customer list in dashboard. Added new templates for Staff section. Prepared siple form for future permission handling for staff members.

Prepared form for permissions to Products section in Dashboard.

1403dce

Changed permissions in model.

82c3bb4

Permissions have to be unique for every model in django app.

Added one test with updating permissions.

f2dce7c

Added tests for changing permissions.

840904b

Removed variable pk from update_permissions.

Added permissions to next models.

1bb9174

Corrected tests.

409f19c

Refactored permissions to use Groups.

3fe4860

Test incomplete.

Added basic tests for groups and staff views.

be0afe4

Revert change in custommer list, because staff can be also a customer.

ed986bf

Revert change in custommer list, because staff can be also a customer.

513852e

Change templates and tests

07995de

- Changed staff and group templates matching the rest of the site. - Add first tests using user with group.

Add group delete

30ad907

- Add group delete functionality. View and templates ready. Tests in progress. - Change groups-list to group-list. - Change dashboard/groups to dashboard/group. - Add some tests of group and staff forms.

Add tests

f3a3a64

- Add tests for views. - Delete group description in group list.

koradon added 2 commits September 18, 2017 13:47

Refactor permissions.py

b95f32a

Refactor permissions.py with better names of variables and simplyfied logic. List of permissions now have name convention as User.has_perm().

Order permissions

5882a1d

Order permissions in permissions.py MODEL_PERMISSIONS.

koradon force-pushed the permissions branch from f148dcf to 5882a1d Compare September 18, 2017 11:48

koradon added 8 commits September 18, 2017 14:24

Update views and templates

7371fe1

Update dashboard.product.views.py with correct required permissions. Update dashboard/product/detail.html with correct required permissions.

Update templates

141193e

Update templates in dashboard.products to hide buttons if user has no permission for respective action.

Add missing migration merge

d35f5d3

Add missing migration merge with changes from read-only PR.

Add updates in templates

a9396b2

Add updates in dashboard/category templates with required permissions.

Add updates in templates

efa054e

Add updates in dashboard templates with required permissions.

Add translation to permissions descriptions

39f0435

Add pgettext_lazy for permissions description in Meta class of objects with model permissions.

Add list permission in group list

7eb0239

Add list of permissions in group list in dashboard. Permissions are listed as <p class='list-item-desc">.

Add tests

5b8b42c

- Add main views tests to tets_permissions. - Change some permissions in order views.

maarcingebala reviewed Sep 19, 2017

View reviewed changes

koradon added 2 commits September 19, 2017 16:22

Add superuser_require

2d35699

- Add superuser_require decorator to dashboard views.py - Add @superuser_required decorators to every view from Configuration side menu in dashboard. - Add superuser check in dashboard/base.html for Configuration side menu. - Corrected migrations.

Add tests

883d94a

Add tests for Configuration with superuser requirements.

maarcingebala mentioned this pull request Sep 20, 2017

Fix input fields rendering by materializecss tag #1123

Closed

2 tasks

koradon added 3 commits September 20, 2017 10:26

Correct test

753d1c1

Correct tests with changes in dashboard orders permissions.

Fix textarea issue

100577f

Fix textarea issue from #1123.

maarcingebala approved these changes Sep 20, 2017

View reviewed changes

koradon added 4 commits September 20, 2017 15:17

Update populatedb

f457a85

Update populatedb script to create Group example "Products Manager" with permissions to view and edit products in dashboard. Script uses get_or_create to create Group only at the first time.

Delete blank lines

357acdd

Delete blank lines in populatedb script.

Fix title trans context

be08914

Fix title trans context in group/list.html. Delete additional line break.

Fix queries

93ccbcd

Fix query for Permission in permissions.py Fix query for staff_members in views.py Delete unused templatetags in modal_group_confirm_delete.

patrys approved these changes Sep 25, 2017

View reviewed changes

maarcingebala merged commit 8c4c039 into master Sep 25, 2017

maarcingebala deleted the permissions branch September 25, 2017 10:28

@@ @@ -14,6 +14,7 @@ @@
               *.log
               *.pot
               *.pyc
+              .envrc

		@@ -17,6 +18,7 @@


		@staff_member_required
		@permission_required('product.edit')



		urlpatterns = [
		url(r'^$', views.groups_list, name="group-list"),

		@@ -0,0 +1,59 @@
		from __future__ import unicode_literals

		from django.shortcuts import redirect, get_object_or_404

		from saleor.dashboard.staff.forms import UserGroupForm


		def test_superuser_permissions(admin_user):

		assert admin_user.has_perm("product.edit_product")


		def test_superuser(admin_user):



		def test_staff_list_view(admin_client):
		response = admin_client.get('/dashboard/staff/')

Add permissions for users #1106

Add permissions for users #1106

Conversation

koradon commented Sep 7, 2017 • edited

codecov-io commented Sep 7, 2017 • edited

Codecov Report

patrys left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koradon Sep 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mirekm commented Sep 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koradon Sep 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koradon commented Sep 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maarcingebala commented Sep 19, 2017

maarcingebala commented Sep 20, 2017

koradon commented Sep 20, 2017

maarcingebala commented Sep 25, 2017

koradon commented Sep 7, 2017 •

edited

codecov-io commented Sep 7, 2017 •

edited

koradon Sep 13, 2017 •

edited

koradon Sep 14, 2017 •

edited