Skip to content
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.

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

Adding oauthn #655

Open
wants to merge 49 commits into
base: main
Choose a base branch
from
Open

Adding oauthn #655

wants to merge 49 commits into from

Conversation

ZohebShaikh
Copy link
Collaborator

@ZohebShaikh ZohebShaikh commented Oct 2, 2024

This pull request introduces a new authentication mechanism using OAuth2 and JWT tokens, along with various related changes across multiple files to integrate this functionality.

Authentication Integration:

  • Added Authentication and AuthenticationType classes to handle OAuth2 device flow and PKCE authentication, including methods for token management (src/blueapi/service/authentication.py).
  • Updated main.py to include OAuth2 authorization code flow and token validation using the new Authentication class (src/blueapi/service/main.py).

CLI Enhancements:

  • Added a new login command to the CLI to initiate the device flow authentication (src/blueapi/cli/cli.py).

REST Client Updates:

  • Modified the REST client to include authorization headers with JWT tokens in requests (src/blueapi/client/rest.py).

Dependency Additions:

  • Added PyJWT and python-multipart to the project dependencies (pyproject.toml).

Alternative to the authentication were investigated:-

Client Libraries

In the end I decided to not use them as we just need to make 2 requests and there is not that much error handling required as well ... We can look into integrating on of the above mentioned alternatives for the OAuth device flow integration.

For the PKCE Flow I have used the FastAPI support to implement the OAuth2

Copy link
Collaborator

@DiamondJoseph DiamondJoseph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly right, we'll talk tomorrow about how it's been going

src/blueapi/service/main.py Outdated Show resolved Hide resolved
src/blueapi/service/authentication.py Outdated Show resolved Hide resolved
src/blueapi/service/main.py Outdated Show resolved Hide resolved
src/blueapi/client/rest.py Outdated Show resolved Hide resolved
src/blueapi/client/rest.py Outdated Show resolved Hide resolved
src/blueapi/service/authentication.py Outdated Show resolved Hide resolved
src/blueapi/service/main.py Outdated Show resolved Hide resolved
src/blueapi/config.py Show resolved Hide resolved
src/blueapi/config.py Outdated Show resolved Hide resolved
src/blueapi/service/authentication.py Outdated Show resolved Hide resolved
src/blueapi/client/rest.py Outdated Show resolved Hide resolved
src/blueapi/service/authentication.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Oct 15, 2024

Codecov Report

Attention: Patch coverage is 95.55556% with 10 lines in your changes missing coverage. Please review.

Project coverage is 92.78%. Comparing base (ac0fee8) to head (d747a89).

Files with missing lines Patch % Lines
src/blueapi/service/main.py 62.96% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #655      +/-   ##
==========================================
+ Coverage   92.56%   92.78%   +0.22%     
==========================================
  Files          35       36       +1     
  Lines        1654     1871     +217     
==========================================
+ Hits         1531     1736     +205     
- Misses        123      135      +12     

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

This commit refactors the authentication process in the Blueapi RestClient and service. It removes the explicit specification of the authentication type and instead uses the default authentication type. Additionally, it adds logic to handle token verification and refreshing in the RestClient. The service code is also updated to use the PKCE authentication type.
@ZohebShaikh ZohebShaikh marked this pull request as ready for review October 16, 2024 22:15
src/blueapi/cli/cli.py Outdated Show resolved Hide resolved
src/blueapi/client/rest.py Outdated Show resolved Hide resolved
tests/unit_tests/example_yaml/valid_auth_config.yaml Outdated Show resolved Hide resolved
tests/unit_tests/test_cli.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@DiamondJoseph DiamondJoseph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bunch of little nits because I am a bad person

src/blueapi/client/rest.py Outdated Show resolved Hide resolved
src/blueapi/service/authentication.py Outdated Show resolved Hide resolved
src/blueapi/service/main.py Outdated Show resolved Hide resolved
src/blueapi/service/main.py Outdated Show resolved Hide resolved
src/blueapi/service/main.py Outdated Show resolved Hide resolved
tests/unit_tests/service/test_rest_api.py Outdated Show resolved Hide resolved
tests/unit_tests/test_cli.py Outdated Show resolved Hide resolved
tests/unit_tests/test_cli.py Outdated Show resolved Hide resolved
tests/unit_tests/test_cli.py Outdated Show resolved Hide resolved
tests/unit_tests/test_cli.py Outdated Show resolved Hide resolved
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.

2 participants