Implement AUTO_REFRESH_MAX_TTL to limit total token lifetime when AUTO_REFRESH = True. #366
Conversation
…O_REFRESH = True.
knox/auth.py
Outdated
|
|
||
| # Do not auto-renew tokens past AUTO_REFRESH_MAX_TTL. | ||
| if knox_settings.AUTO_REFRESH_MAX_TTL is not None: | ||
| new_expiry = min(new_expiry, auth_token.created + knox_settings.AUTO_REFRESH_MAX_TTL) |
There was a problem hiding this comment.
There is quite some indirection here, I think that rather than abusing the expiry value we should raise a clear error that says auto refresh has been « exhausted »
There was a problem hiding this comment.
I guess I don't understand the library enough to get what needs to be different here. This seems to me like an elegant solution to not extending the maximum time since creation date. Is there a different exception or http code or something you want to return to the user instead?
There was a problem hiding this comment.
I would like the error message to clearly state that the token couldn’t be auto refreshed due to the max ttl being reached yes.
While this code would work, it would be quite hard to understand what happened.
Remember the Zen of Python: explicit is better than implicit 😉
There was a problem hiding this comment.
Okay, but the current design deletes token objects once they expire. The error message does not currently distinguish between tokens that never existed and ones that once existed then expired. According to the docstring of authenticate_credentials: "Tokens that have expired will be deleted and skipped", using _cleanup_token.
So I think it's on you to provide a mechanism to explicitly distinguish these things.
There was a problem hiding this comment.
This is not really true.
Nothing prevents us from raising something like AutoRefreshMaxTTLExpired here and handling the exception in authenticate_credentials in order to return a specific error message with exceptions.AuthenticationFailed.
Marking the token has expired and letting it continue its life until it reaches another step of the flow feels wrong to me, really.
Edit: To clarify, I think that adding more potential mechanism for failure / expiration requires more care - this is why building on top of the current behavior is going to make things hard to follow IMHO.
There was a problem hiding this comment.
Reading the code again I realize we actually don’t want an error here as the authentication passed … Then I guess adding a log message stating that the max ttl was reached would be better already.
And maybe for the longer game we should review the way this is designed: decide if a token should be refreshed at the time we identify it is actually expired - this would give us more room for raising appropriate errors but this is definitely outside of the scope of this PR
There was a problem hiding this comment.
Totally fine adding a log message, but there is currently no logging whatsoever in the project, so I would wait for you or one of the other main developers to add that before trying to do it myself. My client is already using our fork of this in its current state, so I'm going to stop working on this PR for now. Please feel free to do anything you need to to merge this into the main codebase.
There was a problem hiding this comment.
AFAIK logging configuration is handled by Django.
To add a log line here you can just follow the usual way:
logger = logging.getLogger(__name__)
and then use the logger accordingly.
There was a problem hiding this comment.
Added a log message when the expiry is capped by AUTO_REFRESH_MAX_TTL.
| self.assertEqual(new_expiry.replace(tzinfo=None), expected_expiry, | ||
| "Expiry time should have been extended to {} but is {}." | ||
| .format(expected_expiry, new_expiry)) | ||
|
|
There was a problem hiding this comment.
This test is ok but I think we need to demonstrate the use case from an http standpoint: status code, error message etc in case auto refresh is not allowed anymore
There was a problem hiding this comment.
Maybe I wasn’t clear in my previous comment but can you add the call that actually fails due to the expired token ?
There was a problem hiding this comment.
Added an http request to the test and showed that it fails.
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Looks good to me, can you look into why you have a pre-commit related commit in your PR?
After that is solved we should be good to merge 😁
@giovannicimolin maybe you want to give this a check too?
This was apparently created by the copied CI system on my fork. It was a linting fix. All it does is rearrange the order of some imports. Please merge this whenever you like. |
|
Any predictions when you'll be able to merge this? |
|
When @giovannicimolin will have free time to review so no, there is no specific timeline |
|
Looking forward to seeing this merged :) Thank you! |
Refs #311