Code

Opened 7 years ago

Closed 4 years ago

Last modified 4 years ago

#5515 closed (duplicate)

Custom Permission Denied Pages

Reported by: Piotr Lewandowski <django@…> Owned by:
Component: Contrib apps Version: master
Severity: Keywords:
Cc: jwilk@…, alexkon@…, rlaager@…, paulc@… Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

CSFR Middleware has hard-encoded error page in _ERROR_MSG string.
It should be located in a separate template -- just like Http500 or Http404 error pages are.

Attachments (1)

5515_make_403_errors_customizable.diff (8.8 KB) - added by progprog 6 years ago.
Patch to make 403 responses work like 404 and 500 responses (customizable via handler)

Download all attachments as: .zip

Change History (22)

comment:1 Changed 7 years ago by ubernostrum

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 6 years ago by progprog

  • Owner changed from nobody to progprog
  • Status changed from new to assigned

Changed 6 years ago by progprog

Patch to make 403 responses work like 404 and 500 responses (customizable via handler)

comment:3 Changed 6 years ago by progprog

  • Has patch set
  • Owner progprog deleted
  • Status changed from assigned to new
  • Summary changed from CSFR has hard-encoded error page to CSRF has hard-encoded error page

Patch with tests added.

I decided that in general Django should have customizable 403 pages, a la 404/500, so my patch deals with a larger scope than is described by this ticket. After making 403 customizable, the CSRF middleware simply raises the PermissionDenied exception with a custom message, and get_response() handles the rest.

One consequence of this is that a 403.html template will have to be declared, similar to 404.html and 500.html.

comment:4 Changed 6 years ago by Simon G <dev@…>

  • Triage Stage changed from Accepted to Ready for checkin

comment:5 follow-up: Changed 6 years ago by jacob

  • Triage Stage changed from Ready for checkin to Design decision needed

I'd like to have more discussion on this before checking it in. I don't like the feature creap of handler403... handler447 is only one more step down that slippery slope.

comment:6 in reply to: ↑ 5 Changed 6 years ago by progprog

Replying to jacob:

I'd like to have more discussion on this before checking it in. I don't like the feature creap of handler403... handler447 is only one more step down that slippery slope.

I agree - as I was doing the ticket I thought "Django needs a more generic way to do this." However, designing and implementing a system of generic hooks for different response codes could take weeks or months, and this patch does fulfill an immediate need, so I decided to forge ahead anyway.

comment:7 Changed 6 years ago by Jakub Wilk <ubanus@…>

  • Cc ubanus@… added

comment:8 Changed 6 years ago by anonymous

  • milestone set to post-1.0

comment:9 Changed 5 years ago by alexkon

  • Cc alexkon@… added

comment:10 Changed 5 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:11 Changed 5 years ago by anonymous

  • Cc rlaager@… added

comment:12 Changed 5 years ago by lukeplant

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

The patch in #9977 addresses this issue by allowing the user to provide their own view function for the CSRF failure view. This is more useful than just a template, as the view can do logic based on the contents of the request. So I'm closing as duplicate, since the other patch, which includes big changes to the CSRF functionality, is very likely to go in post 1.1

comment:13 follow-up: Changed 5 years ago by anonymous

Does the patch in #9977 provide for a 403.html template for PermissionDenied errors? I just installed this patch so I could create user-friendly Permimssion Denied pages.

comment:14 in reply to: ↑ 13 Changed 5 years ago by lukeplant

Replying to anonymous:

Does the patch in #9977 provide for a 403.html template for PermissionDenied errors?

No, it doesn't. I guess if you removed the CSRF stuff from the patch, this would be useful in its own right, and the bug title would probably want to change. A problem I have with the patch, on top of the concerns Jacob raised, is the issue of i18n — if we want to solve this, we want to do it right.

comment:15 Changed 5 years ago by rlaager@…

  • Resolution duplicate deleted
  • Status changed from closed to reopened
  • Summary changed from CSRF has hard-encoded error page to Custom Permission Denied Pages

What i18n concerns do you have? Enabling a 403.html wouldn't necessarily even involve shipping a 403.html. If you wanted to though, surely the text could be wrapped with trans like everything else?

comment:16 Changed 5 years ago by lukeplant

Sorry, I saw a non-i18n-ized template in the patch, but that was just for the tests.

Like Jacob, I've got concerns about whether this is the right thing. In favor of the ticket, it is probably a good idea to allow the user to customise any user visible response that is returned by Django core code, which means providing some way of giving a template for HttpPermissionDenied errors.

Also as the patch stands, I don't like the backwards incompatibility - developers have to create a template for 403s to continue to work. Hardcoding a simple fallback template (possibly in django/views/defaults.py itself) would solve that, even if it doesn't feel very nice.

To summarise, we need to:

  • discuss whether this is the right thing
  • remove CSRF stuff from patch
  • sort out backwards compatibility for 403s.

comment:17 Changed 4 years ago by paulc

  • Cc paulc@… added

Hey everyone,

There seems to have been no movement on this in the past 10 months. We have two major projects at Mozilla using Django (addons.mozilla.org and support.mozilla.com). These 403 handlers with templates would come in handy. So here are my questions:

1) What's keeping this patch from landing? IMO, it makes sense to add 403 handlers treated similarly to 404 and 500 ones.

2) Is there anything I can do to help? We're running Django trunk (because we love all the new features you guys added), so I'd be interested to have it work with 1.2

3) If this can't be landed anytime soon, what would you do as a workaround if you wanted a 403.html template (also to work with Jinja since we're using that).

Thanks a lot and hope to hear from you soon.

PS: Django is awesome.

comment:18 Changed 4 years ago by russellm

The reason there has been no movement in 10 months is that nobody has been driving the issue. If you want it, you have to advocate for it. Also, 10 months spans almost exactly the 1.2 development cycle. At the very least, 8 of those 10 months can be explained by the fact that nobody proposed this for inclusion in 1.2, so it never got on anybody's todo list.

The ticket is currently marked as Design Decision Needed, and there are comments from multiple core developers that question whether this ticket is the right way to go; the most recent comment from Luke indicated several specific issues that need to be addressed.

If you want this, you need to drive the discussion to make it happen. Luckily, we're about to start the 1.3 development process, so your timing is perfect. What you need to do is get your thoughts together and make a proposal on django-developers, and then drive the discussion (which may involve making counter proposals or writing alternate prototype patches) until such time as you get a core developer to sign off on the design.

The ideal time to start this discussion will be in a little over a week -- several of the core team will be at DjangoCon.eu, so attention will be elsewhere for a little bit -- but that gives you a week to get your thoughts together, address the issues that Luke raised, and make a compelling case for the change you want to see.

comment:19 Changed 4 years ago by paulc

Thanks Piotr, I appreciate your quick reply and your thoughts.

I understand that someone needs to drive this issue, though I don't claim to be a Django expert and it would take me some time to understand if this patch is going in the right direction or there's a better option.

I'll spend some time playing with this patch over the next week and reading up on the other two issues mentioned by Luke, see if I can come up with anything. Is there some place I could read up on current plans for status code handlers or other things of the sort? My main resource so far has been the Django /dev/ documentation.

comment:20 Changed 4 years ago by fvox13

  • Resolution set to duplicate
  • Status changed from reopened to closed

The CSRF handling has changed substantially in 1.2, and there are indeed benefits to having separate handling for the 403s raised by CSRF. "You need cookies and you need to stop mucking with the token you l33t hax0r d00d" is different than "you can't access that foo because permission is denied". #9847 covers the second use case, so let's continue this discussion there.

comment:21 Changed 4 years ago by jwilk

  • Cc jwilk@… added; ubanus@… removed

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.