enhance admin integration #593
Conversation
lintingbin
commented
Nov 2, 2018
lintingbin
commented
- only superuser to view the "Object permissions" button.
- overwrite has_module_permission, get_queryset, has_view_permission, has_change_permission, has_delete_permission function of the GuardedModelAdmin class.
- auto add obj permission to the user who create it.
- auto delete obj permission after obj be deleted.
…ian into devel # Conflicts: # guardian/locale/zh_Hans/LC_MESSAGES/django.mo # guardian/locale/zh_Hans/LC_MESSAGES/django.po
| [remove_perm(perm, user_or_group, obj) for perm in perms_dict[user_or_group]] | ||
|
|
||
| def delete_model(self, request, obj): | ||
| self.remove_obj_perms(obj) |
There was a problem hiding this comment.
In the case of Generic Foreign Key, such an operation is useful (as far as I know). However, in the case of Direct Foreign Key, it is responsible for the database, so there is no point in performing this operation.
I am afraid, however, why this operation is to be performed only when using the administration panel. Maybe we should register the listeners of the corresponding signals?
| return super(GuardedModelAdmin, self).delete_model(request, obj) | ||
|
|
||
| def delete_queryset(self, request, queryset): | ||
| [self.remove_obj_perms(obj) for obj in queryset] |
There was a problem hiding this comment.
As in previous comment, but in that situation I see clear way to integrate that in our Manager, so it will works to any delete operation, not only via Admin.
There was a problem hiding this comment.
Thank you for the proposed patch. I am glad that you reported them in such a convenient form, which enabled me to analyze and comment carefully. Could you refer to my remarks and to make appropriate patch to ensure a comfortable use of the library by everyone?
| perms_dict = get_users_with_perms(obj, attach_perms=True) | ||
| perms_dict.update(get_groups_with_perms(obj, attach_perms=True)) | ||
| for user_or_group in perms_dict: | ||
| [remove_perm(perm, user_or_group, obj) for perm in perms_dict[user_or_group]] |
There was a problem hiding this comment.
We extract the code for "shortcuts" instead of static methods. This allows you to re-use the code.
You download the permissions (+1 query) to then delete the permission for each user (+n queries). Could you eliminate this and build the correct query that will do it all at once? Maybe we need to extend our shortcuts to handle that in easy way?
There was a problem hiding this comment.
When an obj is deleted, we need have an api to delete its permission. I think this api put to "shortcuts" is good.
There was a problem hiding this comment.
Could you update pull request to reflect that?
| if not request.user.is_superuser and not change: | ||
| opts = self.opts | ||
| actions = ['view', 'add', 'change', 'delete'] | ||
| [assign_perm('{}.{}_{}'.format(opts.app_label, action, opts.model_name), request.user, obj) |
There was a problem hiding this comment.
This is transferring the internal logic of your application to the library, which should be deprived of such elements. Why do not you use signals for this?
There was a problem hiding this comment.
I understand that you need information about who created it. Usually, I stores such information in the object (createdBy field), so the signal listener have access to it.
There was a problem hiding this comment.
Another way is make that list of permission_for_created_objs configurable by user and make empty by default.
There was a problem hiding this comment.
I think this isn't the internal logic of my application, this function is the normal function that django-guardian need to provide when django-guardian be integrated with Admin.
There was a problem hiding this comment.
The term "internal logic" was not the best term. I see the field for improvement and after thinking it out I believe it is valuable to give permission to the user who created the resource. In my opinion, a persistent set of permissions is not suitable for library. This functionality may work, for example, without using "delete" permissions. This set of permissions can be the default.
| return self.get_model_objs(request).exists() | ||
|
|
||
| def get_queryset(self, request): | ||
| if request.user.is_superuser: |
There was a problem hiding this comment.
Again, moving logic of your application to our library. Is there a shortcut to this?
There was a problem hiding this comment.
I get the impression that transferring logic, because in django-guardian, if there is a possibility, we do not abuse the superuser, and in fact we use permissions. Independently, I see the potential for change in this.
There was a problem hiding this comment.
I think this is no problem. The superuser have the all permission, and "get_queryset" return all objs for superuser is correct.
| @@ -2,7 +2,9 @@ | |||
| {% load i18n admin_urls %} | |||
|
|
|||
| {% block object-tools-items %} | |||
| {% if request.user.is_superuser %} | |||
There was a problem hiding this comment.
I can not accept that way to determine button visibility. It introduces a security risk because the button will have a different display logic from the page to which it directs. I agree that this should be uniform and this is currently an bug. However, we can not solve this in such a simplified way, because we lose flexibility. I rather see in that part reusing that condition: https://github.com/django-guardian/django-guardian/blob/devel/guardian/admin.py#L225
It is possible that this condition should be extracted to separate method, so the result of that method can be overwritten (extra flexibility for end user) and used in two places.
