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

[Václav Řehák]

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.

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

[Mehmet Ince]

Hi Tobias,

Thanks for your comments

> On 14 Mar 2020, at 11:43, Tobias Bengfort  wrote:
> 
> Hi Mehmet,
> 
> On 13/03/2020 21.47, Mehmet Ince wrote:
>> - We must forcefully enable session validation for every endpoint.
>> - Developers must do something to make the unauthenticated endpoint
>> instead of making it authentication protected!
> 
> I agree with you that this would be a better situation from a security
> standpoint. However, there are many important details that make this
> harder than one might think, most of which you already mentioned.
> 
>> - You can enable it by adding
>> 'forceauth.ForceAuthenticationMiddleware' middleware.
> 
> I would avoid the "auth" wording as it is easy to think that this is
> about authorization. The corresponding mixin in django is called
> `LoginRequiredMixin`, so I think it would be a good idea to call this
> one `forcelogin.ForceLoginMiddleware`.

I agree with that naming :). Actually I implemented this middleware just to 
make sure the feature is working without a problem. All the class and function 
names are chosen without thinking carefully. So of course we can choose better 
names when we start actual development of that feature.

> 
>> - If you have a FBV, and it requires to be accessible for
>> unauthenticated users, call *@publicly_accessible_endpoint*
>> decorator. - If you have CBV, and it requires to be accessible for
>> unauthenticated users, inherit *PubliclyAccessibleEndpointMixin*
>> along side other classes that you need like TemplateView, ListView
>> etc.
> 
> I think it is nice that this mirrors the current situation, but the
> implementation feels brittle. Wouldn't it be much easier to add a list
> of ignored paths to settings?

As far as I know, generic cycle is writing a view, and defining a path in url 
list or vice versa. Adding 3rd step like adding url in settings.py can be kind 
a confusing somewhere else rather than urls.py. On the other hand, if the 
project have lots of unauthenticated endpoint it might be hard to maintain the 
list.

> 
>> I'm not talking about authorization
> 
> This is a big one for me. In the projects that I have worked on, there
> was rarely a view that required login but no permissions. So adding the
> middleware could create a false sense of security. Sure, it improves the
> situation quite a bit by requiring authentication, but it hides the
> underlying issue.

I was thinking that `ForceLoginMiddleware` will not be added to the middlewares 
list by default, at least for a short/mid term. If the project rarely requires 
a login for view, there will be a no issue.

> 
> 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.)

Idea looks great. But since I am not as much experienced Django developers as 
people on this mailing list, I cannot say much.

On the other hand, most of the today’s web applications doesn’t suffer from SQL 
Injection anymore because the way people are learning software development is 
mostly based on official documentation. So having that feature in Django Core 
and mentioned about problem and how can people use that middleware can help to 
people know security risk even though they don’t use it at all.

> 
> tobias
> 
> --
> 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/defe8a05-ad60-bc66-03c8-238401e38605%40posteo.de.

-- 
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/E8BE209A-2F86-43D6-94EC-AD11FEEAE4BF%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]

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.

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

[Tobias Bengfort]

On 14/03/2020 09.43, Tobias Bengfort wrote:
> 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.)

https://github.com/xi/django-utils/blob/feature-auth-check-views/utils/auth_check_views.py

The major issue with this approach is that there are many ways to check
`user.is_authenticated`, so it is hard to validate that in static analysis.

tobias

-- 
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/8bbc0821-75a7-76a7-a3da-911b5e451d2b%40posteo.de.

Re: New Merger nomination.

[James Bennett]

On Sat, Mar 14, 2020 at 5:00 AM Markus Holtermann
 wrote:
> Claude has been contributing to Django for almost a decade. His roles in i18n 
> related matters has been a constant help to the project. Providing Claude 
> with commit access to github.com/django/django and giving him the MERGER role 
> will allow him to continue his work and maintain an up to date translation 
> base in the project, especially in the days leading towards a release.

Again, I need to point out this is not how DEP 10 works.

The Merger role is not just a new name for "committer". If Claude is
appointed a Merger, he will be forbidden to do what you are saying --
a Merger cannot push their own contributions directly into Django!

DEP 10 says:

"[A] Merger MUST NOT merge a Minor Change primarily authored by that
Merger, unless the pull request has been approved by another Merger,
by a Technical Board member, or by the Django Security Team."

The reason for the Merger role is that we need people with the ability
to merge pull requests when the decision-making process says it is
time to merge them. Mergers have some power to use judgment on what
PRs get merged, but it is not the power to judge what is or isn't a
good technical change to make to Django -- it is the power to judge
when something does or does not need more discussion on the public
forums.

Mergers are not technical leaders. Their job is to serve the community
by keeping a specific part of the project -- merging pull requests
according to community decision process -- flowing properly. The role
of Merger is also not a reward or a way to recognize someone's past
accomplishments or contributions. Honoring and recognizing and
rewarding people's work is the DSF's job. We do not use "let's give
him commit access" for that anymore.

This is not to diminish the importance of the role, because it is
important. But it is important in a very different way than the old
"committer" role was, and has very different responsibilities and very
limited powers. Since the vote has been asked for, I urge the
Technical Board members to read DEP 10 carefully and be sure you
understand what a Merger does (and what a Merger doesn't do!) as you
consider your votes. Django probably should have only a very few
Mergers -- DEP 10 only requires that there be three of them -- because
the role of Merger is only to do very specific things to keep the
project working smoothly.

If the worry is about ensuring translations are brought in regularly
and smoothly from Transifex, the Technical Board has authority for
that that and can work with the translation team to set the process
(there is a whole section in DEP 10 about interaction between
different teams within the Django project, for this reason). Which
might end up being "Claude initiates the pulls from Transifex" (though
a process that depends on one specific person is a bad process), but
that is not a justification to make someone a Merger. If the worry is
about i18n code in Django itself, Claude already has the same power
anyone else has to open and review and comment on pull requests, start
discussions on the mailing list or forum about the best approaches,
and so on. But even if he is made a Merger, he would not have the
power to write code and commit it directly into Django, because that
is not a power of a Merger.

-- 
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/CAL13Cg92aEfTmcMbeM6HrZGCkXqdbbJf0pSuWE8%3D7aPQd-6F1A%40mail.gmail.com.

Re: New Merger nomination.

[Markus Holtermann]

Thanks James for summarizing the process. Thanks Mariusz for the suggestion.

Let's make it official, then. I'd like to nominate Claude Paroz 
(https://github.com/claudep) to be a Merger for the Django project and ask my 
fellow Technical Board members to cast their votes.

Claude has been contributing to Django for almost a decade. His roles in i18n 
related matters has been a constant help to the project. Providing Claude with 
commit access to github.com/django/django and giving him the MERGER role will 
allow him to continue his work and maintain an up to date translation base in 
the project, especially in the days leading towards a release.

I vote +1

/Markus 

On Sat, Mar 14, 2020, at 10:05 AM, Claude Paroz wrote:
> Hey! Thanks for suggesting me as a merger!
> 
> However, I'd like to clarify that I'm not requesting this commit bit. 
> If the project thinks it's good that I get it, I'll accept that and do 
> my best to use it as the new DEP suggests. If not, I can certainly 
> continue to contribute as I've done in the past.
> 
> Cheers,
> 
> Claude
> 
>  -- 
>  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/a3ae8f2e-b6a8-4d93-8c62-954dc1646b3d%40googlegroups.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/7f64628f-ac87-4992-9663-d651848a18a7%40www.fastmail.com.

Re: New Merger nomination.

[Claude Paroz]

Hey! Thanks for suggesting me as a merger!

However, I'd like to clarify that I'm not requesting this commit bit. If 
the project thinks it's good that I get it, I'll accept that and do my best 
to use it as the new DEP suggests. If not, I can certainly continue to 
contribute as I've done in the past.

Cheers,

Claude

-- 
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/a3ae8f2e-b6a8-4d93-8c62-954dc1646b3d%40googlegroups.com.

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

[Tobias Bengfort]

Hi Mehmet,

On 13/03/2020 21.47, Mehmet Ince wrote:
> - We must forcefully enable session validation for every endpoint.
> - Developers must do something to make the unauthenticated endpoint 
> instead of making it authentication protected!

I agree with you that this would be a better situation from a security
standpoint. However, there are many important details that make this
harder than one might think, most of which you already mentioned.

> - You can enable it by adding 
> 'forceauth.ForceAuthenticationMiddleware' middleware.

I would avoid the "auth" wording as it is easy to think that this is
about authorization. The corresponding mixin in django is called
`LoginRequiredMixin`, so I think it would be a good idea to call this
one `forcelogin.ForceLoginMiddleware`.

> - If you have a FBV, and it requires to be accessible for 
> unauthenticated users, call *@publicly_accessible_endpoint* 
> decorator. - If you have CBV, and it requires to be accessible for 
> unauthenticated users, inherit *PubliclyAccessibleEndpointMixin* 
> along side other classes that you need like TemplateView, ListView 
> etc.

I think it is nice that this mirrors the current situation, but the
implementation feels brittle. Wouldn't it be much easier to add a list
of ignored paths to settings?

> I'm not talking about authorization

This is a big one for me. In the projects that I have worked on, there
was rarely a view that required login but no permissions. So adding the
middleware could create a false sense of security. Sure, it improves the
situation quite a bit by requiring authentication, but it hides the
underlying issue.

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.)

tobias

-- 
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/defe8a05-ad60-bc66-03c8-238401e38605%40posteo.de.

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

[Mehmet Ince]

Hi,

Actually, middlewares can access to the mapped view function/class with 
process_view() method. Within the function we need to decide that view is 
function or class. Easiest way to do it check existence of view_class attribute 
of view_func variable. While __global__ exist on every object, 
Class-based-views only have a view_class.

You can see how I managed to implemented this as a middleware at following link.

https://github.com/mmetince/django-forceauth/blob/master/forceauth/__init__.py

Cheers,
M.

> On 14 Mar 2020, at 05:21, Abhijeet Viswa  wrote:
> 
> 
> Hello,
> 
> If I'm not mistaken, middlewares are not aware of decorators, mixins applied 
> on the request handlers. Therefore, if the middleware is turned on, there 
> wouldn't be a way to selectively not enforce it. At least not with 
> decorators/mixins.
> 
> The rest framework uses a global setting that applies default Authentication 
> and Permissions classes on all views. something like that could be possible 
> in core Django.
> 
> On Sat, 14 Mar, 2020, 02:18 Mehmet Ince,  > wrote:
> Hi everyone,
> 
> I've been working as a security researcher for a long time. Common mistake 
> that I've seen is forgotten decorator and/or Mixin usage on controllers, 
> which leads to OWASP A5 Broken_Access_Control[1]. I believe one of the most 
> important, as well as most used, decorator and/or Mixing in Django world is 
> @login_required.
> 
> Instead of calling that decorator on every single function-based-view is kind 
> a against the DRY. Also even sometimes it's possible to forgot to call 
> decorator, which is scary. For class-based-view it's usually requires to 
> define an abstract controller class, where you call login_required decorator 
> with method_decorator on dispatch method[2]. Of course that approach makes 
> sense and looks like a proper design decision but it still leaves some open 
> doors. For instance, what would happen if new contributor/developer in the 
> team inherits TemplateView directly instead of ProtectedView class ? etc etc 
> etc.
> 
> Since almost %90 of the endpoint of todays web application requires an 
> authentication -I'm not talking about authorization-, I believe we should be 
> writing code in order to make endpoint accessible by unauthenticated request.
> 
> So my proposal is very simple:
> 
> - We must forcefully enable session validation for every endpoint.
> - Developers must do something to make the unauthenticated endpoint instead 
> of making it authentication protected !
> - We should do this in middlewares. Because for my opinion, such as 
> validation and check should be done way before execution reaches to the views.
> 
> Technical implementation can be as follow:
> 
> - You can enable it by adding 'forceauth.ForceAuthenticationMiddleware' 
> middleware. Otherwise Django can work as it.
> - If you have a FBV, and it requires to be accessible for unauthenticated 
> users, call @publicly_accessible_endpoint decorator.
> - If you have CBV,  and it requires to be accessible for unauthenticated 
> users, inherit PubliclyAccessibleEndpointMixin along side other classes that 
> you need like TemplateView,ListView etc.
> - All the other endpoints will be protected by authentication validation on 
> middleware.
> 
> You can see my initial implementation at 
> https://github.com/mmetince/django-forceauth 
>  and read my all thoughts 
> regarding to the approach to the session validation at blog post [3].
> 
> [1] 
> https://owasp.org/www-project-top-ten/OWASP_Top_Ten_2017/Top_10-2017_A5-Broken_Access_Control
>  
> 
> [2] 
> https://docs.djangoproject.com/en/3.0/topics/class-based-views/intro/#decorating-the-class
>  
> 
> [3] 
> https://pentest.blog/why-secure-design-matters-secure-approach-to-session-validation-on-modern-frameworks-django-solution/
>  
> 
> 
> PS: This is my first mail to Django mailing list, if it’s wrong place to 
> discuss such a ideas please let know where to do it properly :)
> 
> --
> 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/F6FB68F9-9287-45BD-B3AD-F59C2B4E23F0%40mehmetince.net
>  
> .