-
Notifications
You must be signed in to change notification settings - Fork 3
Feature/oauth2 authentication #18
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
base: main
Are you sure you want to change the base?
Feature/oauth2 authentication #18
Conversation
| # Find the right headers to use for the request. If we have | ||
| # an OAuth2 token manager, we will use that to get the | ||
| # request headers. If not, we will use the basic Content-Type | ||
| try: | ||
| if self.oauth_token_manager: | ||
| header = self.oauth_token_manager.get_request_headers() | ||
| else: | ||
| header = {"Content-Type": "text/xml; charset=utf-8"} | ||
| except Exception as e: | ||
| logger.warning(f"Failed to get OAuth2 headers, falling back to basic headers: {e}") | ||
| header = {"Content-Type": "text/xml; charset=utf-8"} | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some duplication could be reduced here, along the lines of:
def get_headers(oauth_token_manager: Optional[AuthTokenManager]):
"""If there is an OAuth2 token manager, that will be used to get the request headers. If not, the basic Content-Type is the only header"""
try:
if oauth_token_manager:
return oauth_token_manager.get_request_headers()
except Exception as e:
logger.warning(f"Failed to get OAuth2 headers, falling back to basic headers: {e}")
# Fallback header
return {"Content-Type": "text/xml; charset=utf-8"}
headers = get_headers(self.oauth_token_manager)
| :param path: the URL path that the server listens on (default: /shapeshifter/api/v3/message) | ||
| :param oauth_token_endpoint: the OAuth2 token endpoint to use for obtaining access tokens | ||
| :param oauth_client_id: the OAuth2 client ID to use for obtaining access tokens | ||
| :param oauth_client_secret: the OAuth2 client secret to use for obtaining access tokens |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's missing the token refresh buffer param here
| 'client_secret': self.oauth_client_secret | ||
| } | ||
|
|
||
| headers = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this header duplicated here again, perhaps a constant that's used/imported in multiple places makes sense.
| :return: Dictionary of headers | ||
| """ | ||
| headers = {"Content-Type": "text/xml; charset=utf-8"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See other comment
|
Hello @AnnaShcherenko , thank you for your suggestion/contribution! We definitely want to add OAuth authentication to this library, and the code you have provided seems to be of good quality and fits the current structure of the application. This Pull Request has been discussed in the Technical Steering Comittee and I've made some small comments. |
|
I also just got informed that one of the original maintainers of the Python library is working on a new version, which includes an OAuth implementation: https://github.com/shapeshifter/shapeshifter-library-python/tree/version-2 perhaps you can see if this fits your use-case as well, as this version is currently being tested with a customer and is likely to be released in the upcoming months. |
|
Thank you @KoviaX for responding to my request—I truly appreciate that you're considering my code. |
|
Hi @AnnaShcherenko , The current "2.0" implementation is unlikely to undergo major changes before official release besides bugfixes at this point in time. So you can safely make use of the implementation that is currently built in the version-2 branch! |
|
thank you, @KoviaX |
Issue #16 This PR implements authentication class to use when sending messages
Problem Solved
AuthTokenManagerfor dynamic OAuth2 Bearer token authenticationChanges Made
1. New
AuthTokenManagerClass (shapeshifter_uftp/token_manager.py)2. Enhanced
ShapeshifterService(shapeshifter_uftp/service/base_service.py)AuthTokenManagerwhen OAuth2 credentials provided3. Updated
ShapeshifterClient(shapeshifter_uftp/client/base_client.py)oauth_token_managerparameterUsage
Backward Compatible (existing code unchanged):
New OAuth2 Support:
Testing
Unit test file (test_oauth_token_unit.py) covers:
Integration tests are not implemented