Amazon-cognito-identity-js: localStorage usage should be optional

[simonbuchan]

We will likely have clients that will be using shared work computers, so the potential for credential stealing is there. Since we are essentially a single-page app we don't benefit from the caching and shouldn't need to call clearCachedTokens() to workaround this.

At the moment CognitoUser.authenticateUser() will automatically call CognitoUser.cacheTokens() before returning success, but there is little value in this if CognitoUserPool.getCurrentUser() is not used, i.e. this is an SPA or is using alternative storage already. From a safety perspective you should only pay for what you use, so the dev should be explictly request storing.

At a minimum, the API should be using sessionStorage by default.

Strawman non-breaking API proposal:

• Add property currentUserStorage to CognitoUserPool with interface:

typescript interface CognitoUserStorage { get(): CognitoUserStorageData; set(data: CognitoUserStorageData): void; } interface CognitoUserStorageData { username: string; [key: string]: string; // key currently "idToken", "accessToken", "refreshToken" }

• Add type implementing current behavior

typescript class CognitoUserStorageWebStorage implements CognitoUserStorage { constructor(storage: Storage, keyPrefix: string) { // for exposition this.storage = storage; this.keyPrefix = keyPrefix; } get() { // for exposition var username = this.storage.getItem(this.keyPrefix + 'LastAuthUser'); var userPrefix = this.keyPrefix + username + '.'; var result = { username: username }; for (var i = 0; i != this.storage.length; i++) { var key = this.storage.key(i); if (key.startsWith(userPrefix)) { // key.indexOf(userPrefix) === 0 result[key.substring(userPrefix.length)] = this.storage.getItem(key); } } return result; } set(data) { // for exposition this.storage.setItem(this.keyPrefix + 'LastAuthUser', data.username); var userPrefix = this.keyPrefix + data.username + '.'; for (var key in data) { if (data.hasOwnProperty(key) && key !== 'username') { this.storage.setItem(userPrefix + key, data[key]); } } } }

• Default currentUserStorage to new CognitoUserStorageWebStorage(localStorage, 'CognitoIdentityServiceProvider.' + clientId + '.')
  
  
  
  • or sessionStorage
  
  

Ideally the APIs would also be breakingly changed to:

• make the relationship between the relevant caching and "session" APIs clearer:
  
  
  
  • CognitoUser.cacheTokens(),
  
  
  • CognitoUser.clearCachedTokens(),
  
  
  • CognitoUserPool.getCurrentUser(),
  
  
  • CognitoUser.getSignInUserSession() and
  
  
  • CognitoUser.signOut()
  
  
• make it obvious that getCurrentUser() is referring to a persistant store
• provide support for altenative storage providers (sessionStorage, IndexedDB, flash local storage, files for electron apps, etc...)

[ribomation]

I agree. Caching tokens should not be part of the API, unless one explicitly asks for that service.

[itrestian]

That is a good suggestion indeed. Thank you! I will discuss it with the team so that we include it on our roadmap.

[tranan89]

Any updates on this?

[itrestian]

Yes, I will pickup working on this next. I want to get this out of the way such that the SDK becomes usable in Node.

[kobmic]

Any updates on this?

[hfougere]

I'm getting this error too. Any idea when this will be fixed?

[itrestian]

Closing this because merged the following pull request:

https://github.com/aws/amazon-cognito-identity-js/pull/363

Now storage is either local storage or in memory if local storage doesn't exist. It can also be passed.