Every API request generates a new access token

[jschlade]

Hi,

Again, that you for spending the time to create this API. It's extremely useful.

In reviewing the code, to always set the Authorization header equal to "Bearer UNSET" in OauthInterceptor forces every request to fail and for a new access_token to be generated for every API call.

This is very inefficient as the access_token returned from the very first request has a time to live (TTL) value. It would be better and more efficient to cache the access_token and reuse it until it expires.

Thanks again

[studerw]

I'm pretty sure that the OauthInterceptor only generates a new refresh token upon an explicit failure (401 error). That initiates the code to create a refresh token, and then once a new auth token is obtained, it will never be required to generate it until either the app class is destroyed (which means the auth token is lost) or it times out on the server side, which shouldn't happen unless your client doesn't make a call for X minutes.

Can you write an integration test, where the client makes numerous API calls (get 10 quotes or something like that) and turn on the OAuth logging? If we grep that log, I think we'll see that only one auth token is ever generated.

[jschlade]

Hi,

Look at this code in OauthInterceptor below:

    //This gets updated using the refresh code - the first call will always fail, forcing a
    //new access_token to be set.
    private String accessToken = "UNSET";

    public OauthInterceptor(HttpTdaClient client, Properties properties) {
        this.client = client;
        this.properties = properties;
    }

    @Override
    public Response intercept(Chain chain) throws IOException {

        Request authorizedRequest = chain.request().newBuilder()
                .addHeader("Authorization", "Bearer " + this.accessToken)
                .build();
        Response origResponse = chain.proceed(authorizedRequest);

As you can see you're always setting "Authorization" to "Bearer UNSET".

For example you can see this by using your MainExample example class. In this class make 2 calls to the fetchQuotes API method and set TRACE on in OauthInterceptor if needed. You'll see that a new access_token is being generated on the 2nd call.

Sincerely

[jschlade]

I was thinking about this code some more today on a car ride this afternoon and it dawned on me that I think you wanted to cache the accessToken for the life cycle of HttpTdaClient. If so I think this is a defect.

[studerw]

It is cached. At initialization it isn’t set. The first time it gets a 401 (usually the first api call since the bearer token on that call is under), the interceptor catches it, generates the new token, and then uses that for every call.

Maybe your mistake is that you’re running the integration tests. In that case, every single one of those tests creates a new client, so that’s why you see lots of auth tokens being generated.

But in a real app, you’d only instantiate one client at start, and then it would only generate the auth token on first call (and if the token expires). That’s it.

On Sun, Aug 30, 2020, at 18:59, jschlade wrote:

I was thinking about this code some more today on a car ride this afternoon and it dawned on me that I think you wanted to cache the accessToken for the life cycle of HttpTdaClient. If so I think this is a defect.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/studerw/td-ameritrade-client/issues/28#issuecomment-683481508, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABQHJA5T43LENF6N3VELBUTSDLKTXANCNFSM4QPWPUZA.

[jschlade]

You're right. This was my mistake. I now see that this class does indeed cache the access_token and reuse it.

Sorry for any confusion. Please close this issue.