Opened 8 years ago

Closed 5 years ago

#9847 closed New feature (fixed)

Improved handling for HTTP 403 Errors

Reported by: drakkan Owned by: Chris Beaven
Component: Core (Other) Version: master
Severity: Normal Keywords: sprintSep2010
Cc: paulc@…, jwilk@…, ryan.niemeyer@…, luke crouch, sfllaw@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

in core/handlers/base.py is hardcoded an h1:

<h1>Permission denied</h1>

so this string not transable,

regards
drakkan

Attachments (5)

base.diff (1.0 KB) - added by kgrandis 8 years ago.
base.2.diff (803 bytes) - added by Adam Nelson 7 years ago.
core/base.diff
base.3.diff (2.8 KB) - added by vkryachko 6 years ago.
base.4.diff (3.0 KB) - added by vkryachko 6 years ago.
Sorry base.3.diff was wrong
9847.diff (7.6 KB) - added by fvox13 6 years ago.
Added tests and docs... anyone care to test my tests?

Download all attachments as: .zip

Change History (37)

comment:1 Changed 8 years ago by Jacob

milestone: 1.1
Triage Stage: UnreviewedAccepted

comment:2 Changed 8 years ago by kgrandis

Owner: changed from nobody to kgrandis
Status: newassigned

Changed 8 years ago by kgrandis

Attachment: base.diff added

comment:3 Changed 8 years ago by kgrandis

Has patch: set

comment:4 Changed 8 years ago by Jacob

Resolution: fixed
Status: assignedclosed

(In [10346]) Fixed #9847: mark the permission denied message for translation.

comment:5 Changed 8 years ago by Jacob

Resolution: fixed
Status: closedreopened

Reopening - the fix was a bad idea; see [10347].

comment:6 Changed 8 years ago by kgrandis

Has patch: unset
Owner: kgrandis deleted
Status: reopenednew

comment:7 Changed 8 years ago by Adrian Holovaty

milestone: 1.11.2

Changed 7 years ago by Adam Nelson

Attachment: base.2.diff added

core/base.diff

comment:8 Changed 7 years ago by Adam Nelson

Has patch: set

There is no need to have a message at all. 'Permission Denied' is presumed with the 403 HttpResponseForbidden error. If a message must be delivered, it should be added to django/http/init.py for all HttpResponseForbidden errors.

comment:9 Changed 7 years ago by James Bennett

I think there should be content in the response, since otherwise you'd show a completely blank page to an end user, requiring them to know about HTTP status codes and how to find out what status code we sent back.

I can see a couple options for doing this:

  1. Don't bother trying to translate the response content, and just use the language in the HTTP spec (so just "403 Forbidden").
  2. Set a default content value for HttpResponseForbidden, and mark it for translation. I think we can do that safely since it's over in django.http and settings will already be set up by the time it gets imported.

comment:10 Changed 7 years ago by James Bennett

milestone: 1.21.3

Also, this is a minor enough issue that it really ought to happen on 1.3 or a 1.2.X bugfix release.

comment:11 Changed 7 years ago by Adam Nelson

Patch needs improvement: set

I take back what I said about not having a message at all.

  1. It shouldn't be hardcoded in base.py, maybe it should be in source:django/trunk/django/http/__init__.py ?
  2. All the non-200 HTTP errors should derive from a master error class. Right now it's kind of a hodge-podge it seems.

I agree that milestone:1.3 is best.

comment:12 Changed 6 years ago by vkryachko

Why not create an exception subclass which would be raised in base.py and caught by a middleware to display a custom 403 page, like with Http404 ?

comment:13 Changed 6 years ago by vkryachko

OK, I see middleware is not involved in this process, but it's possible to add a method to the resolver, resolver.resolve403() just like resolve404 and resolve500 which could render 403.html template and if that fails just return 'Permission Denied'

Changed 6 years ago by vkryachko

Attachment: base.3.diff added

Changed 6 years ago by vkryachko

Attachment: base.4.diff added

Sorry base.3.diff was wrong

comment:14 Changed 6 years ago by vkryachko

So here's the patch, I think that this way it would be more flexible so that you can customize the content of 403 as much as you wish. Besides it makes exception handling more consistent, you just create 403.html in the root of your templates dir just like for 404 and 500. What do you think?

comment:15 Changed 6 years ago by vkryachko

Patch needs improvement: unset

comment:16 Changed 6 years ago by paulc

Cc: paulc@… added

comment:17 Changed 6 years ago by fvox13

Summary: hardcoded string in core/handlers/base.pyImproved handling for HTTP 403 Errors

I've closed #5515 and #10360 and changed the title of this ticket to better reflect the problem we hope to solve. Having one ticket instead of three will greatly improve the chances of resolution.

comment:18 Changed 6 years ago by Jakub Wilk

Cc: jwilk@… added

comment:19 Changed 6 years ago by fvox13

Keywords: sprintSep2010 added
Needs documentation: set
Needs tests: set
Owner: set to fvox13
Status: newassigned

I'm going to try and get this ready for checkin today at the djangocon sprint.

comment:20 Changed 6 years ago by rniemeyer

Cc: ryan.niemeyer@… added

Changed 6 years ago by fvox13

Attachment: 9847.diff added

Added tests and docs... anyone care to test my tests?

comment:21 Changed 6 years ago by fvox13

Needs documentation: unset
Needs tests: unset
Version: 1.0SVN

comment:22 Changed 6 years ago by fvox13

Triage Stage: AcceptedDesign decision needed

Need some more feedback on this one, so we're going to hold off... in any case, the tests will be useful even if we do something other than a bunch of resolve4xx stuff. Slippery slope... we don't really want 17 different resolve401, resolve402 etc.

comment:23 Changed 6 years ago by Malcolm Tredinnick

Patch needs improvement: set
Triage Stage: Design decision neededAccepted

Tests and docs are fine. As noted, I'd like a way to avoid requiring the module-level resolver403 function. Those functions are really annoying (they require import * from the default url stuff to be used, for example).

comment:24 Changed 6 years ago by Łukasz Rekucki

Just for the record, after [13590] the handlerXXX functions don't require an "import *". This still calls for a more general solution then just putting another function. Also, the current patch required a "403.html" template, but doesn't provide it. This gets catch by "except:"``, but that's at least bad style to me. If I make a syntax error in the ``"403.html" I would expect a 500 and a trace, so that my tests fail when checking the returned status code.

comment:25 Changed 6 years ago by James Bennett

milestone: 1.3

1.3 is feature-frozen now.

comment:26 Changed 6 years ago by luke crouch

Bump. Is this in the running for 1.3.x?

comment:27 Changed 6 years ago by luke crouch

Cc: luke crouch added

comment:28 Changed 6 years ago by Luke Plant

It can't go in 1.3.X, as we don't backport features. It could get into 1.4. It currently has 'Patch needs improvement' for some reason. If that is resolved, and it can promoted to 'Ready for checkin', it stands a good chance for 1.4. The way to get things included is to follow our contributing howto to make sure the ticket moves along nicely.

comment:29 Changed 6 years ago by Chris Beaven

Severity: Normal
Type: New feature

comment:30 Changed 6 years ago by Chris Beaven

Owner: changed from fvox13 to Chris Beaven
Status: assignednew

comment:31 Changed 5 years ago by Simon Law

Cc: sfllaw@… added
Easy pickings: unset
UI/UX: unset

comment:32 Changed 5 years ago by Jannis Leidel

Resolution: fixed
Status: newclosed

In [16606]:

Fixed #9847 -- Added 403 response handler. Many thanks to kgrandis, adamnelson, vkryachko, fvox13 and Chris Beaven.

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