chore(backend): force token expire #969

thbcmlowk · 2024-08-05T14:24:05Z

Description

Part of FL-1656

Disable never expiring BearerToken

Companion

Substra/substra-frontend#385

How has this been tested?

On dev by:

Creating expiring tokens and one never-expiring
Applying the migration code
Check db results

Creating tokens

 select * from users_bearertoken;

            created            |     note      |         expires_at         |                  id                  | user_id 
-------------------------------+---------------+----------------------------+--------------------------------------+---------
 2024-08-27 12:47:09.093701+00 | expires-60    | 2024-10-26 12:47:08.395+00 | 3a07a145-9ecd-46e8-aa00-35a9a3d2ebf2 |       4
 2024-08-27 12:47:16.34687+00  | never-expires |                            | 456efb44-4f30-49f8-bc12-46a673da9f90 |       4

Applying migration

I have no name!@substra-backend-substra-backend-server-5674697f78-ds9nk:/usr/src/app$ ./manage.py migrate users
Operations to perform:
  Apply all migrations: users
Running migrations:
  Applying users.0008_alter_bearertoken_expires_at... OK

            created            |     note      |          expires_at           |                  id                  | user_id 
-------------------------------+---------------+-------------------------------+--------------------------------------+---------
 2024-08-27 12:47:09.093701+00 | expires-60    | 2024-10-26 12:47:08.395+00    | 3a07a145-9ecd-46e8-aa00-35a9a3d2ebf2 |       4
 2024-08-27 12:47:16.34687+00  | never-expires | 2024-08-27 12:50:24.131665+00 | 456efb44-4f30-49f8-bc12-46a673da9f90 |       4

Checklist

changelog was updated with notable changes
documentation was updated

linear · 2024-08-05T14:24:28Z

FL-1656 Investigate token lifetime

thbcmlowk · 2024-08-06T16:37:23Z

/e2e --tests=substrafl,mnist,camelyon,frontend --refs=substra-frontend=chore/disable-never-expiring-token

Owlfred · 2024-08-06T16:37:26Z

End to end tests: ✔️ SUCCESS

SdgJlbl

One question, otherwise LGTM.
I don't know enough about the OIDC implementation to know if it will work without issues there.
And we have to keep in mind at the next upgrade that all tokens will be revoked.

SdgJlbl · 2024-08-14T14:45:39Z

backend/users/migrations/0008_alter_bearertoken_expires_at.py

+        migrations.AlterField(
+            model_name="bearertoken",
+            name="expires_at",
+            field=models.DateTimeField(default=django.utils.timezone.now),


By default, all existing tokens will expire immediately at the time of the migration. Is that what you want?

I think the idea was rather to set an immediate expiration date for token that does not have one

Updated, thank you

ThibaultFy · 2024-08-14T14:58:39Z

backend/users/models/token.py

@@ -10,7 +10,7 @@

 class BearerToken(Token):
    note = models.TextField(null=True)
-    expires_at = models.DateTimeField(null=True)
+    expires_at = models.DateTimeField(null=False, default=timezone.now)


I don't understand, does that mean that by default it expires instantly ?

Thanks for asking, I am not sure I remember the rational behind this particular change.

It may have been that all already existing tokens are revoked

Do we really want a default value or do we want to force the user to provide it?

What it means is that if no value is provided, the token will be created with instant expire. Note that the fronted will disable the possibility to create token with a null expires_at field. One other option is to remove the default ; meaning that creating a token with a null expires_at field will raise the following error:

django.db.utils.IntegrityError: null value in column "expires_at" of relation "users_bearertoken" violates not-null constraint

Not sure of what would be the best behavior, but happy to rework this if we think it's worth.

Do we really want a default value or do we want to force the user to provide it

Did not refresh, sorry. Obviously better if we force the user to provide it indeed. Is there a clean way to do so?

How did you get this IntegrityError ? The serializer should have returned an error when receiving an empty field for expires_at (and if I recall correctly, DRF does not emit DB django error)

I have the error when running tests that rely on instantiating tokens with null expires_at (eg here) and setting either:

required=True in the BearerTokenSerializer

blank=False in the BearerToken model

thbcmlowk · 2024-08-27T14:45:10Z

/e2e --tests=sdk,substrafl,mnist,camelyon,frontend

Owlfred · 2024-08-27T14:45:14Z

End to end tests: ✔️ SUCCESS

backend/users/serializers/token.py

guilhem-barthes · 2024-08-29T07:09:12Z

backend/api/tests/views/test_views_token.py

+    response = authenticated_client.post(url, payload)
+
+    assert response.json() == {"expires_at": ["This field is required."]}
+    assert response.status_code == status.HTTP_400_BAD_REQUEST


guilhem-barthes

Thanks for your work!

Signed-off-by: Thibault Camalon <135698225+thbcmlowk@users.noreply.github.com>

guilhem-barthes · 2024-08-30T07:44:54Z

/e2e --tests=sdk,substrafl,mnist,camelyon,frontend

Owlfred · 2024-08-30T07:44:59Z

End to end tests: ❌ FAILURE

Jobs status:

Camelyon / camelyon,frontend: ❌
Dispatch Jobs: ✔️
Documentation: ⏭️
MNIST / mnist,frontend: ❌
SubstraFL / substrafl,frontend: ❌
SubstraSDK / sdk: ❌

“You shall not pass!” ― Gandalf, The Lord of the Rings, The Fellowship of the Ring

thbcmlowk force-pushed the chore/disable-never-expiring-token branch from e592fc6 to e37576b Compare August 6, 2024 14:10

thbcmlowk mentioned this pull request Aug 6, 2024

chore(token): disable never expiring tokens Substra/substra-frontend#385

Merged

thbcmlowk force-pushed the chore/disable-never-expiring-token branch from 5722f0c to 2489bd4 Compare August 14, 2024 13:43

SdgJlbl approved these changes Aug 14, 2024

View reviewed changes

ThibaultFy reviewed Aug 14, 2024

View reviewed changes

thbcmlowk force-pushed the chore/disable-never-expiring-token branch from 2489bd4 to d987c1c Compare August 27, 2024 12:55

thbcmlowk marked this pull request as ready for review August 27, 2024 13:04

thbcmlowk requested a review from a team as a code owner August 27, 2024 13:04

thbcmlowk marked this pull request as draft August 27, 2024 13:20

github-actions bot added the api label Aug 27, 2024

guilhem-barthes reviewed Aug 28, 2024

View reviewed changes

backend/users/serializers/token.py Show resolved Hide resolved

thbcmlowk marked this pull request as ready for review August 28, 2024 08:45

guilhem-barthes reviewed Aug 29, 2024

View reviewed changes

guilhem-barthes approved these changes Aug 29, 2024

View reviewed changes

thbcmlowk added 9 commits August 29, 2024 09:51

chore(token): token always expires

e728aaf

Signed-off-by: Thibault Camalon <135698225+thbcmlowk@users.noreply.github.com>

chore(migrations): alter bearer token expires at

bd6e62e

Signed-off-by: Thibault Camalon <135698225+thbcmlowk@users.noreply.github.com>

chore(changelog): update changelog

c9bbf3d

Signed-off-by: Thibault Camalon <135698225+thbcmlowk@users.noreply.github.com>

chore(backend/users/migrations): alter bearertoken expires only if null

17e5eee

Signed-off-by: Thibault Camalon <135698225+thbcmlowk@users.noreply.github.com>

fix(users/migrations): chain AlterField migration operation

c386297

Signed-off-by: Thibault Camalon <135698225+thbcmlowk@users.noreply.github.com>

tests(views/test_views_token): add expiring date and test case

acf44c2

Signed-off-by: Thibault Camalon <135698225+thbcmlowk@users.noreply.github.com>

chore(users/migrations): field to models.DateTimeField()

1fe70f4

Signed-off-by: Thibault Camalon <135698225+thbcmlowk@users.noreply.github.com>

chore(users/models/token): blank=False instead of default value

ad4ef44

Signed-off-by: Thibault Camalon <135698225+thbcmlowk@users.noreply.github.com>

tests(test_views_token): add test cases

716c064

Signed-off-by: Thibault Camalon <135698225+thbcmlowk@users.noreply.github.com>

thbcmlowk force-pushed the chore/disable-never-expiring-token branch from 40b7d8d to 716c064 Compare August 29, 2024 07:51

thbcmlowk merged commit 7b1c1fa into main Aug 29, 2024
8 checks passed

thbcmlowk deleted the chore/disable-never-expiring-token branch August 29, 2024 08:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(backend): force token expire #969

chore(backend): force token expire #969

thbcmlowk commented Aug 5, 2024 •

edited

Loading

linear bot commented Aug 5, 2024

thbcmlowk commented Aug 6, 2024

Owlfred commented Aug 6, 2024 •

edited

Loading

SdgJlbl left a comment

SdgJlbl Aug 14, 2024

thbcmlowk Aug 14, 2024

thbcmlowk Aug 27, 2024

ThibaultFy Aug 14, 2024

thbcmlowk Aug 14, 2024

guilhem-barthes Aug 27, 2024

guilhem-barthes Aug 27, 2024

thbcmlowk Aug 27, 2024

thbcmlowk Aug 27, 2024

guilhem-barthes Aug 28, 2024 •

edited

Loading

thbcmlowk Aug 28, 2024 •

edited

Loading

thbcmlowk commented Aug 27, 2024

Owlfred commented Aug 27, 2024 •

edited

Loading

guilhem-barthes Aug 29, 2024

guilhem-barthes left a comment

guilhem-barthes commented Aug 30, 2024

Owlfred commented Aug 30, 2024 •

edited

Loading

chore(backend): force token expire #969

chore(backend): force token expire #969

Conversation

thbcmlowk commented Aug 5, 2024 • edited Loading

Description

Companion

How has this been tested?

Creating tokens

Applying migration

Checklist

linear bot commented Aug 5, 2024

thbcmlowk commented Aug 6, 2024

Owlfred commented Aug 6, 2024 • edited Loading

SdgJlbl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guilhem-barthes Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

thbcmlowk Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

thbcmlowk commented Aug 27, 2024

Owlfred commented Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

guilhem-barthes left a comment

Choose a reason for hiding this comment

guilhem-barthes commented Aug 30, 2024

Owlfred commented Aug 30, 2024 • edited Loading

thbcmlowk commented Aug 5, 2024 •

edited

Loading

Owlfred commented Aug 6, 2024 •

edited

Loading

guilhem-barthes Aug 28, 2024 •

edited

Loading

thbcmlowk Aug 28, 2024 •

edited

Loading

Owlfred commented Aug 27, 2024 •

edited

Loading

Owlfred commented Aug 30, 2024 •

edited

Loading