Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement AUTO_REFRESH_MAX_TTL to limit total token lifetime when AUTO_REFRESH = True. #366

Merged
merged 4 commits into from
Sep 30, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion docs/settings.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ REST_KNOX = {
'USER_SERIALIZER': 'knox.serializers.UserSerializer',
'TOKEN_LIMIT_PER_USER': None,
'AUTO_REFRESH': False,
'AUTO_REFRESH_MAX_TTL': None,
'MIN_REFRESH_INTERVAL': 60,
'AUTH_HEADER_PREFIX': 'Token',
'EXPIRY_DATETIME_FORMAT': api_settings.DATETIME_FORMAT,
Expand Down Expand Up @@ -75,9 +76,14 @@ This is the reference to the class used to serialize the `User` objects when
successfully returning from `LoginView`. The default is `knox.serializers.UserSerializer`

## AUTO_REFRESH
This defines if the token expiry time is extended by TOKEN_TTL each time the token
This defines if the token expiry time is extended by AUTO_REFRESH_TOKEN_TTL each time the token
christian-oudard marked this conversation as resolved.
Show resolved Hide resolved
is used.

## AUTO_REFRESH_MAX_TTL
When automatically extending token expiry time, limit the total token lifetime. If
AUTO_REFRESH_MAX_TTL is set, then the token lifetime since the original creation date cannot
exceed AUTO_REFRESH_MAX_TTL.

## MIN_REFRESH_INTERVAL
This is the minimum time in seconds that needs to pass for the token expiry to be updated
in the database.
Expand Down
6 changes: 6 additions & 0 deletions knox/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,13 @@ def authenticate_credentials(self, token):
def renew_token(self, auth_token) -> None:
current_expiry = auth_token.expiry
new_expiry = timezone.now() + knox_settings.TOKEN_TTL

# 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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 »

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@johnraz johnraz Sep 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a log message when the expiry is capped by AUTO_REFRESH_MAX_TTL.


auth_token.expiry = new_expiry

# Throttle refreshing of token to avoid db writes
delta = (new_expiry - current_expiry).total_seconds()
if delta > knox_settings.MIN_REFRESH_INTERVAL:
Expand Down
1 change: 1 addition & 0 deletions knox/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
'USER_SERIALIZER': None,
'TOKEN_LIMIT_PER_USER': None,
'AUTO_REFRESH': False,
'AUTO_REFRESH_MAX_TTL': None,
'MIN_REFRESH_INTERVAL': 60,
'AUTH_HEADER_PREFIX': 'Token',
'EXPIRY_DATETIME_FORMAT': api_settings.DATETIME_FORMAT,
Expand Down
31 changes: 30 additions & 1 deletion tests/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ def get_basic_auth_header(username, password):
auto_refresh_knox = knox_settings.defaults.copy()
auto_refresh_knox["AUTO_REFRESH"] = True

auto_refresh_max_ttl_knox = auto_refresh_knox.copy()
auto_refresh_max_ttl_knox["AUTO_REFRESH_MAX_TTL"] = timedelta(hours=12)

token_user_limit_knox = knox_settings.defaults.copy()
token_user_limit_knox["TOKEN_LIMIT_PER_USER"] = 10

Expand Down Expand Up @@ -313,11 +316,37 @@ def test_token_expiry_is_not_extended_within_MIN_REFRESH_INTERVAL(self):
reload(auth) # necessary to reload settings in core code
with freeze_time(in_min_interval):
response = self.client.get(root_url, {}, format='json')
reload(auth) # necessary to reload settings in core code
reload(auth)
christian-oudard marked this conversation as resolved.
Show resolved Hide resolved

self.assertEqual(response.status_code, 200)
self.assertEqual(original_expiry, AuthToken.objects.get().expiry)

def test_token_expiry_is_not_extended_past_max_ttl(self):
ttl = knox_settings.TOKEN_TTL
self.assertEqual(ttl, timedelta(hours=10))
original_time = datetime(2018, 7, 25, 0, 0, 0, 0)

with freeze_time(original_time):
instance, token = AuthToken.objects.create(user=self.user)

self.client.credentials(HTTP_AUTHORIZATION=('Token %s' % token))
five_hours_later = original_time + timedelta(hours=5)
with override_settings(REST_KNOX=auto_refresh_max_ttl_knox):
reload(auth) # necessary to reload settings in core code
self.assertEqual(auth.knox_settings.AUTO_REFRESH, True)
self.assertEqual(auth.knox_settings.AUTO_REFRESH_MAX_TTL, timedelta(hours=12))
with freeze_time(five_hours_later):
response = self.client.get(root_url, {}, format='json')
reload(auth)
self.assertEqual(response.status_code, 200)

# original expiry date was extended, but not past max_ttl:
new_expiry = AuthToken.objects.get().expiry
expected_expiry = original_time + timedelta(hours=12)
self.assertEqual(new_expiry.replace(tzinfo=None), expected_expiry,
"Expiry time should have been extended to {} but is {}."
.format(expected_expiry, new_expiry))

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I wasn’t clear in my previous comment but can you add the call that actually fails due to the expired token ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added an http request to the test and showed that it fails.

def test_expiry_signals(self):
self.signal_was_called = False

Expand Down