This is excellent! Thank you for your comments, they are very helpful and
thorough–far from amateur. I'll address some of them in no particular
order, starting with:
>The 'access' conditions must be passed or else any action will be rejected
and no acknowledgement will be made that it had to do with authorization,
so as to not unnecessarily leak information (e.g. in the case of the given
model, if IsInLibrary doesn't pass, any protected view will simply return a
404 - page not found, instead of 403 - permission denied). I just added an
explicit definition to my proposal. I guess I had expected the reader to
infer that from my (much earlier in the proposal) definition of
'access_conditions'. Not very Pythonic of me!
>Although I see the clear appeal of overriding '|' and '&'–it's both
intuitive and aesthetically pleasing–I'm against it. Unlike doing this with
Q objects, where the result (iirc) is another Q object, the result of the
operators would be an instance of either EveryCondition or AnyCondition.
I'd rather not implicitly thrust an object of a potentially different class
on the developer. Furthermore, I'd rather not make the definition of
BaseCondition refer to EveryCondition and AnyCondition, its sub-classes, if
I can help it.
>As for Conditions as Q objects (mentioned in form fields filtering),
Conditions are simply meant to be more generalized than this. For example,
they might have to evaluate based on the result of the method on a model
instance, which couldn't be translated to a Q object.
On the other hand, there is a pretty good chance that there would be a
sub-class for Conditions based on Q objects. However, in order to get the
view to use them efficiently would require even more special method
overriding. I'm not sure if I'd be able to justify adding that much
complexity to the mixin for a special case that would not even be
particularly hard for the developer to implement given the callback my
preferred implementation of the mixin would provide. Furthermore, this
solution doesn't quite feel *correct*, since it would be using something
other than a Condition's run() and acting as if it were using its logic.
On Friday, March 25, 2016 at 4:21:09 AM UTC+8, is_null wrote:
>
> Hi Connor,
>
> Overall I find it pretty cool that work on this has been started.
> There are a few questions I'd like to ask on this proposal.
>
> In this example:
>
> class ReadingDelete(UserPassesTestMixin, DeleteView):
> model = Reading
>
> def test_func(self):
> user = self.request.user
> reading = self.get_object()
> return reading in user.leader.club.readings.all()
>
> Should we really fetch the object from all objects and then check it
> against a "secure" queryset ? After all, there's absolutely no
> security here between the "get_object()" call and the "reading in ..."
> test. Wouldn't it be more efficient and safer to try to fetch the
> object directly from a secure queryset ? ie: self.queryset =
> user.leader.club.readings.all(); reading = self.get_object(). This
> somehow relates to the Form Choices Filter sections of the proposal:
>
> > running a Condition against every instance in a queryset can quickly
> become very inefficient. For cases when it would be necessary, the mixin
> would provide a callback to allow the developer to use whatever means they
> want to more efficiently narrow down the queryset before the Conditions are
> run against its instances. This may seem like redundant code, however the
> purposes of the two different "narrowing" methods are not the same, one is
> for efficiency, one is for security. Rough implementation:
>
> The implementation is pretty rough indeed: it seems like the queryset
> is narrowed only if method == 'GET'. Doesn't that mean that a
> malicious user can validate a form with selected choice that was not
> displayed in the initial rendering ?
>
> IMHO this is the most challenging part of the subject this proposal is
> about. Perhaps if Conditions could translate to combinations of Q
> objects it would be easier to get the best of both worlds.
>
>
> > As is probably fairly clear from the above code, a user attempting to
> access the above view would have to have theiris_staff attribute set to
> True, otherwise the view will (if the following behavior is not overridden)
> raise a PermissionDeniederror with an appropriate message provided
> automatically by conditions.RequestUserCondition.
>
> What kind of messages would be appropriate to use here ? Wouldn't it
> be tricky to find safe messages, that wouldn't help a malicious user
> gain information about the security they are trying to circumvent ?
> For me, this raises the same problem as with login forms: if an
> attacker gets a "incorrect username or passord" message on a failed
> login test then they don't learn if the username exist, whereas having
> an "incorrect password" message on a failed login test does leak the
> information that the username exists. That said, this kind of feature
> would *definitely* by useful for logs.
>
>
> > Since Clubs no longer have .leader (they now have .leaders), the if
> statement's condition becomes nonsense, and due to Django templates' policy
> of silent failure simply doesn't show the contents of the if statement,
> thus not showing the links to the update and delete pages for that Reading.
>
> If such a bug is found, then a the DoD of the bugfix should include a
> regression test to ensure that leaders always see the edit/delete
> links in the template response. Should Ahmad have duplicate logic in
> templates like this in the first place ? I recon it's the same problem
> everybody has to solve in their projects though: perhaps it would be
> interresting to encapsulate this particular logic in a template filter
> ?
>
> {% if reading.club.leader.user == request.user %}
> <a href="{% url 'reading:update' pk=reading.pk %}">Update</a>
> <a href="{% url 'reading:delete' pk=reading.pk %}">Delete</a>
> {% endif %}
>
> Becomes:
>
> {% if reading|can_edit:request.user %}
> <a href="{% url 'reading:update' pk=reading.pk %}">Update</a>
> <a href="{% url 'reading:delete' pk=reading.pk %}">Delete</a>
> {% endif %}
>
> Or, with a context variable, like in the case of django.contrib.admin:
> {% if has_change_permission %}. That said, I'm all for providing a
> framework for Ahmad to have a convenient, consistent, secure and
> forward-compatible way to encapsulate these permission checks.
>
>
> > Combiners
> > There would also of course be classes for combining multiple conditions
> into one. The two "combiners" would beEveryCondition and AnyCondition.
>
> Would it be nice to leverage & and | operators and perhaps have ~ for
> negation like with Q objects ?
>
>
> In this example:
>
> class Book(models.Model):
> class Meta:
> conditions = {
> 'access': (IsInLibrary,),
> 'create': (IsAuthor, CanWrite),
> 'read': (CanRead, OwnsBook),
> 'update': (OwnsReadPen,),
> 'delete': (IsABadPerson,),
> }
>
> What is the difference between access and read ? Is 'access' here as
> an example of non-default permission type that Django users could
> implement, such as 'borrow', or would this be used by any feature
> provided by the framework itself ?
>
>
> > Eventually, the Conditions API could provide a mixin for views to take
> Conditions that were originally designed to be run on objects and apply
> them to the .cleaned_data of forms. It is uncertain how reliably this could
> really work, but it would certainly increase the usefulness of the
> Conditions API if it did. For example, it would allow a developer to use
> the same Conditions they would to limit the kind of object a user can
> access to limit the kind of object the user can create.
>
> How would that work exactly ? Could you add an example ?
>
>
> Also note that I'm just an amateur django-developer so perhaps don't
> take my review too seriously, I'm just trying to learn and help ;)
>
> Best
>
> James
>
--
You received this message because you are subscribed to the Google Groups
"Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-developers/7c689b62-6789-4927-b52b-3f47f6827d8b%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
