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

Merged
merged 56 commits into from
Oct 25, 2024
Merged

Adding oauthn #655

merged 56 commits into from
Oct 25, 2024

Conversation

ZohebShaikh
Copy link
Contributor

@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
Contributor

@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 98.61111% with 3 lines in your changes missing coverage. Please review.

Project coverage is 93.12%. Comparing base (ac0fee8) to head (5cff5ae).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/blueapi/service/main.py 88.88% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #655      +/-   ##
==========================================
+ Coverage   92.56%   93.12%   +0.56%     
==========================================
  Files          35       36       +1     
  Lines        1654     1862     +208     
==========================================
+ Hits         1531     1734     +203     
- Misses        123      128       +5     

☔ 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.
Copy link
Contributor

@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
src/blueapi/client/rest.py Outdated Show resolved Hide resolved
src/blueapi/service/authentication.py Outdated Show resolved Hide resolved
src/blueapi/service/authentication.py Outdated Show resolved Hide resolved
src/blueapi/service/authentication.py Outdated Show resolved Hide resolved
src/blueapi/service/authentication.py Outdated Show resolved Hide resolved
src/blueapi/service/authentication.py Outdated Show resolved Hide resolved
tests/system_tests/test_blueapi_system.py Show resolved Hide resolved
tests/system_tests/test_blueapi_system.py Show resolved Hide resolved
tests/system_tests/test_blueapi_system.py Outdated Show resolved Hide resolved
Comment on lines 33 to 39
with open(tmp_path / "token", "w") as token_file:
# base64 encoded token
token_file.write(
base64.b64encode(
b'{"access_token":"token","refresh_token":"refresh_token"}'
).decode("utf-8")
)
Copy link
Contributor

Choose a reason for hiding this comment

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

These parts of this ficture really want to be seperated so we can test with auth and with requiring login and with auth and already logged in.

Copy link
Contributor

@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.

Mostly just nits, but if you don't do them I'll go back and change them later

src/blueapi/service/authentication.py Show resolved Hide resolved
tests/system_tests/test_blueapi_system.py Outdated Show resolved Hide resolved
tests/system_tests/test_blueapi_system.py Show resolved Hide resolved
tests/system_tests/test_blueapi_system.py Outdated Show resolved Hide resolved
tests/system_tests/test_blueapi_system.py Show resolved Hide resolved
tests/system_tests/test_blueapi_system.py Show resolved Hide resolved
tests/system_tests/test_blueapi_system.py Outdated Show resolved Hide resolved
@ZohebShaikh ZohebShaikh merged commit d553095 into main Oct 25, 2024
26 of 27 checks passed
@ZohebShaikh ZohebShaikh deleted the adding-oauthn branch October 25, 2024 07:43
ZohebShaikh added a commit that referenced this pull request Oct 25, 2024
ZohebShaikh added a commit that referenced this pull request Oct 25, 2024
Reverts #655

Will be merged after build is fixed
@ZohebShaikh ZohebShaikh restored the adding-oauthn branch October 25, 2024 08:13
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