Opened 3 years ago

Closed 2 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: master
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 Changed 3 years ago by gwahl@…

Has patch: set
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

comment:2 Changed 3 years ago by Baptiste Mispelon

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 Changed 3 years ago by gwahl@…

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

comment:4 Changed 3 years ago by Baptiste Mispelon

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 Changed 3 years ago by Florian Apolloner

-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 Changed 3 years ago by Baptiste Mispelon

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 Changed 3 years ago by Aymeric Augustin

-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 Changed 2 years ago by Berker Peksag

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

comment:9 Changed 2 years ago by Simon Charette

Triage Stage: AcceptedReady for checkin

LGTM as long as the build succeeds.

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

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