From f4873a1ba3ac27cb0ed6c764132995acf35ebef1 Mon Sep 17 00:00:00 2001 From: Dominique Pfister Date: Fri, 12 Jul 2024 17:44:12 +0200 Subject: [PATCH] fix: MemCachePlugin should check equality before calling base (#972) * fix: MemCachePlugin should check equality before calling base * fix: fix message --- .../src/MemCachePlugin.js | 7 ++- .../src/S3CachePlugin.js | 17 +----- .../src/getCachePlugin.js | 2 +- ...manager.test.js => FSCacheManager.test.js} | 0 ...e-plugin.test.js => FSCachePlugin.test.js} | 0 ...-plugin.test.js => MemCachePlugin.test.js} | 0 ...manager.test.js => S3CacheManager.test.js} | 0 ...e-plugin.test.js => S3CachePlugin.test.js} | 19 ------- .../test/getCachePlugin.test.js | 56 +++++++++++++++++-- 9 files changed, 60 insertions(+), 41 deletions(-) rename packages/helix-shared-tokencache/test/{fs-cache-manager.test.js => FSCacheManager.test.js} (100%) rename packages/helix-shared-tokencache/test/{fs-cache-plugin.test.js => FSCachePlugin.test.js} (100%) rename packages/helix-shared-tokencache/test/{mem-cache-plugin.test.js => MemCachePlugin.test.js} (100%) rename packages/helix-shared-tokencache/test/{s3-cache-manager.test.js => S3CacheManager.test.js} (100%) rename packages/helix-shared-tokencache/test/{s3-cache-plugin.test.js => S3CachePlugin.test.js} (95%) diff --git a/packages/helix-shared-tokencache/src/MemCachePlugin.js b/packages/helix-shared-tokencache/src/MemCachePlugin.js index 9c290d39..0e42d580 100644 --- 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 index 79c088d5..998135db 100644 --- 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 index cb45f249..093b8ade 100644 --- 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/packages/helix-shared-tokencache/test/fs-cache-manager.test.js b/packages/helix-shared-tokencache/test/FSCacheManager.test.js similarity index 100% rename from packages/helix-shared-tokencache/test/fs-cache-manager.test.js rename to packages/helix-shared-tokencache/test/FSCacheManager.test.js diff --git a/packages/helix-shared-tokencache/test/fs-cache-plugin.test.js b/packages/helix-shared-tokencache/test/FSCachePlugin.test.js similarity index 100% rename from packages/helix-shared-tokencache/test/fs-cache-plugin.test.js rename to packages/helix-shared-tokencache/test/FSCachePlugin.test.js diff --git a/packages/helix-shared-tokencache/test/mem-cache-plugin.test.js b/packages/helix-shared-tokencache/test/MemCachePlugin.test.js similarity index 100% rename from packages/helix-shared-tokencache/test/mem-cache-plugin.test.js rename to packages/helix-shared-tokencache/test/MemCachePlugin.test.js diff --git a/packages/helix-shared-tokencache/test/s3-cache-manager.test.js b/packages/helix-shared-tokencache/test/S3CacheManager.test.js similarity index 100% rename from packages/helix-shared-tokencache/test/s3-cache-manager.test.js rename to packages/helix-shared-tokencache/test/S3CacheManager.test.js diff --git a/packages/helix-shared-tokencache/test/s3-cache-plugin.test.js b/packages/helix-shared-tokencache/test/S3CachePlugin.test.js similarity index 95% rename from packages/helix-shared-tokencache/test/s3-cache-plugin.test.js rename to packages/helix-shared-tokencache/test/S3CachePlugin.test.js index 147fb31f..b6fbedc1 100644 --- a/packages/helix-shared-tokencache/test/s3-cache-plugin.test.js +++ b/packages/helix-shared-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 index 33bd262f..705fd7e5 100644 --- 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); + }); });