refresh_oauth2_token silently fails if can_refresh_access_token fails type validation

[benefacto]

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 scope is passed as a str (or another type) rather than a list in the token set to the helper method store_xero_oauth2_token, then refresh_oauth2_token will return None rather than the refreshed token set.

Steps to reproduce

1. Pass scopes as a space delimited string within the token set passed to an store_xero_oauth2_token
2. Call refresh_oauth2_token
3. Verify the return value of refresh_oauth2_token is None

What you expected would happen

1. Pass scopes as a space delimited string within the token set passed to an store_xero_oauth2_token
2. Call refresh_oauth2_token
3. can_refresh_access_token raises an error describing why the token cannot be refreshed: in this case, the error should state that scope must be a list of str

What actually happens

1. Pass scopes as a space delimited string within the token set passed to an store_xero_oauth2_token
2. Call refresh_oauth2_token
3. Since the return value of refresh_oauth2_token is not needed since store_xero_oauth2_token is called internally within refresh_oauth2_token, the issue is invisible and a silent failure occurs
4. The issue becomes known when the access token expires since it can never be refreshed due to failing type validation

[scottnri]

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.

[j-osephlong]

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

[benefacto]

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

[j-osephlong]

First ever pull request! Wish me luck.

[palunel]

Running 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

[palunel]

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 TokenApi it expects a string again in the refresh_token method:

    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
        )

[manishT72]

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