webfox / laravel-xero-oauth2

A Laravel integration for Xero using the Oauth 2.0 spec
MIT License
50 stars 32 forks source link

Ability to choose particular tenant (by tenantId) #68

Closed AppoloSoft closed 2 years ago

AppoloSoft commented 2 years ago

First of all, i was super happy to find this package! Definitely reduce the gymnastic around Laravel's OAuth2!!

Is your feature request related to a problem? Please describe. Currently the tenantId is selected by hardcode i found in AuthorizationCallbackController.php line 29 $tenantId = $identity->getConnections()[0]->getTenantId();

Describe the solution you'd like I wonder if we could have the OAuthCredentialManager to store connections as an array which we can iterate to select connection with the right tenantId

Describe alternatives you've considered I am considering to override the vendor's AuthorizationCallbackController.php

Additional context I truly wonder if there is a much better way to allow this Package to support multiple tenant Xero account

AppoloSoft commented 2 years ago

Ah.. i manage to solve my issue by doing the following:

  1. Copy \vendor\webfox\laravel-xero-oauth2\src\Controllers\AuthorizationCallbackController.php to app\Http\Controllers\AuthorizationCallbackController.php

  2. Then i added this to my route (web.php) Route::middleware('web')->group(function() { Route::get('/xero/auth/callback', AuthorizationCallbackController::class)->name('xero.auth.callback'); });

  3. Then on .env I added: XERO_TENANT_NAME="TENANT_NAME_HERE"

  4. Finally i altered the invoke function as shown below: ` public function invoke(Request $request, OauthCredentialManager $oauth, IdentityApi $identity, Oauth2Provider $provider) { try { $this->validate($request, [ 'code' => ['required', 'string'], 'state' => ['required', 'string', "in:{$oauth->getState()}"] ]);

        $accessToken = $provider->getAccessToken('authorization_code', $request->only('code'));
        $identity->getConfig()->setAccessToken((string)$accessToken->getToken());
    
        $tenantId = '';
    
        foreach ($identity->getConnections() as $connection) {
            if ($connection['tenant_name'] == env('XERO_TENANT_NAME')) {
                $tenantId = $connection['tenant_id'];
            }
        }
    
        if ($tenantId == '')
            return $this->onFailure(new \throwable('Tenant Not Found'));
    
        $oauth->store($accessToken, $tenantId);
        Event::dispatch(new XeroAuthorized($oauth->getData()));
    
        return $this->onSuccess();
    } catch (\throwable $e) {
        return $this->onFailure($e);
    }

    } `

hailwood commented 2 years ago

Interesting, We've never really run into a case where we have multiple tenants on a single access token. Is this when the user you've logged into Xero with is connected to multiple organizations?

I'd definitely be open to finding a way of supporting this without users having to copy and callback controller.

AppoloSoft commented 2 years ago

Is this when the user you've logged into Xero with is connected to multiple organizations? yes! :)

Im not sure about other scenario, but right now with mine is i am developing a web app for a client which require me to interact with Xero via XeroAPI.

I am connecting to Xero with my developer account, and i have multiple tenants because i test my codes to Demo Company as the tenant, and the client will soon be another tenant for the web app to connect to :)

My temporary solution above works (for now) but it would be great if we find a way for the others to do this in an easier manner haha.

By the way i also notice that on Xero's official Postman sample environment, they also hard coded it! see below:

postman.setEnvironmentVariable("xero-tenant-id", data[0].tenantId);

tonypartridge commented 2 years ago

This is exactly my current scenario AppoloSoft. I have a client whom has access to their clients accounts (tenants) and we authorise them all but then want to access the details of individual accounts from a single application, kinda like XERO does natively but for an easier generalisation.