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 authorization code method #4

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add authorization code method #4

wants to merge 3 commits into from

Conversation

mboylevt
Copy link
Contributor

Add a method to enable authorization codes (with refresh tokens) in addition to client credentials.

@coveralls
Copy link

coveralls commented Nov 18, 2019

Coverage Status

Coverage decreased (-2.4%) to 76.5% when pulling f7db148 on mb-AdjustOrders into 4b82082 on master.

Copy link

@grimzy grimzy left a comment

Choose a reason for hiding this comment

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

Good to merge after the tiny change.

@@ -47,6 +48,51 @@ def authenticate(self, client_id, client_secret):
print(response.content)
return False

# Oauth2 authentication method
def authenticate_authorization_code(self, client_id, client_secret, authorizaion_code='eBGI9tpcMrBZwD9Yguqz'):
Copy link

Choose a reason for hiding this comment

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

I'd suggest removing the authorization_code's default value since the code is required by the request and quickly expires (default value becomes obsolete).

Copy link

Choose a reason for hiding this comment

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

I agree.

Copy link

@obi11235 obi11235 left a comment

Choose a reason for hiding this comment

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

I'd remove the auth code default if there isn't a reason for it, feels wrong, other than that looks good.

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.

4 participants