[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 :)

[Abhijeet Viswa]

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.

--
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 view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/F6FB68F9-9287-45BD-B3AD-F59C2B4E23F0%40mehmetince.net.

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

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAP1-YrprcKUmLYd3CJUVWJD22jOLHWR1rO3pVZGe601Bg9%2Bd8g%40mail.gmail.com.

[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

[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

[Mehmet Ince]

Hi Tobias,

Thanks for your comments

> On 14 Mar 2020, at 11:43, Tobias Bengfort <tobias....@posteo.de> 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-develop...@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.

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

[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

--

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 view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/4c13eb8d-eb6a-4973-be4d-5abe0fc55bb9%40googlegroups.com.

--

Adam

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

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAMyDDM3Ui3shxaspquwhbvT0%2BrbhJXfkC4Sd8bw-zcS_2u0Q5A%40mail.gmail.com.

[Alasdair Nicol]

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.

Cheers,

Alasdair

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.

--

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-d...@googlegroups.com.

[Mehmet Ince]

Hi Alasdair,

On 16 Mar 2020, at 12:59, Alasdair Nicol <alas...@thenicols.net> 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.

To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/f3916d6d-77c0-4e40-b201-aaabfe2abd58%40googlegroups.com.

[Matt Magin]

Hi all,

I'm surprised nobody has mentioned django-stronghold in this discussion which does exactly what you're looking for.

It's implemented as a middleware, with two settings for exclusions -- A list of named url's or a list of regex's to match url's against.

This works well, and managing the exclusions through settings is not a big deal. Having said that, I do like the idea of putting it inside the Auth middleware as well.

+1 from me to put this into core functionality, I include django-stronghold in basically all my projects already.

Cheers,

Matt

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/2828053D-2340-4C85-B354-47D0096A5ACF%40mehmetince.net.

[Fabio Caritas Barrionuevo da Luz]

> I'm surprised nobody has mentioned django-stronghold in this discussion which does exactly what you're looking for. 

Same here. django-stronghold has been working well in my projects for me for years. 

https://github.com/mgrouchy/django-stronghold 

Hi Alasdair,

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/f3916d6d-77c0-4e40-b201-aaabfe2abd58%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-d...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/2828053D-2340-4C85-B354-47D0096A5ACF%40mehmetince.net.

[Hanne Moa]

The oldest code at https://djangosnippets.org that fixes this is from
2008 so I guess something like this have been considered trivial to
build yourself ever since then, and hence why it's never been added.
Here are some alternatives:

https://djangosnippets.org/search/?q=login+required

I've been using some variation on LoginRequiredMiddleware since 2008
myself, see:

https://gist.github.com/hmpf/0d8faf83e7c7d868126d7c60c8dd0b6a

Some way to open up urls by string is generally necessary for 3rd
party apps otherwise I wouldn't bother to have the PUBLIC_URLS
setting. API's often use a separate authentication mechanism, so best
exempt those too. Any officially supplied login views should mark
themselves as exempt so they don't pollute PUBLIC_URLS though.

Most of my Django sites have only cared about whether you are logged
in or not, then have an "owner" foreign key on models that can be
CRUDed by users. I much prefer roles to permissions since I used to do
a lot with Zope 2.

> To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/651f48c0-dcab-48aa-812a-24ca2c0f7a9f%40googlegroups.com.

[Tobias Bengfort]

Hi Mehmet,

On 19/03/2020 09.49, Mehmet Ince wrote:
> 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 ? 

Remember that many people on this mailing list (me included) are people
just like you, so we can only give you feedback and maybe some
additional ideas.

The best course of action is probably to write a mail in which you sum
up the discussion so far, highlight some controversial points, and
describe why you decided the way you did in your implementation. At
least that is what I would do.

The critical part in my experience is to convince people that this
should be included in django itself rather than a third party library. I
usually fail with that so I cannot say anything useful about that.

tobias

[Adam Johnson]

I personally am in favour of including this in Django at this point.

It seems like a desirable feature, it has circulated the community since 2008 (!), and there are at least two community packages implementing the pattern. No one on this thread is explicitly against it ever happening.

I'm in favour of the approach of a second middleware rather than a setting for AuthenticationMiddleware. We do try to avoid settings and I think this is a more sensible approach.

It would be relatively little code and documentation overhead - a middleware (maybe LoginRequiredAuthenticationMiddleware ?), a decorator (maybe @login_not_required ? - the opposite of @login_required), and a view mixin (LoginNotRequired ?).

--
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 view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/469846ef-6d3a-c0c5-73d0-21b694185e81%40posteo.de.

--

Adam

[Ahmad A. Hussein]

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.

Regards,

Ahmad

[Mehmet Ince]

Hi Ahmad,

On 23 Mar 2020, at 13:23, Ahmad A. Hussein <ahmadah...@gmail.com> 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-develop...@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.