Skip to content
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

Undefined template variables make page look different than default Django login page #62

Closed
Sveder opened this issue Dec 3, 2019 · 12 comments · May be fixed by #76
Closed

Undefined template variables make page look different than default Django login page #62

Sveder opened this issue Dec 3, 2019 · 12 comments · May be fixed by #76

Comments

@Sveder
Copy link

Sveder commented Dec 3, 2019

We've recently started logging missing template variables (similar to: https://docs.djangoproject.com/en/3.0/ref/templates/api/#how-invalid-variables-are-handled). In this process we noticed that the honeypot login page is complaining about three undefined variables:

  1. site_title
  2. site_header
  3. username

Looking at the page visually, it also looked different than the actual Django login page - the title was missing from the honeypot page. After looking at django-admin-honeypot code and tinkering, I think the discrepancy is in views.py - specifically, if I add site_title to the context returned by get_context_data, it solves the issue.

My guess is that Django templates became more and more customizable, the context wasn't updated and so now there is this discrepancy. I wanted to make sure that my understanding of this is correct before submitting a PR.
@dmpayton what do you think? Does it make sense?

(and of course, thanks for the great library!)

@Sveder
Copy link
Author

Sveder commented Jan 12, 2020

@dmpayton ping?

@dmpayton
Copy link
Owner

@Sveder Pong. :)

Thanks for the report. I'm in the middle of updating this app for modern Django, and I'll take a look at this soon.

@kezabelle
Copy link

There's a couple of ways you might do it:

"site_title": admin.site.site_title,
"site_header": admin.site.site_header,
"username": self.request.user.get_username(),

or use admin.site.each_context(...) to fill in the necessary data. Or hard-code the values as:

"site_title": _('Django site admin')
"site_header": _('Django administration')
...

Hard-coding the values is the safest because it'd avoid using/leaking any customisations to the current adminsite instance, but would leave you at the mercy of discrepancies between Django versions where those might change. In which case, you might want to instead use "site_title": AdminSite().site_title so that it's always the "default" for the current Django version being used...

(I noticed this independently after you reminded me this app exists, in combination with a package of my own for errors on missing variables)

@dmpayton
Copy link
Owner

In which case, you might want to instead use "site_title": AdminSite().site_title so that it's always the "default" for the current Django version being used...

This sounds like a reasonable way to go.

@9mido
Copy link

9mido commented Jun 16, 2020

Any ideas when this will get done? Are we all waiting for a PR or what? It's been months since there has been any discussion on this. Seems like it'll never get done at this rate.

@SilverStrings024
Copy link

Does this still need fixed? I might try my hand at it and the fix works, submit a PR.

@dmpayton
Copy link
Owner

dmpayton commented Aug 7, 2020

Hey all, I've been under a work deadline and haven't had a chance to revisit this, and likely won't for another week or so. I'd happily accept a PR that fixes this per #62 (comment). Any takers?

@GitRon
Copy link

GitRon commented Sep 1, 2020

@dmpayton @9mido I created a PR with the idea of @kezabelle.

@9mido
Copy link

9mido commented Sep 1, 2020

@GitRon is a hero! Thank you!

@GitRon
Copy link

GitRon commented Sep 2, 2020

@9mido would you leave me a note when the new version is at pypi?

blag added a commit to blag/django-admin-honeypot that referenced this issue Nov 26, 2021
@blag
Copy link

blag commented Nov 26, 2021

Hi, I have forked this project, included a few PRs (including @GitRon's fix for this), and released a package to PyPI. You might have better luck with my fork.

@dmpayton
Copy link
Owner

A fix for this has been pushed to develop – it uses AdminSite.each_context() to populate the default context variables. Thanks for your patience!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants