LoginRequired and PermissionRequired view mixins #48

Closed
danielsokolowski opened this Issue Aug 31, 2011 · 21 comments

Comments

Projects
None yet
4 participants
@danielsokolowski

Using the django's or guardian's 'loginrequired' and 'permissionrequired' decorators with the new class based views is tricky and cumbersome - django documetations shows some pointers here: https://docs.djangoproject.com/en/dev/topics/class-based-views/#decorating-class-based-views

A different approach would to use view mixins which in my opinion is the easiest and most readable solution. An example class based view with a permission required check could look like this:

class FitterEditView(PermissionRequiredMixin, UpdateView):
    """
    ...
    """

    ### PermissionRequiredMixin settings
    permission_required = 'fitters.change_fitter'

    ### UpdateView settings
    context_object_name="fitter"
    queryset = Fitter.objects.all()
    form_class = FitterForm
    ...

Below is my first attempt at these mixins created for a project I am currently working on. These mixins are designed to work both with vanilla django or with django-guradian. If this is something you would consider including in the future I would love to assist.

On the final note I want to say thank you for this fantastic app which I have been using it since ver 0.2.

class LoginRequiredMixin(object):
    """ 
    A login required mixin for use with class based views. This Class is a light wrapper around the
    `login_required` decorator and hence function parameters are just attributes defined on the class.

    Due to parent class order traversal this mixin must be added as the left most 
    mixin of a view.

    The mixin has exaclty the same flow as `login_required` decorator:

        If the user isn't logged in, redirect to settings.LOGIN_URL, passing the current 
        absolute path in the query string. Example: /accounts/login/?next=/polls/3/.

        If the user is logged in, execute the view normally. The view code is free to 
        assume the user is logged in.

    **Class Settings**
        `redirect_field_name - defaults to "next"
        `login_url` - the login url of your site

    """
    redirect_field_name = REDIRECT_FIELD_NAME
    login_url = None

    @method_decorator(login_required(redirect_field_name=redirect_field_name, login_url=login_url))
    def dispatch(self, request, *args, **kwargs):
        return super(LoginRequiredMixin, self).dispatch(request, *args, **kwargs)

class PermissionRequiredMixin(object):
    """ 
    A view mixin that verifies if the current logged in user has the specified permission 
    by wrapping the ``request.user.has_perm(..)`` method.

    If a `get_object()` method is defined either manually or by including another mixin (for example
    ``SingleObjectMixin``) or ``self.object`` is defiend then the permission will be tested against 
    that specific instance.

    .. NOTE: Testing of a permission against a specific object instance requires an authentication backend
             that supports. Please see ``django-guardian`` to add object level permissions to your project.  

    The mixin does the following:  

        If the user isn't logged in, redirect to settings.LOGIN_URL, passing the current 
        absolute path in the query string. Example: /accounts/login/?next=/polls/3/.

        If the `raise_exception` is set to True than rather than redirect to login page
        a `PermisionDenied` (403) is raised.

        If the user is logged in, and passes the permission check than the view is executed
        normally.

    **Example Usage**

        class FitterEditView(PermissionRequiredMixin, UpdateView):
            ...
            ### PermissionRequiredMixin settings
            permission_required = 'fitters.change_fitter'

            ### other view settings
            context_object_name="fitter"
            queryset = Fitter.objects.all()
            form_class = FitterForm
            ...

    **Class Settings**
        `permission_required` - the permission to check of form "<app_label>.<permission codename>"
                                i.e. 'polls.can_vote' for a permission on a model in the polls application.

        `login_url` - the login url of your site
        `redirect_field_name - defaults to "next"
        `raise_exception` - defaults to False - raise PermisionDenied (403) if set to True

    """
    ### default class view settings
    login_url = settings.LOGIN_URL
    raise_exception = False
    permission_required = None
    redirect_field_name=REDIRECT_FIELD_NAME

    def dispatch(self, request, *args, **kwargs):
        # call the parent dispatch first to pre-populate few things before we check for permissions
        original_return_value = super(PermissionRequiredMixin, self).dispatch(request, *args, **kwargs)

        # verify class settings
        if self.permission_required == None or len(self.permission_required.split('.')) != 2:
            raise ImproperlyConfigured("'PermissionRequiredMixin' requires 'permission_required' attribute to be set to '<app_label>.<permission codename>' but is set to '%s' instead" % self.permission_required)

        # verify permission on object instance if needed
        has_permission = False
        if hasattr(self, 'object')  and self.object is not None: 
            has_permission = request.user.has_perm(self.permission_required, self.object)
        elif hasattr(self, 'get_object') and callable(self.get_object):
            has_permission = request.user.has_perm(self.permission_required, self.get_object())
        else:
            has_permission = request.user.has_perm(self.permission_required)

        # user failed permission
        if not has_permission:
            if self.raise_exception:
                return HttpResponseForbidden()
            else:
                path = urlquote(request.get_full_path())
                tup = self.login_url, self.redirect_field_name, path
                return HttpResponseRedirect("%s?%s=%s" % tup)

        # user passed permission check so just return the result of calling .dispatch()
        return original_return_value
@danielsokolowski

This comment has been minimized.

Show comment
Hide comment
@danielsokolowski

danielsokolowski Aug 31, 2011

Both mixins require the following imports:

from django.conf import settings
from django.http import HttpResponseForbidden, HttpResponseRedirect
from django.utils.http import urlquote
from django.core.exceptions import ImproperlyConfigured
from django.contrib.auth import REDIRECT_FIELD_NAME
from django.contrib.auth.decorators import login_required
from django.utils.decorators import method_decorator

Both mixins require the following imports:

from django.conf import settings
from django.http import HttpResponseForbidden, HttpResponseRedirect
from django.utils.http import urlquote
from django.core.exceptions import ImproperlyConfigured
from django.contrib.auth import REDIRECT_FIELD_NAME
from django.contrib.auth.decorators import login_required
from django.utils.decorators import method_decorator
@lukaszb

This comment has been minimized.

Show comment
Hide comment
@lukaszb

lukaszb Sep 10, 2011

Contributor

Hey man, cool to hear you're using guardian since 0.2! That was... almost year ago, I believe ;-)

I'd definitely like to include those into guardian!

However, have a few notes:

  1. It's much easier for me to go through diff's and being able to see final file at the (almost) same time. I'd be glad if you can work with your changes in a fork (and probably another branch so I can smash commits before merge).
  2. I accept only fully tested pull requests. So, in case you don't have time to write tests, let me know. If you need assist, let me know too.
  3. I'm not sure about the PermissionRequiredMixin.dispatch method. The problem I can think of, is that you try to resolve view before checking for permissions. Let's say our view deletes some objets immediately (without confirm dialog or some other intermediate view). It would be resolved (objects are deleted) and after that we check for permissions. In that case, we get 500 (as those objects were already deleted). And, in fact, permissions are not checked at all. Even if it wasn't a delete action, we still do perform dispatch logic, and, yeah, we show to the user that he/she is not allowed to access object, but we still had done all actions. This is serious security breach. Even if we use such mixin only for showing objects, we still do unnecessary queries to the database. Of course, I may be missing something - but that is exactly some code that I'd like to review tests first :)

Please let me know if you have any feedback for that!

Contributor

lukaszb commented Sep 10, 2011

Hey man, cool to hear you're using guardian since 0.2! That was... almost year ago, I believe ;-)

I'd definitely like to include those into guardian!

However, have a few notes:

  1. It's much easier for me to go through diff's and being able to see final file at the (almost) same time. I'd be glad if you can work with your changes in a fork (and probably another branch so I can smash commits before merge).
  2. I accept only fully tested pull requests. So, in case you don't have time to write tests, let me know. If you need assist, let me know too.
  3. I'm not sure about the PermissionRequiredMixin.dispatch method. The problem I can think of, is that you try to resolve view before checking for permissions. Let's say our view deletes some objets immediately (without confirm dialog or some other intermediate view). It would be resolved (objects are deleted) and after that we check for permissions. In that case, we get 500 (as those objects were already deleted). And, in fact, permissions are not checked at all. Even if it wasn't a delete action, we still do perform dispatch logic, and, yeah, we show to the user that he/she is not allowed to access object, but we still had done all actions. This is serious security breach. Even if we use such mixin only for showing objects, we still do unnecessary queries to the database. Of course, I may be missing something - but that is exactly some code that I'd like to review tests first :)

Please let me know if you have any feedback for that!

@danielsokolowski

This comment has been minimized.

Show comment
Hide comment
@danielsokolowski

danielsokolowski Sep 13, 2011

Hi Lukasz,

Just sending you a quick note that I will make time tomorrow to respond
properly.

On a side note is your nationality polish ?

Daniel

On Sat, 10 Sep 2011 16:46:23 -0400, Lukasz Balcerzak
reply@reply.github.com
wrote:

Hey man, cool to hear you're using guardian since 0.2! That was...
almost year ago, I believe ;-)

I'd definitely like to include those into guardian!

However, have a few notes:

  1. It's much easier for me to go through diff's and being able to see
    final file at the (almost) same time. I'd be glad if you can work with
    your changes in a fork (and probably another branch so I can smash
    commits before merge).
  2. I accept only fully tested pull requests. So, in case you don't have
    time to write tests, let me know. If you need assist, let me know too.
  3. I'm not sure about the PermissionRequiredMixin.dispatch method.
    The problem I can think of, is that you try to resolve view before
    checking for permissions. Let's say our view deletes some objets
    immediately (without confirm dialog or some other intermediate view). It
    would be resolved (objects are deleted) and after that we check for
    permissions. In that case, we get 500 (as those objects were already
    deleted). And, in fact, permissions are not checked at all. Even if it
    wasn't a delete action, we still do perform dispatch logic, and,
    yeah, we show to the user that he/she is not allowed to access object,
    but we still had done all actions. This is serious security breach. Even
    if we use such mixin only for showing objects, we still do unnecessary
    queries to the database. Of course, I may be missing something - but
    that is exactly some code that I'd like to review tests first :)

Please let me know if you have any feedback for that!

Using Opera's revolutionary email client: http://www.opera.com/mail/

Hi Lukasz,

Just sending you a quick note that I will make time tomorrow to respond
properly.

On a side note is your nationality polish ?

Daniel

On Sat, 10 Sep 2011 16:46:23 -0400, Lukasz Balcerzak
reply@reply.github.com
wrote:

Hey man, cool to hear you're using guardian since 0.2! That was...
almost year ago, I believe ;-)

I'd definitely like to include those into guardian!

However, have a few notes:

  1. It's much easier for me to go through diff's and being able to see
    final file at the (almost) same time. I'd be glad if you can work with
    your changes in a fork (and probably another branch so I can smash
    commits before merge).
  2. I accept only fully tested pull requests. So, in case you don't have
    time to write tests, let me know. If you need assist, let me know too.
  3. I'm not sure about the PermissionRequiredMixin.dispatch method.
    The problem I can think of, is that you try to resolve view before
    checking for permissions. Let's say our view deletes some objets
    immediately (without confirm dialog or some other intermediate view). It
    would be resolved (objects are deleted) and after that we check for
    permissions. In that case, we get 500 (as those objects were already
    deleted). And, in fact, permissions are not checked at all. Even if it
    wasn't a delete action, we still do perform dispatch logic, and,
    yeah, we show to the user that he/she is not allowed to access object,
    but we still had done all actions. This is serious security breach. Even
    if we use such mixin only for showing objects, we still do unnecessary
    queries to the database. Of course, I may be missing something - but
    that is exactly some code that I'd like to review tests first :)

Please let me know if you have any feedback for that!

Using Opera's revolutionary email client: http://www.opera.com/mail/

@lukaszb

This comment has been minimized.

Show comment
Hide comment
@lukaszb

lukaszb Sep 13, 2011

Contributor

Daniel, cool, I'd gladly hear what do you think about my thoughts. And yep, I'm from Poland.

Contributor

lukaszb commented Sep 13, 2011

Daniel, cool, I'd gladly hear what do you think about my thoughts. And yep, I'm from Poland.

@danielsokolowski

This comment has been minimized.

Show comment
Hide comment
@danielsokolowski

danielsokolowski Sep 20, 2011

Hi Lukasz,

  1. I have created a fork and updated the files as you asked --- do you still need me to create and use another branch rather than the 'master'?
  2. I have not written any python tests in the past - can you assist me with this or provide some guidance? I am sure I can have it done quickly.
  3. The view's logic can not be called before the permission mixin is called. The dispatch method is the first method called when the FooView.as_view() method is executed (see django.views.generic.base.view which is the base for all class based views).

So when either of the mixins is added as left most mixin it ensures that the view will only execute if the permission are passed or the mixin takes over and blocks the propagation.

This is my understanding based on http://www.python.org/download/releases/2.3/mro/, below is a code example that might clarify this further.


class A(object):
    ...

Class LoginRequiredMixin(object)
    ...

class View(object)
    ...same as django

class C(LoginRequired, A2, B, View):

C.as_view()

Whe the as_view method is called the resolution is as follows LoginRequiredMixin > A > View.as_view(). When the view method is executed it calls the .dispatch() method which is resovled as LoginRequiredMixin.dispatch() and hence our permission check is always executed.

The only way the LoginRequiredMixin might be bypassed is if our view overides either the as_view() method or the dispatch() method in such a way that some kind of database change is made -- but this action does not belong in either of these methods and perhaps there is a way to check for this and raise ImproperlyConfigured exception.

Hi Lukasz,

  1. I have created a fork and updated the files as you asked --- do you still need me to create and use another branch rather than the 'master'?
  2. I have not written any python tests in the past - can you assist me with this or provide some guidance? I am sure I can have it done quickly.
  3. The view's logic can not be called before the permission mixin is called. The dispatch method is the first method called when the FooView.as_view() method is executed (see django.views.generic.base.view which is the base for all class based views).

So when either of the mixins is added as left most mixin it ensures that the view will only execute if the permission are passed or the mixin takes over and blocks the propagation.

This is my understanding based on http://www.python.org/download/releases/2.3/mro/, below is a code example that might clarify this further.


class A(object):
    ...

Class LoginRequiredMixin(object)
    ...

class View(object)
    ...same as django

class C(LoginRequired, A2, B, View):

C.as_view()

Whe the as_view method is called the resolution is as follows LoginRequiredMixin > A > View.as_view(). When the view method is executed it calls the .dispatch() method which is resovled as LoginRequiredMixin.dispatch() and hence our permission check is always executed.

The only way the LoginRequiredMixin might be bypassed is if our view overides either the as_view() method or the dispatch() method in such a way that some kind of database change is made -- but this action does not belong in either of these methods and perhaps there is a way to check for this and raise ImproperlyConfigured exception.

@danielsokolowski

This comment has been minimized.

Show comment
Hide comment
@danielsokolowski

danielsokolowski Sep 26, 2011

Hi Lukasz,

Have you had a chance to review my response? I look forward to what your thoughts are.

Thanks

Hi Lukasz,

Have you had a chance to review my response? I look forward to what your thoughts are.

Thanks

lukaszb added a commit that referenced this issue Oct 9, 2011

@lukaszb

This comment has been minimized.

Show comment
Hide comment
@lukaszb

lukaszb Oct 9, 2011

Contributor

Hey,

I've just written first test for your mixins. A few notes...

A. Test is damn simple. Just run

python manage.py test guardian.TestViewMixins

in order to see it's output (you can do that at example project. I'm not sure if I can put my previous description more clearly. Line e86f86e#L3R82 should be put just before the return statement in order to fix the test (well, actually there are more problems but we'd get to this). Your seem to understand mro and mixins (it's nor rocket sience after all...) but you still seem to miss the point I was writing about in my last comment - at dispatch method you're calling underlying view login BEFORE permission is actually checked. Please refer to the test - it may be more clear as it's also an example :)

Hope those would help use to get into the same page!
B. What I missed before - raise_exception option is misleading. Setting this to True won't raise any exception (docstring says it should) - it just returns HttpResponseForbidden. This is not the same as raising django.core.exceptions.PermissionDenied explicitly.

C. You haven't included imports. Did you actually tried that code?

D. I'm definitely against putting Eclipse project files into repository :P (and there is nothing about that I'm rather vim user - it's just not best practice ;-))

Contributor

lukaszb commented Oct 9, 2011

Hey,

I've just written first test for your mixins. A few notes...

A. Test is damn simple. Just run

python manage.py test guardian.TestViewMixins

in order to see it's output (you can do that at example project. I'm not sure if I can put my previous description more clearly. Line e86f86e#L3R82 should be put just before the return statement in order to fix the test (well, actually there are more problems but we'd get to this). Your seem to understand mro and mixins (it's nor rocket sience after all...) but you still seem to miss the point I was writing about in my last comment - at dispatch method you're calling underlying view login BEFORE permission is actually checked. Please refer to the test - it may be more clear as it's also an example :)

Hope those would help use to get into the same page!
B. What I missed before - raise_exception option is misleading. Setting this to True won't raise any exception (docstring says it should) - it just returns HttpResponseForbidden. This is not the same as raising django.core.exceptions.PermissionDenied explicitly.

C. You haven't included imports. Did you actually tried that code?

D. I'm definitely against putting Eclipse project files into repository :P (and there is nothing about that I'm rather vim user - it's just not best practice ;-))

@danielsokolowski

This comment has been minimized.

Show comment
Hide comment
@danielsokolowski

danielsokolowski Nov 3, 2011

Hi Lukasz

I just noticed that you have updated this issue; I will review this as soon as I can and respond accordingly. Thanks!

Hi Lukasz

I just noticed that you have updated this issue; I will review this as soon as I can and respond accordingly. Thanks!

@electroniceagle

This comment has been minimized.

Show comment
Hide comment
@electroniceagle

electroniceagle Dec 9, 2011

I pulled this feature branch and am using it on my test site and it looks like it is working fine.

Is it safe to have nested decorators and do the following?
class MyDetailView(LoginRequiredMixin, PermissionRequiredMixin, DetailView)

I pulled this feature branch and am using it on my test site and it looks like it is working fine.

Is it safe to have nested decorators and do the following?
class MyDetailView(LoginRequiredMixin, PermissionRequiredMixin, DetailView)

@danielsokolowski

This comment has been minimized.

Show comment
Hide comment
@danielsokolowski

danielsokolowski Dec 14, 2011

Hi electroniceagle,

As long as you keep the order of the mixins correct (see MTO - http://www.python.org/getit/releases/2.3/mro/) and know to call super() properly if you override the get method in your view then I do not see any issues what so ever.

Hi electroniceagle,

As long as you keep the order of the mixins correct (see MTO - http://www.python.org/getit/releases/2.3/mro/) and know to call super() properly if you override the get method in your view then I do not see any issues what so ever.

@pykler

This comment has been minimized.

Show comment
Hide comment
@pykler

pykler Jun 18, 2012

Can you chain these mixins in a view? does it work?

pykler commented Jun 18, 2012

Can you chain these mixins in a view? does it work?

@electroniceagle

This comment has been minimized.

Show comment
Hide comment
@electroniceagle

electroniceagle Jun 18, 2012

Yes. The mixins haven't been merged into the upstream branch so we're not using them in production. I've been using them on our development site, but want to understand why the upstream tests are failing before going live.

Sent from my iPad

On Jun 17, 2012, at 11:22 PM, Hatem Nassratreply@reply.github.com wrote:

Can you chain these mixins in a view? does it work?


Reply to this email directly or view it on GitHub:
#48 (comment)

Yes. The mixins haven't been merged into the upstream branch so we're not using them in production. I've been using them on our development site, but want to understand why the upstream tests are failing before going live.

Sent from my iPad

On Jun 17, 2012, at 11:22 PM, Hatem Nassratreply@reply.github.com wrote:

Can you chain these mixins in a view? does it work?


Reply to this email directly or view it on GitHub:
#48 (comment)

@danielsokolowski

This comment has been minimized.

Show comment
Hide comment
@danielsokolowski

danielsokolowski Jun 19, 2012

Are you saying these mixins are failing or there is test failure in
another area?

On 17/06/2012 23:52, Brian Schott wrote:

Yes. The mixins haven't been merged into the upstream branch so we're not using them in production. I've been using them on our development site, but want to understand why the upstream tests are failing before going live.

Sent from my iPad

On Jun 17, 2012, at 11:22 PM, Hatem Nassratreply@reply.github.com wrote:

Can you chain these mixins in a view? does it work?


Reply to this email directly or view it on GitHub:

#48 (comment)

Reply to this email directly or view it on GitHub:
#48 (comment)

Daniel Sokolowski
Web Engineer
Danols Web Engineering
http://webdesign.danols.com/

Are you saying these mixins are failing or there is test failure in
another area?

On 17/06/2012 23:52, Brian Schott wrote:

Yes. The mixins haven't been merged into the upstream branch so we're not using them in production. I've been using them on our development site, but want to understand why the upstream tests are failing before going live.

Sent from my iPad

On Jun 17, 2012, at 11:22 PM, Hatem Nassratreply@reply.github.com wrote:

Can you chain these mixins in a view? does it work?


Reply to this email directly or view it on GitHub:

#48 (comment)

Reply to this email directly or view it on GitHub:
#48 (comment)

Daniel Sokolowski
Web Engineer
Danols Web Engineering
http://webdesign.danols.com/

@electroniceagle

This comment has been minimized.

Show comment
Hide comment
@electroniceagle

electroniceagle Jun 19, 2012

The mixins appear to be working from our testing and you can chain them, but they currently fail django-guardian compliance tests and we haven't investigated the cause yet. There was some discussion in the issues on lukaszb upstream repo related to the pull request, but as far as I know it hasn't been resolved.

Brian Schott
bfschott@gmail.com

On Jun 19, 2012, at 11:58 AM, Daniel Sokolowski wrote:

Are you saying these mixins are failing or there is test failure in
another area?

On 17/06/2012 23:52, Brian Schott wrote:

Yes. The mixins haven't been merged into the upstream branch so we're not using them in production. I've been using them on our development site, but want to understand why the upstream tests are failing before going live.

Sent from my iPad

On Jun 17, 2012, at 11:22 PM, Hatem Nassratreply@reply.github.com wrote:

Can you chain these mixins in a view? does it work?


Reply to this email directly or view it on GitHub:

#48 (comment)

Reply to this email directly or view it on GitHub:
#48 (comment)

Daniel Sokolowski
Web Engineer
Danols Web Engineering
http://webdesign.danols.com/


Reply to this email directly or view it on GitHub:
#48 (comment)

The mixins appear to be working from our testing and you can chain them, but they currently fail django-guardian compliance tests and we haven't investigated the cause yet. There was some discussion in the issues on lukaszb upstream repo related to the pull request, but as far as I know it hasn't been resolved.

Brian Schott
bfschott@gmail.com

On Jun 19, 2012, at 11:58 AM, Daniel Sokolowski wrote:

Are you saying these mixins are failing or there is test failure in
another area?

On 17/06/2012 23:52, Brian Schott wrote:

Yes. The mixins haven't been merged into the upstream branch so we're not using them in production. I've been using them on our development site, but want to understand why the upstream tests are failing before going live.

Sent from my iPad

On Jun 17, 2012, at 11:22 PM, Hatem Nassratreply@reply.github.com wrote:

Can you chain these mixins in a view? does it work?


Reply to this email directly or view it on GitHub:

#48 (comment)

Reply to this email directly or view it on GitHub:
#48 (comment)

Daniel Sokolowski
Web Engineer
Danols Web Engineering
http://webdesign.danols.com/


Reply to this email directly or view it on GitHub:
#48 (comment)

@lukaszb

This comment has been minimized.

Show comment
Hide comment
@lukaszb

lukaszb Jun 19, 2012

Contributor

Mixins were not merged into upstream because of security problem I've been writing about earlier. To clear things up I'm going to go through the test once again:

from django.contrib.auth.models import User
from django.contrib.contenttypes.models import ContentType
from django.test import TestCase
from django.test.client import RequestFactory
from django.views.generic import View

from guardian.view_mixins import PermissionRequiredMixin


class DatabaseRemovedError(Exception):
    pass


class RemoveDatabaseView(View):
    def get(self, request, *args, **kwargs):
        raise DatabaseRemovedError("You've just allowed db to be removed!")

class TestView(PermissionRequiredMixin, RemoveDatabaseView):
    permission_required = 'contenttypes.change_contenttype'
    raise_exception = True
    object = None # should be set at each tests explicitly


class TestViewMixins(TestCase):

    def setUp(self):
        self.ctype = ContentType.objects.create(name='foo', model='bar',
            app_label='fake-for-guardian-tests')
        self.factory = RequestFactory()
        self.user = User.objects.create_user('joe', 'joe@doe.com', 'doe')
        self.client.login(username='joe', password='doe')

    def test_permission_is_checked_before_view_is_computed(self):
        """
        This test would fail if permission is checked **after** view is
        actually resolved.
        """
        request = self.factory.get('/')
        request.user = self.user
        view = TestView.as_view(object=self.ctype)
        response = view(request)
        self.assertEqual(response.status_code, 403)

(this is basically same as code in branch but I've added missing

    raise_exception = True

to the TestView)

I hope that RemoveDatabaseView is straightforward - basically, we don't want this to be executed at all ;-) For this we use provided PermissionRequiredMixin and require user to have 'contenttypes.change_contenttype' permission assigned for particular object, otherwise DatabaseRemovedError would be raised (as our view's get method is going to be executed). This is failing because permission checks are evaluated after method is executed. This is wrong. This needs to be fixed before merging to upstream (or before any one would use that code, in my opinion).

If still this is unclear, please change line that raises DatabaseRemovedError to:

        print(" => RemoveDatabaseView.get is being executed, even if user would be redirected or shown 403 page")

and run tests in console. Test passes, but the method is actually executed (you should see output in the console). Test passes as the final response is actually telling the user that he has no access to that page. Even if he/she already accessed it.

On the other hand: it seems like using those mixins is basically safe for views that do not execute anything important on the backend side but are used for showing/not showing page to the user.

Contributor

lukaszb commented Jun 19, 2012

Mixins were not merged into upstream because of security problem I've been writing about earlier. To clear things up I'm going to go through the test once again:

from django.contrib.auth.models import User
from django.contrib.contenttypes.models import ContentType
from django.test import TestCase
from django.test.client import RequestFactory
from django.views.generic import View

from guardian.view_mixins import PermissionRequiredMixin


class DatabaseRemovedError(Exception):
    pass


class RemoveDatabaseView(View):
    def get(self, request, *args, **kwargs):
        raise DatabaseRemovedError("You've just allowed db to be removed!")

class TestView(PermissionRequiredMixin, RemoveDatabaseView):
    permission_required = 'contenttypes.change_contenttype'
    raise_exception = True
    object = None # should be set at each tests explicitly


class TestViewMixins(TestCase):

    def setUp(self):
        self.ctype = ContentType.objects.create(name='foo', model='bar',
            app_label='fake-for-guardian-tests')
        self.factory = RequestFactory()
        self.user = User.objects.create_user('joe', 'joe@doe.com', 'doe')
        self.client.login(username='joe', password='doe')

    def test_permission_is_checked_before_view_is_computed(self):
        """
        This test would fail if permission is checked **after** view is
        actually resolved.
        """
        request = self.factory.get('/')
        request.user = self.user
        view = TestView.as_view(object=self.ctype)
        response = view(request)
        self.assertEqual(response.status_code, 403)

(this is basically same as code in branch but I've added missing

    raise_exception = True

to the TestView)

I hope that RemoveDatabaseView is straightforward - basically, we don't want this to be executed at all ;-) For this we use provided PermissionRequiredMixin and require user to have 'contenttypes.change_contenttype' permission assigned for particular object, otherwise DatabaseRemovedError would be raised (as our view's get method is going to be executed). This is failing because permission checks are evaluated after method is executed. This is wrong. This needs to be fixed before merging to upstream (or before any one would use that code, in my opinion).

If still this is unclear, please change line that raises DatabaseRemovedError to:

        print(" => RemoveDatabaseView.get is being executed, even if user would be redirected or shown 403 page")

and run tests in console. Test passes, but the method is actually executed (you should see output in the console). Test passes as the final response is actually telling the user that he has no access to that page. Even if he/she already accessed it.

On the other hand: it seems like using those mixins is basically safe for views that do not execute anything important on the backend side but are used for showing/not showing page to the user.

@danielsokolowski

This comment has been minimized.

Show comment
Hide comment
@danielsokolowski

danielsokolowski Jun 19, 2012

And the culprit is

def dispatch(self, request, _args, *_kwargs):
# call the parent dispatch first to pre-populate few things
before we check for permissions
original_return_value = super(PermissionRequiredMixin,
self).dispatch(request, _args, *_kwargs)

Which is being called so as to prepopulate the self.object --- Lukasz
thanks for crystal clear clarification. I believe I can come up with a
fix - stay tuned.

On 19/06/2012 12:32, Lukasz Balcerzak wrote:

Mixins were not merged into upstream because of security problem I've been writing. To clear things up I'm going to go through the test once again:

from django.contrib.auth.models import User
from django.contrib.contenttypes.models import ContentType
from django.test import TestCase
from django.test.client import RequestFactory
from django.views.generic import View

from guardian.view_mixins import PermissionRequiredMixin


class DatabaseRemovedError(Exception):
     pass


class RemoveDatabaseView(View):
     def get(self, request, *args, **kwargs):
         raise DatabaseRemovedError("You've just allowed db to be removed!")

class TestView(PermissionRequiredMixin, RemoveDatabaseView):
     permission_required = 'contenttypes.change_contenttype'
     raise_exception = True
     object = None # should be set at each tests explicitly


class TestViewMixins(TestCase):

     def setUp(self):
         self.ctype = ContentType.objects.create(name='foo', model='bar',
             app_label='fake-for-guardian-tests')
         self.factory = RequestFactory()
         self.user = User.objects.create_user('joe', 'joe@doe.com', 'doe')
         self.client.login(username='joe', password='doe')

     def test_permission_is_checked_before_view_is_computed(self):
         """
         This test would fail if permission is checked **after** view is
         actually resolved.
         """
         request = self.factory.get('/')
         request.user = self.user
         view = TestView.as_view(object=self.ctype)
         response = view(request)
         self.assertEqual(response.status_code, 403)

(this is basically same as code in branch but I've added missing

     raise_exception = True

to the TestView)

I hope that RemoveDatabaseView is straightforward - basically, we don't want this to be executed at all ;-) For this we use provided PermissionRequiredMixin and require user to have 'contenttypes.change_contenttype' permission assigned for particular object, otherwise DatabaseRemovedError would be raised (as our view's get method is going to be executed). This is failing because permission checks are evaluated after method is executed. This is wrong. This needs to be fixed before merging to upstream (or before any one would use that code, in my opinion).

If still this is unclear, please change line that raises DatabaseRemovedError to:

         print(" =>  RemoveDatabaseView.get is being executed, even if user would be redirected or shown 403 page")

and run tests in console. Test passes, but the method is actually executed (you should see output in the console). Test passes as the final response is actually telling the user that he has no access to that page. Even if he/she already accessed it.

On the other hand: it seems like using those mixins is basically safe for views that do not execute anything important on the backend side but are used for showing/not showing page to the user.


Reply to this email directly or view it on GitHub:
#48 (comment)

Daniel Sokolowski
Web Engineer
Danols Web Engineering
http://webdesign.danols.com/

And the culprit is

def dispatch(self, request, _args, *_kwargs):
# call the parent dispatch first to pre-populate few things
before we check for permissions
original_return_value = super(PermissionRequiredMixin,
self).dispatch(request, _args, *_kwargs)

Which is being called so as to prepopulate the self.object --- Lukasz
thanks for crystal clear clarification. I believe I can come up with a
fix - stay tuned.

On 19/06/2012 12:32, Lukasz Balcerzak wrote:

Mixins were not merged into upstream because of security problem I've been writing. To clear things up I'm going to go through the test once again:

from django.contrib.auth.models import User
from django.contrib.contenttypes.models import ContentType
from django.test import TestCase
from django.test.client import RequestFactory
from django.views.generic import View

from guardian.view_mixins import PermissionRequiredMixin


class DatabaseRemovedError(Exception):
     pass


class RemoveDatabaseView(View):
     def get(self, request, *args, **kwargs):
         raise DatabaseRemovedError("You've just allowed db to be removed!")

class TestView(PermissionRequiredMixin, RemoveDatabaseView):
     permission_required = 'contenttypes.change_contenttype'
     raise_exception = True
     object = None # should be set at each tests explicitly


class TestViewMixins(TestCase):

     def setUp(self):
         self.ctype = ContentType.objects.create(name='foo', model='bar',
             app_label='fake-for-guardian-tests')
         self.factory = RequestFactory()
         self.user = User.objects.create_user('joe', 'joe@doe.com', 'doe')
         self.client.login(username='joe', password='doe')

     def test_permission_is_checked_before_view_is_computed(self):
         """
         This test would fail if permission is checked **after** view is
         actually resolved.
         """
         request = self.factory.get('/')
         request.user = self.user
         view = TestView.as_view(object=self.ctype)
         response = view(request)
         self.assertEqual(response.status_code, 403)

(this is basically same as code in branch but I've added missing

     raise_exception = True

to the TestView)

I hope that RemoveDatabaseView is straightforward - basically, we don't want this to be executed at all ;-) For this we use provided PermissionRequiredMixin and require user to have 'contenttypes.change_contenttype' permission assigned for particular object, otherwise DatabaseRemovedError would be raised (as our view's get method is going to be executed). This is failing because permission checks are evaluated after method is executed. This is wrong. This needs to be fixed before merging to upstream (or before any one would use that code, in my opinion).

If still this is unclear, please change line that raises DatabaseRemovedError to:

         print(" =>  RemoveDatabaseView.get is being executed, even if user would be redirected or shown 403 page")

and run tests in console. Test passes, but the method is actually executed (you should see output in the console). Test passes as the final response is actually telling the user that he has no access to that page. Even if he/she already accessed it.

On the other hand: it seems like using those mixins is basically safe for views that do not execute anything important on the backend side but are used for showing/not showing page to the user.


Reply to this email directly or view it on GitHub:
#48 (comment)

Daniel Sokolowski
Web Engineer
Danols Web Engineering
http://webdesign.danols.com/

@lukaszb

This comment has been minimized.

Show comment
Hide comment
@lukaszb

lukaszb Jun 19, 2012

Contributor

Yes, that's exactly the spot, Daniel!

Contributor

lukaszb commented Jun 19, 2012

Yes, that's exactly the spot, Daniel!

@electroniceagle

This comment has been minimized.

Show comment
Hide comment
@electroniceagle

electroniceagle Jun 19, 2012

Cool. This has been on our story backlog for months...

Brian Schott
bfschott@gmail.com

On Jun 19, 2012, at 3:24 PM, Daniel Sokolowski wrote:

And the culprit is

def dispatch(self, request, _args, *_kwargs):
# call the parent dispatch first to pre-populate few things
before we check for permissions
original_return_value = super(PermissionRequiredMixin,
self).dispatch(request, _args, *_kwargs)

Which is being called so as to prepopulate the self.object --- Lukasz
thanks for crystal clear clarification. I believe I can come up with a
fix - stay tuned.

On 19/06/2012 12:32, Lukasz Balcerzak wrote:

Mixins were not merged into upstream because of security problem I've been writing. To clear things up I'm going to go through the test once again:

from django.contrib.auth.models import User
from django.contrib.contenttypes.models import ContentType
from django.test import TestCase
from django.test.client import RequestFactory
from django.views.generic import View

from guardian.view_mixins import PermissionRequiredMixin


class DatabaseRemovedError(Exception):
    pass


class RemoveDatabaseView(View):
    def get(self, request, *args, **kwargs):
        raise DatabaseRemovedError("You've just allowed db to be removed!")

class TestView(PermissionRequiredMixin, RemoveDatabaseView):
    permission_required = 'contenttypes.change_contenttype'
    raise_exception = True
    object = None # should be set at each tests explicitly


class TestViewMixins(TestCase):

    def setUp(self):
        self.ctype = ContentType.objects.create(name='foo', model='bar',
            app_label='fake-for-guardian-tests')
        self.factory = RequestFactory()
        self.user = User.objects.create_user('joe', 'joe@doe.com', 'doe')
        self.client.login(username='joe', password='doe')

    def test_permission_is_checked_before_view_is_computed(self):
        """
        This test would fail if permission is checked **after** view is
        actually resolved.
        """
        request = self.factory.get('/')
        request.user = self.user
        view = TestView.as_view(object=self.ctype)
        response = view(request)
        self.assertEqual(response.status_code, 403)

(this is basically same as code in branch but I've added missing

    raise_exception = True

to the TestView)

I hope that RemoveDatabaseView is straightforward - basically, we don't want this to be executed at all ;-) For this we use provided PermissionRequiredMixin and require user to have 'contenttypes.change_contenttype' permission assigned for particular object, otherwise DatabaseRemovedError would be raised (as our view's get method is going to be executed). This is failing because permission checks are evaluated after method is executed. This is wrong. This needs to be fixed before merging to upstream (or before any one would use that code, in my opinion).

If still this is unclear, please change line that raises DatabaseRemovedError to:

        print(" =>  RemoveDatabaseView.get is being executed, even if user would be redirected or shown 403 page")

and run tests in console. Test passes, but the method is actually executed (you should see output in the console). Test passes as the final response is actually telling the user that he has no access to that page. Even if he/she already accessed it.

On the other hand: it seems like using those mixins is basically safe for views that do not execute anything important on the backend side but are used for showing/not showing page to the user.


Reply to this email directly or view it on GitHub:
#48 (comment)

Daniel Sokolowski
Web Engineer
Danols Web Engineering
http://webdesign.danols.com/


Reply to this email directly or view it on GitHub:
#48 (comment)

Cool. This has been on our story backlog for months...

Brian Schott
bfschott@gmail.com

On Jun 19, 2012, at 3:24 PM, Daniel Sokolowski wrote:

And the culprit is

def dispatch(self, request, _args, *_kwargs):
# call the parent dispatch first to pre-populate few things
before we check for permissions
original_return_value = super(PermissionRequiredMixin,
self).dispatch(request, _args, *_kwargs)

Which is being called so as to prepopulate the self.object --- Lukasz
thanks for crystal clear clarification. I believe I can come up with a
fix - stay tuned.

On 19/06/2012 12:32, Lukasz Balcerzak wrote:

Mixins were not merged into upstream because of security problem I've been writing. To clear things up I'm going to go through the test once again:

from django.contrib.auth.models import User
from django.contrib.contenttypes.models import ContentType
from django.test import TestCase
from django.test.client import RequestFactory
from django.views.generic import View

from guardian.view_mixins import PermissionRequiredMixin


class DatabaseRemovedError(Exception):
    pass


class RemoveDatabaseView(View):
    def get(self, request, *args, **kwargs):
        raise DatabaseRemovedError("You've just allowed db to be removed!")

class TestView(PermissionRequiredMixin, RemoveDatabaseView):
    permission_required = 'contenttypes.change_contenttype'
    raise_exception = True
    object = None # should be set at each tests explicitly


class TestViewMixins(TestCase):

    def setUp(self):
        self.ctype = ContentType.objects.create(name='foo', model='bar',
            app_label='fake-for-guardian-tests')
        self.factory = RequestFactory()
        self.user = User.objects.create_user('joe', 'joe@doe.com', 'doe')
        self.client.login(username='joe', password='doe')

    def test_permission_is_checked_before_view_is_computed(self):
        """
        This test would fail if permission is checked **after** view is
        actually resolved.
        """
        request = self.factory.get('/')
        request.user = self.user
        view = TestView.as_view(object=self.ctype)
        response = view(request)
        self.assertEqual(response.status_code, 403)

(this is basically same as code in branch but I've added missing

    raise_exception = True

to the TestView)

I hope that RemoveDatabaseView is straightforward - basically, we don't want this to be executed at all ;-) For this we use provided PermissionRequiredMixin and require user to have 'contenttypes.change_contenttype' permission assigned for particular object, otherwise DatabaseRemovedError would be raised (as our view's get method is going to be executed). This is failing because permission checks are evaluated after method is executed. This is wrong. This needs to be fixed before merging to upstream (or before any one would use that code, in my opinion).

If still this is unclear, please change line that raises DatabaseRemovedError to:

        print(" =>  RemoveDatabaseView.get is being executed, even if user would be redirected or shown 403 page")

and run tests in console. Test passes, but the method is actually executed (you should see output in the console). Test passes as the final response is actually telling the user that he has no access to that page. Even if he/she already accessed it.

On the other hand: it seems like using those mixins is basically safe for views that do not execute anything important on the backend side but are used for showing/not showing page to the user.


Reply to this email directly or view it on GitHub:
#48 (comment)

Daniel Sokolowski
Web Engineer
Danols Web Engineering
http://webdesign.danols.com/


Reply to this email directly or view it on GitHub:
#48 (comment)

@danielsokolowski

This comment has been minimized.

Show comment
Hide comment
@danielsokolowski

danielsokolowski Jun 20, 2012

Lukasz, take a look #73 - the issue should be resolved.

Lukasz, take a look #73 - the issue should be resolved.

@lukaszb

This comment has been minimized.

Show comment
Hide comment
@lukaszb

lukaszb Jul 13, 2012

Contributor

Phew, it is done! :)

6adec98

Contributor

lukaszb commented Jul 13, 2012

Phew, it is done! :)

6adec98

@lukaszb lukaszb closed this Jul 13, 2012

@danielsokolowski

This comment has been minimized.

Show comment
Hide comment
@danielsokolowski

danielsokolowski Jul 17, 2012

You sir are epic, please do know that. :)

On 13/07/2012 08:08, Lukasz Balcerzak wrote:

Phew, it is done! :)

6adec98


Reply to this email directly or view it on GitHub:
#48 (comment)

Daniel Sokolowski
Web Engineer
Danols Web Engineering
http://webdesign.danols.com/
Office: 613-817-6833
Fax: 613-817-4553
Toll Free: 1-855-5DANOLS
Kingston, ON K7L 1H3, Canada

Notice of Confidentiality:
The information transmitted is intended only for the person or entity to which it is addressed and may contain confidential and/or privileged material. Any review re-transmission dissemination or other use of or taking of any action in reliance upon this information by persons or entities other than the intended recipient is prohibited. If you received this in error please contact the sender immediately by return electronic transmission and then immediately delete this transmission including all attachments without copying distributing or disclosing same.

You sir are epic, please do know that. :)

On 13/07/2012 08:08, Lukasz Balcerzak wrote:

Phew, it is done! :)

6adec98


Reply to this email directly or view it on GitHub:
#48 (comment)

Daniel Sokolowski
Web Engineer
Danols Web Engineering
http://webdesign.danols.com/
Office: 613-817-6833
Fax: 613-817-4553
Toll Free: 1-855-5DANOLS
Kingston, ON K7L 1H3, Canada

Notice of Confidentiality:
The information transmitted is intended only for the person or entity to which it is addressed and may contain confidential and/or privileged material. Any review re-transmission dissemination or other use of or taking of any action in reliance upon this information by persons or entities other than the intended recipient is prohibited. If you received this in error please contact the sender immediately by return electronic transmission and then immediately delete this transmission including all attachments without copying distributing or disclosing same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment