spotify / spotify-web-api-ts-sdk

A Typescript SDK for the Spotify Web API with types for returned data.
Other
351 stars 66 forks source link

`ProvidedAccessTokenStrategy` and `AccessTokenHelpers.refreshCachedAccessToken` do not properly refresh access token #113

Open dmmulroy opened 3 months ago

dmmulroy commented 3 months ago

ProvidedAccessTokenStrategy and AccessTokenHelpers.refreshCachedAccessToken do not properly refresh access token

There are multiple problems in the current client on main - this is unreleased, I know, but figured I should still file a issue - when trying to refresh access tokens.

  1. The refresh token endpoint requires the Authorization: Basic base64(client_id:client_secret) header, despite the docs saying it does not. image

  2. Because of ☝️, the ProvidedAccessTokenStrategy incorrectly implements refreshTokenAction by means of AccessTokenHelpers.refreshCachedAccessToken

    private static async refreshToken(
        clientId: string,
        refreshToken: string,
    ): Promise<AccessToken> {
        const params = new URLSearchParams();
        params.append("client_id", clientId);
        params.append("grant_type", "refresh_token");
        params.append("refresh_token", refreshToken);

        const result = await fetch("https://accounts.spotify.com/api/token", {
            method: "POST",
            headers: {
                "Content-Type": "application/x-www-form-urlencoded",
            },
            body: params,
        });

        const text = await result.text();

        if (!result.ok) {
            throw new Error(`Failed to refresh token: ${result.statusText}, ${text}`);
        }

        const json: AccessToken = JSON.parse(text);
        return json;
    }

As you can see, the Authorization header is missing - I've validated that this request does work when provided the header (I've patched it locally and linked it to my project).

The consequence of this, currently, is that you then need to pass the client_secret the whole way through the stack to the AccessTokenHelpers.refreshCachedAccessToken function which is what my patch dos.

This leads to the next issue....

  1. The refreshToken endpoint response type/docs is wrong or has a bug.

    {
    access_token: 'BQBLuPRYBQ...BP8stIv5xr-Iwaf4l8eg',
    token_type: 'Bearer',
    expires_in: 3600,
    refresh_token: 'AQAQfyEFmJJuCvAFh...cG_m-2KTgNDaDMQqjrOa3',
    scope: 'user-read-email user-read-private'
    }

    This is incorrect as the refresh_token field is no returned - this cause a bug in the ProvidedAccessTokenStrategy as the current code assumes the refresh_token is there.

    You can see that when this.refreshToken is called, the accessToken in memory is completely replaced. This then causes the bug to occur next time we attempt to refresh because there is no refresh_token on the access_token

dmmulroy commented 3 months ago

In addition, I would love to see the ability to pass in a AuthenticationStrategy to SpotifyApi.withAccessToken so that I don't have to immediately call client.switchAuthenticationStrategy. My example is that I want to persist my accessToken to a json file to read on service start.

const client = SpotifyApi.withAccessToken(
  config.clientId,
  Secret.value(config.clientSecret),
  config.accessToken,
  // I would like to pass a AuthenticationStrategy here
);

client.switchAuthenticationStrategy(
  new ProvidedAccessTokenStrategy(
    config.clientId,
    Secret.value(config.clientSecret),
    config.accessToken,
    async (clientId, clientSecret, accessToken) => {
      const refreshedToken =
        await AccessTokenHelpers.refreshCachedAccessToken(
          clientId,
          clientSecret,
          accessToken,
      );

      await Bun.write(
        "src/do_not_open_on_stream/access-token.json",
        JSON.stringify(refreshedToken, null, 2),
      ).catch(console.error);

      return refreshedToken;
    },
  ),
);