-
Notifications
You must be signed in to change notification settings - Fork 188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed all the errors for django==4.0 version #88
base: develop
Are you sure you want to change the base?
Fixed all the errors for django==4.0 version #88
Conversation
Working |
@dmpayton Please, merge this PR to fix the mentioned issues. |
@dmpayton Can we merge this branch? Thanks! |
def __str__(self): | ||
return self.username | ||
from django.db import models | ||
from django.utils.translation import gettext_lazy as _ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this import is ok, but just work for Django 4, if anyone is using Django 3, this is version is not working with it, I think is better import djanngo, and do an if like:
import django
if django.VERSION < (4,):
from django.utils.translation import ugettext_lazy as _
else:
from django.utils.translation import gettext_lazy as _
Can please have this merged? Thanks |
Can you please merge this PR? Thank you! |
@dmpayton Any chance this could finally get merged? Thanks |
Can please have this merged? Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this was forked from an earlier commit and then rebased on a more recent commit from dmpayton's repository, but contains some rebase errors.
As it stands, I think it needs some cleanup and discussion before it would be ready for merge.
@@ -18,16 +17,19 @@ def get_actions(self, request): | |||
return actions | |||
|
|||
def get_session_key(self, instance): | |||
return format_html('<a href="?session_key={sk}">{sk}</a>', sk=instance.session_key) | |||
return '<a href="?session_key=%(key)s">%(key)s</a>' % {'key': instance.session_key} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't like these changes.
Reading up on the rationale for using format_html
, it would seem to prevent XSS attacks, and is easier to use than %
string interpolation.
For the case of building up small HTML fragments, this function is to be preferred over string interpolation using % or str.format() directly, because it applies escaping to all arguments - just like the template system applies escaping by default.
So, instead of writing:
mark_safe( "%s <b>%s</b> %s" % ( some_html, escape(some_text), escape(some_other_text), ) )You should instead use:
format_html( "{} <b>{}</b> {}", mark_safe(some_html), some_text, some_other_text, )This has the advantage that you don’t need to apply escape() to each argument and risk a bug and an XSS vulnerability if you forget one.
@@ -3,6 +3,26 @@ | |||
from admin_honeypot import listeners | |||
|
|||
|
|||
class LoginAttempt(models.Model): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this model was accidentally copied and pasted here. It's a duplicate of the model defined on 26, and usually people try to keep import
statements above other code, including model and class definitions.
from django.dispatch import Signal | ||
|
||
honeypot = dispatch.Signal() | ||
honeypot = Signal('request') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would try not to adjust imports if you don't absolutely need to. They make reading a pull request more difficult.
The big change here is adding a string argument named 'request' to the signal. But the Signal
class doesn't have an __init__
argument where this would make any sense:
class Signal:
...
def __init__(self, use_caching=False):
...
Can you explain a bit more about what you were trying to fix here?
|
||
app_name = 'admin_honeypot' | ||
|
||
urlpatterns = [ | ||
path('login/', views.AdminHoneypot.as_view(), name='login'), | ||
re_path(r'^login/$', views.AdminHoneypot.as_view(), name='login'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the original path
is likely going to be faster. Avoid re_path
where possible.
|
||
from ipware import get_client_ip | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this removed? It solved a valid problem.
Everybody, support for Django 4.0 is already merged from a different PR. It is even tested in the tox configuration in the We no longer need this PR merged - it can be closed without merging. However, the code that supports Django 4 has not been included in a release on PyPI. You should be able to install directly from the pip install git+https://github.com/dmpayton/django-admin-honeypot.git@develop Alternatively, you may use my fork: django-admin-honeypot-blag @dmpayton It would be nice if you would push a new release to PyPI. They make it really easy to setup publishing with GitHub Actions if you don't want to do it manually all the time. |
No description provided.