Opened 9 years ago

Closed 9 years ago

#25057 closed Cleanup/optimization (wontfix)

Permission for view (CRUD) and correct handling of inactive users for business applications

Reported by: Grigoriy Kramarenko Owned by: nobody
Component: contrib.auth Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Grigoriy Kramarenko)

Our framework is beautiful, it is convenient to make sites, but not business applications.
You have to write a lot of code and/or wrappers to do elementary things, in order not to lose performance.
The most striking problem: when you need in a single view to decide what data to display, depending on which groups the user belongs to.
For example, a simple task:

def dash(request):
    ctx = {}
    user = request.user

    if not user.has_module_perms('myadminapp'):
        return HttpResponseForbidden()

    if user.has_perm('auth.view_group'):
        ctx['groups'] = Group.objects.all()
    if user.has_perm('auth.view_permission'):
        ctx['permissions'] = Permissions.objects.all()
    if user.has_perm('auth.view_user'):
        ctx['users'] = User.objects.all()

    # Also the external applications to which you cannot add custom permissions:
    if user.has_perm('partner.view_supplier'):
        ctx['suppliers'] = Supplier.objects.all()
    if user.has_perm('partner.view_org'):
        ctx['orgs'] = Org.objects.all()
    if user.has_perm('partner.view_address'):
        ctx['addresses'] = Address.objects.all()
    if user.has_perm('order.view_summaryorder'):
        ctx['summary_orders'] = SummaryOrder.objects.all()
    if user.has_perm('order.view_customorder'):
        ctx['custom_orders'] = CustomOrder.objects.all()
    if user.has_perm('order.view_document'):
        ctx['docs'] = Document.objects.all()
    ...
    # 0 additional requests in database

Today turns into:

def dash(request):
    ctx = {}
    user = request.user

    if not user.has_module_perms('myadminapp') or not user.is_active:
        return HttpResponseForbidden()

    if user.groups.filter(name='Has view groups').count():
        ctx['groups'] = Group.objects.all()
    if user.groups.filter(name='Has view permissions').count():
        ctx['permissions'] = Permissions.objects.all()
    if user.groups.filter(name='Has view users').count():
        ctx['users'] = User.objects.all()

    # Also the external applications to which you cannot add custom permissions:
    if user.groups.filter(name='Has view suppliers').count():
        ctx['suppliers'] = Supplier.objects.all()
    if user.groups.filter(name='Has view orgs').count():
        ctx['orgs'] = Org.objects.all()
    if user.groups.filter(name='Has view addresses').count():
        ctx['addresses'] = Address.objects.all()
    if user.groups.filter(name='Has view summary orders').count():
        ctx['summary_orders'] = SummaryOrder.objects.all()
    if user.groups.filter(name='Has view custom orders').count():
        ctx['custom_orders'] = CustomOrder.objects.all()
    if user.groups.filter(name='Has view documents').count():
        ctx['docs'] = Document.objects.all()
    ...
    # +9 additional requests in database

Or add to the database by hand after installing the apps.
But if someone of the admins accidentally rename a group? The horror!
For large and complex applications permission to view inside framework the only decent solution.

While all of this madness can be reduced to two simple solutions.

  1. Set four default permissions in db.models.options.Options.default_permissions = ('view', 'add', 'change', 'delete'). Also, would the names of the permission form on the language set in settings.LANGUAGE_CODE. It's a matter of principle any "perfectionist".
  2. Inactive user should not have any permissions. Now has. For exceptional cases of inactive user should be a special parameter for the function that changes its behavior.

I have three patches for master branch and and can do for stable/1.8.x. I think these changes will not affect existing projects, because the permissions have already been recorded in the database during the migration and shall not affect the administrative interface.

Each of the patches tested.

P.S.: Please forgive me for my English. I write through the translator.

Attachments (3)

25057_inactive_user.diff (3.3 KB ) - added by Grigoriy Kramarenko 9 years ago.
25057_permission_for_view_with_locales.diff (5.1 KB ) - added by Grigoriy Kramarenko 9 years ago.
25057_permission_for_view_without_locales.diff (1.9 KB ) - added by Grigoriy Kramarenko 9 years ago.

Download all attachments as: .zip

Change History (5)

by Grigoriy Kramarenko, 9 years ago

Attachment: 25057_inactive_user.diff added

by Grigoriy Kramarenko, 9 years ago

by Grigoriy Kramarenko, 9 years ago

comment:1 by Grigoriy Kramarenko, 9 years ago

Description: modified (diff)

comment:2 by Tim Graham, 9 years ago

Resolution: wontfix
Status: newclosed

Thanks for the suggestions.

The request for a "view" permission is a duplicate of #820 which was closed as "won't fix". If you want to reopen the discussion, please write to the DevelopersMailingList.

I don't find your proposal for permissions checking compelling because these ideas can be implemented with a custom user model. That seems more elegant that requiring an application to pass check_active=False everywhere it does permission checks. Also, the default ModelBackend doesn't return any permissions for inactive users so that would need to be overridden as well.

For future reference, please keep a ticket focused on one issue.

Note: See TracTickets for help on using tickets.
Back to Top