From cffb5088b414075d0b34412bf472afc35cf91300 Mon Sep 17 00:00:00 2001 From: Jackson Harper Date: Thu, 9 Jun 2022 11:37:14 -0700 Subject: [PATCH 1/8] If creating a request from a file URL, dont try fuzzy matching on URL --- .../api/src/resolvers/upload_files/index.ts | 25 +++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/packages/api/src/resolvers/upload_files/index.ts b/packages/api/src/resolvers/upload_files/index.ts index d0888c3f8..e766527da 100644 --- a/packages/api/src/resolvers/upload_files/index.ts +++ b/packages/api/src/resolvers/upload_files/index.ts @@ -19,6 +19,12 @@ import { env } from '../../env' import { createPage, getPageByParam, updatePage } from '../../elastic/pages' import { PageType } from '../../elastic/types' import { generateSlug } from '../../utils/helpers' +import { validateUrl } from '../../services/create_page_save_request' + +const isFileUrl = (url: string): boolean => { + const parsedUrl = new URL(url) + return parsedUrl.protocol == 'file://' +} export const uploadFileRequestResolver: ResolverFn< UploadFileRequestResult, @@ -60,6 +66,17 @@ export const uploadFileRequestResolver: ResolverFn< if (!fileName) { fileName = 'content.pdf' } + + if (!isFileUrl(url)) { + try { + validateUrl(url) + } catch (error) { + console.log('illegal file input url', error) + return { + errorCodes: [UploadFileRequestErrorCode.BadInput], + } + } + } } catch { return { errorCodes: [UploadFileRequestErrorCode.BadInput] } } @@ -84,10 +101,14 @@ export const uploadFileRequestResolver: ResolverFn< let createdPageId: string | undefined = undefined if (input.createPageEntry) { - const page = await getPageByParam({ + // If we have a file:// URL, don't try to match it + // and create a copy of the page, just create a + // new item. + const page = isFileUrl(input.url) ? await getPageByParam({ userId: claims.uid, url: input.url, - }) + }) : undefined + if (page) { if ( !(await updatePage( From ef072cb2c9339d63d59ba2c3001f0f24e723984b Mon Sep 17 00:00:00 2001 From: Jackson Harper Date: Thu, 9 Jun 2022 11:51:01 -0700 Subject: [PATCH 2/8] When creating pages from uploaded files use their signed URL as the page URL --- packages/api/src/datalayer/upload_files/model.ts | 2 +- packages/api/src/resolvers/upload_files/index.ts | 13 +++++++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/packages/api/src/datalayer/upload_files/model.ts b/packages/api/src/datalayer/upload_files/model.ts index c6a2055d8..49db669e2 100644 --- a/packages/api/src/datalayer/upload_files/model.ts +++ b/packages/api/src/datalayer/upload_files/model.ts @@ -46,7 +46,7 @@ export const createKeys = exclude(keys, defaultedKeys) export type CreateSet = PickTuple & Partialize -export const updateKeys = ['status'] as const +export const updateKeys = ['url', 'status'] as const export type UpdateSet = PickTuple diff --git a/packages/api/src/resolvers/upload_files/index.ts b/packages/api/src/resolvers/upload_files/index.ts index e766527da..cc689ded2 100644 --- a/packages/api/src/resolvers/upload_files/index.ts +++ b/packages/api/src/resolvers/upload_files/index.ts @@ -23,7 +23,7 @@ import { validateUrl } from '../../services/create_page_save_request' const isFileUrl = (url: string): boolean => { const parsedUrl = new URL(url) - return parsedUrl.protocol == 'file://' + return parsedUrl.protocol == 'file:' } export const uploadFileRequestResolver: ResolverFn< @@ -99,6 +99,15 @@ export const uploadFileRequestResolver: ResolverFn< input.contentType ) + // If this is a file URL, we swap in the GCS signed + // URL + if (isFileUrl(input.url)) { + await models.uploadFile.update(uploadFileData.id, { + url: uploadSignedUrl, + status: UploadFileStatus.Initialized, + }) + } + let createdPageId: string | undefined = undefined if (input.createPageEntry) { // If we have a file:// URL, don't try to match it @@ -126,7 +135,7 @@ export const uploadFileRequestResolver: ResolverFn< } else { const pageId = await createPage( { - url: input.url, + url: isFileUrl(input.url) ? uploadSignedUrl : input.url, id: input.clientRequestId || '', userId: claims.uid, title: title, From e23eada168df50565aa59d41724c9d70d4f4af6e Mon Sep 17 00:00:00 2001 From: Jackson Harper Date: Thu, 9 Jun 2022 11:59:01 -0700 Subject: [PATCH 3/8] Use the public GCS URL for local uploaded files --- packages/api/src/resolvers/upload_files/index.ts | 10 ++++++---- packages/api/src/utils/uploads.ts | 7 +++++++ 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/packages/api/src/resolvers/upload_files/index.ts b/packages/api/src/resolvers/upload_files/index.ts index cc689ded2..64174da9e 100644 --- a/packages/api/src/resolvers/upload_files/index.ts +++ b/packages/api/src/resolvers/upload_files/index.ts @@ -11,6 +11,7 @@ import { WithDataSourcesContext } from '../types' import { generateUploadSignedUrl, generateUploadFilePathName, + getFilePublicUrl, } from '../../utils/uploads' import path from 'path' import normalizeUrl from 'normalize-url' @@ -99,11 +100,12 @@ export const uploadFileRequestResolver: ResolverFn< input.contentType ) - // If this is a file URL, we swap in the GCS signed - // URL + const publicUrl = getFilePublicUrl(uploadFilePathName) + + // If this is a file URL, we swap in the GCS public URL if (isFileUrl(input.url)) { await models.uploadFile.update(uploadFileData.id, { - url: uploadSignedUrl, + url: publicUrl, status: UploadFileStatus.Initialized, }) } @@ -135,7 +137,7 @@ export const uploadFileRequestResolver: ResolverFn< } else { const pageId = await createPage( { - url: isFileUrl(input.url) ? uploadSignedUrl : input.url, + url: isFileUrl(input.url) ? publicUrl : input.url, id: input.clientRequestId || '', userId: claims.uid, title: title, diff --git a/packages/api/src/utils/uploads.ts b/packages/api/src/utils/uploads.ts index 2fc5535d6..24f44723f 100644 --- a/packages/api/src/utils/uploads.ts +++ b/packages/api/src/utils/uploads.ts @@ -15,6 +15,13 @@ const storage = env.fileUpload?.gcsUploadSAKeyFilePath : new Storage() const bucketName = env.fileUpload.gcsUploadBucket +export const getFilePublicUrl = (filePathName: string): string => { + return storage + .bucket(bucketName) + .file(filePathName) + .publicUrl() +} + export const generateUploadSignedUrl = async ( filePathName: string, contentType: string, From 05ea57f76fa7929107f961e02399bc95cc8934b9 Mon Sep 17 00:00:00 2001 From: Jackson Harper Date: Thu, 9 Jun 2022 12:08:42 -0700 Subject: [PATCH 4/8] Linting --- packages/api/src/resolvers/upload_files/index.ts | 10 ++++++---- packages/api/src/utils/uploads.ts | 5 +---- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/packages/api/src/resolvers/upload_files/index.ts b/packages/api/src/resolvers/upload_files/index.ts index 64174da9e..d6c9684e5 100644 --- a/packages/api/src/resolvers/upload_files/index.ts +++ b/packages/api/src/resolvers/upload_files/index.ts @@ -115,10 +115,12 @@ export const uploadFileRequestResolver: ResolverFn< // If we have a file:// URL, don't try to match it // and create a copy of the page, just create a // new item. - const page = isFileUrl(input.url) ? await getPageByParam({ - userId: claims.uid, - url: input.url, - }) : undefined + const page = isFileUrl(input.url) + ? await getPageByParam({ + userId: claims.uid, + url: input.url, + }) + : undefined if (page) { if ( diff --git a/packages/api/src/utils/uploads.ts b/packages/api/src/utils/uploads.ts index 24f44723f..6c4492479 100644 --- a/packages/api/src/utils/uploads.ts +++ b/packages/api/src/utils/uploads.ts @@ -16,10 +16,7 @@ const storage = env.fileUpload?.gcsUploadSAKeyFilePath const bucketName = env.fileUpload.gcsUploadBucket export const getFilePublicUrl = (filePathName: string): string => { - return storage - .bucket(bucketName) - .file(filePathName) - .publicUrl() + return storage.bucket(bucketName).file(filePathName).publicUrl() } export const generateUploadSignedUrl = async ( From 98bc1470b577e82daf945fec35da462203c2f826 Mon Sep 17 00:00:00 2001 From: Jackson Harper Date: Thu, 9 Jun 2022 12:22:41 -0700 Subject: [PATCH 5/8] Add a test for upload file requests --- .../resolvers/upload_file_request.test.ts | 100 ++++++++++++++++++ 1 file changed, 100 insertions(+) create mode 100644 packages/api/test/resolvers/upload_file_request.test.ts diff --git a/packages/api/test/resolvers/upload_file_request.test.ts b/packages/api/test/resolvers/upload_file_request.test.ts new file mode 100644 index 000000000..150534524 --- /dev/null +++ b/packages/api/test/resolvers/upload_file_request.test.ts @@ -0,0 +1,100 @@ +import { createTestUser, deleteTestUser } from '../db' +import { + generateFakeUuid, + graphqlRequest, + request, +} from '../util' +import * as chai from 'chai' +import { expect } from 'chai' +import 'mocha' +import { User } from '../../src/entity/user' +import chaiString from 'chai-string' +import { + PageContext, +} from '../../src/elastic/types' +import { createPubSubClient } from '../../src/datalayer/pubsub' +import { + deletePage, + getPageById, +} from '../../src/elastic/pages' + +chai.use(chaiString) + +// INPUT +// clientRequestId?: InputMaybe; +// contentType: Scalars['String']; +// createPageEntry?: InputMaybe; +// url: Scalars['String']; + +const uploadFileRequest = async ( + authToken: string, + inputUrl: string, + clientRequestId: string, + createPageEntry = true + ) => { + const query = ` + mutation { + uploadFileRequest( + input: { + contentType: "application/pdf", + clientRequestId: "${clientRequestId}", + createPageEntry: ${createPageEntry}, + url: "${inputUrl}" + } + ) { + ... on ArchiveLinkSuccess { + linkId + } + ... on ArchiveLinkError { + errorCodes + } + } + } + ` + return graphqlRequest(query, authToken).expect(200) +} + +describe('uploadFileRequest API', () => { + const username = 'fakeUser' + let authToken: string + let user: User + let ctx: PageContext + + before(async () => { + // create test user and login + user = await createTestUser(username) + const res = await request + .post('/local/debug/fake-user-login') + .send({ fakeEmail: user.email }) + + authToken = res.body.authToken + + ctx = { + pubsub: createPubSubClient(), + refresh: true, + uid: user.id, + } + }) + + after(async () => { + await deleteTestUser(username) + }) + + describe('UploadFileRequest', () => { + context('when create article is true', () => { + const clientRequestId = generateFakeUuid() + + after(async () => { + await deletePage(clientRequestId, ctx) + }) + + it('should create an article if create article is true', async () => { + const res = uploadFileRequest(authToken, 'https://www.google.com', clientRequestId, true).expect(200) + expect(res.body.data.uploadFileRequest.createdPageId).to.eql(clientRequestId) + const page = await getPageById(clientRequestId) + expect(page).to.be + }) + }) + }) +}) + From 70d655f591c130d08d34cc527f50373ee708e2a9 Mon Sep 17 00:00:00 2001 From: Jackson Harper Date: Thu, 9 Jun 2022 12:25:58 -0700 Subject: [PATCH 6/8] Add an uploadFileTest to verify GCS URLs are used for file:// objects --- .../OmnivoreKit/Sources/Models/DataModels/PDFItem.swift | 2 +- packages/api/test/resolvers/upload_file_request.test.ts | 9 ++++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/apple/OmnivoreKit/Sources/Models/DataModels/PDFItem.swift b/apple/OmnivoreKit/Sources/Models/DataModels/PDFItem.swift index 3380e48f1..fcfb3a23b 100644 --- a/apple/OmnivoreKit/Sources/Models/DataModels/PDFItem.swift +++ b/apple/OmnivoreKit/Sources/Models/DataModels/PDFItem.swift @@ -22,7 +22,7 @@ public struct PDFItem { objectID: item.objectID, itemID: item.unwrappedID, pdfURL: URL(string: item.unwrappedPageURLString), - localPdfURL: item.localPdfURL.flatMap { URL(string: $0) }, + localPdfURL: nil, // item.localPdfURL.flatMap { URL(string: $0) }, title: item.unwrappedID, slug: item.unwrappedSlug, readingProgress: item.readingProgress, diff --git a/packages/api/test/resolvers/upload_file_request.test.ts b/packages/api/test/resolvers/upload_file_request.test.ts index 150534524..7c19fb532 100644 --- a/packages/api/test/resolvers/upload_file_request.test.ts +++ b/packages/api/test/resolvers/upload_file_request.test.ts @@ -89,11 +89,18 @@ describe('uploadFileRequest API', () => { }) it('should create an article if create article is true', async () => { - const res = uploadFileRequest(authToken, 'https://www.google.com', clientRequestId, true).expect(200) + const res = await uploadFileRequest(authToken, 'https://www.google.com', clientRequestId, true) expect(res.body.data.uploadFileRequest.createdPageId).to.eql(clientRequestId) const page = await getPageById(clientRequestId) expect(page).to.be }) + + it('should not save a file:// URL', async () => { + const res = await uploadFileRequest(authToken, 'file://foo.bar', clientRequestId, true) + expect(res.body.data.uploadFileRequest.createdPageId).to.eql(clientRequestId) + const page = await getPageById(clientRequestId) + expect(page.url).to.startWith("https://") + }) }) }) }) From 9c5bc1021f22d2f7cb9f939280850e92a3cf7625 Mon Sep 17 00:00:00 2001 From: Jackson Harper Date: Thu, 9 Jun 2022 12:33:16 -0700 Subject: [PATCH 7/8] Revert this change --- apple/OmnivoreKit/Sources/Models/DataModels/PDFItem.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apple/OmnivoreKit/Sources/Models/DataModels/PDFItem.swift b/apple/OmnivoreKit/Sources/Models/DataModels/PDFItem.swift index fcfb3a23b..3380e48f1 100644 --- a/apple/OmnivoreKit/Sources/Models/DataModels/PDFItem.swift +++ b/apple/OmnivoreKit/Sources/Models/DataModels/PDFItem.swift @@ -22,7 +22,7 @@ public struct PDFItem { objectID: item.objectID, itemID: item.unwrappedID, pdfURL: URL(string: item.unwrappedPageURLString), - localPdfURL: nil, // item.localPdfURL.flatMap { URL(string: $0) }, + localPdfURL: item.localPdfURL.flatMap { URL(string: $0) }, title: item.unwrappedID, slug: item.unwrappedSlug, readingProgress: item.readingProgress, From 5d4c8f00e562439bf1418fed98381b8bc1d54e56 Mon Sep 17 00:00:00 2001 From: Jackson Harper Date: Thu, 9 Jun 2022 12:38:25 -0700 Subject: [PATCH 8/8] Fix test --- packages/api/test/resolvers/upload_file_request.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/api/test/resolvers/upload_file_request.test.ts b/packages/api/test/resolvers/upload_file_request.test.ts index 7c19fb532..2ec593195 100644 --- a/packages/api/test/resolvers/upload_file_request.test.ts +++ b/packages/api/test/resolvers/upload_file_request.test.ts @@ -99,7 +99,7 @@ describe('uploadFileRequest API', () => { const res = await uploadFileRequest(authToken, 'file://foo.bar', clientRequestId, true) expect(res.body.data.uploadFileRequest.createdPageId).to.eql(clientRequestId) const page = await getPageById(clientRequestId) - expect(page.url).to.startWith("https://") + expect(page?.url).to.startWith("https://") }) }) })