Skip to content

[REFACTOR][ENHANCEMENT] argilla server: better OAuth2 integration#5689

Merged
frascuchon merged 38 commits intodevelopfrom
refactor/argilla-server/better-oauth2-integration
Jan 29, 2025
Merged

[REFACTOR][ENHANCEMENT] argilla server: better OAuth2 integration#5689
frascuchon merged 38 commits intodevelopfrom
refactor/argilla-server/better-oauth2-integration

Conversation

@frascuchon
Copy link
Member

@frascuchon frascuchon commented Nov 13, 2024

Description

This PR improves the social_core integration, making setup backends simpler.

Improvements:

  • Reading SOCIAL_AUTH_* environment variables
  • Remove unnecessary settings attribute enabled
  • Add more backends with no-code: extra_backends* (some backends may be not fully supported, but users can create issues for that)

Type of change

  • Refactor (change restructuring the codebase without changing functionality)
  • Improvement (change adding some improvement to an existing functionality)
  • Documentation update

How Has This Been Tested

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
  • I confirm My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have added relevant notes to the CHANGELOG.md file (See https://keepachangelog.com/)

@codecov
Copy link

codecov bot commented Nov 13, 2024

Codecov Report

Attention: Patch coverage is 80.69307% with 39 lines in your changes missing coverage. Please review.

Project coverage is 91.55%. Comparing base (105888a) to head (daf2d4a).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
...erver/src/argilla_server/api/handlers/v1/oauth2.py 50.00% 18 Missing ⚠️
..._server/security/authentication/oauth2/provider.py 72.72% 9 Missing ⚠️
...server/security/authentication/oauth2/_backends.py 90.27% 7 Missing ⚠️
...ver/security/authentication/oauth2/auth_backend.py 0.00% 2 Missing ⚠️
...illa-server/src/argilla_server/validators/users.py 92.59% 2 Missing ⚠️
...argilla_server/security/authentication/userinfo.py 91.66% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #5689      +/-   ##
===========================================
- Coverage    91.93%   91.55%   -0.38%     
===========================================
  Files          161      159       -2     
  Lines         6927     6987      +60     
===========================================
+ Hits          6368     6397      +29     
- Misses         559      590      +31     
Flag Coverage Δ
argilla-server 91.55% <80.69%> (-0.38%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@frascuchon frascuchon added this to the v2.6.0 milestone Nov 14, 2024
@frascuchon frascuchon marked this pull request as ready for review November 18, 2024 08:51
@frascuchon frascuchon requested a review from jfcalvo November 18, 2024 08:52
@frascuchon frascuchon changed the title [REFACTOR][ENHANCEMENT] argilla server: better oauth2 integration [REFACTOR][ENHANCEMENT] argilla server: better OAuth2 integration Nov 18, 2024
frascuchon and others added 3 commits November 19, 2024 15:57
# Description
<!-- Please include a summary of the changes and the related issue.
Please also include relevant motivation and context. List any
dependencies that are required for this change. -->

This PR adds logic to the sign-in page to include a default OAuth
provider button. So, users can sign using whatever provider is defined.

Refs #5689

**Type of change**
<!-- Please delete options that are not relevant. Remember to title the
PR according to the type of change -->

- Improvement (change adding some improvement to an existing
functionality)
- Documentation update

**How Has This Been Tested**
<!-- Please add some reference about how your feature has been tested.
-->

**Checklist**
<!-- Please go over the list and make sure you've taken everything into
account -->

- 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
- I confirm My changes generate no new warnings
- I have added tests that prove my fix is effective or that my feature
works
- I have added relevant notes to the CHANGELOG.md file (See
https://keepachangelog.com/)

---------

Co-authored-by: Leire Aguirre <leire@argilla.io>
@github-actions
Copy link

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

frascuchon and others added 6 commits November 21, 2024 09:25
# Description
<!-- Please include a summary of the changes and the related issue.
Please also include relevant motivation and context. List any
dependencies that are required for this change. -->

Docs for changes included in
#5689

**Type of change**
<!-- Please delete options that are not relevant. Remember to title the
PR according to the type of change -->

- Documentation update

**How Has This Been Tested**
<!-- Please add some reference about how your feature has been tested.
-->

**Checklist**
<!-- Please go over the list and make sure you've taken everything into
account -->

- 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
- I confirm My changes generate no new warnings
- I have added tests that prove my fix is effective or that my feature
works
- I have added relevant notes to the CHANGELOG.md file (See
https://keepachangelog.com/)
@frascuchon frascuchon removed this from the v2.6.0 milestone Jan 20, 2025
frascuchon and others added 6 commits January 28, 2025 11:03
# Add keycloak SSO 

Based on discussion in #5691

Points that need some feedback:
- A lot of configurations are set via env variables now. Not sure if
that's ideal, error messages if something is not set correctly can be
rather cryptic with social auth lib
- I added the Keycloak logo to the Oauth button id the provider is
keycloak, generally the same could also be done for the HF logo not
having a separate button
- Is the documentation to set-up a keycloak server sufficient?

**Type of change**
- Improvement (change adding some improvement to an existing
functionality)
- Documentation update

**How Has This Been Tested**
Local build & Keycloak installation as described in the documentation.

**Checklist**
<!-- Please go over the list and make sure you've taken everything into
account -->

- 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
- I confirm My changes generate no new warnings
- I have added tests that prove my fix is effective or that my feature
works
- TODO I have added relevant notes to the CHANGELOG.md file (See
https://keepachangelog.com/)

---------

Co-authored-by: Paco Aranda <frascuchon@gmail.com>
Co-authored-by: Francisco Aranda <francis@argilla.io>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Paco Aranda <francisco.aranda@huggingface.co>
@frascuchon frascuchon merged commit 6d6f96e into develop Jan 29, 2025
14 of 15 checks passed
@frascuchon frascuchon deleted the refactor/argilla-server/better-oauth2-integration branch January 29, 2025 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants