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

Add Enode authentication class #81

Merged
merged 29 commits into from
Apr 21, 2023
Merged

Add Enode authentication class #81

merged 29 commits into from
Apr 21, 2023

Conversation

jackypark9852
Copy link

@jackypark9852 jackypark9852 commented Apr 14, 2023

Pull Request

Description

Adds httpx auth class for enode api requests in enode_auth.py

  • get_enode_access_token() retrieves enode api token
  • EnodeAuth class has auth_flow that refreshes api token using the function

Please delete the italicised instruction text!
Please include a summary of the change and which issue is fixed.
Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes #73, #74

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.
Please also list any relevant details for your test configuration

  • Yes

If your changes affect data processing, have you plotted any changes? i.e. have you done a quick sanity check?

  • Yes

Checklist:

  • My code follows OCF's coding style guidelines
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked my code and corrected any misspellings

@jackypark9852 jackypark9852 reopened this Apr 14, 2023
@jackypark9852 jackypark9852 changed the base branch from main to h4i/enode April 14, 2023 00:16
@codecov
Copy link

codecov bot commented Apr 14, 2023

Codecov Report

Merging #81 (c931e79) into h4i/enode (c8bf403) will decrease coverage by 6.82%.
The diff coverage is 0.00%.

@@              Coverage Diff              @@
##           h4i/enode      #81      +/-   ##
=============================================
- Coverage      91.63%   84.82%   -6.82%     
=============================================
  Files              8        9       +1     
  Lines            311      336      +25     
=============================================
  Hits             285      285              
- Misses            26       51      +25     
Impacted Files Coverage Δ
pv_site_api/enode_auth.py 0.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jackypark9852 jackypark9852 changed the title H4i/enode auth Add Enode authentication class Apr 14, 2023


def get_enode_access_token() -> str:
# Replace these with your actual credentials
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this comment perhaps—seems outdated

client_id = os.getenv("CLIENT_ID")
client_secret = os.getenv("CLIENT_SECRET")

url = "https://oauth.sandbox.enode.io/oauth2/token"
Copy link
Collaborator

Choose a reason for hiding this comment

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

we might want to put this in an env variable?

Copy link
Contributor

@AndrewLester AndrewLester left a comment

Choose a reason for hiding this comment

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

Nice work, a few comments.

pv_site_api/enode_auth.py Outdated Show resolved Hide resolved
pv_site_api/main.py Outdated Show resolved Hide resolved

if response.status_code == 401:
# The access token is no longer valid, refresh it
self.access_token = get_enode_access_token()
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not entirely sure if this will work in the way HTTPX wants it to... consider the example they have in the docs: https://www.python-httpx.org/advanced/#customizing-authentication:~:text=refresh_response%20%3D%20yield%20self.build_refresh_request()

The way they do this is have the function they use to fetch the new token return a Request object, which they then call yield on. I think we'll want to do the same thing here. This also means that you'll need to grab the .json() and the right key from the response in this code rather than your other function.

@AndrewLester
Copy link
Contributor

Also, I think we may want to add tests for this logic...

Here's a library that allows you to mock HTTPX requests: https://github.com/Colin-b/pytest_httpx

Feel free to look into this, I will also. We can work on adding tests to this before merging.

Copy link
Contributor

@AndrewLester AndrewLester left a comment

Choose a reason for hiding this comment

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

Very nice, last few things.

pv_site_api/enode_auth.py Outdated Show resolved Hide resolved
pv_site_api/enode_auth.py Outdated Show resolved Hide resolved
def __init__(self, access_token):
self.access_token = access_token

def auth_flow(self, request):
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a type hint

Suggested change
def auth_flow(self, request):
def auth_flow(self, request: httpx.Request):

pv_site_api/enode_auth.py Outdated Show resolved Hide resolved
@jackypark9852 jackypark9852 merged commit a506477 into h4i/enode Apr 21, 2023
@AndrewLester AndrewLester deleted the h4i/enode-auth branch April 23, 2023 17:30
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.

Create HTTPX Custom Auth to handle token refresh
3 participants