feat: access_token is stored in global conf not local .aio (#150)

This change checks where an IMS context is defined, and stores the tokens in the same location (i.e. local or global). Furthermore, on logout, _only_ the tokens are removed, rather than the entire IMS context being rewritten, as was the case previously. The context data is the merged view of global and local configs for a context, so rewriting it risks accidentally copying data between the local IMS context definition and the global one. Co-authored-by: rliechti <[email protected]>
adobe · Oct 1, 2024 · 9b25ba8 · 9b25ba8
1 parent bb41343
commit 9b25ba8
Show file tree

Hide file tree

Showing 8 changed files with 128 additions and 34 deletions.
diff --git a/src/ctx/ConfigCliContext.js b/src/ctx/ConfigCliContext.js
@@ -33,7 +33,7 @@ class ConfigCliContext extends Context {
    */
   async getCli () {
     aioLogger.debug('get cli')
-    return this.getContextValue(this.keyNames.CLI)
+    return (await this.getContextValue(this.keyNames.CLI))?.data
   }
 
   /**
@@ -54,7 +54,7 @@ class ConfigCliContext extends Context {
     // make sure to not merge any global config into local and vice versa
     const getCli = source => this.getContextValueFromOptionalSource(this.keyNames.CLI, source)
     const existingData = merge ? (local ? getCli('local') : getCli('global')) : {}
-    this.setContextValue(`${this.keyNames.CLI}`, { ...existingData, ...contextData }, local)
+    this.setContextValue(`${this.keyNames.CLI}`, { ...existingData.data, ...contextData }, local)
   }
 
   /**
@@ -110,7 +110,10 @@ class ConfigCliContext extends Context {
   /** @private */
   getContextValueFromOptionalSource (key, source) {
     const fullKey = `${this.keyNames.IMS}.${this.keyNames.CONTEXTS}.${key}`
-    return source !== undefined ? this.aioConfig.get(fullKey, source) : this.aioConfig.get(fullKey)
+    return {
+      data: this.aioConfig.get(fullKey, source),
+      local: source === 'local' || (!source && !!this.aioConfig.get(fullKey, 'local'))
+    }
   }
 }
 

diff --git a/src/ctx/Context.js b/src/ctx/Context.js
@@ -51,6 +51,7 @@ class Context {
    *
    *   - `name`: The actual context name used
    *   - `data`: The IMS context data
+   *   - `local`: Whether the context data is stored locally or not
    *
    * @param {string} contextName Name of the context information to return.
    * @returns {Promise<object>} The configuration object
@@ -63,14 +64,16 @@ class Context {
     }
 
     if (contextName) {
+      const { data, local } = await this.getContextValue(contextName)
       return {
         name: contextName,
-        data: await this.getContextValue(contextName)
+        data,
+        local
       }
     }
 
     // missing context and no current context
-    return { name: contextName, data: undefined }
+    return { name: contextName, data: undefined, local: false }
   }
 
   /**
@@ -139,7 +142,7 @@ class Context {
 
   /**
    * @param {string} contextName context name
-   * @returns {Promise<any>} context value
+   * @returns {Promise<{data: any, local: boolean}>} context value
    * @protected
    * @ignore
    */

diff --git a/src/ctx/StateActionContext.js b/src/ctx/StateActionContext.js
@@ -58,7 +58,10 @@ class StateActionContext extends Context {
     aioLogger.debug('getContextValue(%s)', key)
     // on first run load the tokens from the cloud State
     await this.loadTokensOnce()
-    return cloneDeep(this.data[this.keyNames.CONTEXTS][key])
+    return {
+      data: cloneDeep(this.data[this.keyNames.CONTEXTS][key]),
+      local: false
+    }
   }
 
   /**

diff --git a/src/token-helper.js b/src/token-helper.js
@@ -47,13 +47,15 @@ const IMS_TOKEN_MANAGER = {
 
     return this._resolveContext(contextName)
       .then(context => { return { ...context, result: this._getOrCreateToken(context.data, options) } })
-      .then(result => this._persistTokens(result.name, result.data, result.result))
+      .then(result => this._persistTokens(result.name, result.data, result.result, result.local))
   },
 
   async invalidateToken (contextName, force) {
     aioLogger.debug('invalidateToken(%s, %s)', contextName, force)
 
     const tokenLabel = force ? REFRESH_TOKEN : ACCESS_TOKEN
+    // check if token is stored locally, important if context is present both globally and locally
+    const { local } = await this._context.get(`${contextName}.${tokenLabel}`)
     const { name, data } = await this._resolveContext(contextName)
     const ims = new Ims(data.env)
     return this.getTokenIfValid(data[tokenLabel])
@@ -66,11 +68,12 @@ const IMS_TOKEN_MANAGER = {
       })
       .then(token => ims.invalidateToken(token, data.client_id, data.client_secret))
       .then(() => {
-        delete data[tokenLabel]
+        // remove only the token(s), in order to avoid accidentally storeing merged local and global values
+        this._context.set(`${contextName}.${tokenLabel}`, undefined, local)
         if (force) {
-          delete data[ACCESS_TOKEN]
+          this._context.set(`${contextName}.${ACCESS_TOKEN}`, undefined, local)
         }
-        return this._context.set(name, data)
+        return this._context.get(name)
       })
   },
 
@@ -144,9 +147,10 @@ const IMS_TOKEN_MANAGER = {
    * @param {string} context the ims context name
    * @param {object} contextData the ims context data to persist
    * @param {Promise} resultPromise the promise that contains the results (access token, or access token and refresh token)
+   * @param {boolean} local whether or not the token should be persisted locally, defaults to false
    * @returns {Promise<string>} resolves to the access token
    */
-  async _persistTokens (context, contextData, resultPromise) {
+  async _persistTokens (context, contextData, resultPromise, local = false) {
     aioLogger.debug('persistTokens(%s, %o, %o)', context, contextData, resultPromise)
 
     const result = await resultPromise
@@ -155,12 +159,11 @@ const IMS_TOKEN_MANAGER = {
     }
 
     return this.getTokenIfValid(result.access_token)
-      .then(() => { contextData.access_token = result.access_token })
+      .then(() => this._context.set(`${context}.${ACCESS_TOKEN}`, result.access_token, local))
       .then(() => this.getTokenIfValid(result.refresh_token))
       .then(
-        () => { contextData.refresh_token = result.refresh_token },
+        () => this._context.set(`${context}.${REFRESH_TOKEN}`, result.refresh_token, local),
         () => true)
-      .then(() => this._context.set(context, contextData))
       .then(() => result.access_token.token)
   },
 

diff --git a/test/ctx/ConfigCliContext.test.js b/test/ctx/ConfigCliContext.test.js
@@ -11,10 +11,47 @@ governing permissions and limitations under the License.
 */
 
 const TheContext = require('../../src/ctx/ConfigCliContext')
-
+const { merge, getValue, setValue } = require('@adobe/aio-lib-core-config/src/util')
 const aioConfig = require('@adobe/aio-lib-core-config')
 jest.mock('@adobe/aio-lib-core-config')
 
+/**
+ * Utility to merge values into an object. Intended for us within
+ * `mockConfig`'s `configCallback`.
+ * @param obj the object to modify
+ * @param key the dot-separated deep key of the value
+ * @param value the value to assign to the key
+ * @return void
+ * @private
+ */
+function addValue (obj, key, value) {
+  Object.assign(obj, setValue(key, value, obj))
+}
+
+/**
+ * Utility for mocking configs with 'local' and 'global' parts
+ *
+ * @param configCallback callback that receives a "global" and a "local"
+ *                       config object, both of which can be modified
+ * @return returns a function compatible with mockImplementation calls on aioConfig.get()
+ * @private
+ */
+function mockConfig (configCallback) {
+  const config = {
+    global: {},
+    local: {}
+  }
+  configCallback(config.global, config.local)
+
+  return (key, source) => {
+    if (!source) {
+      const result = merge(config.global, config.local)
+      return getValue(result, key)
+    }
+    return getValue(config[source], key)
+  }
+}
+
 const keyNames = {
   IMS: 'a',
   CONFIG: 'b',
@@ -40,9 +77,12 @@ describe('constructor', () => {
 
 describe('getCli', () => {
   test('(<no args>)', async () => {
-    aioConfig.get.mockReturnValue('value')
+    aioConfig.get.mockImplementation(mockConfig((global, _) => {
+      addValue(global, `${keyNames.IMS}.${keyNames.CONTEXTS}.${keyNames.CLI}`, 'value')
+    }))
     await expect(context.getCli()).resolves.toEqual('value')
-    expect(aioConfig.get).toHaveBeenCalledWith(`${keyNames.IMS}.${keyNames.CONTEXTS}.${keyNames.CLI}`)
+    expect(aioConfig.get).toHaveBeenNthCalledWith(1, `${keyNames.IMS}.${keyNames.CONTEXTS}.${keyNames.CLI}`, undefined)
+    expect(aioConfig.get).toHaveBeenNthCalledWith(2, `${keyNames.IMS}.${keyNames.CONTEXTS}.${keyNames.CLI}`, 'local')
   })
 })
 
@@ -55,7 +95,9 @@ describe('setCli', () => {
   })
 
   test('{ the: value }, prev={ another: fakevalue }', async () => {
-    aioConfig.get.mockReturnValue({ another: 'fakevalue' })
+    aioConfig.get.mockImplementation(mockConfig((global, _) => {
+      addValue(global, `${keyNames.IMS}.${keyNames.CONTEXTS}.${keyNames.CLI}`, { another: 'fakevalue' })
+    }))
     await expect(context.setCli({ the: 'value' })).resolves.toEqual(undefined)
     expect(aioConfig.get).toHaveBeenCalledWith(`${keyNames.IMS}.${keyNames.CONTEXTS}.${keyNames.CLI}`, 'global')
     expect(aioConfig.set).toHaveBeenCalledWith(`${keyNames.IMS}.${keyNames.CONTEXTS}.${keyNames.CLI}`, { the: 'value', another: 'fakevalue' }, false)
@@ -88,9 +130,14 @@ describe('setCli', () => {
 
 describe('getContextValue', () => {
   test('(fake)', async () => {
-    aioConfig.get.mockReturnValue('value')
-    await expect(context.getContextValue('fake')).resolves.toEqual('value')
-    expect(aioConfig.get).toHaveBeenCalledWith(`${keyNames.IMS}.${keyNames.CONTEXTS}.fake`)
+    aioConfig.get.mockImplementation(mockConfig((global, _) => {
+      addValue(global, `${keyNames.IMS}.${keyNames.CONTEXTS}.fake`, 'value')
+    }))
+    await expect(context.getContextValue('fake')).resolves.toEqual({ data: 'value', local: false })
+    expect(aioConfig.get)
+      .toHaveBeenNthCalledWith(1, `${keyNames.IMS}.${keyNames.CONTEXTS}.fake`, undefined)
+    expect(aioConfig.get)
+      .toHaveBeenNthCalledWith(2, `${keyNames.IMS}.${keyNames.CONTEXTS}.fake`, 'local')
   })
 })
 

diff --git a/test/ctx/Context.test.js b/test/ctx/Context.test.js
@@ -70,25 +70,25 @@ describe('setCurrent', () => {
 describe('get', () => {
   test('(<no args>), current=fake', async () => {
     context.getConfigValue = jest.fn().mockResolvedValue('fake')
-    context.getContextValue = jest.fn().mockResolvedValue({ fake: 'data' })
+    context.getContextValue = jest.fn().mockResolvedValue({ data: { fake: 'data' }, local: true })
     const ret = await context.get()
-    expect(ret).toEqual({ data: { fake: 'data' }, name: 'fake' })
+    expect(ret).toEqual({ data: { fake: 'data' }, name: 'fake', local: true })
     expect(context.getContextValue).toHaveBeenCalledWith('fake')
     expect(context.getConfigValue).toHaveBeenCalledWith(keyNames.CURRENT)
   })
   test('(<no args>), current=undefined', async () => {
     context.getConfigValue = jest.fn().mockResolvedValue(undefined)
-    context.getContextValue = jest.fn().mockResolvedValue({ fake: 'data' })
+    context.getContextValue = jest.fn().mockResolvedValue({ data: { fake: 'data' }, local: true })
     const ret = await context.get()
-    expect(ret).toEqual({ data: undefined, name: undefined })
+    expect(ret).toEqual({ data: undefined, name: undefined, local: false })
     expect(context.getContextValue).toHaveBeenCalledTimes(0)
     expect(context.getConfigValue).toHaveBeenCalledWith(keyNames.CURRENT)
   })
   test('(fake)', async () => {
     context.getConfigValue = jest.fn().mockResolvedValue(undefined)
-    context.getContextValue = jest.fn().mockResolvedValue({ fake: 'data' })
+    context.getContextValue = jest.fn().mockResolvedValue({ data: { fake: 'data' }, local: false })
     const ret = await context.get('fake')
-    expect(ret).toEqual({ data: { fake: 'data' }, name: 'fake' })
+    expect(ret).toEqual({ data: { fake: 'data' }, name: 'fake', local: false })
     expect(context.getContextValue).toHaveBeenCalledWith('fake')
     expect(context.getConfigValue).toHaveBeenCalledTimes(0)
   })

diff --git a/test/ctx/StateActionContext.test.js b/test/ctx/StateActionContext.test.js
@@ -107,21 +107,21 @@ describe('setConfigValue', () => {
 
 describe('getContextValue', () => {
   test('key=fake, context data = {}, no state tokens', async () => {
-    await expect(context.getContextValue('fake')).resolves.toEqual(undefined)
+    expect((await context.getContextValue('fake')).data).toEqual(undefined)
     // does interact with state
     expect(context.state).toBe(mockStateInstance)
     expect(mockState.init).toHaveBeenCalledTimes(1)
   })
   test('2 calls: key=fake, context data = {}, no state tokens', async () => {
-    await expect(context.getContextValue('fake')).resolves.toEqual(undefined)
-    await expect(context.getContextValue('fake')).resolves.toEqual(undefined)
+    expect((await context.getContextValue('fake')).data).toEqual(undefined)
+    expect((await context.getContextValue('fake')).data).toEqual(undefined)
     // does interact with state, make sure we init once only
     expect(context.state).toBe(mockStateInstance)
     expect(mockState.init).toHaveBeenCalledTimes(1)
   })
   test('key=fake, context data = { fake: value }, no state tokens', async () => {
     context.data[keyNames.CONTEXTS].fake = 'value'
-    await expect(context.getContextValue('fake')).resolves.toEqual('value')
+    expect((await context.getContextValue('fake')).data).toEqual('value')
     expect(cloneDeep).toHaveBeenCalledWith('value')
     // does interact with state
     expect(context.state).toBe(mockStateInstance)
@@ -131,7 +131,7 @@ describe('getContextValue', () => {
   test('key=fake, context data = { fake: value, fake2: value2 }, no state tokens', async () => {
     context.data[keyNames.CONTEXTS].fake = 'value'
     context.data[keyNames.CONTEXTS].fake2 = 'value2'
-    await expect(context.getContextValue('fake')).resolves.toEqual('value')
+    expect((await context.getContextValue('fake')).data).toEqual('value')
     expect(cloneDeep).toHaveBeenCalledWith('value')
     // does interact with state twice
     expect(context.state).toBe(mockStateInstance)
@@ -150,7 +150,7 @@ describe('getContextValue', () => {
       if (k.endsWith('fake2')) return { value: { access_token: 789, other: 'dontcare' } }
       if (k.endsWith('fake3')) return { value: { refresh_token: 120, other: 'dontcare' } }
     })
-    await expect(context.getContextValue('fake')).resolves.toEqual({ the: 'value', access_token: 123, refresh_token: 456 })
+    expect((await context.getContextValue('fake')).data).toEqual({ the: 'value', access_token: 123, refresh_token: 456 })
     expect(cloneDeep).toHaveBeenCalledWith({ the: 'value', access_token: 123, refresh_token: 456 })
     // does interact with state twice
     expect(context.state).toBe(mockStateInstance)

diff --git a/test/token-helper.test.js b/test/token-helper.test.js
@@ -169,6 +169,41 @@ test('getTokenWithOptions - string (jwt)', async () => {
   await expect(IMS_TOKEN_MANAGER.getToken(contextName, { testToken: '123abc' })).resolves.toEqual('123abc')
 })
 
+test('getToken - string (jwt) with local=false', async () => {
+  const contextName = 'known-context-jwt'
+  const context = {
+    [contextName]: {
+      client_id: 'bar',
+      client_secret: 'baz',
+      technical_account_id: 'foo@bar',
+      meta_scopes: [],
+      ims_org_id: 'ABCDEFG',
+      private_key: 'XYXYXYX'
+    }
+  }
+
+  setImsPluginMock('jwt', 'abc123')
+  config.get.mockImplementation(
+    createHandlerForContext(context)
+  )
+
+  // Mock _persistTokens to use local=false
+  const originalPersistTokens = IMS_TOKEN_MANAGER._persistTokens
+  const persistTokensMock = jest.fn((context, contextData, resultPromise, local) => {
+    return originalPersistTokens.call(IMS_TOKEN_MANAGER, context, contextData, resultPromise)
+  })
+  IMS_TOKEN_MANAGER._persistTokens = persistTokensMock
+
+  // no force
+  await expect(IMS_TOKEN_MANAGER.getToken(contextName, false)).resolves.toEqual('abc123')
+
+  // force
+  await expect(IMS_TOKEN_MANAGER.getToken(contextName, true)).resolves.toEqual('abc123')
+
+  // Restore the original _persistTokens method
+  IMS_TOKEN_MANAGER._persistTokens = originalPersistTokens
+})
+
 describe('WEBPACK_ACTION_BUILD set', () => {
   let ISOLATED_TOKEN_MANAGER
   beforeEach(() => {