feat: Modify Auth Callback Controller to support Xero errors

[joe-niland]

What

• adds error param validation
• throw custom OAuthException if an error is found
• update README to describe error handling
• makes the Connection ID available in the tenant data, which is required to delete a connection (disconnect a tenant) (related to Log out from Xero? #65)

Why

• the app we were building needed to gracefully handle the case where a user clicks Cancel on the Xero Authorisation page
• without this change, the callback controller redirects back to Xero, which generates a 500 error

[hailwood]

Thanks @joe-niland,
This looks really good!

[hailwood]

This has been released as 4.5.0 :)

[joe-niland]

Thanks for maintaining this great project @hailwood !

[JamesFreeman]

Could this be a breaking change - I've just done a composer update and it's broken my tests.

public function test_that_you_can_do_a_normal_auth_callback(): void
    {
        Event::fake();

        $user = $this->login();

        $this->mock(Xero::class)->shouldReceive('switchInstance')->once();

        $this->mock(OauthCredentialManager::class, function (MockInterface $mock) {
            $mock->shouldReceive('getState')->once()->andReturn('foo');
            $mock->shouldReceive('store')->once();
            $mock->shouldReceive('getData')->once()->andReturn([
                'token' => 'token',
                'tenants' => 'tenants',
                'refresh_token' => 'refresh_token',
                'id_token' => 'id_token',
                'expires' => 'expires',
            ]);
        });

        $this->mock(IdentityApi::class, function (MockInterface $mock) {
            $mock->shouldReceive('getConfig')->once()->andReturn($this->getConfigMock());
            $mock->shouldReceive('getConnections')->once()->andReturn($this->getConnectionsMock());
        });

        $this->mock(Oauth2Provider::class, function (MockInterface $mock) {
            $mock->shouldReceive('getAccessToken')
                ->with('authorization_code', ['code' => 'bar'])
                ->once()
                ->andReturn($this->getAccessTokenMock());
        });

        $this->get(route('xero.auth.callback', [
            'clientId' => app('encrypter')->encrypt($user->client_id),
            'code' => 'bar',
            'state' => 'foo',
        ]))->assertRedirectToRoute('xero.auth.success');

        Event::assertDispatched(XeroAuthorized::class);
    }

Expected response status code [201, 301, 302, 303, 307, 308] but received 500.
Failed asserting that false is true.
/Users/xxx/Sites/xxx/vendor/laravel/framework/src/Illuminate/Testing/TestResponseAssert.php:45
/Users/xxx/Sites/xxx/vendor/laravel/framework/src/Illuminate/Testing/TestResponse.php:224
/Users/xxx/Sites/xxx/tests/Feature/App/XeroCallbackTest.php:66


Test code or tested code did not remove its own error handlers

Test code or tested code did not remove its own exception handlers

It's worth noting - this is a really cool change, and will be super useful.

Just tested this on my staging and I'm now getting a 500.

Is it possible for the package to have a default error handling, and then if we want to override it in own apps, we can?

[hailwood]

Hi @JamesFreeman,

I had considered releasing this as a 5.0.0 release, but thought that adding additional array keys shouldn't be breaking unless you're comparing the exact array which outside of tests I couldn't think of any given reason to so considered it a feature release.

Though I realise now reading your last part that the error response logic has indeed changed as an error will result in a 500 rather than a 422 validation exception, which I hadn't considered.

I apologise for releasing this as a breaking change,
I'll remove the current release and publish this as 5.0.0.

Thank you for bringing it to my attention!

[hailwood]

Alright, that release has been reverted and released as version 5.0.0 along with a description of what are breaking changes.

@JamesFreeman,
If you have a chance, I'd love to add tests to the package so we can prevent issues like this in the future. I've always intended to add them but have never had the time.

Would you consider providing any tests that you think would be useful for the package to have? Even if you don't have time for a PR, just dumping them in an issue so I can reference them when I finally add tests would be immensely helpful.

[joe-niland]

Thanks @JamesFreeman for raising this.

@hailwood thanks for sorting this out. It didn't occur to me that it could be a breaking change because, at least in my experience, the previous default behaviour was to redirect back to the Xero auth page, on which Xero would return a 500 error.

I'm not too sure how to implement this change in an alternative way, other than perhaps adding a config item to control whether an exception is raised or not.

@JamesFreeman what was your code doing in response to an 'access declined' error from Xero?