Rotate tokens every login

[mbakke]

Hello,

This PR ensures that API tokens are rotated every login.

Using a token within the expiry window will extend the expiry time so e.g. automated jobs can keep using the same token.

[coveralls]

Coverage increased (+0.09%) to 98.609% when pulling 0e9deb9 on mbakke:token-rotation into fca26db on unioslo:master.

[mbakke]

Errh, I did not try logging into an existing mreg installation after this change, and just blindly trusted the tests.

Logging in with a user that already has a token causes a crash:

psycopg2.errors.UniqueViolation: duplicate key value violates unique constraint "authtoken_token_user_id_key"                                                                                       
DETAIL:  Key (user_id)=(2) already exists.

Demoted to draft status until I find a solution.

[oyvindkolbu]

Errh, I did not try logging into an existing mreg installation after this change, and just blindly trusted the tests.

Possibly because it already contained a token from created by Token and not TokenExpire, therefore you need to
add a similar cleanup of Token here
https://github.com/unioslo/mreg/pull/442/files#diff-98101bc9229886064733bd64cbf050fde15778a4361b28382277fa40f295d3eaR30

The tests don't have stale data in them, so that's why it didn't catch it.

Could easily be provoked by adding a Token.object.create... before the post in test_token_rotation. Also check that it is gone afterwards. Probably better as a dedicated test.

[mbakke]

Could easily be provoked by adding a Token.object.create... before the post in test_token_rotation. Also check that it is gone afterwards. Probably better as a dedicated test.

Right, thanks for the hint. I had to override get_token_client to prevent ExpiringToken from being issued first and made a new test class:

diff --git a/mreg/api/v1/tests/tests.py b/mreg/api/v1/tests/tests.py
index 9790561..4140ebe 100644
--- a/mreg/api/v1/tests/tests.py
+++ b/mreg/api/v1/tests/tests.py
@@ -238,6 +238,39 @@ class APITokenAuthenticationTestCase(MregAPITestCase):
         self.assert_post_and_400("/api/token-auth/", {"who":"someone","why":"because"})
         self.assert_post_and_400("/api/token-auth/", {})
 
+
+from rest_framework.authtoken.models import Token
+class MigratingTokenTestCase(MregAPITestCase):
+    """ Ensure that an existing Token is replaced by ExpiringToken. """
+    def get_token_client(self, username=None, superuser=True, adminuser=False):
+        if username is None:
+            if superuser:
+                username = 'superuser'
+            elif adminuser:
+                username = 'adminuser'
+            else:
+                username = 'nobody'
+        self.user = get_user_model().objects.create_user(username=username,
+                                                         password="test")
+        self.user.groups.clear()
+        token, created = Token.objects.get_or_create(user=self.user)
+        if superuser:
+            self.add_user_to_groups('SUPERUSER_GROUP')
+        if adminuser:
+            self.add_user_to_groups('ADMINUSER_GROUP')
+        client = APIClient()
+        client.credentials(HTTP_AUTHORIZATION='Token ' + token.key)
+        return client
+
+    def test_conflicting_token(self):
+        self.assert_post_and_401("/api/token-auth/",
+                                 {"username": self.user,
+                                  "password":"test"})
+        self.assertFalse(Token.objects.get(user=self.user))
+
 class APIAutoupdateZonesTestCase(MregAPITestCase):
     """This class tests the autoupdate of zones' updated_at whenever
        various models are added/deleted/renamed/changed etc."""

...however the request never makes it to our view in the test and is 401'd somewhere in APIView AFAICT. When running the server normally, clearing the Token inside ObtainExpiringAuthToken works. Any idea what can cause this discrepancy?

[mbakke]

To clarify, this does work in practice:

diff --git a/mreg/api/views.py b/mreg/api/views.py
index adacb03..9833e66 100644
--- a/mreg/api/views.py
+++ b/mreg/api/views.py
@@ -9,6 +9,8 @@ from rest_framework import serializers
 from rest_framework.exceptions import AuthenticationFailed
 
 from mreg.models import ExpiringToken
+# Include the "vanilla" DRF Token in order to migrate existing installations.
+from rest_framework.authtoken.models import Token
 
 
 class ObtainExpiringAuthToken(ObtainAuthToken):
@@ -26,6 +28,9 @@ class ObtainExpiringAuthToken(ObtainAuthToken):
 
         user = serializer.validated_data['user']
 
+        # Delete any "old-style" DRF Token to prevent a conflict.
+        Token.objects.filter(user=user).delete()
+
         # Force token rotation.
         ExpiringToken.objects.filter(user=user).delete()
 

...but I haven't been able to write a test for it.

I wonder if we can take a "shotgun" approach and just clear all tokens at server startup?

[oyvindkolbu]

To clarify, this does work in practice:

diff --git a/mreg/api/views.py b/mreg/api/views.py
index adacb03..9833e66 100644
--- a/mreg/api/views.py
+++ b/mreg/api/views.py
@@ -9,6 +9,8 @@ from rest_framework import serializers
 from rest_framework.exceptions import AuthenticationFailed
 
 from mreg.models import ExpiringToken
+# Include the "vanilla" DRF Token in order to migrate existing installations.
+from rest_framework.authtoken.models import Token
 
 
 class ObtainExpiringAuthToken(ObtainAuthToken):
@@ -26,6 +28,9 @@ class ObtainExpiringAuthToken(ObtainAuthToken):
 
         user = serializer.validated_data['user']
 
+        # Delete any "old-style" DRF Token to prevent a conflict.
+        Token.objects.filter(user=user).delete()
+
         # Force token rotation.
         ExpiringToken.objects.filter(user=user).delete()
 

This makes sense.

I wonder if we can take a "shotgun" approach and just clear all tokens at server startup?

A crude but efficient solution. All old tokens will be invalidated on first usage anyhow, so
should be fine.

[mbakke]

I wonder if we can take a "shotgun" approach and just clear all tokens at server startup?

A crude but efficient solution. All old tokens will be invalidated on first usage anyhow, so
should be fine.

It turns out that doing database operations during initialization is unsafe and discouraged:

https://docs.djangoproject.com/en/dev/ref/applications/#django.apps.AppConfig.ready

Instead I opted to add a management command to delete all tokens. It should be run before starting the server, and only needs to be done once. I also added it to the Docker image and will remove once its consumers are updated.

Oh, and there is a new Admin view to inspect the new ExpiringToken. The old Token view still works, but does not show last_used.