Opened 9 years ago

Closed 9 years ago

Last modified 8 years ago

#24796 closed Cleanup/optimization (fixed)

Middleware ordering hints don't mention SecurityMiddleware

Reported by: Taymon A. Beal Owned by: MZ
Component: Documentation Version: 1.8
Severity: Normal Keywords:
Cc: MZ Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The documentation offers hints on how to order different middleware classes, but doesn't say anything about where one should put SecurityMiddleware relative to the other classes. The default project template puts it at the bottom, but the old django-secure documentation suggests putting it near the top.

Change History (12)

comment:1 by Claude Paroz, 9 years ago

Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

comment:2 by Carl Meyer, 9 years ago

Tim, do you recall why you put it at the bottom of the list when you did the integration?

I think I recommended top of the list in the django-secure docs just because I figured if you're going to turn on the redirect-to-HTTPS, it may as well happen sooner rather than after running through a bunch of other unnecessary middleware.

Trying to think what the reasoning would be for having it at the bottom of the list: I guess if you had other middleware that wanted to access the value of some headers set by SecurityMiddleware?

Mostly I think it just really doesn't matter very much where you put it, unless you're in an unusual situation.

comment:3 by Tim Graham, 9 years ago

I don't recall putting any thought into its position in the list.

comment:4 by Yamila, 9 years ago

Hi! I would like to help here, but I'm not sure what's the decission. As far as I see:

  • This middleware is not documented in the "ordering middleware" part. Should be?
  • This middleware appears in a different possition than older versions. It would'n affect the documentation, and ¿maybe? is another ticket.

I guess the documentation should follow the code, and I'm not sure if it's going to be changed.

comment:5 by MZ, 9 years ago

Owner: changed from nobody to MZ
Status: newassigned

comment:6 by Carl Meyer, 9 years ago

I don't have strong feelings about this, but if we're going to clarify the situation, the only reasoning I have for putting it anywhere is what I mentioned; that it should go near the top of the list if you're using the SSL redirect, for efficiency. So my inclination would be to a) mention that in the docs, and b) change the startproject template to move it up top, just so we're following our own advice.

comment:8 by MZ, 9 years ago

Cc: MZ added
Has patch: set

comment:9 by Carl Meyer, 9 years ago

Triage Stage: AcceptedReady for checkin

I think the docs addition sentence could probably be reworded slightly for better flow, but that can be handled by whoever merges - I think this patch is basically ready to go. Thanks for the pull request!

comment:10 by Tim Graham <timograham@…>, 9 years ago

Resolution: fixed
Status: assignedclosed

In 8b1f39a7:

Fixed #24796 -- Added a hint on placement of SecurityMiddleware in MIDDLEWARE_CLASSES.

Also moved it in the project template.

comment:11 by Ed Morley, 8 years ago

I don't suppose this could be backported to 1.8? (or at least just the docs part)

The current 1.8 docs caused some confusion in:
https://github.com/evansd/whitenoise/issues/100

Thanks :-)

comment:12 by Tim Graham <timograham@…>, 8 years ago

In 358ae4a:

[1.8.x] Fixed #24796 -- Moved SecurityMiddleware in MIDDLEWARE_CLASSES docs.

Partial backport of 8b1f39a727be91aab40bdb37235718ed63ae1d50 from master

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