Django-debug-toolbar: SHOW_TOOLBAR_CALLBACK no longer supports lambda

[ross]

in my setup i always have seperate dev settings and in those settings i enable debug toolbar for all requests. i've always done that with the following config

DEBUG_TOOLBAR_CONFIG = {
    'SHOW_TOOLBAR_CALLBACK': lambda request: True,
}

after updating my virtualenv this morning i started getting errors that i've tracked down to trying to run rsplit on func_path

line 45 of debu_toolbar/middleware.py:

    mod_path, func_name = func_path.rsplit('.', 1)

it looks like the meaning of SHOW_TOOLBAR_CALLBACK may have changed to require a string and not support a callable. if that's the case i'm not sure what the solution would be. i'd rather not have to define a function somewhere and point this at it just to return true. i'd prefer if SHOW_TOOLBAR_CALLBACK supported being presented a callable.

[aaugustin]

This change was made on purpose to prevent people from copy-pasting the insecure example that was in the old documentation.

Is there a good reason for using this definition? If you simply copy-pasted from the docs without bothering to read the surrounding text, remove it and move on.

[ross]

as i explained in the original description, i'm not just copy-n-pasting.

there isn't a security issue as debug_toolbar isn't even installed on the public/production boxes, much less configured this way. it's in a dev only config file/setup. it's used by developers to run with their local boxes where it's safe/desirable to allow blanket access.

[aaugustin]

All Django settings have primitive values, to avoid the temptation to import stuff in settings. The Debug Toolbar follows this philosophy. I'm sorry that it makes your use case marginally less practical, but I still think it's a good trade-off for the community at large.

I would suggest the following (untested) code:

DEBUG_TOOLBAR_CONFIG = {
    'SHOW_TOOLBAR_CALLBACK': "%s.true" % __name__,
}

def true(request):
    return True

[ross]

that's effectively the work-around i've put in place and it's not that big a deal.

as a general it's a good idea to give a better error message for something that previously worked fine and you're specifically going to break. the following results in users having to open up the source and figure out what went wrong.

AttributeError: 'function' object has no attribute 'rstrip'

something like the following would be preferable.

...
if callable(func_path):
    ImproperlyConfigured('invalid SHOW_TOOLBAR_CALLBACK, a dotted path to a function is required')
...

[aaugustin]

Fair enough!

[ross]

thanks. liking the updated/cleaned up stuff

[tomkins]

Given that the exact same thing can be done as demonstrated above, was there a need to remove the old behaviour? Just feels like it results in a slightly more ugly settings file for my local/dev instances.

My opinion for the best option would be to default to 'debug_toolbar.middleware.show_toolbar' by default as the recommended option in the documentation, with iscallable being used for those of us who actually want it to be a function/lambda.

It's a debug toolbar, I'd hope that anyone installing it would recognise the fact that it can show a lot of sensitive information and that we don't need protecting in this way.

[aaugustin]

You cannot make this assumption. You'd be surprised at how many people run sites in production with DEBUG = True. (More than half of them last time I had the opportunity to check a significant sample.)

:(

[aaugustin]

In fact, I had already included such a warning: https://github.com/django-debug-toolbar/django-debug-toolbar/blob/master/debug_toolbar/settings.py#L66-L69. You should have seen it.

I'm hesitating a bit; I could be convinced to restore support for callables, especially if one of the other maintainers wanted it.

[zsoldosp]

just 2c - the warnings don't show up on the web interface, just in the logs. While it's quick to google (http://stackoverflow.com/a/20727979), but it would be nice to have a the same error message on the UI as it is in the warnings.

[foxx]

Although the __name__ snippet from @aaugustin works, this really was quite annoying and wasted quite a bit of time. That being said, I do agree that it's better to avoid putting callables into settings.

Therefore, I'd be +1 for a docs update using the __name__ snippet, but -1 on adding support for callables.

[aaugustin]

@tim-schilling Do you have an opinion on this?

At this point, I'm vaguely +0 on restoring support for callables, in the name of practicality.

[tim-schilling]

I think the toolbar should follow Django's practice of keeping the settings' values as primitives.

However, I don't think a DeprecationWarning is correct. We should raise a ImproperlyConfigured since the old values will no longer work with the new version. While this doesn't allow people to use callables, it would help them resolve the issue faster.

[ross]

as previously mentioned if the regression is going to happen (which at this point it has and everyone has moved on) a better error message is the right path. snippet for that is above, https://github.com/django-debug-toolbar/django-debug-toolbar/issues/523#issuecomment-31880503

[dcramer]

+1 on restoring callables

The "Django way" is not always the right way, and this turns a simple problem into a complex problem in many situations.

[aaugustin]

@dcramer There's also a better reason which I had forgotten about: bug reports showed that many people copy-pasted the insecure example that was in the old documentation without realizing the security consequences.

I wanted to reduce the risk of receiving one day a bug report along the lines of "I was hacked because I copy-pasted the example settings from the DjDT's docs".

[dcramer]

I dont think developers being ignorant is a good reason to remove simplicity :)

[ross]

personally at this point i don't care what happens, either restore the previous behavior or put a better error message in place. either is fine, but please just do one of them and close this ticket.

[Simanas]

Spend more than an hour on this until I finally got it working on my dev server. This is so frustrating! It is impossible to save idiots from making stupid mistakes. They will still find many workaround and do stupid security shit. Default security measures just makes this wonderful tool so complicated and frustrating to work with. I just want to plug and play with it. Will take over security stuff my self as I am a developer and that's my job.

All of this is completely out of the scope of The Zen of Python: https://www.python.org/dev/peps/pep-0020/

There should be no additional options at all for displaying toolbar or not. If you have it in your installed apps it's displayed if not, it's not displayed. That's it. Leave everything else for developers to decide.

Anyway, thanks for running this amazing tool! I am a huge fan of it, except for these stupid security measures.

[chris-martin]

:+1: for simplicity. A dict with a string that resolves to a function that evaluates to a boolean that determines whether to expose potentially sensitive information doesn't seem to respect "economy of mechanism" as a security principle.