Opened 3 years ago

Last modified 3 years ago

#7539 enhancement new

Timeout after 15 minutes when using DigestCredentialFactory

Reported by: Phillip Knauss Owned by: Phillip Knauss
Priority: normal Milestone:
Component: web Keywords:
Cc: jknight Branch:
Author:

Description

When using DigestCredentialFactory for authorization, after 15 minutes idle the user is asked to re-enter their username/password. This timeout period should be configurable.

Attachments (4)

digest_cred_timeout.patch (3.0 KB) - added by Phillip Knauss 3 years ago.
digest_cred_timeout_doc.patch (881 bytes) - added by Phillip Knauss 3 years ago.
Updates CHALLENGE_LIFETIME_SECS to specify the 15-minute timeout
digest_cred_timeout_3.patch (5.9 KB) - added by Phillip Knauss 3 years ago.
Implementation with original reviewer concerns addressed
digest_cred_timeout_4.patch (5.8 KB) - added by Phillip Knauss 3 years ago.
Implementation with original reviewer concerns addressed - Disregard #3

Download all attachments as: .zip

Change History (15)

comment:1 Changed 3 years ago by DefaultCC Plugin

Cc: jknight added

comment:2 Changed 3 years ago by Phillip Knauss

DigestCredentialFactory currently uses the CHALLENGE_LIFETIME_SECS class variable to determine this timeout. It should instead be a keyword arg on init so that it can be set as required, defaulting to the same 15 minutes.

Changed 3 years ago by Phillip Knauss

Attachment: digest_cred_timeout.patch added

comment:3 Changed 3 years ago by Jean-Paul Calderone

Owner: set to Phillip Knauss

Please put the review keyword on tickets that you think will be resolved by a patch attached to them. See ReviewProcess for more details.

This change is backwards incompatible ... somewhat. It's already possible to control the timeout by setting the class attribute you pointed out. This isn't a fantastic API, of course. Still, it works so we shouldn't break it.

A compatible change would be to make the code use the same attribute but look it up on self instead of on the class object. This preserves the existing behavior - code that changes it on the class gets the desired result - and improves the API somewhat - by making it possible to change the timeout on a single instance instead of for all instances.

This feature also needs to have unit tests that demonstrate it actually works.

Finally, the new feature should be described in a news fragment.

Thanks.

comment:4 Changed 3 years ago by Phillip Knauss

Retaining backwards compatibility would require assigning to class variable that's formatted to look like a constant:

self.CHALLENGE_LIFETIME_SECS = 15 * 60

Doing that would make the API only slightly better at the cost of readability.

My biggest issue with the existing API was the lack of documentation. It appears this was addressed as of 13.0.0, but naive Google searches link to older versions. The only change I still want to make is to note the default timeout of 15 minutes, this should prevent that from being a surprise for others.

Changed 3 years ago by Phillip Knauss

Updates CHALLENGE_LIFETIME_SECS to specify the 15-minute timeout

comment:5 Changed 3 years ago by Phillip Knauss

Keywords: review added

comment:6 Changed 3 years ago by Jean-Paul Calderone

Keywords: review removed

It seems unfortunate to duplicate the information already present. Perhaps this is actually an issue against pydoctor for doing something better with class attributes.

comment:7 Changed 3 years ago by Phillip Knauss

Agreed, but apparently beside the point. My original change is actually necessary. As implemented, the setting DigestCredentialFactory.CHALLENGE_LIFETIME_SECS doesn't work because while the instance is kept around, the loaded DigestCredentialFactory reference seems to have come from elsewhere (maybe it's being handled in a different thread?).

I need to dig into this more, but it's increasingly looking like the timeout setting necessarily needs to be set per-instance in order to cover my use case.

Changed 3 years ago by Phillip Knauss

Attachment: digest_cred_timeout_3.patch added

Implementation with original reviewer concerns addressed

comment:8 Changed 3 years ago by Phillip Knauss

Keywords: review added

This implementation is similar my original attempt but addresses the reviewer concerns in the original.

  • Simply changing the lookup to self.CHALLENGE_LIFETIME_SECS did not resolve the problem on my end. I have not been able to produce a minimal case for that behaviour, however.
  • The latest patch retains backwards compatibility by falling back to the value of the class attribute if the instance variable has not been set. It checks this immediately before the comparison in which it is used to behave correctly regardless of when CHALLENGE_LIFETIME_SECS may have been changed (before or after creating an instance).
  • Unit test was added to verify the behaviour when the instance variable is specified. An existing unit test already covers the case when the instance variable is not specified.

Changed 3 years ago by Phillip Knauss

Attachment: digest_cred_timeout_4.patch added

Implementation with original reviewer concerns addressed - Disregard #3

comment:9 Changed 3 years ago by Jean-Paul Calderone

Owner: Phillip Knauss deleted

Submitted for review by knauss so probably not being reviewed by knauss.

comment:10 Changed 3 years ago by Adi Roiban

Keywords: review removed
Owner: set to Phillip Knauss

Many thanks for the patch.

Please note that I am just a junior reviewer so I might be wrong.


The newsfile is documented here https://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles

You are not supposed to manually edit the changelog/relese-notes files.


From what I see challengeLifetimeSecs is both an instance variable and an argument to init. I am not an epydoc expert but I think that challengeLifetimeSecs should be documented as ivar at class level and as a normal param for init.

Do we want to make the new challengeLifetimeSecs public ? ie does it need to be changed by someone else after initialization?

I would prefer to keep it private to reduce the public API of this class.

It is always much easier to start with a private member and if required convert it later to public or removed at any time without any deprecations process.


I find that the following comment don't add any value... and in future refactoring might get out of sync with the code.

# Verify the timeout

I know that there is already a # Verify the key comment. If you find that code is complicated, hard to read and needs to be explained, you can extract the whole logic into a private method. In this way it much easier to keep comment and sync and is much easier to identify the scope of the comment.

def _validateSession(self): 
    """
    Raise an exception if things are bad.
    """
    challengeLifetimeSecs = self.CHALLENGE_LIFETIME_SECS 
    if self.challengeLifetimeSecs is not None: 
       challengeLifetimeSecs = self.challengeLifetimeSecs 
	           
    if int(self._getTime()) - when > challengeLifetimeSecs:
       raise error.LoginFailed()


# in _verifyOpaque just call
self._validateSession()
   

I think that in the test docstring

older than C{DigestCredentialFactory.CHALLENGE_LIFETIME_SECS}
with overridden value.

should be

older than challengeLifetimeSecs passed at object initialization
and which overwrites C{DigestCredentialFactory.CHALLENGE_LIFETIME_SECS}.

I am not happy with this patch as it can lead to having different values in CHALLENGE_LIFETIME_SECS and challengeLifetimeSecs.

Why not convert CHALLENGE_LIFETIME_SECS on demand into an instance variable


# Define as class member
CHALLENGE_LIFETIME_SECS = 42

def __init__(self, challengeLifetimeSecs=None):
   if challengeLifetimeSecs is not None:
      # overwritten as instance member.
      self.CHALLENGE_LIFETIME_SECS = challengeLifetimeSecs
   else:
      # left as class member

# and use it like
if (int(self._getTime()) - when > self.CHALLENGE_LIFETIME_SECS): 


Please check my comments. Feel free to address them, or otherwise resubmit the ticker for review and someone else will take a look at the changes.

Many thanks again!

comment:11 Changed 3 years ago by Jean-Paul Calderone

self.CHALLENGE_LIFETIME_SECS = challengeLifetimeSecs

UPPER_CASE is a widely used convention for constants. Changing their value is bad form and likely to surprise maintainers. If you want to make the implementation work like this, use a different variable name.

Note: See TracTickets for help on using tickets.