Code

Opened 9 months ago

Closed 4 months ago

Last modified 2 months ago

#21293 closed Bug (fixed)

Some authentication-related templates are broken

Reported by: EvilDMP Owned by: nobody
Component: contrib.auth Version: master
Severity: Release blocker Keywords:
Cc: bmispelon@…, timo Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: yes

Description

Commit a962286b74f1e8c8cb19fb45a057800da8c2fb56 https://github.com/django/django/commit/a962286b74f1e8c8cb19fb45a057800da8c2fb56 breaks the default template for /user/password/reset/, and possibly others.

See https://www.dropbox.com/s/zqng1mftd1rgcqb/Screenshot%202013-10-19%2009.19.51.png - the "Welcome, daniele. Change password / Log out" text in <div id="user-tools"> is not visible, because it has a white background.

Attachments (2)

ticket-21293.diff (750 bytes) - added by bmispelon 9 months ago.
21293-css.diff (645 bytes) - added by timo 4 months ago.

Download all attachments as: .zip

Change History (16)

Changed 9 months ago by bmispelon

comment:1 Changed 9 months ago by bmispelon

  • Cc bmispelon@… added
  • Triage Stage changed from Unreviewed to Accepted

The problem comes from the fact that password_reset does not pass a site_header variable to its context. Consequently, the header ends up being empty and shrinks.
Therefore, the text that should have been printed on the header's dark background gets printed onto the page's white background, which makes it invisible.

I attached a tentative solution but I'm not sure if it's ideal. Tweaking the CSS might be a better idea.

I couldn't find whether this had been backported to 1.6 (the comments on github by adrian indicate that was the original intent but I can't find a backport commit on stable/1.6.x).
If it has indeed been backported, I think it should be marked as a release blocker since it changes the old behavior.

comment:2 Changed 9 months ago by EvilDMP

The removal of {% trans 'Django administration' %} in admin/base_site.html is what breaks (for example) password_reset_form.html, since https://github.com/django/django/blob/master/django/contrib/auth/views.py#L133 doesn't pass in a site_header. I am not sure how to resolve this, since probably we should not be showing "Django administration" text on non-admin authentication pages.

Options:

  • change the views in auth.views
  • change the HTML/CSS in base_site.html so that the user_tools div is not at the mercy of the background on the h1.

comment:3 Changed 9 months ago by timo

I can confirm this wasn't backported to 1.6.

comment:4 Changed 8 months ago by Claude Paroz <claude@…>

In 9c6b57f709bb6123d8abae6ba05e92085ec2f426:

Merge pull request #1877 from Bouke/patches/4

Fixed missing admindocs' site_header, refs #21293

comment:5 Changed 6 months ago by claudep

  • Severity changed from Normal to Release blocker

Blocker for 1.7

Changed 4 months ago by timo

comment:6 Changed 4 months ago by timo

  • Cc timo added
  • Has patch set

Added a proposed fix for the CSS: removed the absolute positioning of the usertools div and used float positioning to allow the #header div to expand based on the height on its content.

comment:7 Changed 4 months ago by andrewgodwin

Timo: Is there anything that means we shouldn't commit this? The fix looks valid to me.

comment:8 follow-up: Changed 4 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 6f470650d097abbc06021efd50b2bdc3c9ce4d3b:

Fixed #21293 -- Adjusted admin header CSS to fix admin password reset template.

By removing the absolute positioning of the usertools div and using
float positioning, the #header div will expand based on the height of
its content.

Thanks EvilDMP for the report.

comment:9 in reply to: ↑ 8 Changed 4 months ago by ramiro

  • Resolution fixed deleted
  • Status changed from closed to new

Replying to EvilDMP:

Commit a962286b74f1e8c8cb19fb45a057800da8c2fb56 https://github.com/django/django/commit/a962286b74f1e8c8cb19fb45a057800da8c2fb56 breaks the default template for /user/password/reset/,

Are you sure this happened actually for the password reset view (the one in which the user enters his email address so a token is sent there)?

AFAIK there is no access to that particular view from the admin app and the only two password related pages there are:

a) The one in which a staff user can set his new password by entering the old one (i.e. /admin/password_change/) and
b) The one in which a superuser (or a staff user with the right permissions) can force a new password on any user (i.e. /admin/auth/user/<pk>/password/).

Yes, the naming is a bit confusing :)

I observed the problem reported by this ticket only in case b.

and possibly others.

Was this actually a problem for any other view?

Replying to EvilDMP:

I am not sure how to resolve this, since probably we should not be showing "Django administration" text on non-admin authentication pages.

In case b there is no danger of this because it's fully handled in the admin realm in django/contrib/auth/admin.py:userAdmin.user_change_password()

Replying to Tim Graham <timograham@…>:

Fixed #21293 -- Adjusted admin header CSS to fix admin password reset template.

By removing the absolute positioning of the usertools div and using
float positioning, the #header div will expand based on the height of
its content.

With this change the height of the header is fixed but, when compared to 1.6, the "Django administration" title text usually located on the left is still missing.

For that, I've opened https://github.com/django/django/pull/2418 that updates the context passed to the template in the same spirit of 9c6b57f709bb6123d8abae6ba05e92085ec2f426.

Last edited 4 months ago by ramiro (previous) (diff)

comment:10 Changed 4 months ago by Ramiro Morales <cramm0@…>

In 1d42a86ec7e16336955735474f88e7632a34f3cb:

Tweak password admin change form view context. Refs #21293.

comment:11 Changed 4 months ago by Ramiro Morales <cramm0@…>

In 6d3ebe10f4f8c4b91c9cbeccf9696ca79b6909f9:

Merge pull request #2418 from ramiro/21293-tweak

Tweak password admin change form view context. Refs #21293.

comment:12 Changed 4 months ago by ramiro

  • Resolution set to fixed
  • Status changed from new to closed

I've merged the fix for the missing "Django administration" title in the force-new-password-by-staff view. Will analyze the password reset view (https://docs.djangoproject.com/en/dev/ref/contrib/admin/#adding-a-password-reset-feature) issue after beta.

comment:13 Changed 2 months ago by Ramiro Morales <cramm0@…>

In 5ea34f3f86af307e1a1c02b49136f51a4f63da88:

Restored site header text in password reset view.

It's the one shown when the optional integration described in
https://docs.djangoproject.com/en/dev/ref/contrib/admin/#adding-a-password-reset-feature
is used.

Follow-up to commits 6f470650d0 and 1d42a86ec7, together they fix
different small UI regressions after a962286b74.

Refs #21293.

comment:14 Changed 2 months ago by Ramiro Morales <cramm0@…>

In bc82c0dbac33ced1a4d6953e8626236ae05af956:

[1.7.x] Restored site header text in password reset view.

It's the one shown when the optional integration described in
https://docs.djangoproject.com/en/dev/ref/contrib/admin/#adding-a-password-reset-feature
is used.

Follow-up to commits 6f470650d0 and 1d42a86ec7, together they fix
different small UI regressions after a962286b74.

Refs #21293.

5ea34f3f86 from master.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.