Opened 11 years ago

Closed 10 years ago

#21363 closed Cleanup/optimization (fixed)

TimestampSigner.unsign should accept a timedelta for max_age

Reported by: gwahl@… Owned by: Berker Peksag
Component: Core (Other) Version: dev
Severity: Normal Keywords:
Cc: bmispelon@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


Currently, the max_age parameter to TimestampSigner.unsign is a number representing the max age of the signature as a number of seconds. In Python, the correct way to represent an time interval (an age) is a datetime.timedelta.

A timedelta is very obvious and readable:



max_age=12 * 24 * 60 * 60

A timedelta provides much better error reporting for long intervals: 'Signature age 1296123 > 1209600 seconds' versus 'Signature age 15 days, 0:02:03 > 14 days, 0:00:00'

A timedelta is also the intuitive type for the max_age parameter. I asked a few of my colleagues what they thought should be passed as a max_age, and all of them said a timedelta. Using a value object instead of a simple number also provides type safety by only allowing semantically valid operations.

It is unfortunate that to preserve backwards compatibility there is now more than one way to pass a timeout to unsign. My patch currently accepts both, but it would be easy to deprecate the seconds version.

Change History (10)

comment:1 by gwahl@…, 11 years ago

Has patch: set

comment:2 by Baptiste Mispelon, 11 years ago

Cc: bmispelon@… added
Component: UncategorizedCore (Other)
Needs documentation: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization
Version: 1.4master


I think this is a good idea too and it makes for a nicer API.

You should follow the deprecation policy [1] on passing integers for max_age so that we can phaze it out.

A full patch will also need a bit of documentation (at least amending the current docs to indicate that a timedelta should be passed as well as a note in the release notes for 1.7).



comment:3 by gwahl@…, 11 years ago

Ok, I pushed an updated patch that deprecates the number-of-seconds version.

comment:4 by Baptiste Mispelon, 11 years ago

Needs documentation: unset
Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

It looks good to me.

I'm marking this as ready for checkin to give a chance to other core devs to review it.

Thanks for your contribution!

comment:5 by Florian Apolloner, 11 years ago

-1 (yes this is a veto) for removing seconds support; we don't remove features just cause we can. Personally I don't see much gain, especially since this is security related; changing stuff cause it looks better doesn't count here imo.

comment:6 by Baptiste Mispelon, 11 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

Removing the ready for checkin status in line with apollo13's veto.

As discussed on IRC, the other option left is to remove the deprecation and allow both int and timedelta objects to be passed as the max_age argument.

comment:7 by Aymeric Augustin, 11 years ago

-1 on removing support for integers. While the new API in marginally better, I don't see the value in forcing this change on all users of Django.

+0 on adding support for timedeltas in addition to integers.

comment:8 by Berker Peksag, 10 years ago

Owner: changed from nobody to Berker Peksag
Patch needs improvement: unset
Status: newassigned

comment:9 by Simon Charette, 10 years ago

Triage Stage: AcceptedReady for checkin

LGTM as long as the build succeeds.

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

Resolution: fixed
Status: assignedclosed

In d2d6c0c097072e2a8ece755fdc2d50c111104e7d:

Fixed #21363 -- Added datetime.timedelta support to TimestampSigner.unsign().

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