Opened 20 months ago

Closed 8 months ago

#21363 closed Cleanup/optimization (fixed)

TimestampSigner.unsign should accept a timedelta for max_age

Reported by: gwahl@… Owned by: berkerpeksag
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

Description

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.

Change History (10)

comment:1 Changed 20 months ago by gwahl@…

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

comment:2 Changed 20 months ago by bmispelon

  • Cc bmispelon@… added
  • Component changed from Uncategorized to Core (Other)
  • Needs documentation set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Cleanup/optimization
  • Version changed from 1.4 to master

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

comment:3 Changed 20 months ago by gwahl@…

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

comment:4 Changed 20 months ago by bmispelon

  • Needs documentation unset
  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready 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 20 months ago by apollo13

-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 20 months ago by bmispelon

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

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 20 months ago by aaugustin

-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 8 months ago by berkerpeksag

  • Owner changed from nobody to berkerpeksag
  • Patch needs improvement unset
  • Status changed from new to assigned

comment:9 Changed 8 months ago by charettes

  • Triage Stage changed from Accepted to Ready for checkin

LGTM as long as the build succeeds.

comment:10 Changed 8 months ago by Tim Graham <timograham@…>

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

In d2d6c0c097072e2a8ece755fdc2d50c111104e7d:

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

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