[GSoC 2016] Please Critique (Condition API - Related to Auth)

[Connor Boyle]

Hello Everyone,

I would love to hear your critiques on my proposal and original idea for GSoC 2016, hopefully before the deadline tomorrow–although I plan on pursuing it no matter what, so any critique at any time would be welcome (including those questioning the merit/need of the concept).

In case you haven't seen any of my other posts, it is closely based on Django Rest Framework's 'permissions', however it extends significantly beyond them in terms of what a developer can use them to accomplish.

I am on Beijing time (UTC +8) so I will be asleep and then in class for the next twelve hours, so sorry if I don't respond until after that.

Thanks,

Connor

[James Pic]

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

[Connor Boyle]

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.

[Connor Boyle]

> On your last point: This may be a very bad idea from the beginning, but I'd hope to experiment with making a wrapper object for form.cleaned_data whose (the wrapper object's) .__getattribute__() returns the value for that key in form.cleaned_data.

For example:

calling wrapper.name would get the value stored in form.cleaned_data['name']

On Friday, March 25, 2016 at 4:21:09 AM UTC+8, is_null wrote:

[Connor Boyle]

Hello all,

Is there any chance I could get some feedback on why this was not accepted? I am a little surprised since Jacob Kaplan-Moss seemed to indicate to me over IRC that this was very likely to get an award for GSoC.

Thanks,

Connor

[Tim Graham]

Hi Connor,

Unfortunately we didn't have anyone sufficiently intrigued by your ideas who wanted to mentor it. In case you'd like to try again next year, I'd suggest to get involved in the project and start submitting patches as you're able. Mentors are more wiling to invest their time to help someone who has a record of solid contributions.

Tim

[Connor Boyle]

Thanks for the response, Tim.