refactor: define provider based on auth backends #4539

frascuchon · 2024-01-26T13:50:30Z

Description

This PR is a refactor of the authentication provider. A new authentication provider is created based on the Starlette Authentication Backends. This change simplifies the auth. workflow and prepare incoming oauth integration.

Since the authentication backends should be used by the auth middleware component, the implementation done in this PR does not use it in that way since there are some problems exposing the db session at the middleware level (using global dependencies does not work).

Type of change

(Please delete options that are not relevant. Remember to title the PR according to the type of change)

New feature (non-breaking change which adds functionality)
Refactor (change restructuring the codebase without changing functionality)
Improvement (change adding some improvement to an existing functionality)

How Has This Been Tested

(Please describe the tests that you ran to verify your changes. And ideally, reference tests)

Running locally

Test A
Test B

Checklist

I added relevant documentation
I followed the style guidelines of this project
I did a self-review of my code
I made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
I filled out the contributor form (see text above)
I have added relevant notes to the CHANGELOG.md file (See https://keepachangelog.com/)

for more information, see https://pre-commit.ci

github-actions · 2024-01-26T15:11:32Z

The URL of the deployed environment for this PR is https://argilla-quickstart-pr-4539-ki24f765kq-no.a.run.app

jfcalvo · 2024-01-26T14:30:56Z

src/argilla/server/security/settings.py

+        Encryption algorithm for token data
+
+    token_expiration_in_minutes:
+        The session token expiration in minutes. Default=30000


Suggested change

The session token expiration in minutes. Default=30000

The session token expiration in minutes. Default=15

This is an old comment. I will update it.

jfcalvo · 2024-01-26T14:37:29Z

src/argilla/server/security/authentication/db/api_key_backend.py

+        api_key: str = await self.scheme(request)
+
+        if not api_key:
+            return None


Suggested change

api_key: str = await self.scheme(request)

if not api_key:

return None

api_key: str = await self.scheme(request)

if not api_key:

return None

jfcalvo · 2024-01-26T14:38:24Z

src/argilla/server/security/authentication/db/api_key_backend.py

+        db = request.state.db
+        user = await accounts.get_user_by_api_key(db, api_key=api_key)
+
+        if not user:
+            return None


Suggested change

db = request.state.db

user = await accounts.get_user_by_api_key(db, api_key=api_key)

if not user:

return None

user = await accounts.get_user_by_api_key(request.state.db, api_key=api_key)

if not user:

return None

jfcalvo · 2024-01-26T14:39:20Z

src/argilla/server/security/authentication/db/api_key_backend.py

+            return None
+
+        return AuthCredentials(["authenticated"]), UserInfo(
+            username=user.username, name=user.full_name, role=user.role, identity=str(user.id)


Suggested change

username=user.username, name=user.full_name, role=user.role, identity=str(user.id)

identity=str(user.id), username=user.username, name=user.full_name, role=user.role

jfcalvo · 2024-01-26T14:44:58Z

src/argilla/server/security/authentication/db/bearer_token_backend.py

+        credentials = await self.scheme(request)
+
+        if not credentials:
+            return None


Suggested change

credentials = await self.scheme(request)

if not credentials:

return None

credentials = await self.scheme(request)

if not credentials:

return None

src/argilla/server/security/authentication/provider.py

jfcalvo · 2024-01-26T15:40:01Z

src/argilla/server/security/settings.py

+        if v is not None:
+            return v
+        return values["token_expiration_in_minutes"] * 60


Suggested change

if v is not None:

return v

return values["token_expiration_in_minutes"] * 60

if v is not None:

return v

return values["token_expiration_in_minutes"] * 60

jfcalvo · 2024-01-26T15:46:28Z

src/argilla/server/security/settings.py

+    secret_key: str = uuid4().hex
+    algorithm: str = "HS256"
+
+    token_expiration_in_minutes: int = 15


This is a really small default expiration. Do we have any good reason to set such a small expiration time?

jfcalvo · 2024-01-26T15:49:45Z

src/argilla/server/security/settings.py

+    algorithm: str = "HS256"
+
+    token_expiration_in_minutes: int = 15
+    token_expiration: Optional[int]


I think it's a little bit confusing to have a token_expiration_in_minutes and at the same time token_expiration. I believe we should go for only token_expiration (using seconds) to avoid unnecessary complex scenarios (for example specifying values to the two settings at the same time).

for more information, see https://pre-commit.ci

jfcalvo

Looks fine to me (apart from the too small expiration time default value that we discussed).

…hub.com:argilla-io/argilla into refactor/define-provider-based-on-auth-backends

…4639) # Description I'm proposing to add a mention to the var `ARGILLA_AUTH_SECRET_KEY` introduced in #4539 to the `docker-compose`-related documentation in docs/_source/getting_started/installation/deployments/docker_compose.md As far as I am aware, that env var needs to be set in order to run the service in docker-compose. It's a small edit, I didn't create (yet) an issue for this. - [x] Documentation update **How Has This Been Tested** - [ ] `sphinx-autobuild` I didn't run sphinx! I had troubles w/ cloning, will update this as soon as I can **Checklist** - [x] I added relevant documentation - [ ] I followed the style guidelines of this project - [ ] I did a self-review of my code - [ ] I made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I filled out [the contributor form](https://tally.so/r/n9XrxK) (see text above) - [ ] I have added relevant notes to the `CHANGELOG.md` file (See https://keepachangelog.com/) --------- Co-authored-by: David Berenstein <david.m.berenstein@gmail.com>

frascuchon added 4 commits January 26, 2024 14:32

add base components

1abe68b

add db-based authentication backends

6b41bbc

Define new auth provider

74ed9d1

Add new security settings

2096a28

frascuchon self-assigned this Jan 26, 2024

frascuchon requested a review from jfcalvo January 26, 2024 13:50

pre-commit-ci bot and others added 2 commits January 26, 2024 13:52

[pre-commit.ci] auto fixes from pre-commit.com hooks

ff56ab7

for more information, see https://pre-commit.ci

fix import

8d7c61d

frascuchon changed the title ~~Refactor/define provider based on auth backends~~ refactor: define provider based on auth backends Jan 26, 2024

fix settings

f3a71f1

frascuchon marked this pull request as ready for review January 26, 2024 14:26

update CHANGELOG

6170fd0

frascuchon force-pushed the refactor/define-provider-based-on-auth-backends branch from 3d428ae to 6170fd0 Compare January 26, 2024 14:28

frascuchon added 2 commits January 26, 2024 15:29

docs: Update auth vars references

86ef4ab

Using new auth

8b4d860

jfcalvo requested changes Jan 26, 2024

View reviewed changes

fix user property name

ee4953c

frascuchon force-pushed the refactor/define-provider-based-on-auth-backends branch from ea37b2a to ee4953c Compare January 26, 2024 22:28

frascuchon added 6 commits January 26, 2024 23:41

base setting on token_experation

6445d71

chore: reveert elastiscsearch version

7d25c61

chore: Apply code format suggestions

9241dcc

chore: Update comments and link deprecation issue

b29e699

chore: Apply code suggestions

da6d75c

chore: Update changelog

4cd2c54

tests: Add basic test for settings

816ef60

frascuchon force-pushed the refactor/define-provider-based-on-auth-backends branch from 3930b23 to 816ef60 Compare January 29, 2024 10:55

frascuchon requested a review from jfcalvo January 29, 2024 10:56

[pre-commit.ci] auto fixes from pre-commit.com hooks

7012df1

for more information, see https://pre-commit.ci

dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Jan 29, 2024

jfcalvo approved these changes Jan 29, 2024

View reviewed changes

dosubot bot added the lgtm This PR has been approved by a maintainer label Jan 29, 2024

frascuchon added 2 commits January 29, 2024 13:14

chore: increase default token expiration

b340451

Merge branch 'refactor/define-provider-based-on-auth-backends' of git…

558bf68

…hub.com:argilla-io/argilla into refactor/define-provider-based-on-auth-backends

frascuchon merged commit cf64861 into feature/oauth-integration Jan 29, 2024
8 of 10 checks passed

frascuchon deleted the refactor/define-provider-based-on-auth-backends branch January 29, 2024 12:15

davidefiocco mentioned this pull request Mar 7, 2024

docs: mention ARGILLA_AUTH_SECRET_KEY in docker-compose deployment #4639

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: define provider based on auth backends #4539

refactor: define provider based on auth backends #4539

frascuchon commented Jan 26, 2024 •

edited

Loading

github-actions bot commented Jan 26, 2024

jfcalvo Jan 26, 2024

frascuchon Jan 29, 2024

jfcalvo Jan 26, 2024

jfcalvo Jan 26, 2024

jfcalvo Jan 26, 2024

jfcalvo Jan 26, 2024

jfcalvo Jan 26, 2024

jfcalvo Jan 26, 2024

jfcalvo Jan 26, 2024

jfcalvo left a comment

	The session token expiration in minutes. Default=30000
	The session token expiration in minutes. Default=15

	username=user.username, name=user.full_name, role=user.role, identity=str(user.id)
	identity=str(user.id), username=user.username, name=user.full_name, role=user.role

refactor: define provider based on auth backends #4539

refactor: define provider based on auth backends #4539

Conversation

frascuchon commented Jan 26, 2024 • edited Loading

Description

github-actions bot commented Jan 26, 2024

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

jfcalvo left a comment

Choose a reason for hiding this comment

frascuchon commented Jan 26, 2024 •

edited

Loading