Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.While 60 seconds is probably a reasonable default, some people might want to give it a longer timespan.
The attached patch proposes introducing a site variable, cram_nonce_ttl, which is assigned to a constant CRAM_NONCE_TTL. This also removes redundancy in having to specify the TTL several places (currently only on line 50 and 130 (pre-patch) though). One suggestion for usability is to have a dropdown instead of a textfield, with pre-populated values (e.g. "30 seconds", "1 minute", "2 minutes", "5 minutes", "10 minutes", "15 minutes", "30 minutes", "45 minutes", "1 hour", ...). Also, the wording of cram_admin_settings['cram_nonce_ttl'] strings could probably use a loving eye.
If this is accepted, I can probably look into a backport for 5.x as well. If this is not accepted as-is, the constant might still be worth considering, to avoid having "60" 60 different places and forget to update one of the when you decide to switch to "90". ;)
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | cram.d5.nonce_ttl.patch | 2.79 KB | Freso |
| #5 | cram.d6.nonce_ttl.patch | 3.27 KB | Freso |
| #4 | cram.d6.nonce_ttl.patch | 3.66 KB | Freso |
| cram.d6.nonce_ttl.patch | 3.24 KB | Freso |
Comments
Comment #1
Freso commentedNote that this might fail if applied after #224835: Do proper checking of cram_nonce.valid (but hopefully it'll just apply fuzzily).
Comment #2
selmanj commentedI like this idea, but before we commit it we should probably decide what reasonable limits are for the timeout. Obviously having a timeout of 1 second is probably impossible to use, and having a timeout of 1 day would be really bad too (at least on a busy site).
I would say 5 minutes would be a good default, and then let it be configurable down to 1 minute and up to... say, 1 hour?
Also, I'm not sure why the code uses a constant in place of just using variable_get() wherever appropriate. Is this so if the default changes, we don't have to change it everywhere in the code?
Comment #3
Freso commentedExactly. It's also more readable IMHO (compare to ).
Comment #4
Freso commentedHere's an updated patch, which has a dropdown instead of a text field. Works here. I can probably port it when it's checked in, but I don't want to do it before, in case stuff needs changing, and some of the areas this patch affects aren't so similar between D5 and D6 that I trust it to be a piece of cake to do. It probably is, but... let's just get this completed polished up beforehand. :)
Comment #5
Freso commentedAnd here's a patch without the _validate function.
Comment #6
selmanj commentedIt looks good to me, but I don't know if I want to apply this yet without a D5 patch as well. I'm afraid of features creeping into d6 that don't get backported into D5.
Marking this as CNW until you or I get a backport written.
Comment #7
Freso commentedI've checked this in and will port this during the weekend (if not tonight).
Comment #8
Freso commentedWell, it turned out to indeed be tonight. :)
I've tested it, and it would appear it works as intended (or at least as well as for D6), so I've also checked it in.
Comment #9
Anonymous (not verified) commentedAutomatically closed -- issue fixed for two weeks with no activity.