saloonphp / saloon

🤠 Build beautiful API integrations and SDKs with Saloon
https://docs.saloon.dev
MIT License
2.03k stars 105 forks source link

Skipping AuthenticatePendingRequest when fetching a new accessToken using OAuth2 plugins #385

Closed leroy closed 1 month ago

leroy commented 5 months ago

I wanted to fetch the access token using ->getAccessToken() from within the default auth so every request has a valid access token by default.

However, getAccessToken in turn triggers a defaultAuth() call resulting in an infinite loop.

Would it be an idea to skip the AuthenticatePendingRequest somehow when running getAccessToken()?

Willing to propose PR!

public function defaultAuth(): ?TokenAuthenticator
    {
        $cachedToken = cache()->remember('token, now()->addHour(), function() {
            return $this->getAccessToken()->getAccessToken() // Results in recursive loop as underlying request also tries to define defaultAuth();
        });

        return new TokenAuthenticator($cachedToken);
    }
Sammyjo20 commented 5 months ago

Hey @leroy thanks for your patience on my response

This is an interesting one! I'm guessing we probably don't need to call the defaultAuth() method for the OAuth2 requests however I could imagine that there might be other ways people need to authenticate so I think we should keep the functionality.

There's a couple of workarounds I can think of for your situation - first is a custom authenticator class. Within this authenticator class you'll get access to the PendingRequest class so you can ignore specific requests from applying the authentication. For example:

<?php

use Saloon\Http\PendingRequest;
use Saloon\Contracts\Authenticator;

class CustomAuthenticator implements Authenticator
{
    public function set(PendingRequest $pendingRequest): void
    {
        $request = $pendingRequest->getRequest();

        if ($request instanceof GetAccessTokenRequest) {
           return;
        }

        $connector = $pendingRequest->getConnector();

        $cachedToken = cache()->remember('token, now()->addHour(), function() use ($connector) {
            return $connector->getAccessToken();
        });

        $pendingRequest->authenticate(new TokenAuthenticator($cachedToken));
    }
}

You can then use this authenticator like you do already:

public function defaultAuth(): CustomAuthenticator
{
    return new CustomAuthenticator;
}

Let me know your thoughts on this - the other way I was thinking was pretty much the same but using the boot() method on the connector, however I think a custom authenticator is cleaner.

Sammyjo20 commented 3 months ago

Hey @leroy did the above solution work okay for you?

patrickcarlohickman commented 3 months ago

Another option is to use a new connector instance with a null authenticator to get the access token.

public function defaultAuth(): ?TokenAuthenticator
{
    $cachedToken = cache()->remember('token, now()->addHour(), function() {
        $connector = static::make();

        return $connector
            ->authenticate(new class implements \Saloon\Contracts\Authenticator
                {
                    public function set(\Saloon\Http\PendingRequest $pendingRequest): void
                    {
                        //
                    }
                }
            )
            ->getAccessToken();
    });

    return new TokenAuthenticator($cachedToken);
}

By setting an authenticator using the authenticate() method on the connector, it will prevent the defaultAuth() method from being called on the connector. Additionally, we do this on a new instance of the connector, because if we do it on $this, then all subsequent calls by $this connector will also use the null authenticator instead of calling defaultAuth() again.

NB: you can clean up the code a little by extracting the anonymous class to an actual NullAuthenticator class. I've done this in my own projects, but it is something I'd like to submit a PR for.

Sammyjo20 commented 3 months ago

Hey @leroy how did you get on with these suggested changes?