Opened 17 years ago

Closed 14 years ago

Last modified 14 years ago

#5515 closed (duplicate)

Custom Permission Denied Pages

Reported by: Piotr Lewandowski <django@…> 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)

5515_make_403_errors_customizable.diff (8.8 KB ) - added by progprog 16 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 by James Bennett, 17 years ago

Triage Stage: UnreviewedAccepted

comment:2 by progprog, 16 years ago

Owner: changed from nobody to progprog
Status: newassigned

by progprog, 16 years ago

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

comment:3 by progprog, 16 years ago

Has patch: set
Owner: progprog removed
Status: assignednew
Summary: CSFR has hard-encoded error pageCSRF 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 Simon G <dev@…>, 16 years ago

Triage Stage: AcceptedReady for checkin

comment:5 by Jacob, 16 years ago

Triage Stage: Ready for checkinDesign 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.

in reply to:  5 comment:6 by progprog, 16 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 Jakub Wilk <ubanus@…>, 16 years ago

Cc: ubanus@… added

comment:8 by anonymous, 16 years ago

milestone: post-1.0

comment:9 by Alexander Konovalenko, 15 years ago

Cc: alexkon@… added

comment:10 by (none), 15 years ago

milestone: post-1.0

Milestone post-1.0 deleted

comment:11 by anonymous, 15 years ago

Cc: rlaager@… added

comment:12 by Luke Plant, 15 years ago

Resolution: duplicate
Status: newclosed

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 by anonymous, 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.

in reply to:  13 comment:14 by Luke Plant, 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 rlaager@…, 15 years ago

Resolution: duplicate
Status: closedreopened
Summary: CSRF has hard-encoded error pageCustom 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 Luke Plant, 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 paulc, 14 years ago

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 by Russell Keith-Magee, 14 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 paulc, 14 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 Steven L Smith, 14 years ago

Resolution: duplicate
Status: reopenedclosed

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 Jakub Wilk, 14 years ago

Cc: jwilk@… added; ubanus@… removed
Note: See TracTickets for help on using tickets.
Back to Top