knox: handle race condition on concurrent logout call

[xrmx]

With AUTO_REFRESH enabled authentication is racing against token removal of logout. If a token gets removed updating its expiry would led to a DatabaseError raised by the Django ORM.

Fix that by ignoring DatabaseError exception returned by renew_token so that the last request will get a AuthenticationFailed exception instead of a 500 error.

[johnraz]

Hello 👋 and thanks for the contribution!

Your branch seems to be based on master and not develop, could you please rebase on develop?

I believe the changelog would also need a note about the bug fix.

I left a comment for a potential improvement in the code as well.

Thanks again!

[johnraz]

I would rather move this try/except around the save call within renew_token. Also, I’m worried that swallowing every database error like this could lead to some other unrelated error being swallowed. Can we check the error code ? The error message maybe ?

[xrmx]

The .save is the only database interaction in that hunk of code so there's not much chances to swallow unrelated exceptions. Regarding moving the handling inside renew_token I don't see much value in it: it will duplicate the error handling as we really can do something sensible only in authenticate_credentials.

[johnraz]

You could swallow any other database error and mistaken it with an authentication error which would be confusing in a debug session. renew_token could change in the future and have more database interactions so I think moving it around the save makes sense. I don’t understand your argument about code duplication as this is exactly what I’m trying to avoid, using refresh token somewhere else would require to duplicate the error handling, right? Moving the error handling inside refresh token makes it safe to be used anywhere.

[xrmx]

By duplicating error handling i mean that on top of the exception handling i have to check the return value. In my opinion it's making the code less explicit.

diff --git a/knox/auth.py b/knox/auth.py
index 1d453ca..5345175 100644
--- a/knox/auth.py
+++ b/knox/auth.py
@@ -74,10 +74,8 @@ class TokenAuthentication(BaseAuthentication):
                 raise exceptions.AuthenticationFailed(msg)
             if compare_digest(digest, auth_token.digest):
                 if knox_settings.AUTO_REFRESH and auth_token.expiry:
-                    try:
-                        self.renew_token(auth_token)
-                    except DatabaseError:
-                        # avoid race condition with concurrent logout calls
+                    renewed = self.renew_token(auth_token)
+                    if not renewed:
                         continue
                 return self.validate_user(auth_token)
         raise exceptions.AuthenticationFailed(msg)
@@ -89,7 +87,12 @@ class TokenAuthentication(BaseAuthentication):
         # Throttle refreshing of token to avoid db writes
         delta = (new_expiry - current_expiry).total_seconds()
         if delta > knox_settings.MIN_REFRESH_INTERVAL:
-            auth_token.save(update_fields=('expiry',))
+            try:
+                auth_token.save(update_fields=('expiry',))
+            except DatabaseError:
+                # avoid race condition with concurrent logout calls
+                return False
+        return True

[johnraz]

Yes I see your point now. I still have a code smell around this. I’m even wondering if the authentication failed error makes sense in this case? You logged in but it failed to refresh... Maybe raising a 409 (conflict) would be better, we could do a get right before the save and trigger the exception if the get fails ? That would remove the database error check I don’t like and the error checking duplication you don’t like.

[johnraz]

@xrmx this is a very valid point, thanks for the valuable link as well which helped refresh my mind on this topic 👍

Another approach we could then take and which would probably make more sense would be to do a select for update when getting the token on login.

The rationale would be that as soon as a login is in progress, no change should be applied to a token. Basically, one can not change / destroy a token if someone is actually using it to log in but they should instead retry after the login has completed properly.

What do you think ?

[xrmx]

I think we are overthinking this a bit :) I think in this case taking locks will create more chances to mess things than what are preventing.

[johnraz]

Not sure, what do you have in mind that could make this worse?

[johnraz]

oh and I just wanted to add that I agree on the overthinking bit as well, this is the feeling I have too but I don't see how to solve it simply while being "correct". Maybe we should leave it as is 😝

[xrmx]

A worse scenario for me is code taking locks and not releasing them

[xrmx]

@johnraz Thanks for the review, it's based on master because develop is missing some 4.1.0 commits. Will add changelog.

[johnraz]

If you base your work on develop and create the PR against develop, missing commits from master should not be an issue. Those are only release related commits I believe.

Writing from my phone so I may be missing a point. Maybe some other contributors could have a look. Cheers.

[xrmx]

@johnraz yeah, but master commits should be merged back to develop or you'll have conflicts, e.g. like in this specific case in the CHANGELOG

[johnraz]

Yes agreed on the merge back to develop but please let’s do that in a separate PR, maybe @belugame can help, won’t have access to my computer soon (travelling right now).

[xrmx]

Updated PR:

• rebased on top of develop
• renamed tests and added some comments to help understand the scenario
• rewritten commit message to make it hopefully more understandable
• changed the continue to a break when handling the exception as we found the token

[johnraz]

I added the help wanted on this PR to get it rebased and merged.

@giovannicimolin if you feel like coding be my guest 😄