[Django] #21363: TimestampSigner.unsign should accept a timedelta for max_age
Django
-------------------------------+--------------------
Reporter: gwahl@… | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.4
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21363>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: gwahl@… | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.4
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by gwahl@…):
* needs_better_patch: => 0
* has_patch: 0 => 1
* needs_tests: => 0
* needs_docs: => 0
Comment:
Here's the patch https://github.com/django/django/pull/1835
--
Ticket URL: <https://code.djangoproject.com/ticket/21363#comment:1>
Django
Reporter: gwahl@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by bmispelon):
* cc: bmispelon@… (added)
* needs_better_patch: 0 => 1
* component: Uncategorized => Core (Other)
* version: 1.4 => master
* needs_docs: 0 => 1
* type: Uncategorized => Cleanup/optimization
* stage: Unreviewed => Accepted
Comment:
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
--
Ticket URL: <https://code.djangoproject.com/ticket/21363#comment:2>
Django
Reporter: gwahl@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Comment (by gwahl@…):
Ok, I pushed an updated patch that deprecates the number-of-seconds
version.
--
Ticket URL: <https://code.djangoproject.com/ticket/21363#comment:3>
Django
Reporter: gwahl@… | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Core (Other) | Resolution:
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by bmispelon):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
* needs_docs: 1 => 0
Comment:
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!
--
Ticket URL: <https://code.djangoproject.com/ticket/21363#comment:4>
Django
Reporter: gwahl@… | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Core (Other) | Resolution:
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21363#comment:5>
Django
Reporter: gwahl@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
Changes (by bmispelon):
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21363#comment:6>
Django
Reporter: gwahl@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21363#comment:7>
Django
Reporter: gwahl@… | Owner:
Type: | berkerpeksag
Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by berkerpeksag):
* owner: nobody => berkerpeksag
* needs_better_patch: 1 => 0
* status: new => assigned
Comment:
https://github.com/django/django/pull/3512
--
Ticket URL: <https://code.djangoproject.com/ticket/21363#comment:8>
Django
Reporter: gwahl@… | Owner:
Type: | berkerpeksag
Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Has patch: 1 | checkin
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by charettes):
* stage: Accepted => Ready for checkin
Comment:
LGTM as long as the build succeeds.
--
Ticket URL: <https://code.djangoproject.com/ticket/21363#comment:9>
Django
Reporter: gwahl@… | Owner:
Type: | berkerpeksag
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Ready for
Has patch: 1 | checkin
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"d2d6c0c097072e2a8ece755fdc2d50c111104e7d"]:
{{{
#!CommitTicketReference repository=""
revision="d2d6c0c097072e2a8ece755fdc2d50c111104e7d"
Fixed #21363 -- Added datetime.timedelta support to
TimestampSigner.unsign().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21363#comment:10>