TimestampSigner.unsign should accept a timedelta for max_age

[gwahl@…]

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=datetime.timedelta(days=12)

Versus

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.

[gwahl@…]

Here's the patch ​https://github.com/django/django/pull/1835

[Baptiste Mispelon]

Hi,

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).

Thanks.

[1] ​https://docs.djangoproject.com/en/dev/internals/release-process/#internal-release-deprecation-policy

[gwahl@…]

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

[Baptiste Mispelon]

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!

[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.

[Baptiste Mispelon]

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.

[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.

[Berker Peksag]

​https://github.com/django/django/pull/3512

[Simon Charette]

LGTM as long as the build succeeds.

[Tim Graham <timograham@…>]

In d2d6c0c097072e2a8ece755fdc2d50c111104e7d:

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