sdk.ensureLoggedIn returns prior user's login status

[ElridgeDMello]

Hello,

We have a use case where we need to have two users log into the RC SDK and use the two instances for different purposes (SMS sending vs non-SMS API requests).

What we found is that when we create new SDK instances and then check the login status, the second instance will reflect the login status of the first. We are using "@ringcentral/sdk": "4.7.4"

Code below to demonstrate the issue:

const SDK = require('@ringcentral/sdk').SDK

async function getRingcentralClient({
    server,
    clientId,
    clientSecret,
    username,
    extension,
    password,
}) {
    const sdk = new SDK({
        server,
        clientId,
        clientSecret,
    })
    console.log(`checking login status for: `, username)
    try {
        await sdk.ensureLoggedIn()
    } catch (e) {
        console.log(`logging in ${username}`)
        await sdk.login({
            username,
            password,
            extension,
        })
    }
    console.log(`returning sdk instance for ${username}`)
    return sdk
}

module.exports = {
    getRingcentralClient,
}

Code to test the above module/demonstrate the issue:

async function _testFn() {
    const server = 'https://platform.devtest.ringcentral.com'
    const clientId = 'myClientId'
    const clientSecret = 'myClientSecret'

    // root user
    const rootUserCreds = {
        username: '+1xxxyyyy4538',
        password: 'myRootPassword',
        extension: '101',
    }

    // sms service account user:
    const smsServiceUserCreds = {
        username: '+1aaabbbb1966',
        password: 'myServiceUserPassword',
        extension: '102',
    }

    await getRingcentralClient({
        server,
        clientId,
        clientSecret,
        ...rootUserCreds,
    })

    console.log(`\n\n`)

    await getRingcentralClient({
        server,
        clientId,
        clientSecret,
        ...smsServiceUserCreds,
    })
}

_testFn()

Output of running the code with valid credentials (notice that the logging in 1aaabbbb1966 output is missing!):

checking login status for:  +1xxxyyy4538
logging in +1xxxyyy4538
returning sdk instance for +1xxxyyy4538


checking login status for:  +1aaabbbb1966
returning sdk instance for +1aaabbbb1966

What we would expect (since we're creating a new sdk instance):

checking login status for:  +1xxxyyy4538
logging in +1xxxyyy4538
returning sdk instance for +1xxxyyy4538


checking login status for:  +1aaabbbb1966
logging in +1aaabbbb1966
returning sdk instance for +1aaabbbb1966

[tylerlong]

After reading the code carefully, it seems be a JS programming issue. You are printing username, which is one of the function parameters. I don't see how RC SDK could change the function parameter.

In JS, primitive data types such as string are passed by value. So even the SDK changes the value of the username you passed to it, it won't affect the original value.

I am trying to reproduce it.

[ElridgeDMello]

Hi @tylerlong,

Thanks for looking into this and trying to reproduce this issue on your end.

The point of logging the username was to illustrate the point that even though we are creating a new instance of the sdk like so:

    const sdk = new SDK({
        server,
        clientId,
        clientSecret,
    })

and then calling sdk.ensureLoggedIn() for each instance, the second instance of sdk indicates that it is already logged in, even though we didn't yet login to it.

[tylerlong]

It is by design (not designed by me though).

ringcentral-js/sdk/src/platform/Auth.ts

Lines 14 to 40 in dcb63a6

	export default class Auth {
	private _cache: Cache;
	private readonly _cacheId: string;
	private readonly _refreshHandicapMs: number;
	public constructor({cache, cacheId, refreshHandicapMs = DEFAULT_RENEW_HANDICAP_MS}: AuthOptionsConstructor) {
	this._cache = cache;
	this._cacheId = cacheId;
	this._refreshHandicapMs = refreshHandicapMs;
	}
	public async data(): Promise<AuthData> {
	return (
	(await this._cache.getItem(this._cacheId)) || {
	token_type: '',
	access_token: '',
	expire_time: 0,
	expires_in: '',
	refresh_token: '',
	refresh_token_expires_in: '',
	refresh_token_expire_time: 0,
	scope: '',
	}
	);
	}

By default it will read the cache in local storage:

ringcentral-js/sdk/src/core/Cache.ts

Line 39 in dcb63a6

const item = this._externals.localStorage.getItem(this._prefixKey(key));

[tylerlong]

To workaround it, for different SDK instances, specify different cachePrefix:

ringcentral-js/sdk/src/SDK.ts

Line 61 in dcb63a6

const {cachePrefix, defaultRequestInit, handleRateLimit} = options;

[ElridgeDMello]

@tylerlong great, thanks will do that!

[tylerlong]

It's causing more trouble to developers:

The issue is that your SDK internally caches login sessions WITHOUT DIFFERENTIATING between client IDs - unless I explicitly pass a "cachePrefix" option. Tremendously bad design - with dire security implications.

Let's find a way to fix it.

[u9520107]

It's causing more trouble to developers:

The issue is that your SDK internally caches login sessions WITHOUT DIFFERENTIATING between client IDs - unless I explicitly pass a "cachePrefix" option. Tremendously bad design - with dire security implications.

Let's find a way to fix it.

Umm... If they are developing something that has multiple client-ids, using those client-ids should be fairly explicit? Can't they just have a cachePrefix set for each client-ids used?

I thought cachePrefix is implemented exactly to solve this problem?

---

Honestly, I don't think the cachePrefix design here is exactly great. But implicitly partition the cache based on client-id can be problematic as well.

I'd rather have something like

const cache = new Cache('myNameSpace');
const sdk = new SDK({ ..., cache }); 

and just let developers manage their own caching space explicitly.
And if we have a standard interface for this Cache object, preferably with async support, we can swap out the localstorage use with indexeddb, or even remote storage solutions and be very flexible.

---

@embbnux Dunno, I think I'd be very supportive of a breaking change for this for a major release.
By default, if no cache object is provided, we should just throw an error. This makes is very explicit that the developers need to consider the caching solution. We can provide a built-in localstorage based cache class that allow them to easily get the old bahavior by explicit creating a cache object. We also clearly define the interface so developers can define their own caching solution and plug into the sdk. I think we can also provide a default indexeddb based caching solution to demonstrate the interface.
If they don't want to cache, instead of what we do in the current version, by overriding the storage object with in-memory storage object to basically highjack the cache behavior, we can allow developers to just pass false or explicitly pass a null value to disable caching.

[tylerlong]

Most developers don't know that the SDK caches tokens. It will surprise them that two SDK instances magically share the same token. And sometimes it will cause security issues because the SDK instance may perform actions on behalf of a wrong user.

For next major release. We could make it explicit: developers need to explicitly specify cache settings. Even they don't need cache at all, they need to explicitly specify something like "cache=false".

It will be a breaking change anyway. Break it at development time so that it won't affect production.