#5515 closed (duplicate)
Custom Permission Denied Pages
Reported by: | Owned by: | ||
---|---|---|---|
Component: | Contrib apps | Version: | dev |
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: | no | UI/UX: | no |
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)
Change History (22)
comment:1 by , 17 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 17 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
by , 17 years ago
Attachment: | 5515_make_403_errors_customizable.diff added |
---|
comment:3 by , 17 years ago
Has patch: | set |
---|---|
Owner: | removed |
Status: | assigned → new |
Summary: | CSFR has hard-encoded error page → 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 by , 17 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
follow-up: 6 comment:5 by , 17 years ago
Triage Stage: | Ready for checkin → 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 by , 17 years ago
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 by , 16 years ago
Cc: | added |
---|
comment:8 by , 16 years ago
milestone: | → post-1.0 |
---|
comment:9 by , 16 years ago
Cc: | added |
---|
comment:11 by , 15 years ago
Cc: | added |
---|
comment:12 by , 15 years ago
Resolution: | → duplicate |
---|---|
Status: | new → 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
follow-up: 14 comment:13 by , 15 years ago
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 by , 15 years ago
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 by , 15 years ago
Resolution: | duplicate |
---|---|
Status: | closed → reopened |
Summary: | CSRF has hard-encoded error page → 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 by , 15 years ago
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 by , 15 years ago
Cc: | 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 by , 15 years ago
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 by , 15 years ago
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 by , 14 years ago
Resolution: | → duplicate |
---|---|
Status: | reopened → 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 by , 14 years ago
Cc: | added; removed |
---|
Patch to make 403 responses work like 404 and 500 responses (customizable via handler)