From 73adb11a205f0e92a7d9febd210656ceda923449 Mon Sep 17 00:00:00 2001 From: Kamil Kisiela Date: Mon, 21 Nov 2022 17:23:22 +0100 Subject: [PATCH] Use null when token is not found (#658) --- integration-tests/testkit/flow.ts | 20 ++++ integration-tests/tests/api/tokens.spec.ts | 91 +++++++++++++++++++ packages/libraries/client/src/version.ts | 2 +- .../modules/token/providers/token-storage.ts | 16 +++- packages/services/storage/src/tokens.ts | 2 +- packages/services/tokens/src/api.ts | 36 ++++++-- packages/services/tokens/src/cache.ts | 10 ++ packages/services/tokens/src/index.ts | 15 ++- packages/services/tokens/src/storage.ts | 6 +- 9 files changed, 180 insertions(+), 18 deletions(-) create mode 100644 integration-tests/tests/api/tokens.spec.ts diff --git a/integration-tests/testkit/flow.ts b/integration-tests/testkit/flow.ts index 515501d9a..08e30562a 100644 --- a/integration-tests/testkit/flow.ts +++ b/integration-tests/testkit/flow.ts @@ -8,6 +8,7 @@ import type { CreateProjectInput, UpdateProjectNameInput, CreateTokenInput, + DeleteTokensInput, OrganizationMemberAccessInput, SchemaCheckInput, PublishPersistedOperationInput, @@ -297,6 +298,9 @@ export function createToken(input: CreateTokenInput, authToken: string) { createToken(input: $input) { ok { secret + createdToken { + id + } } } } @@ -308,6 +312,22 @@ export function createToken(input: CreateTokenInput, authToken: string) { }); } +export function deleteTokens(input: DeleteTokensInput, authToken: string) { + return execute({ + document: gql(/* GraphQL */ ` + mutation deleteTokens($input: DeleteTokensInput!) { + deleteTokens(input: $input) { + deletedTokens + } + } + `), + authToken, + variables: { + input, + }, + }); +} + export function readTokenInfo(token: string) { return execute({ document: gql(/* GraphQL */ ` diff --git a/integration-tests/tests/api/tokens.spec.ts b/integration-tests/tests/api/tokens.spec.ts new file mode 100644 index 000000000..29ef57140 --- /dev/null +++ b/integration-tests/tests/api/tokens.spec.ts @@ -0,0 +1,91 @@ +import { ProjectType } from '@app/gql/graphql'; +import { createOrganization, createProject, createToken, readTokenInfo, deleteTokens } from '../../testkit/flow'; +import { authenticate } from '../../testkit/auth'; + +test('deleting a token should clear the cache', async () => { + const { access_token: owner_access_token } = await authenticate('main'); + const orgResult = await createOrganization( + { + name: 'foo', + }, + owner_access_token + ); + + const org = orgResult.body.data!.createOrganization.ok!.createdOrganizationPayload.organization; + + const projectResult = await createProject( + { + organization: org.cleanId, + type: ProjectType.Single, + name: 'foo', + }, + owner_access_token + ); + + const project = projectResult.body.data!.createProject.ok!.createdProject; + const target = projectResult.body.data!.createProject.ok!.createdTargets[0]; + + // member should not have access to target:registry:write + const tokenResult = await createToken( + { + name: 'test', + organization: org.cleanId, + project: project.cleanId, + target: target.cleanId, + organizationScopes: [], + projectScopes: [], + targetScopes: [], + }, + owner_access_token + ); + + expect(tokenResult.body.errors).not.toBeDefined(); + const secret = tokenResult.body.data?.createToken.ok?.secret; + const createdToken = tokenResult.body.data?.createToken.ok?.createdToken; + + expect(secret).toBeDefined(); + + let tokenInfoResult = await readTokenInfo(secret!); + expect(tokenInfoResult.body.errors).not.toBeDefined(); + + if (tokenInfoResult.body.data?.tokenInfo.__typename === 'TokenNotFoundError' || !createdToken) { + throw new Error('Token not found'); + } + + const tokenInfo = tokenInfoResult.body.data?.tokenInfo; + // organization + expect(tokenInfo?.hasOrganizationRead).toBe(true); + expect(tokenInfo?.hasOrganizationDelete).toBe(false); + expect(tokenInfo?.hasOrganizationIntegrations).toBe(false); + expect(tokenInfo?.hasOrganizationMembers).toBe(false); + expect(tokenInfo?.hasOrganizationSettings).toBe(false); + // project + expect(tokenInfo?.hasProjectRead).toBe(true); + expect(tokenInfo?.hasProjectDelete).toBe(false); + expect(tokenInfo?.hasProjectAlerts).toBe(false); + expect(tokenInfo?.hasProjectOperationsStoreRead).toBe(false); + expect(tokenInfo?.hasProjectOperationsStoreWrite).toBe(false); + expect(tokenInfo?.hasProjectSettings).toBe(false); + // target + expect(tokenInfo?.hasTargetRead).toBe(true); + expect(tokenInfo?.hasTargetDelete).toBe(false); + expect(tokenInfo?.hasTargetSettings).toBe(false); + expect(tokenInfo?.hasTargetRegistryRead).toBe(false); + expect(tokenInfo?.hasTargetRegistryWrite).toBe(false); + expect(tokenInfo?.hasTargetTokensRead).toBe(false); + expect(tokenInfo?.hasTargetTokensWrite).toBe(false); + + // test invalidation + await deleteTokens( + { + organization: org.cleanId, + project: project.cleanId, + target: target.cleanId, + tokens: [createdToken.id], + }, + owner_access_token + ); + + tokenInfoResult = await readTokenInfo(secret!); + expect(tokenInfoResult.body.errors).toHaveLength(1); +}); diff --git a/packages/libraries/client/src/version.ts b/packages/libraries/client/src/version.ts index 1c5b9fd45..3fcf28293 100644 --- a/packages/libraries/client/src/version.ts +++ b/packages/libraries/client/src/version.ts @@ -1 +1 @@ -export const version = '0.20.0'; +export const version = '0.21.1'; diff --git a/packages/services/api/src/modules/token/providers/token-storage.ts b/packages/services/api/src/modules/token/providers/token-storage.ts index f8604fb97..d67f976b7 100644 --- a/packages/services/api/src/modules/token/providers/token-storage.ts +++ b/packages/services/api/src/modules/token/providers/token-storage.ts @@ -135,11 +135,19 @@ export class TokenStorage { this.logger.debug('Fetching token (token=%s)', maskToken(token)); try { - return await this.tokensService.query('getToken', { token }); - } catch (e: any) { - this.logger.error(e); + const tokenInfo = await this.tokensService.query('getToken', { token }); - throw new HiveError('Invalid token provided!'); + if (!tokenInfo) { + throw new Error('Token not found'); + } + + return tokenInfo; + } catch (error: any) { + this.logger.error(error); + + throw new HiveError('Invalid token provided', { + originalError: error, + }); } } } diff --git a/packages/services/storage/src/tokens.ts b/packages/services/storage/src/tokens.ts index 19ef0a809..e3a2b23ca 100644 --- a/packages/services/storage/src/tokens.ts +++ b/packages/services/storage/src/tokens.ts @@ -24,7 +24,7 @@ export async function createTokenStorage(connection: string, maximumPoolSize: nu return result.rows; }, async getToken({ token }: { token: string }) { - return pool.one>( + return pool.maybeOne>( sql` SELECT * FROM public.tokens diff --git a/packages/services/tokens/src/api.ts b/packages/services/tokens/src/api.ts index a1e374e43..eef83e470 100644 --- a/packages/services/tokens/src/api.ts +++ b/packages/services/tokens/src/api.ts @@ -56,10 +56,17 @@ export type Context = { logger: FastifyLoggerInstance; errorHandler: ReturnType; getStorage: ReturnType['getStorage']; - tokenReadFailuresCache: LruType<{ - error: string; - checkAt: number; - }>; + tokenReadFailuresCache: LruType< + | { + type: 'error'; + error: string; + checkAt: number; + } + | { + type: 'not-found'; + checkAt: number; + } + >; errorCachingInterval: number; }; @@ -186,9 +193,13 @@ export const tokensApiRouter = trpc const failedRead = ctx.tokenReadFailuresCache.get(hash); if (failedRead) { - // let's re-throw the same error + // let's re-throw the same error (or return null) if (failedRead.checkAt >= Date.now()) { - throw new Error(failedRead.error); + if (failedRead.type === 'error') { + throw new Error(failedRead.error); + } else { + return null; + } } // or look for it again if last time we checked was 10 minutes ago } @@ -197,15 +208,26 @@ export const tokensApiRouter = trpc const storage = await ctx.getStorage(); const result = await storage.readToken(hash); - // removes the token from the failures cache + // removes the token from the failures cache (in case the value expired) ctx.tokenReadFailuresCache.delete(hash); + if (!result) { + // set token read as not found + // so we don't try to read it again for next X minutes + ctx.tokenReadFailuresCache.set(hash, { + type: 'not-found', + checkAt: Date.now() + ctx.errorCachingInterval, + }); + } + return result; } catch (error) { ctx.errorHandler(`Failed to get a token "${alias}"`, error as Error, ctx.logger); // set token read as failure + // so we don't try to read it again for next X minutes ctx.tokenReadFailuresCache.set(hash, { + type: 'error', error: (error as Error).message, checkAt: Date.now() + ctx.errorCachingInterval, }); diff --git a/packages/services/tokens/src/cache.ts b/packages/services/tokens/src/cache.ts index 77d2b846a..4fbf047c1 100644 --- a/packages/services/tokens/src/cache.ts +++ b/packages/services/tokens/src/cache.ts @@ -166,6 +166,11 @@ export function useCache( } const item = await readToken(hashed_token); + + if (!item) { + return null; + } + // Read the tokens of the target and cache them await readAndFill(item.target).catch(() => {}); cacheMisses.inc(1); @@ -184,6 +189,11 @@ export function useCache( }), deleteToken: tracker.wrap(async hashed_token => { const item = await cachedStorage.readToken(hashed_token); + + if (!item) { + return; + } + invalidate(item.target); return storage.deleteToken(hashed_token); diff --git a/packages/services/tokens/src/index.ts b/packages/services/tokens/src/index.ts index c74017947..194c60972 100644 --- a/packages/services/tokens/src/index.ts +++ b/packages/services/tokens/src/index.ts @@ -38,10 +38,17 @@ export async function main() { try { const { start, stop, readiness, getStorage } = useCache(createStorage(env.postgres), server.log); - const tokenReadFailuresCache = LRU<{ - error: string; - checkAt: number; - }>(50); + const tokenReadFailuresCache = LRU< + | { + type: 'error'; + error: string; + checkAt: number; + } + | { + type: 'not-found'; + checkAt: number; + } + >(200); // Cache failures for 10 minutes const errorCachingInterval = ms('10m'); diff --git a/packages/services/tokens/src/storage.ts b/packages/services/tokens/src/storage.ts index 9abe5e5d3..c45d89fe6 100644 --- a/packages/services/tokens/src/storage.ts +++ b/packages/services/tokens/src/storage.ts @@ -16,7 +16,7 @@ export interface StorageItem { export interface Storage { destroy(): Promise; readTarget(targetId: string, res?: FastifyReply): Promise; - readToken(token: string, res?: FastifyReply): Promise; + readToken(token: string, res?: FastifyReply): Promise; writeToken(item: Omit): Promise; deleteToken(token: string): Promise; touchTokens(tokens: Array<{ token: string; date: Date }>): Promise; @@ -52,6 +52,10 @@ export async function createStorage(config: Parameters