okta / okta-auth-js

The official js wrapper around Okta's auth API
Other
454 stars 265 forks source link

Token getters return indeterminate result according to object iteration order #1363

Open rwev opened 1 year ago

rwev commented 1 year ago

Describe the bug?

The return value of OktaAuth.getAccessToken. depends on order of iteration over the storage location. The last token of type AccessToken seen in the iteration is returned.

The root problem lies in getTokensSync:

getTokensSync(): Tokens {
    const tokens = {} as Tokens;
    const tokenStorage = this.storage.getStorage();
    Object.keys(tokenStorage).forEach(key => {
      const token = tokenStorage[key];
      if (isAccessToken(token)) {
        tokens.accessToken = token;
      } else if (isIDToken(token)) {
        tokens.idToken = token;
      } else if (isRefreshToken(token)) { 
        tokens.refreshToken = token;
      }
    });
    return tokens;
  }

The same problem and root cause applies to the getters for the other token types, e.g. getRefreshToken().

What is expected to happen?

Return the accessToken at ACCESS_TOKEN_STORAGE_KEY

Similarly, ror getRefreshToken, return the token at REFRESH_TOKEN_STORAGE_KEY

What is the actual behavior?

Returns an indeterminate token according the the object iteration order.

Reproduction Steps?

Use any of the aforementioned getters with multiple tokens of each type in storage.

SDK Versions

@latest

Execution Environment

All applicable

Additional Information?

No response

denysoblohin-okta commented 1 year ago

Do you have several access tokens in storage with different keys? You can use tokenManager.get(key) or tokenManager.getSync(key) to get token by custom key

rwev commented 1 year ago

Correct. We have multiple tokens, one of which is named accessToken and is the default token for the session. Further access tokens under different names are used for various different operations in the microarchitecture.

We did end up using tokenManager.getSync(ACCESS_TOKEN_STORAGE_KEY) instead of getAccessToken(), but the issue of misaligned expectations still remains.

Is there a benefit gained from resolving token getters on iteration order?

jaredperreault-okta commented 1 year ago

@rwev unfortunately our tokenManager layer was designed assuming a single token set (access, refresh, id). This a design decision made years ago that we are actively re-evaluating. In the meantime, I recommend using get or getSync directly, as Denys suggested