ForbiddenMixin for Django?
Jari Pennanen
order to make the class based view permission checking easier.
I don't know do other think that they need to be easier to check, but
I think following API would be simple:
class ProtectedView(TemplateView, ForbiddenMixin):
template_name = 'secret.html'
forbidden_checks = (login_required,
has_perms('auth.change_user'),)
forbidden_checks is list of functions (request, *args, **kwargs) ->
bool, has_perms is thus function that returns function.
This API would also allow to *override* the checks in subclassed
views, even make them empty again if needed.
I have not yet tried to create ForbiddenMixin but I don't think it
should be too difficult.
Jari Pennanen
because of MRO: https://gist.github.com/808516
If it is subclassed from View, then above would throw error and one
could not even define them in wrong order: AuthedViewSecond(View,
ForbiddenMixin):
Jari Pennanen
https://gist.github.com/809208
Jari Pennanen
2.6
has class decorators." - Russel
Why to use decorators? They cannot be overridden neatly in subclasses,
see my above gist about this subclassing.
Russell Keith-Magee
> "Meh - this seems like reinventing a syntactic wheel to me. Python
> 2.6
> has class decorators." - Russel
The pair of Ls in my email address aren't just there for show. Russell. Two Ls.
> Why to use decorators? They cannot be overridden neatly in subclasses,
> see my above gist about this subclassing.
Why use decorators? Because they're an established pattern. They're
conceptually well aligned -- take this existing thing, and wrap it
with this extra logic. And using decorators for this purpose is a
usage that already has precedent for exactly this purpose with
function-based views.
The catch is that it isn't easy to build a decorator that does what we
need it to do.
My problem with the approach you're describing is that I don't see
that it has precedent. Part of good API design is that it isn't
surprising. Why is a randomly named class variable the right place to
define the list of decorators that will be invoked before a dispatch?
What analogs exist in Django, or the wider Python community, that
would point to changing behavior based upon the value stored in a
class variable.
To my mind, this isn't quite the same as setting template_name. That's
a simple configuration issue, not something that fundamentally alters
view behavior. To reinforce the point -- as an alternative to
specifying template_name, you can override the function
get_template_names() which programatically returns the right template.
It would strike me as strange for the list of decorators to be applied
to a view to be configured in the same way. Yet it would be
contradictory to have some class variables extendable by function, but
not others.
I'm not saying I have a a better alternative waiting to be used -- I'm
just letting you know why I feel a design decision is required.
Yours,
Russ Magee %-)
Jari Pennanen
wrote:
> The pair of Ls in my email address aren't just there for show. Russell. Two Ls.
But I would be happy for class decorators too, as long as it is
simpler.
> To my mind, this isn't quite the same as setting template_name. That's
> a simple configuration issue, not something that fundamentally alters
> view behavior. To reinforce the point -- as an alternative to
> specifying template_name, you can override the function
> get_template_names() which programatically returns the right template.
> It would strike me as strange for the list of decorators to be applied
> to a view to be configured in the same way. Yet it would be
> contradictory to have some class variables extendable by function, but
> not others.
checks the request/input if the view is allowed to view. Secondly why
couldn't there be get_forbidden_checks() getter too if that is the
problem?
I personally don't find this behavior surprising. If there really is
something fundamentally different with forbidden_checks and other
class attributes like model, queryset, template_name,
context_object_name, ... then there has to be a way to define the
difference.
I can't see those other attributes any less important, shit will break
if one changes them unless changing other parts too (usually).
Łukasz Rekucki
>
> But I would be happy for class decorators too, as long as it is
> simpler.
def view_decorator(fdec):
def decorator(cls):
original = cls.as_view.im_func
@wraps(original)
def as_view(current, **initkwargs):
return fdec(original(current, **initkwargs))
cls.as_view = classonlymethod(as_view)
return cls
return decorator
Using this transformer, you can decorate classes just like normal
views. It's simple and not-magical. The only drawback is that it
modifies the given class, so it can't be used "in-place", so it didn't
make it (Although, I'm using it quite successfully for some time now
in my projects and we didn't had any trouble with it yet). Other
version was to create a subclass "on-the-fly", but it turned out to be
even more dangerous.
>
> This forbidden_checks is not list of decorators, just functions that
> checks the request/input if the view is allowed to view.
If they're not decorators, then that's even worse, 'cause you need to
duplicate all existing permission checking decorators.
> Secondly why
> couldn't there be get_forbidden_checks() getter too if that is the
> problem?
Something similar was already proposed earlier by Luke Plant and I
made an example implementation that takes MRO into account:
https://github.com/lqc/django/commit/ef7a7adfb72c79e4c0068841e4b71ac27c138b28
It does work quite nicely, but as Russell said - it's a magic
attribute, a bit like the Meta class in models, but worse as it
duplicates the idea of class decorators.
>
> I personally don't find this behavior surprising. If there really is
> something fundamentally different with forbidden_checks and other
> class attributes like model, queryset, template_name,
> context_object_name, ... then there has to be a way to define the
> difference.
things like `template_name` or `model` just overwrite whatever was
previously defined in the base classes. In case of permission checks,
you will most likely want to *add* checks, not overwrite. Also, if I
have 2 views/mixins that do some checks (which most likely are
required for proper operation of those views/mixins), then if I
inherit from both, my subclass should do all those checks, not only
part of them.
--
Łukasz Rekucki
Jari Pennanen
in mind that removing the ability to override them practicly means
that only Final class should define them.
That could lead to following annoying situations: Base classes could
e.g. enforce using some requirement for User.user_permission and
subclass could require using per object permission. What would happen
now? The programmer could not subclass from that class only because
the permissions checking is enforced in super class but otherwise a
good class, not exactly a good solution.
So I think the overriding ability in subclasses is a good to have.
On Feb 3, 6:15 pm, Lukasz Rekucki <lreku...@gmail.com> wrote:
> If they're not decorators, then that's even worse, 'cause you need to
> duplicate all existing permission checking decorators.
does two things in one function: Checking permissions from request
(what I propose to separate) and being decorator at the same time.
They break the principle of one function one task, and should be
breaken apart to checker functions and decorators. Similarly as
validator functions are separated and reusable.
This is what I said in my ticket that *checker* functions are reusable
in decorators (not otherway around.)
> things like `template_name` or `model` just overwrite whatever was
> previously defined in the base classes. In case of permission checks,
> you will most likely want to *add* checks, not overwrite. Also, if I
> have 2 views/mixins that do some checks (which most likely are
> required for proper operation of those views/mixins), then if I
> inherit from both, my subclass should do all those checks, not only
> part of them.
that because of need to override I said above), by iterating over
classes (in __mro__*) and combining the checker functions to one list.
However I find that would be odd behavior for class/instance
attribute. Also programmer should be able to remember all the checker
functions in all superclasses, and the overriding the earlier values
becomes impossible (or requires other way to do it).
*One can iterate over classes in mro order using e.g. inspect.getmro().
Łukasz Rekucki
> If everyone is happy with class decorators we should do it. But bear
> in mind that removing the ability to override them practicly means
> that only Final class should define them.
Which I think is actually a good thing from my experience, but I may be biased.
>
> That could lead to following annoying situations: Base classes could
> e.g. enforce using some requirement for User.user_permission and
> subclass could require using per object permission. What would happen
> now? The programmer could not subclass from that class only because
> the permissions checking is enforced in super class but otherwise a
> good class, not exactly a good solution.
Which is an argument against putting permission checks into generic
reusable views, just like the current views don't use the
TemplateMixin by default (well, they do, but they have a Base*
duplicates). I'm not saying, that having a permission checking mixin
is a bad idea, but I don't like the proposed API. For starters, you
mentioned "object permissions" - how do I check that in your
implementation, if all checkers are evaluated in dispatch() (and
without fetching the object multiply times) ?
> So I think the overriding ability in subclasses is a good to have.
Ok, but I rather override some methods then a list.
>
> On Feb 3, 6:15 pm, Lukasz Rekucki <lreku...@gmail.com> wrote:
>> On 3 February 2011 16:09, Jari Pennanen <jari.penna...@gmail.com> wrote:
>> If they're not decorators, then that's even worse, 'cause you need to
>> duplicate all existing permission checking decorators.
>
> In fact current decorator permission checking mechanism is wrong, it
> does two things in one function: Checking permissions from request
> (what I propose to separate) and being decorator at the same time.
> They break the principle of one function one task, and should be
> breaken apart to checker functions and decorators. Similarly as
> validator functions are separated and reusable.
It's actually a shortcut for user_passes_test() decorator factory and
a very short lambda, so there is not much to reuse (apart from the
redirection code maybe).
> Making the attribute to *add only* is possible (but I don't support
> that because of need to override I said above), by iterating over
> classes (in __mro__*) and combining the checker functions to one list.
Yes, I know it's possible and I even showed you that exact
implementation. It's not exactly "add only" as you can always override
`get_decorators()` to filter out or replace the list entirely.
> However I find that would be odd behavior for class/instance
> attribute. Also programmer should be able to remember all the checker
> functions in all superclasses, and the overriding the earlier values
> becomes impossible (or requires other way to do it).
Without the add semantics, every time I want to add a check, I need to
repeat all the checks in base classes, which is not very DRY if you
ask me. If the checks come from a 3rd party app and it ever changes, I
would need to go through all my code and update the lists.
Maybe your mixin could provide a method to run the checks (so the view
can decide when to invoke it) and some other methods which do the
actual checks that can be overridden. Just a thought.
--
Łukasz Rekucki
Jari Pennanen
On Feb 3, 7:49 pm, Łukasz Rekucki <lreku...@gmail.com> wrote:
>
> > If everyone is happy with class decorators we should do it. But bear
> > in mind that removing the ability to override them practicly means
> > that only Final class should define them.
>
> Which I think is actually a good thing from my experience, but I may be biased.
but in python there is no final classes.
> I'm not saying, that having a permission checking mixin
> is a bad idea, but I don't like the proposed API. For starters, you
> mentioned "object permissions" - how do I check that in your
> implementation, if all checkers are evaluated in dispatch() (and
> without fetching the object multiply times) ?
should do. Additionally my checkers does not take in account the
*view*. To make object permission checkers they should be more general
like func(view, request, *args, **kwargs) -> bool. This way they could
access the view.get_object()...
> It's actually a shortcut for user_passes_test() decorator factory and
> a very short lambda, so there is not much to reuse (apart from the
> redirection code maybe).
or func(request) -> bool.
> > Making the attribute to *add only* is possible (but I don't support
> > that because of need to override I said above), by iterating over
> > classes (in __mro__*) and combining the checker functions to one list.
>
> Yes, I know it's possible and I even showed you that exact
> implementation. It's not exactly "add only" as you can always override
> `get_decorators()` to filter out or replace the list entirely.
used similar way for my QuerySet basing (entirely different thing).
So what you are saying is that you guys cooked up already something
I'm reimplementing, list of decorators that can be overridden or used
as add only. Now we just need the decision what to use!
> Without the add semantics, every time I want to add a check, I need to
> repeat all the checks in base classes, which is not very DRY if you
> ask me. If the checks come from a 3rd party app and it ever changes, I
> would need to go through all my code and update the lists.
case scenario, so we are arguing over a very little improvement even
if it were overridable or only defined in final-like class.
> Maybe your mixin could provide a method to run the checks (so the view
> can decide when to invoke it) and some other methods which do the
> actual checks that can be overridden. Just a thought.
of boiler plate code, or utility libraries. And more than anything I
hate the fact that there is no single decision to choose class method
decorators or anything else *shorter*.
(E.g. I just discovered that get_or_404 is included in django, I
wonder how long it took to be made. I've been dreaming for a long time
for Model.objects.get_or_404, but I think module level will do too. So
this is one less utility function I need to "maintain".)