is_authenticated as property

[Thomas Guettler]

Hi,

is_staff, is_active, is_superuser are attributes.

is_anonymous, is_authenticated are methods.

This is insecure if you are not careful while programming:

if user.is_authenticated:
....# Always true, since it is a method!

It would be nice to find a solution. Here is what I thought:

Make is_authenticated a property which returns a object
which evaluates to the proper boolean value. This object
has a method __call__ which returns the same value.

This is backwards compatible.

Thomas

--
Thomas Guettler, http://www.thomas-guettler.de/
E-Mail: guettli (*) thomas-guettler + de

[David Cramer]

I wouldn't say insecure, but its a big gotcha. I've done it a quite a
few times where I forgot the () :)

On Apr 10, 5:53 am, Thomas Guettler <h...@tbz-pariv.de> wrote:
> Hi,
>
> is_staff, is_active, is_superuser are attributes.
>
> is_anonymous, is_authenticated are methods.
>
> This is insecure if you are not careful while programming:
>
> if user.is_authenticated:
> ....# Always true, since it is a method!
>
> It would be nice to find a solution. Here is what I thought:
>
> Make is_authenticated a property which returns a object
> which evaluates to the proper boolean value. This object
> has a method __call__ which returns the same value.
>
> This is backwards compatible.
>
> Thomas
>
> --

> Thomas Guettler,http://www.thomas-guettler.de/

[Tim Graham]

Someone created a ticket to raise this issue again. I found several improper usages with GitHub code search. Is there any support for the idea or would it be too much magic? My own opinion is that if you don't have tests to catch the mistake in the first place, you're doing it wrong.

https://code.djangoproject.com/ticket/25847

[Tom Christie]

> My own opinion is that if you don't have tests to catch the mistake in the first place, you're doing it wrong.

Not sure I'd necessarily agree there - easy to miss the anonymous case.

No obvious best thing to do tho'.

On the one hand it's too easy to get wrong, on the other the current visual distinction between the model fields, and model methods may be helpful.

[Collin Anderson]

On a somewhat unrelated note, is_authenticated really only makes sense when using request.user.is_authenticated. If you simply query a user from the database, is_authenticated will be True, which doesn't make any sense outside the context of a request. is_anonymous makes sense, is_authenticated doesn't work as well.

--
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-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/df236217-bc38-4ceb-8d1e-1da18268c81c%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Aymeric Augustin]

Django 1.8 worsens the situation significantly:

    {% if request.user.is_authenticated %}

does the right thing in a Django template but is a security vulnerability in a Jinja2 template!

We could implement a property that returns an object:

- that is still callable, for backwards compatibility

- that evaluates correctly in a boolean context

Then we can consider deprecating the ability to call it.

class CallableBool:

    def __init__(self, value):

        self.value = value

    def __bool__(self):

        return self.value

    def __call__(self):

        return self.value

    def __repr__(self):

        return 'CallableBool(%r)' % self.value

CallableFalse = CallableBool(False)

CallableTrue = CallableBool(True)

It's a bit of a hack, but what Pythonista doesn't like using Python like that? ;-)

-- 

Aymeric.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAFO84S7AWu_0a4nPS%3DsZAxwy3MvXXYg%3DqBbwKDHwrhD-rVpWag%40mail.gmail.com.

[Shai Berger]

On Wednesday 02 December 2015 18:51:03 Aymeric Augustin wrote:
>
> We could implement a property that returns an object:
>
> - that is still callable, for backwards compatibility
> - that evaluates correctly in a boolean context
>
> Then we can consider deprecating the ability to call it.
>
>
> class CallableBool:
>
> def __init__(self, value):
> self.value = value
>
> def __bool__(self):
> return self.value
>
> def __call__(self):
> return self.value
>
> def __repr__(self):
> return 'CallableBool(%r)' % self.value
>
> CallableFalse = CallableBool(False)
>
> CallableTrue = CallableBool(True)
>

More general alternative:

class IdempotentCallMixin(object):
def __call__(self): return self

However, you can't extend bool, so

class CallableBool(IdempotentCallMixin, bool): pass

does not work, and you'd have to do something like

class CallableBool(IdempotentCallMixin, int): pass

which is less nice (because of __repr__ etc)

Shai

[Josh Smeaton]

I agree with the ticket. The imbalance between is_superuser and is_authenticated() should be enough to consider updating the API. The security concerns and, in particular, Aymeric's Jinja example, make this ticket more compelling.

I think we should be asking.. why not? If there's not a good reason not to, let's make it a callable and a property.

[Aymeric Augustin]

2015-12-03 7:02 GMT+01:00 Josh Smeaton <josh.s...@gmail.com>:

I think we should be asking.. why not? If there's not a good reason not to, let's make it a callable and a property.

The original discussion dates back to 2008; back then I believe we were slightly more resistant to change, especially when it came to complicating things to increase user-friendliness ;-)

--

Aymeric.

[Tim Graham]

Florian has raised the issue of custom user models which define these methods, "Allowing custom user models to have a method is a security risk since all checks will now return true." I proposed a compatibility system check error to detect that situation to alert the user. Do you think the backwards incompatibility is justified here or do you prefer some other solution? I cannot think of much we could do besides monkey-patching the custom-user model.

[Florian Apolloner]

On Monday, April 11, 2016 at 6:57:46 PM UTC+2, Tim Graham wrote:

I cannot think of much we could do besides monkey-patching the custom-user model.

I would not call checking and rewriting the class in __new__ monkey-patching, but…

[Sven R. Kunze]

Am Montag, 11. April 2016 18:57:46 UTC+2 schrieb Tim Graham:

Do you think the backwards incompatibility is justified here or do you prefer some other solution?

I for one think it is justified here. 

[Florian Apolloner]

Absolutely not, what are you basing your justification on?

[Sven R. Kunze]

Am Montag, 25. April 2016 10:38:23 UTC+2 schrieb Florian Apolloner:

Absolutely not, what are you basing your justification on?

The fact that I know real cases where this was a security issue. I'd rather have a backwards incompatibility than a security hole. But that may just be me.

One might say that it's the responsibility of the developers and the testsuite/quality management. However, human make errors.

[Florian Apolloner]

This is imo an argument for a good and solid backwards path with loud warnings.

[Shai Berger]

I think this is what we must do in order to keep backwards-compatibility while
resolving the issue.

If we don't resolve the issue, people will just keep using the method as if it
were a property, opening themselves up in various ways;

If we just make it a property in the builtin classes, custom user classes will
re-introduce the issue.

We can handle it in two ways, as far as I can see:

A) What Florian alluded to above -- give AbstractBaseUser a metaclass which
replaces the methods with Aymeric's "CallableBool"s

B) Implement in AbstractBaseUser a __getattribute__ which, essentially, does
the same thing when asked for the methods.

When idea which I discarded was to "force" the attributes to be used as
methods -- to use a callable which raises an exception when evaluated as
boolean. But this replacement suffers exactly the same problems as the
CallableBool replacement, unless also supported by "monkey patching"
techniques, and a CallableBool is much more user-friendly.

Have fun,
Shai.

[Tim Graham]

Do you think my suggestion of a system check to flag this is unacceptable?

diff --git a/django/contrib/auth/checks.py b/django/contrib/auth/checks.py
index d1b0a44..347ad75 100644
--- a/django/contrib/auth/checks.py
+++ b/django/contrib/auth/checks.py
@@ -73,6 +73,14 @@ def check_user_model(app_configs=None, **kwargs):
                 )
             )
 
+    if not isinstance(cls.is_anonymous, property):
+        errors.append(
+            checks.Error('%s.is_anonymous must be a property.' % cls)
+        )
+    if not isinstance(cls.is_authenticated, property):
+        errors.append(
+            checks.Error('%s.is_authenticated must be a property.' % cls)
+        )
     return errors

[Florian Apolloner]

Are errors silence able via SILENCED_SYSTEM_CHECKS? If yes, I am against it -- this has to be an hard unrecoverable error for security reasons or fully backwards compatible (I am leaning towards the latter if possible in any way).

[Markus Holtermann]

I haven't read the entire thread, did you account for custom user models
that don't inherit from AbstractBaseUser? Do the system checks stil
work? A Metaclass certainly would not, would it?

Cheers,

/Markus

>--
>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-develop...@googlegroups.com.
>To post to this group, send email to django-d...@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/aa589a27-cb1d-4233-b942-9f40d8416185%40googlegroups.com.

[Shai Berger]

Right, right and right.

Tim, I somehow missed that suggestion. It is certainly workable.

Markus, Custom user classes which do not inherit AbstractBaseUser are a point
I hadn't considered, and in fact they make my suggestions problematic, leaving
_only_ the checks approach.

However, descriptors which aren't properties should also be acceptable... in
particular, even plain attributes. I would consider writing the check not as

if not isinstance(cls.is_anonymous, property):

but instead using

if not isinstance(cls().is_anonymous, bool):

or even

if isinstance(cls().is_anonymous, six.types.MethodType):

which assumes that if there's anything other than a method on the instance,
then the user probably knows what they're doing.

The problem with these suggestions is that they create a user object during
checks, and that might be an obstacle for some users (side effects and required
initializer parameters are the two most obvious issues).

Shai.

[Florian Apolloner]

On Thursday, April 28, 2016 at 8:07:36 PM UTC+2, Markus Holtermann wrote:

I haven't read the entire thread, did you account for custom user models
that don't inherit from AbstractBaseUser? Do the system checks stil
work? A Metaclass certainly would not, would it?

Yeah, we probably would need both -- ie be backwards compatible if you inherit some BaseUser (which should be the common case I'd say) and otherwise bail out. I'd like to make it at least somewhat backwards compatible if possible at all…
 

[Florian Apolloner]

On Thursday, April 28, 2016 at 10:48:02 PM UTC+2, Shai Berger wrote:

The problem with these suggestions is that they create a user object during
checks, and that might be an obstacle for some users (side effects and required
initializer parameters are the two most obvious issues).

cls() would only go through model.__init__ which is imo an implementation detail unless one knows what they are doing -- so sideeffects or required params should not be an issue here (would probably not work well if you ever want to load a user from the db etc anyways…)

[Shai Berger]

Well, actually, there is a problem with the check approach: People use proxy
models with user models, to account for different types of users; and it could
be that the actual class is decided before the user is authenticated (because
of different entry URLs, different authentication backends, whatever). The
settings-defined user model will not always be the actual class of user
objects, so checking it may not be enough (although I would agree that the
case where it isn't is probably a particularly rare edge-case).

Shai