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.

Sign up for GitHub

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

Jump to bottom

feat!: add support for multiple API tokens with expanded functionality #639

Merged

IC-1101asterisk merged 5 commits into main from feat-multi-token

May 30, 2023

Contributor

IC-1101asterisk commented Apr 24, 2023 •

edited

Loading

Description

https://app.asana.com/0/1204424791229302/1204269294564790

New BearerToken model:

has note attribute
has token_id attribute
has expiry attribute
allows for multiple tokens per user

active-api-tokens received a new delete method
api-token-auth changed, now you can specify an expiry date and a note, no expiry date means an imperishable token

How has this been tested?

Checklist

changelog was updated with notable changes
documentation was updated

TODO

add tests
add expiration in authentication backend
remove the reuse TOKEN_STRATEGY

IC-1101asterisk force-pushed the feat-multi-token branch from 0dfa4b3 to 868b14f Compare

April 24, 2023 14:05

oleobal reviewed

View reviewed changes

backend/backend/views.py Outdated Show resolved Hide resolved

IC-1101asterisk force-pushed the feat-multi-token branch from 868b14f to 9aab3dd Compare

April 24, 2023 14:24

oleobal reviewed

View reviewed changes

backend/backend/views.py Outdated Show resolved Hide resolved

oleobal reviewed

View reviewed changes

backend/backend/views.py Outdated Show resolved Hide resolved

oleobal reviewed

View reviewed changes

backend/users/models/token.py Outdated Show resolved Hide resolved

oleobal reviewed

View reviewed changes

backend/users/models/token.py Outdated Show resolved Hide resolved

IC-1101asterisk force-pushed the feat-multi-token branch 3 times, most recently from 202b3d4 to ff4b57c Compare

April 25, 2023 07:43

guilhem-barthes reviewed

View reviewed changes

Contributor

guilhem-barthes left a comment

Thanks for your work, I think some part of this code can be optimized!

backend/users/models/token.py Outdated Show resolved Hide resolved

backend/backend/views.py Outdated Show resolved Hide resolved

backend/backend/views.py Outdated Show resolved Hide resolved

backend/backend/views.py Outdated Show resolved Hide resolved

backend/backend/views.py Outdated Show resolved Hide resolved

backend/backend/views.py Outdated Show resolved Hide resolved

backend/users/utils/bearer_token.py Outdated Show resolved Hide resolved

github-actions bot added the api label

IC-1101asterisk force-pushed the feat-multi-token branch 7 times, most recently from a241594 to aa6ba6e Compare

April 27, 2023 10:04

IC-1101asterisk changed the title ~~feat!: multi token~~ feat!: add support for multiple API tokens with expended functionality

IC-1101asterisk changed the title ~~feat!: add support for multiple API tokens with expended functionality~~ feat!: add support for multiple API tokens with expanded functionality

guilhem-barthes reviewed

View reviewed changes

Contributor

guilhem-barthes left a comment

Thanks for the changes!

Although not entirely linked with this PR, I would mention that the file organization seems a little messy (test in one Django application, views in another Django app and lastly tests in a third one).

Following discussions on another medium to separate the different tokens (SDK and Frontend), if you decide having different tables for the different kinds of tokens and they are both using the same (or a similar) model I would recommend looking in abstract classes

backend/backend/views.py Outdated Show resolved Hide resolved

backend/backend/views.py Outdated Show resolved Hide resolved

backend/backend/views.py Outdated Show resolved Hide resolved

github-actions bot added the documentation label

IC-1101asterisk force-pushed the feat-multi-token branch 6 times, most recently from 3dfdde1 to 1ddd19e Compare

April 28, 2023 15:10

IC-1101asterisk mentioned this pull request

feat: token management page Substra/substra-frontend#198

Merged

IC-1101asterisk force-pushed the feat-multi-token branch 2 times, most recently from 6295db2 to 9cf85ca Compare

May 16, 2023 08:37

IC-1101asterisk marked this pull request as ready for review

May 16, 2023 08:37

IC-1101asterisk requested a review from a team as a code owner

May 16, 2023 08:37

IC-1101asterisk and others added 4 commits

May 16, 2023 10:38


          feat: multi token working

2a5c761

Signed-off-by: Léo-Paul HAUET <[email protected]>


          feat!: tokens can be deleted

8f78ac7

Signed-off-by: Léo-Paul HAUET <[email protected]>


          feat!: working tests

b6d9678

Signed-off-by: Léo-Paul HAUET <[email protected]>


          feat: add implicit token to replicate old behavior

f211071

Signed-off-by: Léo-Paul HAUET <[email protected]>

IC-1101asterisk force-pushed the feat-multi-token branch from 9cf85ca to 6a08f72 Compare

May 16, 2023 08:40

IC-1101asterisk closed this

IC-1101asterisk reopened this

guilhem-barthes reviewed

View reviewed changes

Contributor

guilhem-barthes left a comment

Thanks for all the changes, it looks good, although I think some parts still needs small refinements.

backend/api/tests/views/test_views_authentication.py Outdated Show resolved Hide resolved

backend/users/models/token.py Outdated Show resolved Hide resolved

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

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

docs/authentication.md Outdated Show resolved Hide resolved

backend/api/tests/views/test_views_token.py Outdated Show resolved Hide resolved

backend/backend/views.py Outdated Show resolved Hide resolved

backend/users/models/token.py Outdated Show resolved Hide resolved

backend/users/models/token.py Outdated Show resolved Hide resolved

backend/users/models/token.py Outdated Show resolved Hide resolved

IC-1101asterisk force-pushed the feat-multi-token branch 2 times, most recently from 57f4f49 to 7b7db22 Compare

May 17, 2023 14:15

IC-1101asterisk requested review from oleobal and guilhem-barthes

May 22, 2023 09:04

guilhem-barthes approved these changes

View reviewed changes

Contributor

guilhem-barthes left a comment

Thanks for your work! I just noticed a comment that seems to be not meant to be left in the code

backend/api/tests/views/test_views_token.py Outdated

+                  valid_auth_token_header = f"Token {token_2}"
+                  api_client.credentials(HTTP_AUTHORIZATION=valid_auth_token_header)
+                  response = api_client.get("/active-api-tokens/")  # none type error here ??

Contributor

guilhem-barthes May 29, 2023

This comment looks temporary


          feat!: working serializers

bfc373e

Signed-off-by: Léo-Paul HAUET <[email protected]>

IC-1101asterisk force-pushed the feat-multi-token branch from 7b7db22 to bfc373e Compare

May 30, 2023 08:43

IC-1101asterisk merged commit ef1d3ca into main

IC-1101asterisk deleted the feat-multi-token branch

May 30, 2023 09:52

oleobal mentioned this pull request

fix: concurrent issuance of implicit bearer tokens #659

Merged

2 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api documentation