fix: MemCachePlugin should check equality before calling base (#972)

* fix: MemCachePlugin should check equality before calling base * fix: fix message
adobe · Jul 12, 2024 · f4873a1 · f4873a1
1 parent bdfdd1e
commit f4873a1
Show file tree

Hide file tree

Showing 9 changed files with 60 additions and 41 deletions.
diff --git a/packages/helix-shared-tokencache/src/MemCachePlugin.js b/packages/helix-shared-tokencache/src/MemCachePlugin.js
@@ -9,6 +9,7 @@
  * OF ANY KIND, either express or implied. See the License for the specific language
  * governing permissions and limitations under the License.
  */
+import { isDeepStrictEqual } from 'util';
 import { isAuthTokenEmpty } from './utils.js';
 
 const caches = new Map();
@@ -96,8 +97,12 @@ export class MemCachePlugin {
       log.info('mem: write token cache done, ignoring empty data', this.key);
       return false;
     }
-    log.debug('mem: write token cache', this.key);
     const cache = this.#getOrCreateCache();
+    if (cache.data && isDeepStrictEqual(data, JSON.parse(cache.data))) {
+      log.debug('mem: we were told cache has changed, but contents didn\'t');
+      return false;
+    }
+    log.debug('mem: write token cache', this.key);
     cache.data = JSON.stringify(data);
     if (this.base) {
       log.debug('mem: write token cache done. telling base', this.key);

diff --git a/packages/helix-shared-tokencache/src/S3CachePlugin.js b/packages/helix-shared-tokencache/src/S3CachePlugin.js
@@ -9,7 +9,6 @@
  * OF ANY KIND, either express or implied. See the License for the specific language
  * governing permissions and limitations under the License.
  */
-import { isDeepStrictEqual } from 'util';
 import {
   DeleteObjectCommand,
   GetObjectCommand,
@@ -45,13 +44,12 @@ export class S3CachePlugin {
     this.s3 = new S3Client();
     this.meta = null;
     this.data = null;
-    this.lastModified = null;
   }
 
   async deleteCache() {
     const { log, key, bucket } = this;
     try {
-      log.debug('s3: read token cache', key);
+      log.debug('s3: delete token cache', key);
       await this.s3.send(new DeleteObjectCommand({
         Bucket: bucket,
         Key: key,
@@ -75,7 +73,6 @@ export class S3CachePlugin {
         Bucket: bucket,
         Key: key,
       }));
-      this.lastModified = res.LastModified;
       let raw = await new Response(res.Body, {}).buffer();
       if (secret) {
         raw = decrypt(secret, raw).toString('utf-8');
@@ -113,7 +110,7 @@ export class S3CachePlugin {
 
   async #saveData() {
     const {
-      log, secret, key, bucket, lastModified,
+      log, secret, key, bucket,
     } = this;
     try {
       if (this.readOnly) {
@@ -136,12 +133,6 @@ export class S3CachePlugin {
         Body: raw,
         ContentType: secret ? 'application/octet-stream' : 'text/plain',
       }));
-      this.lastModified = new Date();
-
-      const id = key.split('/')[0];
-      const before = lastModified?.toISOString() ?? 'never';
-      const now = this.lastModified.toISOString();
-      log.info(`s3: write token cache [${id}]: ${before} => ${now}`);
       return true;
     } catch (e) {
       log.warn('s3: unable to serialize token cache', e);
@@ -163,10 +154,6 @@ export class S3CachePlugin {
       log.info('s3: write token cache, ignoring empty data', this.key);
       return false;
     }
-    if (isDeepStrictEqual(data, this.data)) {
-      log.debug('s3: we were told cache has changed, but contents didn\'t');
-      return false;
-    }
     this.data = data;
     return this.#saveData();
   }

diff --git a/packages/helix-shared-tokencache/src/getCachePlugin.js b/packages/helix-shared-tokencache/src/getCachePlugin.js
@@ -82,7 +82,7 @@ export async function getCachePlugin(opts, type) {
     type,
   };
   if (process.env.HELIX_ONEDRIVE_LOCAL_AUTH_CACHE) {
-    cacheOpts.caches = new Map();
+    cacheOpts.caches = opts.caches ?? new Map();
   }
   return new MemCachePlugin(cacheOpts);
 }
diff --git a/...-tokencache/test/fs-cache-manager.test.js → ...ed-tokencache/test/FSCacheManager.test.js b/...-tokencache/test/fs-cache-manager.test.js → ...ed-tokencache/test/FSCacheManager.test.js
diff --git a/...d-tokencache/test/fs-cache-plugin.test.js → ...red-tokencache/test/FSCachePlugin.test.js b/...d-tokencache/test/fs-cache-plugin.test.js → ...red-tokencache/test/FSCachePlugin.test.js
diff --git a/...-tokencache/test/mem-cache-plugin.test.js → ...ed-tokencache/test/MemCachePlugin.test.js b/...-tokencache/test/mem-cache-plugin.test.js → ...ed-tokencache/test/MemCachePlugin.test.js
diff --git a/...-tokencache/test/s3-cache-manager.test.js → ...ed-tokencache/test/S3CacheManager.test.js b/...-tokencache/test/s3-cache-manager.test.js → ...ed-tokencache/test/S3CacheManager.test.js
diff --git a/...d-tokencache/test/s3-cache-plugin.test.js → ...red-tokencache/test/S3CachePlugin.test.js b/...d-tokencache/test/s3-cache-plugin.test.js → ...red-tokencache/test/S3CachePlugin.test.js
@@ -195,25 +195,6 @@ describe('S3CachePlugin Test', () => {
     assert.strictEqual(ret, false);
   });
 
-  it('does not write data to s3 if contents is unchanged', async () => {
-    const p = new S3CachePlugin({
-      bucket: 'test-bucket',
-      key: 'myproject/auth-default/json',
-      secret: '',
-    });
-
-    nock('https://test-bucket.s3.us-east-1.amazonaws.com')
-      .get('/myproject/auth-default/json?x-id=GetObject')
-      .reply(200, JSON.stringify(toAuthContent('1234')));
-
-    const ctx = new MockTokenCacheContext({
-      cacheHasChanged: true,
-    });
-    await p.beforeCacheAccess(ctx);
-    const ret = await p.afterCacheAccess(ctx);
-    assert.strictEqual(ret, false);
-  });
-
   it('does not write data to s3 if contents has empty Account key', async () => {
     const p = new S3CachePlugin({
       bucket: 'test-bucket',

diff --git a/packages/helix-shared-tokencache/test/getCachePlugin.test.js b/packages/helix-shared-tokencache/test/getCachePlugin.test.js
@@ -11,8 +11,10 @@
  */
 /* eslint-env mocha */
 import assert from 'assert';
+import { encrypt } from '../src/index.js';
 import { getCachePlugin } from '../src/getCachePlugin.js';
-import { Nock } from './utils.js';
+import { MockTokenCacheContext } from './MockTokenCacheContext.js';
+import { Nock, toAuthContent } from './utils.js';
 
 const DEFAULT_ENV = {
   AZURE_HELIX_SERVICE_CLIENT_ID: 'client-id',
@@ -38,8 +40,9 @@ describe('getCachePlugin tests', () => {
     process.env = savedProcessEnv;
   });
 
+  const contentBusId = '0123456789abcdef0123456789abcdef0123456789abcdef0123456789a';
+
   it('uses derived opts if contentBusId is available', async () => {
-    const contentBusId = '0123456789abcdef0123456789abcdef0123456789abcdef0123456789a';
     nock('https://helix-content-bus.s3.us-east-1.amazonaws.com')
       .head(`/${contentBusId}/.helix-auth/auth-onedrive-content.json`)
       .reply(200);
@@ -54,7 +57,6 @@ describe('getCachePlugin tests', () => {
   });
 
   it('contentBusId has precedence over owner', async () => {
-    const contentBusId = '0123456789abcdef0123456789abcdef0123456789abcdef0123456789a';
     nock('https://helix-content-bus.s3.us-east-1.amazonaws.com')
       .head(`/${contentBusId}/.helix-auth/auth-onedrive-content.json`)
       .reply(200);
@@ -70,7 +72,6 @@ describe('getCachePlugin tests', () => {
   });
 
   it('falls back to owner if contentBusId not found', async () => {
-    const contentBusId = '0123456789abcdef0123456789abcdef0123456789abcdef0123456789a';
     nock('https://helix-content-bus.s3.us-east-1.amazonaws.com')
       .head(`/${contentBusId}/.helix-auth/auth-onedrive-content.json`)
       .reply(404);
@@ -89,7 +90,6 @@ describe('getCachePlugin tests', () => {
   });
 
   it('falls back to default if contentBusId and owner not found', async () => {
-    const contentBusId = '0123456789abcdef0123456789abcdef0123456789abcdef0123456789a';
     nock('https://helix-content-bus.s3.us-east-1.amazonaws.com')
       .head(`/${contentBusId}/.helix-auth/auth-onedrive-content.json`)
       .reply(404);
@@ -117,4 +117,50 @@ describe('getCachePlugin tests', () => {
     assert.ok(cachePlugin);
     assert.strictEqual(cachePlugin.location, 'helix-content-bus/default/.helix-auth/auth-onedrive-content.json');
   });
+
+  it('does not store back unchanged global auth', async () => {
+    nock('https://helix-content-bus.s3.us-east-1.amazonaws.com')
+      .head(`/${contentBusId}/.helix-auth/auth-onedrive-content.json`)
+      .twice()
+      .reply(200)
+      .get(`/${contentBusId}/.helix-auth/auth-onedrive-content.json?x-id=GetObject`)
+      .reply(200, encrypt(contentBusId, JSON.stringify(toAuthContent('1234'))));
+
+    const caches = new Map();
+
+    // step 1: create plugin and read token from underlying S3 storage
+    let cachePlugin = await getCachePlugin({
+      log: console,
+      env: DEFAULT_ENV,
+      contentBusId,
+      caches,
+    }, 'onedrive');
+
+    await cachePlugin.beforeCacheAccess(
+      new MockTokenCacheContext({
+        cacheHasChanged: true,
+      }),
+    );
+
+    // step 2: verify cache has been filled
+    assert.strictEqual(caches.size, 1);
+
+    // step 3: create another plugin instance with the same cache
+    cachePlugin = await getCachePlugin({
+      log: console,
+      env: DEFAULT_ENV,
+      contentBusId,
+      caches,
+    }, 'onedrive');
+
+    // step 4: read cache back into context
+    const ctx = new MockTokenCacheContext({
+      cacheHasChanged: true,
+    });
+    await cachePlugin.beforeCacheAccess(ctx);
+
+    // step 5: and write cache back without changes
+    const ret = await cachePlugin.afterCacheAccess(ctx);
+    assert.strictEqual(ret, false);
+  });
 });