Re: [Feature Request] Having an middleware to be able to force authentication on views by default

[Adam Johnson]

Hi Mehmet,

I like your move to fail-closed here. I've certainly seen missing auth
decorators as a recurring issue in my career, and I do think as an OWASP
top ten we should try tackle it better in the framework.

Your implementation is very few lines of code. It could be made more
robust, using the attribute approach that the CSRF middleware uses:
https://github.com/django/django/blob/7075d27b0c75f1431f29497f4353cd742906b357/django/middleware/csrf.py#L209
. And it could easily be added to django.contrib.auth, and the default
project template. AuthenticationMiddleware doesn't in fact have a
process_view method at current, so we could even add it there with a
setting to control it.

I doubt many would be against it as an optional extra.

Thanks,

Adam

On Sun, 15 Mar 2020 at 06:36, Václav Řehák  wrote:

> Hi Tobias,
>
> Dne sobota 14. března 2020 9:44:16 UTC+1 Tobias Bengfort napsal(a):
>>
>>
>> Another option could be to add system checks for this: Instead of
>> silently "fixing" missing code it would inform developers about missing
>> decorators/mixins. (If I have time I might try to come up with a
>> prototype of this.)
>
>
> my thinking about this is exactly the same as yours. I have seen issues
> with developers forgetting to add LoginRequiredMixin almost on all projects
> I worked on and I think all of this issues would have been prevented if the
> developers were force to explicitly specify the desired authentication
> requirements for each view (probably using the checks system).
>
> In my current project we added a testcase to go through all urls in
> urlconf and compare then to whitelist of public urls. But it is ugly as it
> hardcodes urls as strings (similar to the django-utils repo) defeating the
> flexibility of url patterns.
>
> --
> 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 view this discussion on the web visit
> https://groups.google.com/d/msgid/django-developers/4c13eb8d-eb6a-4973-be4d-5abe0fc55bb9%40googlegroups.com
> 
> .
>


-- 
Adam

-- 
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 view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/CAMyDDM3Ui3shxaspquwhbvT0%2BrbhJXfkC4Sd8bw-zcS_2u0Q5A%40mail.gmail.com.

Re: [Probably BUG] set_password and check_password accept values other than string as parameters

[Adam Johnson]

Dawid, thank you for checking these other implementations. I agree it's
somewhat surprising and clearly something the developers of the other
password libraries decided to guard against.

One question I have is - did you experience any real world issue with this?
Reading back over the thread, you haven't mentioned this. As Tom says, this
is the first mention he can remember.

We could add type guards to many of the thousands of functions in Django to
prevent potential bugs. I'm sure some would. I'm sure many would just be
"academic". And they come with both the implementation and maintenance
costs, plus making duck-typing harder for users.

To add one here, it would go through the normal deprecation process:
https://docs.djangoproject.com/en/dev/internals/release-process/#internal-release-deprecation-policy
. This means PR's for 3.1, 3.2, and 4.0, changing the code, documentation,
and release notes.

I'd say at this point I'm convinced this *could* be a (slightly) useful
change. But you have to be aware - I don't think you'll find anyone who's
willing to do this work other than yourself. There are many more
interesting changes to be made to Django if you check Trac!

Thanks,

Adam



On Sat, 14 Mar 2020 at 14:53, Dawid Czeluśniak 
wrote:

> Tom,
>
> The behavior of the make_password method is quite surprising to be honest
>>
>
> I'd go even further and say that currently the behaviour of the
> make_password function is *wrong* and *unsafe*. Again, let's look at
> hashing functions from other libraries. None of them fails silently and
> casts object to bytes using __str__().  Werkzeug and passlib are the most
> notable examples how to handle things correctly:
>
>
> *1. Werkzeug*
> In [1]: from werkzeug.security import generate_password_hash
>
> In [2]: generate_password_hash(dict())
> TypeError: Expected bytes
>
>
> *2. Passlib*
> In [1]: from passlib.hash import pbkdf2_sha256
>
> In [2]: pbkdf2_sha256.hash(dict())
> TypeError: secret must be unicode or bytes, not dict
>
>
> *3. Django*
> In [1]: from django.contrib.auth.hashers import make_password
>
> In [2]: make_password(dict())
> Out[2]:
> 'pbkdf2_sha256$18$dimMkJ5wvrpn$eHh6CNAY+hTagaDmsofHMlJEbVOXEeIEfcT059Me2ho='
> (seriously???)
>
> This is especially *wrong* because programmers who are *not* aware of
> this strange behaviour can accidentally do things that they *really *don't
> want to do. I can imagine scenarios in which this can have some serious
> unintended consequences.
>
> maybe the advantages of being able to pass any object into the method is
>> entirely academic because nobody passes anything but strings on purpose
>
>
> Exactly. I'd even say that there are *no* advantages of being able to
> pass any object into this function and it can have bad consequences.
>
> Dawid
>
> --
> 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 view this discussion on the web visit
> https://groups.google.com/d/msgid/django-developers/CAHzshFuQnEUrAdk53apDWw3wnPBNq%2BYQE9bxyfOpbFfyQS04dw%40mail.gmail.com
> 
> .
>


-- 
Adam

-- 
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 view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/CAMyDDM0h8aD4-vutrZ3m2kZ_CuTXoY3sYN5N83Vr_9HtEWLuYw%40mail.gmail.com.

Re: [Feature Request] Having an middleware to be able to force authentication on views by default

[Mehmet Ince]

Hi Adam,

Thanks for your comments. I was thinking to implemented this as a separated 
middleware but, as you said, AuthenticationMiddleware is much better place to 
do it.

I already started to implementing this in AuthenticationMiddleware. I would 
like to send a PR if it’s okay to everyone ?

I’m not sure it’s okay to discuss this in mailing list but I need a help about 
naming convention for following variables/class/function:

- Variable name to control this in settings.py. ( FORCE_LOGIN_REQUIRED maybe ? )
- Mixing name for CBV and decorator name FBV  ( AnonymousUserMixin and 
@anonymous_user ? )

Thanks,
M.

> On 15 Mar 2020, at 17:13, Adam Johnson  wrote:
> 
> Hi Mehmet,
> 
> I like your move to fail-closed here. I've certainly seen missing auth 
> decorators as a recurring issue in my career, and I do think as an OWASP top 
> ten we should try tackle it better in the framework.
> 
> Your implementation is very few lines of code. It could be made more robust, 
> using the attribute approach that the CSRF middleware uses: 
> https://github.com/django/django/blob/7075d27b0c75f1431f29497f4353cd742906b357/django/middleware/csrf.py#L209
>  
> 
>  . And it could easily be added to django.contrib.auth, and the default 
> project template. AuthenticationMiddleware doesn't in fact have a 
> process_view method at current, so we could even add it there with a setting 
> to control it.
> 
> I doubt many would be against it as an optional extra.
> 
> Thanks,
> 
> Adam
> 
> On Sun, 15 Mar 2020 at 06:36, Václav Řehák  > wrote:
> Hi Tobias,
> 
> Dne sobota 14. března 2020 9:44:16 UTC+1 Tobias Bengfort napsal(a):
> 
> Another option could be to add system checks for this: Instead of
> silently "fixing" missing code it would inform developers about missing
> decorators/mixins. (If I have time I might try to come up with a
> prototype of this.)
> 
> my thinking about this is exactly the same as yours. I have seen issues with 
> developers forgetting to add LoginRequiredMixin almost on all projects I 
> worked on and I think all of this issues would have been prevented if the 
> developers were force to explicitly specify the desired authentication 
> requirements for each view (probably using the checks system).
> 
> In my current project we added a testcase to go through all urls in urlconf 
> and compare then to whitelist of public urls. But it is ugly as it hardcodes 
> urls as strings (similar to the django-utils repo) defeating the flexibility 
> of url patterns.
> 
> --
> 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 view this discussion on the web visit 
> https://groups.google.com/d/msgid/django-developers/4c13eb8d-eb6a-4973-be4d-5abe0fc55bb9%40googlegroups.com
>  
> .
> 
> 
> --
> Adam
> 
> --
> 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 view this discussion on the web visit 
> https://groups.google.com/d/msgid/django-developers/CAMyDDM3Ui3shxaspquwhbvT0%2BrbhJXfkC4Sd8bw-zcS_2u0Q5A%40mail.gmail.com
>  
> .

-- 
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 view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/6AF68B76-A6FC-4C59-A909-26E581C1F1C9%40mehmetince.net.


signature.asc
Description: Message signed with OpenPGP

Re: [Probably BUG] set_password and check_password accept values other than string as parameters

[Dawid Czeluśniak]

Adam,

One question I have is - did you experience any real world issue with this?


Personally I didn't, but I can imagine scenarios where this *could* be an
issue for other programmers. Suppose you want to create a password hash
from another SHA256 hash, but you're only a human
 and you forget to call
hexdigest() on the sha object:

In [1]: import hashlib

In [2]: password = hashlib.sha256('some_dummy_password'.encode())

In [3]: password
Out[3]: 

In [4]: user.set_password(password)

In [5]: user.check_password('')
Out[5]: True

Indeed, you may think that this is a pretty uncommon scenario, but it
*could* happen. For some reason libraries like Werkzeug
 and passlib
 raise the TypeError in this
case and therefore a programmer can actually see that something is wrong.
On the other hand, Django fails silently and the application will save a
wrong password to the database. Yes, there are no doubts that in the end it's
programmer's fault, but on the Django's main page we can read:

Django takes security seriously and helps developers avoid many common
> security mistakes.
>

I believe that adding a type guard to make_password function will help
developers avoid a security mistake.

We could add type guards to many of the thousands of functions in Django to
> prevent potential bugs.
>

Of course we could, but that would be useless and waste of time. The
question is if a function is important enough to add a type guard. Well, I
think that function that is responsible for generating password hashes *is*
important from the security perspective. And it seems that creators of
Werkzeug  and passlib
 libraries think so as well. But
if you think the opposite, I'm fine with that. In this case, a small change
in the documentation on make_password behaviour might be helpful.

Thanks,
Dawid

-- 
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 view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/CAHzshFusSJr0QN46TN%3DLQ2PQ03b9XaceKRDkQ2%2B961wqWL%2Bjxw%40mail.gmail.com.