#20495: add login failure events to django.security logger
--------------------------------------+------------------------------------
Reporter: ptone | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Changes (by ptone):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
* needs_docs: 0 => 1
Comment:
Thanks for the patch!
This patch overall looks good, and needs only a few minor improvements.
'''deprecation:'''
We have a procedure we follow anytime we remove a feature, it is
documented in a limited way here under "minor releases"
https://docs.djangoproject.com/en/dev/internals/release-process/
We don't have any real contributor documentation about how to actually
write the deprecation - but searching for the "DeprecationWarning" in the
code will lead you to some examples of the different levels.
As to whether we should deprecate the signal - that can be a harder call.
My instinct is to remove it, as I was never really happy with the cleaning
of potentially sensitive info we do given that any code can subscribe to a
signal, and do potentially dumb things that might expose something by
accident:
https://github.com/django/django/commit/7cc4068c4470876c526830778cbdac2fdfd6dc26#L0R38
The reasons in favor of keeping the signal are that there could be times
you want to take some action, not just monitor (can't think of any ones
that are a good idea though) and the signal captures the cleaned
credentials providing more info (and we probably don't want to wholesale
log those either).
The compromise is to do keep both (signal and log), but strongly suggest
using the logging unless you have some strong need to take action based on
the credentials.
I know you are just starting out, but what are your thoughts?
If we deprecate the logging signal, the warning would have to come not
when the signal is issued, but when something connects to the signal - not
sure how messy that gets to do - off the top of my head probably a custom
signal subclass that wraps the 'connect' method.
'''logging levels:'''
logging levels across different python projects are an ambiguous mess! My
esteemed colleague Mr. Gaynor has argued that there are only two levels -
oops and OH NO.
In Django we sort of do this in that we have ERROR level set up to email
admins, and is more of OH NO, while a failed login should probably just be
a 'warning' level, so that would be a minor change.
'''Docs'''
You've added a feature!, we need to make sure people know about it. This
kind of change would warrant a small mention in the release notes, and
also some brief mention of its availability in the logging docs, or
perhaps alternatively/additionally in the Auth docs. I can take a look if
you need more pointers. If deprecation of the signal happens, that is an
additional entry that goes into the deprecation timeline.
Bravo on including a test.
'''Trac housekeeping:'''
We have a bunch of meta-data around the tickets here, and so one thing you
can do after submitting a patch is mark the ticket as "has patch"
https://docs.djangoproject.com/en/dev/internals/contributing/triaging-
tickets/#has-patch
This helps direct other people who are willing to contribute by reviewing
patches.
Thanks again for contributing!
--
Ticket URL: <https://code.djangoproject.com/ticket/20495#comment:2>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/063.97bd3de4a8dc964f272b8b3315e81cab%40djangoproject.com.
For more options, visit https://groups.google.com/groups/opt_out.
