Singleton causes issues with Auth token when running in parallel

[crash13override]

There is a big problem with the fact that you're returning a Singleton from the GoogleServiceProvider.

Since there is just one instance of the PulkitJalan\Google\Client class, there are a lots of problems when I call the getClient() method, which returns always the same instance as well, and then I try to set up different Auth keys with the setAccessToken() method.

What happens is that if I have 2 queue jobs running in the background almost at the same time, then the Auth tokens are not saved separately, but one overrides the other, causing of course tons of problem and mixing the user's account!

Instead of using $this->app->singleton() you should probably use $this->app->make() so that it's easier to instantiate a new instance every time.

Thanks!

[sebastiansson]

+1

[pulkitjalan]

Hi,

Can you submit a PR and I’ll have a look. Thanks.

On 7 Nov 2017, at 15:27, Sebastian Marcusson notifications@github.com wrote:

+1

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[crash13override]

Hi @pulkitjalan ,

sorry but I think it will take a bit more time than a couple line of code changes and a PR to fix this.

I think the approach of using the Facade like in this example that you provide is just not working

$googleClient = Google::getClient();
$googleClient->setAccessToken('123456');
$calendar = Google::make('calendar');

What I'm currently using to work around the problem I explained is the following:

$client = new \PulkitJalan\Google\Client(config('google'));
$googleClient = $client->getClient();
$googleClient->setAccessToken('123456');
$calendar = $client->make('calendar');

So I don't see a way to use your library properly with a Facade if running in a Queue for the way it is structured. I would rather advise in the documentation to use the second version (without Facade) if running in a Queue and it should be sufficient.

I'm just saying this because I spend quite some time wondering why my app was having this weird bug until I finally found it :)

Thanks for your great work btw!

[pulkitjalan]

Thanks for the great explanation. I’ll add that to the docs.

I’ll have a think about a more permanent fix for this problem also.

Thanks.

On 7 Nov 2017, at 23:14, crash13override notifications@github.com wrote:

Hi @pulkitjalan ,

sorry but I think it will take a bit more time than a couple line of code changes and a PR to fix this.

I think the approach of using the Facade like in this example that you provide is just not working

$googleClient = Google::getClient(); $googleClient->setAccessToken('123456'); $calendar = Google::make('calendar'); What I'm currently using to work around the problem I explained is the following:

$client = new \PulkitJalan\Google\Client(config('google')); $googleClient = $client->getClient(); $googleClient->setAccessToken('123456'); $calendar = $client->make('calendar'); So I don't see a way to use your library properly with a Facade if running in a Queue for the way it is structured. I would rather advise in the documentation to use the second version (without Facade) if running in a Queue and it should be sufficient.

I'm just saying this because I spend quite some time wondering why my app was having this weird bug until I finally found it :)

Thanks for your great work btw!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[ingageco]

Recently I encountered an issue and I think it's related to the comment by @crash13override . We use a queue system for video uploads in Laravel and we were using the first implementation (Google::getClient()). This week, some videos were uploaded to the wrong Youtube accounts when the queued jobs were processed close together. It seems like the token was not reset when i used set access token.

I've switched to the "workaround" @crash13override provided. Shouldn't using this method (new instantiation versus static) be a better method to avoid the client possibly keeping a wrong access token in it's properties?

[stevelacey]

Yep +1 this is quite a significant issue if you're doing any sensitive with Google APIs, leaking access tokens between jobs is quite a bad scenario

[stevelacey]

I don't understand why this needs to be any more complicated than switching from singleton to bind

=> PulkitJalan\Google\Client {#3493}
>>> App::get(PulkitJalan\Google\Client::class)
=> PulkitJalan\Google\Client {#3493}
>>> App::get(PulkitJalan\Google\Client::class)
=> PulkitJalan\Google\Client {#3493}

becomes

=> PulkitJalan\Google\Client {#3493}
>>> App::get(PulkitJalan\Google\Client::class)
=> PulkitJalan\Google\Client {#3495}
>>> App::get(PulkitJalan\Google\Client::class)
=> PulkitJalan\Google\Client {#3484}

Any dependency injection will resolve a new instance, as will the Facade given it pulls from the DI – am I missing something?

Edit: after further experimentation I see the facade still returns a single instance – and perhaps that's how they work? Either way it still fixes the problem for anyone using DI

Edit 2: yeah that's just how facades work – https://stackoverflow.com/a/40322524

I suggest switching to bind, and encouraging DI over facades in this case – and perhaps even removing the facade