XeroAPI / xero-python

Official Xero OAuth 2.0 python SDK
MIT License
133 stars 53 forks source link

When have multi clients, The API client's Oauth2Token keeps the same #99

Open ezioruan opened 2 years ago

ezioruan commented 2 years ago

I have an app that connects to many client_ids, and I found their's client_id and client_secret keep the same even though I put different config, here are some test codes:


from xero_python.api_client import ApiClient
from xero_python.api_client.configuration import Configuration
from xero_python.api_client.oauth2 import OAuth2Token

api_client1 = ApiClient(
    Configuration(
        debug=True,
        oauth2_token=OAuth2Token(
            client_id='id1', client_secret='secret1'
        ),
    ),
    pool_threads=1,
)

api_client2 = ApiClient(
    Configuration(
        debug=True,
        oauth2_token=OAuth2Token(
            client_id='id2', client_secret='secret2'
        ),
    ),
    pool_threads=1,
)

print('client1', api_client1.configuration.oauth2_token.__dict__)
print('client2', api_client2.configuration.oauth2_token.__dict__)

And the output is :

client1 {'client_id': 'id1', 'client_secret': 'secret1', 'expiration_buffer': 60, 'token_type': 'Bearer', 'access_token': None, 'id_token': None, 'refresh_token': None, 'scope': None, 'expires_at': None, 'expires_in': None}

client2 {'client_id': 'id1', 'client_secret': 'secret1', 'expiration_buffer': 60, 'token_type': 'Bearer', 'access_token': None, 'id_token': None, 'refresh_token': None, 'scope': None, 'expires_at': None, 'expires_in': None}

You can see they are the same client_id and client_secrets

I am wondering if it's the correct behavior or is a bug,

I found the root cause here https://github.com/XeroAPI/xero-python/blob/master/xero_python/api_client/configuration.py#L41.

class Configuration(metaclass=TypeWithDefault):

The Configuration's metadata makes him always copy attributes from the same instance. What's the purpose of doing this?

RettBehrens commented 2 years ago

Hmmm. Could you detail your use case? What's the reasoning for swapping between ClientID and ClientSecret credentials?

ezioruan commented 2 years ago

@RettBehrens We set up each app for each billing entity because some business logic there has different scopes and we need to separate them. And run some corn job base on them. for now I have to assign the OauthToken again to avoid this issues

 oauth2_token = OAuth2Token(
        client_id=client_id,
        client_secret=client_secret,
    )
    configuration = Configuration(
        debug=True,
        oauth2_token=oauth2_token,
    )

    api_client = ApiClient(
        configuration=configuration,
        pool_threads=1,
    )
    api_client.configuration.oauth2_token = oauth2_token

I am not sure if other languages' SDK also have this behavior.

ezioruan commented 2 years ago

@RettBehrens Test from nodejs sdk. it works as excepted. differences API clients have different config. here's the test code

const { XeroClient } = require('xero-node');

const xero1 = new XeroClient({
  clientId: 'clientId1', // required
  clientSecret: 'clientSecret1', // required
  redirectUris: [`http://localhost/callback`], // not used for client_credentials auth flow
  grantType: 'client_credentials', // only used for client_credentials auth flow
  scopes: 'openid profile email accounting.transactions offline_access'.split(" "), // not used for client_credentials auth flow
  state: 'returnPage=my-sweet-dashboard', // custom params (optional), not used for client_credentials auth flow
  httpTimeout: 3000 // ms (optional)
});

const xero2 = new XeroClient({
  clientId: 'clientId2', // required
  clientSecret: 'clientSecret2', // required
  redirectUris: [`http://localhost/callback`], // not used for client_credentials auth flow
  grantType: 'client_credentials', // only used for client_credentials auth flow
  scopes: 'openid profile email accounting.transactions offline_access'.split(" "), // not used for client_credentials auth flow
  state: 'returnPage=my-sweet-dashboard', // custom params (optional), not used for client_credentials auth flow
  httpTimeout: 3000 // ms (optional)
});

console.log("xero1",xero1.config)
console.log("xero2",xero2.config)

output:

xero1 {
  clientId: 'clientId1',
  clientSecret: 'clientSecret1',
  redirectUris: [ 'http://localhost/callback' ],
  grantType: 'client_credentials',
  scopes: [
    'openid',
    'profile',
    'email',
    'accounting.transactions',
    'offline_access'
  ],
  state: 'returnPage=my-sweet-dashboard',
  httpTimeout: 3000
}
xero2 {
  clientId: 'clientId2',
  clientSecret: 'clientSecret2',
  redirectUris: [ 'http://localhost/callback' ],
  grantType: 'client_credentials',
  scopes: [
    'openid',
    'profile',
    'email',
    'accounting.transactions',
    'offline_access'
  ],
  state: 'returnPage=my-sweet-dashboard',
  httpTimeout: 3000
}
RettBehrens commented 2 years ago

Hi @ezioruan what you're describing is against Xero Developer Platform Ts&Cs. Specifically section 7b Be a Good Citizen to our Ecosystem:

You share Xero and our Developer Platform with your fellow developers and you should write Your Application as you’d want others to write theirs. You agree not to use, nor permit any third party to use, the Developer Platform to:

Suggested resolution - use one set of ClientID and ClientSecret credentials, adjusting what scopes are requested during auth/consent

ezioruan commented 2 years ago

@RettBehrens Thanks for your reply, so it's the right behavior. but why node js client can have a differences client_id for each api_client?

RettBehrens commented 2 years ago

@ezioruan configuration.py in Xero-Python SDK is based on a fork of OpenAPITools openapi-generator python module.

Looking at the source file from openapi-generator, I can see there is no TypeWithDefault class there, so it's presumably something the original author added.

In the Xero-Node SDK, while most of the files generated, XeroClient.ts was home-rolled rather than generated, so it's possible this restriction should have been added but was missed.

I will need to investigate further and consult with the team to determine which is the correct / expected behavior.

ezioruan commented 2 years ago

Looks good. Thanks for your reply. I don't know how to use the generate tools. Could you take a look to see if the newly generated files are correct (without TypeWithDefault )

pernofence commented 2 years ago

We are having the same issue. We are currently using two client objects, one is using a private connection auth, and the other is using code flow auth. When using both instances in the same process it fails because the client id/secret gets mixed up.

RettBehrens commented 2 years ago

An update on this topic. I believe I've found an issue identifying the same problem with OpenAPITools openapi-generator. Following the thread, it seems TypeWithDefault was added at some time and ultimately removed - our fork of the generator must have occurred after it was added but before it was removed. Going to take some time but I'll create a ticket to refactor the configuration so it doesn't use TypeWithDefault.

ezioruan commented 2 years ago

Looks like the bug was reported years ago but still have problems, hope we can solve this .

If there's some document about how to generate those codes I think it would be a big help

tamhv commented 1 year ago

if we buy 2 custom connections for 2 organizations and use them in a python app, then we would end up here, the 2nd client_id can't be used

pernofence commented 1 year ago

This workaround seems to work for me:

from xero_python.api_client import Configuration as BaseConfiguration
from xero_python.api_client.configuration import TypeWithDefault

class Workaround(TypeWithDefault):
    def __call__(cls, *args, **kwargs):
        return super(TypeWithDefault, cls).__call__(*args, **kwargs)

class Configuration(BaseConfiguration, metaclass=Workaround):
    pass