Opened 6 years ago

Closed 4 years ago

#9847 closed New feature (fixed)

Improved handling for HTTP 403 Errors

Reported by: drakkan Owned by: SmileyChris
Component: Core (Other) Version: master
Severity: Normal Keywords: sprintSep2010
Cc: paulc@…, jwilk@…, ryan.niemeyer@…, groovecoder, 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 6 years ago.
base.2.diff (803 bytes) - added by adamnelson 5 years ago.
core/base.diff
base.3.diff (2.8 KB) - added by vkryachko 5 years ago.
base.4.diff (3.0 KB) - added by vkryachko 5 years ago.
Sorry base.3.diff was wrong
9847.diff (7.6 KB) - added by fvox13 5 years ago.
Added tests and docs... anyone care to test my tests?

Download all attachments as: .zip

Change History (37)

comment:1 Changed 6 years ago by jacob

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

comment:2 Changed 6 years ago by kgrandis

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

Changed 6 years ago by kgrandis

comment:3 Changed 6 years ago by kgrandis

  • Has patch set

comment:4 Changed 6 years ago by jacob

  • Resolution set to fixed
  • Status changed from assigned to closed

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

comment:5 Changed 6 years ago by jacob

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

comment:6 Changed 6 years ago by kgrandis

  • Has patch unset
  • Owner kgrandis deleted
  • Status changed from reopened to new

comment:7 Changed 6 years ago by adrian

  • milestone changed from 1.1 to 1.2

Changed 5 years ago by adamnelson

core/base.diff

comment:8 Changed 5 years ago by adamnelson

  • 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 5 years ago by ubernostrum

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 5 years ago by ubernostrum

  • milestone changed from 1.2 to 1.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 5 years ago by adamnelson

  • 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 5 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 5 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 5 years ago by vkryachko

Changed 5 years ago by vkryachko

Sorry base.3.diff was wrong

comment:14 Changed 5 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 5 years ago by vkryachko

  • Patch needs improvement unset

comment:16 Changed 5 years ago by paulc

  • Cc paulc@… added

comment:17 Changed 5 years ago by fvox13

  • Summary changed from hardcoded string in core/handlers/base.py to Improved 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 5 years ago by jwilk

  • Cc jwilk@… added

comment:19 Changed 5 years ago by fvox13

  • Keywords sprintSep2010 added
  • Needs documentation set
  • Needs tests set
  • Owner set to fvox13
  • Status changed from new to assigned

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

comment:20 Changed 5 years ago by rniemeyer

  • Cc ryan.niemeyer@… added

Changed 5 years ago by fvox13

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

comment:21 Changed 5 years ago by fvox13

  • Needs documentation unset
  • Needs tests unset
  • Version changed from 1.0 to SVN

comment:22 Changed 5 years ago by fvox13

  • Triage Stage changed from Accepted to Design 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 5 years ago by mtredinnick

  • Patch needs improvement set
  • Triage Stage changed from Design decision needed to Accepted

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 4 years ago by lrekucki

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 4 years ago by ubernostrum

  • milestone 1.3 deleted

1.3 is feature-frozen now.

comment:26 Changed 4 years ago by groovecoder

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

comment:27 Changed 4 years ago by groovecoder

  • Cc groovecoder added

comment:28 Changed 4 years ago by lukeplant

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 4 years ago by SmileyChris

  • Severity set to Normal
  • Type set to New feature

comment:30 Changed 4 years ago by SmileyChris

  • Owner changed from fvox13 to SmileyChris
  • Status changed from assigned to new

comment:31 Changed 4 years ago by sfllaw

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

comment:32 Changed 4 years ago by jezdez

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

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