Opened 11 years ago

Closed 10 years ago

Last modified 9 years ago

#21648 closed Cleanup/optimization (fixed)

Deprecate "is_admin_site" option of auth.views.password_reset()

Reported by: brain@… Owned by: nobody
Component: contrib.auth Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Hello,

Here https://docs.djangoproject.com/en/1.6/topics/auth/default/#django.contrib.auth.views.password_reset the option "is_admin_site" is not documented and its name doesn't describe what it is doing.

According to https://github.com/django/django/blob/master/django/contrib/auth/views.py#L160 and https://github.com/django/django/blob/master/django/contrib/auth/forms.py#L248-253 it uses the host of the request instead of the site framework, therefor it should be name something like "use_domain_from_request".

Kind regards,

Change History (11)

comment:1 by Tim Graham, 11 years ago

Easy pickings: unset
Triage Stage: UnreviewedAccepted

Yes, it seems poorly named, but I'm not sure it's worth renaming due to backwards compatibility concerns at this point. The parameter was added in the merge of magic-removal pre-1.0, but the functionality didn't appear to be used or tested until [9305c0e1].

comment:2 by Claude Paroz, 11 years ago

Is the use case for that parameter still accurate? Shouldn't we deprecate it?

comment:3 by Tim Graham, 10 years ago

Component: Documentationcontrib.auth
Has patch: set
Summary: the "is_admin_site" option of password_reset is undocumented and the name ambiguousDeprecate "is_admin_site" option of auth.views.password_reset()
Type: BugCleanup/optimization
Version: 1.6master

Deprecation looks reasonable to me. I couldn't find a use case for it because if contrib.sites isn't installed, contrib.sites.requests.RequestSite (via get_current_site() in PasswordResetForm) uses request.get_host() as well.

PR

comment:4 by Tim Graham <timograham@…>, 10 years ago

Resolution: fixed
Status: newclosed

In e39af5ea592bbe4e7a643e67f0c379adc10ed392:

Fixed #21648 -- Deprecated is_admin_site option to auth.views.password_reset().

comment:5 by Philipp Metzler, 10 years ago

A use case is if you are using just one Django instance (=> only one settings.py => only one settings.SITE_ID) and you want to use django CMS (which relies on django.contrib.sites) to host a website on xyz.domain.tld and you have several subdomains abc.domain.tld, xyz.domain.tld, etc. which do not use django CMS but only provide a (branded) login where users can also click the "forgot password" link and in the resulting email the referring URL (e.g. xyz.domain.tld) should be used.

django CMS:

/cms/models/pagemodel.py

from django.contrib.sites.models import Site
site = models.ForeignKey(Site, help_text=_('The site the page is accessible at.'), verbose_name=_("site"))

http://docs.django-cms.org/en/2.4.0/extending_cms/api_references.html
cms.models.pagemodel.Page
site (django.contrib.sites.models.Site instance) – Site to put this page on

http://docs.django-cms.org/en/latest/getting_started/integrate.html#installing
Be sure to have 'django.contrib.sites' in INSTALLED_APPS and set SITE_ID parameter in your settings: they may be missing from the settings file generated by django-admin depending on your Django version and project template.

Last edited 10 years ago by Philipp Metzler (previous) (diff)

comment:6 by Tim Graham, 10 years ago

Do you have each subdomain setup as a site?

comment:7 by Philipp Metzler, 10 years ago

No I've got:

  • One Django instance with one settings.py which contains SITE_ID = 1
  • Django CMS installed and INSTALLED_APPS containing django.contrib.sites The django CMS pages are only rendered for www.domain.tld
  • Several subdomains pointing to the same Django instance. nginx is redirecting / to /admin on these subdomains as we only allow login and password reset but do not display any django CMS content there: abc.domain.tld, def.domain.tld, ghi.domain.tld, jkl.domain.tld
  • In the templates I use a templatetag which returns different configurations (CSS, etc.) for the subdomains in use based on the host: host = context['request'].get_host()

I could remove the single sites object in the database and then password_reset would return RequestSite(request).domain instead of Site.objects.get_current().domain but the sites object in the database is used at other places in the code. So this is not an option. I will get rid of django CMS in this Django instance in the near future so I will be able to remove django.contrib.sites from INSTALLED_APPS - but that will just be a solution for me at that point in time.

Last edited 10 years ago by Philipp Metzler (previous) (diff)

comment:8 by Tim Graham, 10 years ago

Thanks for the details. Do you think removing the option isn't acceptable then? Any sense whether this sort of setup is common? Would updating your site to use the sites framework be too much work?

comment:9 by Philipp Metzler, 10 years ago

Good question. I think that updating to the sites framework would be a lot of overhead for us as we just provide a branded site based on the URL. I don't know if this is a common setup but probably its worth thinking about why the domain defined for a site shall be used for the password reset link in the email and if it would be sufficient to just use request.get_host() Because if the user visits the domain of the site framework it's the current domain of the current request anyway.

Version 0, edited 10 years ago by Philipp Metzler (next)

comment:10 by Tim Graham, 10 years ago

It looks like #15089 would address these concerns, so I will try to move that ticket forward.

comment:11 by Tim Graham <timograham@…>, 9 years ago

In f1761e3:

Refs #21648 -- Removed is_admin_site option from password_reset() view.

Per deprecation timeline.

Note: See TracTickets for help on using tickets.
Back to Top