feat: improve ImplicitBearerToken

CHANGELOG.md

@@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 - New `SECRET_KEY` optional environment variable ([#671](https://github.com/Substra/substra-backend/pull/671))
+- `/api-token-auth/` and the associated tokens can now be disabled through the `EXPIRY_TOKEN_ENABLED` environment variable and `server.allowImplicitLogin` chart value ([#698](https://github.com/Substra/substra-backend/pull/698))
+- Tokens issued by `/api-token-auth/` can now be deleted like other API tokens, through a `DELETE` request on the `/active-api-tokens` endpoint ([#698](https://github.com/Substra/substra-backend/pull/698))
 
 ### Changed
 
@@ -20,6 +22,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - BREAKING: `SECRET_KEY_PATH` and `SECRET_KEY_LOAD_AND_STORE` environment variables ([#671](https://github.com/Substra/substra-backend/pull/671))
 - Removed logic for storing `SECRET_KEY` at startup, in order to increase stability; it should be done at a higher level, i.e. the chart ([#671](https://github.com/Substra/substra-backend/pull/671))
 
+## Fixed
+- `/api-token-auth/` sometimes handing out tokens that are about to expire ([#698](https://github.com/Substra/substra-backend/pull/698))
+
 ## [0.40.0](https://github.com/Substra/substra-backend/releases/tag/0.40.0) 2023-07-25
 
 ### Fixed

backend/api/tests/views/test_views_authentication.py

@@ -113,41 +113,61 @@ def test_obtain_token(self):
         endpoint = "/api-token-auth/"
         # clean use
         response = self.client.post(endpoint, {"username": "foo", "password": "baz"})
-        self.assertEqual(response.status_code, 400)
+        self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
 
         response = self.client.post(endpoint, {"username": "foo", "password": "bar"})
-        self.assertEqual(response.status_code, 200)
-        token_old = response.json()["token"]
-        self.assertTrue(token_old)
+        self.assertEqual(response.status_code, status.HTTP_200_OK)
+        token_old = response.json()
+        self.assertTrue(token_old["token"])
 
-        # token should be updated after a second post
         response = self.client.post(endpoint, {"username": "foo", "password": "bar"})
-        self.assertEqual(response.status_code, 200)
-        token = response.json()["token"]
-        self.assertTrue(token)
+        self.assertEqual(response.status_code, status.HTTP_200_OK)
+        token_new = response.json()
+        self.assertTrue(token_new["token"])
 
-        # tokens should be the same
-        self.assertEqual(token_old, token)
+        # tokens shouldn't be the same
+        self.assertNotEqual(token_old["token"], token_new["token"])
 
-        # token count should still be 1
-        tokens_count = ImplicitBearerToken.objects.count()
-        self.assertEqual(tokens_count, 1)
+        def _count_tokens(target):
+            tokens_count = ImplicitBearerToken.objects.count()
+            self.assertEqual(tokens_count, target)
 
-        # test tokens validity
+            # they are reported on the active-api-tokens enpoint
 
-        valid_auth_token_header = f"Token {token}"
-        self.client.credentials(HTTP_AUTHORIZATION=valid_auth_token_header)
+            response = self.client.get("/active-api-tokens/")
+            self.assertEqual(response.status_code, status.HTTP_200_OK)
+            reported_tokens = response.json()["implicit_tokens"]
+            self.assertEqual(len(reported_tokens), target)
 
-        with mock.patch("api.views.utils.get_owner", return_value="foo"):
-            response = self.client.get(self.function_url)
-            self.assertEqual(status.HTTP_200_OK, response.status_code)
+        def _use_token(token, target_code):
+            self.client.credentials(HTTP_AUTHORIZATION=f"Token {token['token']}")
+
+            with mock.patch("api.views.utils.get_owner", return_value="foo"):
+                response = self.client.get(self.function_url)
+                self.assertEqual(response.status_code, target_code)
+
+        self.client.credentials(HTTP_AUTHORIZATION=f"Token {token_new['token']}")
+        _count_tokens(2)
+
+        # test tokens work
+        _use_token(token_old, status.HTTP_200_OK)
+        _use_token(token_new, status.HTTP_200_OK)
+
+        # delete token
+        response = self.client.delete(f"/active-api-tokens/?id={token_old['id']}")
+        self.assertEqual(response.status_code, status.HTTP_200_OK)
+
+        _count_tokens(1)
+
+        # deleted token doesn't work anymore
+        _use_token(token_new, status.HTTP_200_OK)
+        _use_token(token_old, status.HTTP_401_UNAUTHORIZED)
 
         # usage with an existing token
         # the token should be ignored since the purpose of the view is to authenticate via user/password
-        valid_auth_token_header = f"Token {token}"
-        self.client.credentials(HTTP_AUTHORIZATION=valid_auth_token_header)
+        self.client.credentials(HTTP_AUTHORIZATION=f"Token {token_new['token']}")
         response = self.client.post(endpoint, {"username": "foo", "password": "bar"})
-        self.assertEqual(response.status_code, 200)
+        self.assertEqual(response.status_code, status.HTTP_200_OK)
 
 
 @override_settings(MEDIA_ROOT=MEDIA_ROOT)

backend/api/views/exception_handler.py

@@ -9,8 +9,7 @@
 logger = structlog.get_logger(__name__)
 
 
-def api_exception_handler(exc, context):
-    """API Exception handler."""
+def _get_response_for_exception(exc, context):
     if isinstance(exc, orchestrator.error.OrcError):
         return response.Response({"detail": exc.details}, status=exc.http_status)
 
@@ -23,3 +22,13 @@ def api_exception_handler(exc, context):
         return exc.response()
 
     return views.exception_handler(exc, context)
+
+
+def api_exception_handler(exc, context):
+    """API Exception handler."""
+
+    response = _get_response_for_exception(exc, context)
+    if hasattr(exc, "substra_identifier"):
+        response.data["substra_identifier"] = exc.substra_identifier
+
+    return response

backend/backend/exceptions.py

@@ -0,0 +1,7 @@
+from django.utils.translation import gettext_lazy as _
+from rest_framework.exceptions import PermissionDenied
+
+
+class ImplicitLoginDisabled(PermissionDenied):
+    default_detail = _("Implicit login is disabled on this server")
+    substra_identifier = "implicit_login_disabled"

backend/backend/settings/common.py

@@ -13,7 +13,6 @@
 import json
 import os
 import secrets
-from datetime import timedelta
 
 import structlog
 from django.core.files.storage import FileSystemStorage
@@ -242,7 +241,6 @@
 # Uploaded file max size, in bytes
 DATA_UPLOAD_MAX_SIZE = int(os.environ.get("DATA_UPLOAD_MAX_SIZE", 1024 * 1024 * 1024))  # bytes
 
-EXPIRY_TOKEN_LIFETIME = timedelta(minutes=int(os.environ.get("EXPIRY_TOKEN_LIFETIME", 24 * 60)))  # minutes
 
 GZIP_MODELS = to_bool(os.environ.get("GZIP_MODELS", False))
 

backend/backend/settings/deps/restframework.py

@@ -1,4 +1,7 @@
 import os
+from datetime import timedelta
+
+from .utils import to_bool
 
 REST_FRAMEWORK = {
     "TEST_REQUEST_DEFAULT_FORMAT": "json",
@@ -8,8 +11,6 @@
     ),
     "DEFAULT_AUTHENTICATION_CLASSES": [
         "users.authentication.SecureJWTAuthentication",  # JWT for front
-        "users.authentication.ImplicitBearerTokenAuthentication",  # Legacy Bearer token for api-token-auth/
-        # must be loaded BEFORE BearerTokenAuthentication
         "users.authentication.BearerTokenAuthentication",  # Bearer token for SDK
     ],
     "DEFAULT_PERMISSION_CLASSES": [
@@ -38,3 +39,12 @@
 SPECTACULAR_SETTINGS = {
     "SERVE_PERMISSIONS": ["libs.permissions.IsAuthorized"],
 }
+
+EXPIRY_TOKEN_LIFETIME = timedelta(minutes=int(os.environ.get("EXPIRY_TOKEN_LIFETIME", 24 * 60)))  # minutes
+EXPIRY_TOKEN_ENABLED = to_bool(os.environ.get("EXPIRY_TOKEN_ENABLED", True))
+if EXPIRY_TOKEN_ENABLED:
+    REST_FRAMEWORK["DEFAULT_AUTHENTICATION_CLASSES"] = [
+        "users.authentication.ImplicitBearerTokenAuthentication"
+    ] + REST_FRAMEWORK["DEFAULT_AUTHENTICATION_CLASSES"]
+    # Legacy Bearer token for api-token-auth/
+    # must be loaded BEFORE BearerTokenAuthentication

backend/backend/views.py

@@ -1,6 +1,4 @@
 from django.conf import settings
-from django.core.exceptions import ObjectDoesNotExist
-from django.db import transaction
 from django.urls import reverse
 from rest_framework import status
 from rest_framework.authtoken.views import ObtainAuthToken as DRFObtainAuthToken
@@ -16,9 +14,12 @@
 from substrapp.utils import get_owner
 from users.models.token import BearerToken
 from users.models.token import ImplicitBearerToken
+from users.models.token import get_implicit_bearer_token
 from users.serializers.token import BearerTokenSerializer
 from users.serializers.token import ImplicitBearerTokenSerializer
 
+from .exceptions import ImplicitLoginDisabled
+
 
 class ObtainBearerToken(DRFObtainAuthToken):
     """
@@ -30,15 +31,15 @@ class ObtainBearerToken(DRFObtainAuthToken):
     throttle_classes = [AnonRateThrottle, UserLoginThrottle]
 
     def post(self, request, *args, **kwargs):
+        if not settings.EXPIRY_TOKEN_ENABLED:
+            raise ImplicitLoginDisabled()
+
         serializer = self.serializer_class(data=request.data, context={"request": request})
         serializer.is_valid(raise_exception=True)
         user = serializer.validated_data["user"]
 
-        with transaction.atomic():
-            token, just_created = ImplicitBearerToken.objects.get_or_create(user=user)
-            if not just_created:
-                token = token.handle_expiration()
-            return ApiResponse(ImplicitBearerTokenSerializer(token, include_payload=True).data)
+        token = get_implicit_bearer_token(user)
+        return ApiResponse(ImplicitBearerTokenSerializer(token, include_payload=True).data)
 
 
 class AuthenticatedBearerToken(DRFObtainAuthToken):
@@ -67,26 +68,23 @@ def get(self, request, *args, **kwargs):
             BearerTokenSerializer(token).data
             for token in BearerToken.objects.filter(user=request.user).order_by("-created")
         ]
-        try:
-            implicit_token = ImplicitBearerTokenSerializer(ImplicitBearerToken.objects.get(user=request.user)).data
-
-        except ObjectDoesNotExist:
-            implicit_token = None
+        implicit_tokens = [
+            ImplicitBearerTokenSerializer(token).data
+            for token in ImplicitBearerToken.objects.filter(user=request.user).order_by("-created")
+        ]
         return ApiResponse(
             {
                 "tokens": tokens,
-                "implicit_token": implicit_token,
+                "implicit_tokens": implicit_tokens,
             }
         )
 
     def delete(self, request, *args, **kwargs):
-        try:
-            token = BearerToken.objects.get(id=request.GET.get("id"))
-            if request.user == token.user:
-                token.delete()
+        for model in [BearerToken, ImplicitBearerToken]:
+            tokens = model.objects.filter(id=request.GET.get("id"))
+            if len(tokens) == 1 and request.user == tokens[0].user:
+                tokens[0].delete()
                 return ApiResponse(data={"detail": "Token removed"}, status=status.HTTP_200_OK)
-        except BearerToken.ObjectDoesNotExist or BearerToken.MultipleObjectsReturned:
-            pass
         return ApiResponse(data={"detail": "Token not found"}, status=status.HTTP_404_NOT_FOUND)
 
 

backend/users/authentication.py

@@ -31,7 +31,7 @@ class BearerTokenAuthentication(DRFTokenAuthentication):
     def authenticate_credentials(self, key):
         _, token = super().authenticate_credentials(key)
         if token.is_expired:
-            raise AuthenticationFailed("The Token is expired")
+            raise AuthenticationFailed("The token is expired")
 
         check_oidc_user_is_valid(token.user)
         return token.user, token
@@ -50,7 +50,7 @@ def authenticate_credentials(self, key):
         except AuthenticationFailed:
             return None  # allow the authentication process to continue by swallowing the exception raised
         if token.is_expired:
-            raise AuthenticationFailed("The Token is expired")
+            raise AuthenticationFailed("The token is expired")
 
         check_oidc_user_is_valid(token.user)
         return token.user, token

backend/users/migrations/0007_implicitbearertoken_id_and_more.py

@@ -0,0 +1,32 @@
+# Generated by Django 4.2.1 on 2023-08-08 09:38
+
+import uuid
+
+import django.db.models.deletion
+from django.conf import settings
+from django.db import migrations
+from django.db import models
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        migrations.swappable_dependency(settings.AUTH_USER_MODEL),
+        ("users", "0006_alter_bearertoken_user"),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name="implicitbearertoken",
+            name="id",
+            field=models.UUIDField(default=uuid.uuid4, editable=False),
+        ),
+        migrations.AlterField(
+            model_name="implicitbearertoken",
+            name="user",
+            field=models.ForeignKey(
+                on_delete=django.db.models.deletion.CASCADE,
+                related_name="implicit_bearer_tokens",
+                to=settings.AUTH_USER_MODEL,
+            ),
+        ),
+    ]

backend/users/models/token.py

@@ -23,9 +23,13 @@ def is_expired(self) -> bool:
 
 class ImplicitBearerToken(Token):
     """
-    Legacy token to make the endpoint api-token-auth/ work like it used to
+    Separate from frontend-visible BearerTokens,
+    so the behavior of /api-token-auth/ stays the same
     """
 
+    user = models.ForeignKey(settings.AUTH_USER_MODEL, related_name="implicit_bearer_tokens", on_delete=models.CASCADE)
+    id = models.UUIDField(default=uuid.uuid4, editable=False)
+
     @property
     def expires_at(self) -> datetime:
         return self.created + settings.EXPIRY_TOKEN_LIFETIME
@@ -34,14 +38,17 @@ def expires_at(self) -> datetime:
     def is_expired(self) -> bool:
         return self.expires_at < timezone.now()
 
-    @transaction.atomic
-    def handle_expiration(self) -> "ImplicitBearerToken":
-        """
-        If token is expired a new token will be created and the old one removed.
-        """
-        if self.is_expired:
-            user = self.user
-            self.delete()
-            token = ImplicitBearerToken.objects.create(user=user)
-            return token
-        return self
+
+@transaction.atomic
+def get_implicit_bearer_token(user) -> ImplicitBearerToken:
+    """
+    clean up expired tokens
+    """
+    tokens = ImplicitBearerToken.objects.filter(user=user)
+    to_delete = []
+    for token in tokens:
+        if token.is_expired:
+            to_delete.append(token)
+    for token in to_delete:
+        token.delete()
+    return ImplicitBearerToken.objects.create(user=user)

backend/users/serializers/token.py

@@ -29,7 +29,7 @@ class ImplicitBearerTokenSerializer(serializers.ModelSerializer):
 
     class Meta:
         model = ImplicitBearerToken
-        fields = ["expires_at", "created_at", "token"]
+        fields = ["id", "expires_at", "created_at", "token"]
 
     def __init__(self, *args, **kwargs):
         include_payload = kwargs.pop("include_payload", False)

charts/substra-backend/CHANGELOG.md

@@ -1,5 +1,11 @@
 # Changelog
 
+## [22.8.0] - 2023-08-16
+
+## Added
+
+- New `server.allowImplicitLogin` field, controlling whether "implicit login" (`Client.login` in the Substra SDK) is enabled
+
 ## [22.7.1] - 2023-08-16
 
 ### Changed

charts/substra-backend/Chart.yaml

@@ -1,7 +1,7 @@
 apiVersion: v2
 name: substra-backend
 home: https://github.com/Substra
-version: 22.7.1
+version: 22.8.0
 appVersion: 0.40.0
 kubeVersion: ">= 1.19.0-0"
 description: Main package for Substra

charts/substra-backend/README.md

@@ -45,6 +45,7 @@ See [UPGRADE.md](https://github.com/Substra/substra-backend/blob/main/charts/sub
 | `server.commonHostDomain`                         | The common host under which the backend and frontend are served                                                                                    | `""`                                       |
 | `server.uwsgiProcesses`                           | The number of uwsgi processes                                                                                                                      | `20`                                       |
 | `server.uwsgiThreads`                             | The number of uwsgi threads                                                                                                                        | `10`                                       |
+| `server.allowImplicitLogin`                       | Allow clients to get API tokens directly with username+password on the `/api-token-auth/` endpoint (ie `Client.login` in the Substra SDK)          | `true`                                     |
 | `server.image.registry`                           | Substra backend server image registry                                                                                                              | `ghcr.io`                                  |
 | `server.image.repository`                         | Substra backend server image repository                                                                                                            | `substra/substra-backend`                  |
 | `server.image.tag`                                | Substra backend server image tag (defaults to AppVersion)                                                                                          | `nil`                                      |

charts/substra-backend/templates/configmap-settings.yaml

@@ -24,6 +24,8 @@ data:
   WORKER_REPLICA_SET_NAME: {{ template "substra.fullname" . }}-worker
   ENABLE_DATASAMPLE_STORAGE_IN_SERVERMEDIAS: {{ .Values.DataSampleStorageInServerMedia | quote }}
 
+  EXPIRY_TOKEN_ENABLED: {{ .Values.server.allowImplicitLogin | quote }}
+
   {{- range $key, $value := default dict .Values.config }}
   {{ $key }}: {{ $value | quote }}
   {{- end }}