From a9779a51e81bee9182c8d4e893e338e6097ff706 Mon Sep 17 00:00:00 2001 From: Hongbo Wu Date: Tue, 16 Apr 2024 09:56:57 +0800 Subject: [PATCH 1/4] fix: LABEL_ALREADY_EXIST bug occured during label creation * comparing both user_id and label name when looking for an existing matching label --- packages/api/src/repository/label.ts | 7 ++++-- packages/api/src/resolvers/labels/index.ts | 2 +- packages/api/test/resolvers/labels.test.ts | 28 +++++++++++++++++++++- 3 files changed, 33 insertions(+), 4 deletions(-) diff --git a/packages/api/src/repository/label.ts b/packages/api/src/repository/label.ts index baaac24bb..0faebd3db 100644 --- a/packages/api/src/repository/label.ts +++ b/packages/api/src/repository/label.ts @@ -46,9 +46,12 @@ export const labelRepository = appDataSource.getRepository(Label).extend({ return this.findOneBy({ id }) }, - findByName(name: string) { + findByName(name: string, userId: string) { return this.createQueryBuilder() - .where('LOWER(name) = LOWER(:name)', { name }) // case insensitive + .where('user_id = :userId AND LOWER(name) = LOWER(:name)', { + name, + userId, + }) // case insensitive .getOne() }, diff --git a/packages/api/src/resolvers/labels/index.ts b/packages/api/src/resolvers/labels/index.ts index 1da2f3053..cd5ec824b 100644 --- a/packages/api/src/resolvers/labels/index.ts +++ b/packages/api/src/resolvers/labels/index.ts @@ -90,7 +90,7 @@ export const createLabelResolver = authorized< CreateLabelError, MutationCreateLabelArgs >(async (_, { input }, { authTrx, uid }) => { - const existingLabel = await labelRepository.findByName(input.name) + const existingLabel = await labelRepository.findByName(input.name, uid) if (existingLabel) { return { errorCodes: [CreateLabelErrorCode.LabelAlreadyExists], diff --git a/packages/api/test/resolvers/labels.test.ts b/packages/api/test/resolvers/labels.test.ts index 8d1175c78..0dccc54d1 100644 --- a/packages/api/test/resolvers/labels.test.ts +++ b/packages/api/test/resolvers/labels.test.ts @@ -145,7 +145,7 @@ describe('Labels API', () => { }) }) - context('when name exists', () => { + context('when name exists in the user library', () => { let existingLabel: Label before(async () => { @@ -177,6 +177,32 @@ describe('Labels API', () => { }) }) + context('when name exists in the other user library', () => { + let existingLabel: Label + let otherUser: User + + before(async () => { + otherUser = await createTestUser('otherUser') + existingLabel = await createLabel('label3', '#ffffff', otherUser.id) + }) + + after(async () => { + // delete other user will also delete the label + await deleteUser(otherUser.id) + }) + + it('creates the label', async () => { + const res = await graphqlRequest(query, authToken, { + input: { name: existingLabel.name }, + }).expect(200) + const label = await findLabelById( + res.body.data.createLabel.label.id, + user.id + ) + expect(label).to.exist + }) + }) + it('responds status code 400 when invalid query', async () => { const invalidQuery = ` mutation { From d79cc1c39fedd5c405c17165bce6240b3034893a Mon Sep 17 00:00:00 2001 From: Hongbo Wu Date: Tue, 16 Apr 2024 11:26:29 +0800 Subject: [PATCH 2/4] wrap select in a auth transaction --- packages/api/src/resolvers/labels/index.ts | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/packages/api/src/resolvers/labels/index.ts b/packages/api/src/resolvers/labels/index.ts index cd5ec824b..2e3b9c6d2 100644 --- a/packages/api/src/resolvers/labels/index.ts +++ b/packages/api/src/resolvers/labels/index.ts @@ -90,17 +90,22 @@ export const createLabelResolver = authorized< CreateLabelError, MutationCreateLabelArgs >(async (_, { input }, { authTrx, uid }) => { - const existingLabel = await labelRepository.findByName(input.name, uid) - if (existingLabel) { + const label = await authTrx(async (tx) => { + const repo = tx.withRepository(labelRepository) + const existingLabel = await repo.findByName(input.name, uid) + if (existingLabel) { + return null + } + + return repo.createLabel(input, uid) + }) + + if (!label) { return { errorCodes: [CreateLabelErrorCode.LabelAlreadyExists], } } - const label = await authTrx(async (tx) => - tx.withRepository(labelRepository).createLabel(input, uid) - ) - analytics.capture({ distinctId: uid, event: 'label_created', From 6ce95194ee20443c8d716b9d28a2f3e33a2718cb Mon Sep 17 00:00:00 2001 From: Hongbo Wu Date: Tue, 16 Apr 2024 11:41:19 +0800 Subject: [PATCH 3/4] alter read_labels policy to check user_id --- .../migrations/0173.do.alter_labels_table_policy.sql | 11 +++++++++++ .../0173.undo.alter_labels_table_policy.sql | 11 +++++++++++ 2 files changed, 22 insertions(+) create mode 100755 packages/db/migrations/0173.do.alter_labels_table_policy.sql create mode 100755 packages/db/migrations/0173.undo.alter_labels_table_policy.sql diff --git a/packages/db/migrations/0173.do.alter_labels_table_policy.sql b/packages/db/migrations/0173.do.alter_labels_table_policy.sql new file mode 100755 index 000000000..1e7f4afdb --- /dev/null +++ b/packages/db/migrations/0173.do.alter_labels_table_policy.sql @@ -0,0 +1,11 @@ +-- Type: DO +-- Name: alter_labels_table_policy +-- Description: Alter labels table select policy to check user_id + +BEGIN; + +ALTER POLICY read_labels ON omnivore.labels + TO omnivore_user + USING (user_id = omnivore.get_current_user_id()); + +COMMIT; diff --git a/packages/db/migrations/0173.undo.alter_labels_table_policy.sql b/packages/db/migrations/0173.undo.alter_labels_table_policy.sql new file mode 100755 index 000000000..552a9e2c7 --- /dev/null +++ b/packages/db/migrations/0173.undo.alter_labels_table_policy.sql @@ -0,0 +1,11 @@ +-- Type: UNDO +-- Name: alter_labels_table_policy +-- Description: Alter labels table select policy to check user_id + +BEGIN; + +ALTER POLICY read_labels ON omnivore.labels + TO omnivore_user + USING (true); + +COMMIT; From 8e31ac4feab5b41a6ea78e6fceab339a2178b4f7 Mon Sep 17 00:00:00 2001 From: Hongbo Wu Date: Tue, 16 Apr 2024 12:24:20 +0800 Subject: [PATCH 4/4] fix tests --- packages/api/src/resolvers/highlight/index.ts | 9 +++++---- packages/api/test/resolvers/highlight.test.ts | 5 ++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/api/src/resolvers/highlight/index.ts b/packages/api/src/resolvers/highlight/index.ts index 8e0f22c9e..a142b3299 100644 --- a/packages/api/src/resolvers/highlight/index.ts +++ b/packages/api/src/resolvers/highlight/index.ts @@ -81,7 +81,7 @@ export const mergeHighlightResolver = authorized< MergeHighlightSuccess, MergeHighlightError, MutationMergeHighlightArgs ->(async (_, { input }, { log, pubsub, uid }) => { +>(async (_, { input }, { authTrx, log, pubsub, uid }) => { const { overlapHighlightIdList, ...newHighlightInput } = input /* Compute merged annotation form the order of highlights appearing on page */ @@ -90,9 +90,10 @@ export const mergeHighlightResolver = authorized< const mergedColors: string[] = [] try { - const existingHighlights = await highlightRepository.findByLibraryItemId( - input.articleId, - uid + const existingHighlights = await authTrx((tx) => + tx + .withRepository(highlightRepository) + .findByLibraryItemId(input.articleId, uid) ) existingHighlights.forEach((highlight) => { diff --git a/packages/api/test/resolvers/highlight.test.ts b/packages/api/test/resolvers/highlight.test.ts index a81602cd1..aef7aecae 100644 --- a/packages/api/test/resolvers/highlight.test.ts +++ b/packages/api/test/resolvers/highlight.test.ts @@ -1,10 +1,9 @@ +/* eslint-disable @typescript-eslint/restrict-template-expressions */ import * as chai from 'chai' import { expect } from 'chai' import chaiString from 'chai-string' import 'mocha' -import { Highlight } from '../../src/entity/highlight' import { User } from '../../src/entity/user' -import { getRepository } from '../../src/repository' import { createHighlight, deleteHighlightById, @@ -157,7 +156,7 @@ describe('Highlights API', () => { .post('/local/debug/fake-user-login') .send({ fakeEmail: user.email }) - authToken = res.body.authToken + authToken = res.body.authToken as string itemId = (await createTestLibraryItem(user.id)).id })