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

[Mehmet Ince]

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.


signature.asc
Description: Message signed with OpenPGP

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,  <mailto:meh...@mehmetince.net>> 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 
> <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
>  
> <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
>  
> <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/
>  
> <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 
> <mailto:djang

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: [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: [Feature Request] Having an middleware to be able to force authentication on views by default

[Mehmet Ince]

Hi Alasdair,

> On 16 Mar 2020, at 12:59, Alasdair Nicol  wrote:
> 
> Hi,
> 
> Creating Django settings is often discouraged, so perhaps it would be better 
> to create a subclass of AuthenticationMiddleware instead of adding a setting. 
> Then users would update MIDDLEWARE to enable the new functionality.

I see, that supports my initial idea. I was planning to have new middleware but 
creating a subclass of AuthenticationMiddleware with a process_view function is 
way better. People who want to enabled that feature can directly replace 
AuthenticationMiddleware with our new middleware.

To be honest, I’m kind a confused about how to proceed. Should I continue with 
settings to control it or subclass of Auth middleware ?

I really want to contribute to the Django thus I’m willing to send PR for that 
feature, because it looks like newbie friendly in terms of implementation, but 
I think I need a kind of confirmation from core devs.

> 
> Cheers,
> Alasdair
> 
> On Sunday, 15 March 2020 17:46:48 UTC, Mehmet Ince wrote:
> 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
>>  
>> <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-d...@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
>>  
>> <https://groups.google.com/d/msgid/django-developers/4c13eb8d-eb6a-4973-be4d-5abe0fc55bb9%40googlegroups.com?utm_medium=email&utm_source=footer>.
>> 
>> 
>> --
>> Adam
>> 
>> --
>> You received this message because you are subscribed to the Google Groups 
>> "Django developers (Contributi

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

[Mehmet Ince]

Hi Ahmad,

> On 23 Mar 2020, at 13:23, Ahmad A. Hussein  wrote:
> 
> I completely agree with what has already been said by everyone here; 
> moreover, this is a battery missing from Django in my opinion. It would make 
> Django more "batteries-included" if this was part of core rather than third 
> party-libraries. If you need help with documentation, I can definitely throw 
> a hand.

Thank you very much. I definitely going to need a help with documentation !

Also, thank you very much for everyone who’s involved to the discussion. It's 
great to see that everyone is supporting to have this in django-core.

As far as I can see, we have a common ground about following items.

- Creating a LoginRequiredAuthenticationMiddleware class which extends our 
current AuthenticationMiddleware class.
- Creating @login_not_required decorator and LoginNotRequiredMixin class. They 
won’t do anything, but marking views as a ’not login required for that 
endpoint’ so that our middleware can pass the request.
- We have some default views within auth/views such as, LoginView, 
redirect_to_login and PasswordResetView. They must use our new decorator or 
Mixin.

PS: I like those names by the way. Thanks for func and class name suggestions 
Adam.

One thing that isn’t clear for me is URL exclusions list in settings.py. @Matt 
Magin and @Hanne Moa has mentioned about it. It’s basically a list of url 
and/or regex that exempt the view from login validation.

I truly understand that such a list can be very useful. But I personally don’t 
support adding that functionality. Because, I believe people will use wildcard 
rules, like LOGIN_EXEMPT_URL = ['/api/*'], which will disables the protection 
we are trying to put in the first place.

Maybe even package maintainers will use a wide range of rule definitions in 
their own settings file in order to make their package compatible with 
Django-core release. I think that kind of compatibility issue should be 
addressed by 3rd party package maintainers.

> 
> 
> Regards,
> Ahmad
> 
> --
> 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/ef6ad79b-e37b-46b3-87c3-60d7c97e5395%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/3ECF27A7-9308-4CAF-94DC-1A78DC411F82%40mehmetince.net.


signature.asc
Description: Message signed with OpenPGP