refresh_oauth2_token silently fails if can_refresh_access_token fails type validation #68
Description
Activity
I encountered this issue. My work around was to convert the scopes in the json to a list.
e,g. change
"scope": "email profile openid accounting.transactions offline_access"
to
"scope": ["email", "profile", "openid", "accounting.transactions", "offline_access"]Seems to work fine now.
Reacted by Ben and Joseph LongThis can't be intended behavior, right? When undergoing Xero's own oauth2 flow, the token is returned with it's scopes as a space delimited string, which causes refreshing to never work? I just came across this issue and after looking at the source code, it's as simple as relaxing the check on the scope value in can_refresh_access_token.
def can_refresh_access_token(self): """ Check current instance has all data required to perform refresh token API call. :return: bool """ return ( self.refresh_token and isinstance(self.scope, (list, tuple)) and self.client_id and self.client_secret )
I would make a push request to change that one line to
and isinstance(self.scope, (list, tuple, str))but to be honest I'm not sure what the original check might be guarding against. Regardless, it's wild that this is still an unexplained issue years later.I'm pretty surprised that this is still an issue after so long. I'm not a maintainer, nor have touched this SDK in years, but I tried to trigger the Jira ticket creation GitHub action to make this issue more visible, though it looks like it only is triggered when new issues are created.
@j-osephlong I remember when I last spoke to one of the former maintainers, Xero was lacking sufficient resources to maintain the Python SDK and would welcome contributions. It looks like most pull requests get merged.
That or perhaps one of the current maintainers could look into this? @manishT72 @Raghunath-S-S-J
Reacted by Joseph LongFirst ever pull request! Wish me luck.
Reacted by Ben and medelmetouiReacted by BenRunning into the same issue. It worries me that the Python SDK is not maintained properly, we intend to hang a fairly large app on it. Any idea who I can contact? Can this PR be reviewed and approved? @manishT72 @Raghunath-S-S-J
I implemented this work around:
class CustomOAuth2Token(OAuth2Token): def update_token(self, **kwargs): valid_keys = {'access_token', 'refresh_token', 'token_type', 'expires_in', 'scope', 'expires_at'} filtered_kwargs = {k: v for k, v in kwargs.items() if k in valid_keys} filtered_kwargs['scope'] = [(scope,) for scope in filtered_kwargs['scope'].split()] super().update_token(**filtered_kwargs)
which made this pass:
def can_refresh_access_token(self): """ Check current instance has all data required to perform refresh token API call. :return: bool """ return ( self.refresh_token and isinstance(self.scope, (list, tuple)) and self.client_id and self.client_secret )
but then in the
TokenApiit expects a string again in therefresh_tokenmethod:def refresh_token(self, refresh_token, scope): """ Call xero identity API to refresh auth2 access token using refresh token :param refresh_token: str auth2 refresh token :param scope: list of auth2 scopes :return: dictionary with new auth2 token """ post_data = { "grant_type": "refresh_token", "scope": " ".join(scope), "refresh_token": refresh_token, "client_id": self.client_id, "client_secret": self.client_secret, }
So my final workaround is as follows (hoping @j-osephlong 's PR will be approved and merged.):
class CustomOAuth2Token(OAuth2Token): def update_token(self, **kwargs): valid_keys = {'access_token', 'refresh_token', 'token_type', 'expires_in', 'scope', 'expires_at'} filtered_kwargs = {k: v for k, v in kwargs.items() if k in valid_keys} super().update_token(**filtered_kwargs) def can_refresh_access_token(self): """ Check current instance has all data required to perform refresh token API call. :return: bool """ return ( self.refresh_token and isinstance(self.scope, (list, tuple, str)) and self.client_id and self.client_secret )
Reacted by Joseph LongApologies for the delay and thanks for the PR. We are looking into the issue and will try to release the updated SDK soon. In the meanwhile, may I suggest the workaround of splitting scope string before storing it, just like @scottnri suggested.
Reacted by Joseph Long and palunelHi, We've released a new SDK Version which fixes the issue.
Kindly check and confirm if we're good to close the issue.
Thanks,
Which version of the SDK are you using?
1.10.0
A quick summary and/or background
Discovered this issue when debugging refresh token issues with @RettBehrens .
If
scopeis passed as astr(or another type) rather than alistin the token set to the helper methodstore_xero_oauth2_token, thenrefresh_oauth2_tokenwill returnNonerather than the refreshed token set.Steps to reproduce
scopesas a space delimited string within the token set passed to anstore_xero_oauth2_tokenrefresh_oauth2_tokenrefresh_oauth2_tokenisNoneWhat you expected would happen
scopesas a space delimited string within the token set passed to anstore_xero_oauth2_tokenrefresh_oauth2_tokencan_refresh_access_tokenraises an error describing why the token cannot be refreshed: in this case, the error should state that scope must be alistofstrWhat actually happens
scopesas a space delimited string within the token set passed to anstore_xero_oauth2_tokenrefresh_oauth2_tokenrefresh_oauth2_tokenis not needed sincestore_xero_oauth2_tokenis called internally withinrefresh_oauth2_token, the issue is invisible and a silent failure occurs