Merge pull request #1939 from omnivore-app/fix/deleted-article-saving-request

fix/deleted article saving request

packages/api/src/elastic/pages.ts

@@ -328,33 +328,30 @@ export const deletePage = async (
 }
 
 export const getPageByParam = async <K extends keyof ParamSet>(
-  param: Record<K, ParamSet[K]>,
+  params: Record<K, ParamSet[K] | ParamSet[K][]>,
   includeOriginalHtml = false
 ): Promise<Page | undefined> => {
   try {
-    const params = {
-      query: {
-        bool: {
-          filter: Object.keys(param)
-            .filter(
-              (key) => param[key as K] !== undefined && param[key as K] !== null
-            ) // filter out undefined and null values
-            .map((key) => ({
-              term: {
-                [key]: param[key as K],
-              },
-            })),
-        },
-      },
-      size: 1,
-      _source: {
+    let builder = esBuilder()
+      .size(1)
+      .rawOption('_source', {
         excludes: includeOriginalHtml ? [] : ['originalHtml'],
-      },
-    }
-
+      })
+    // filter out undefined and null values and empty arrays
+    // and build the query
+    Object.entries<ParamSet[K] | ParamSet[K][]>(params)
+      .filter(
+        ([, value]) =>
+          value != null && !(Array.isArray(value) && value.length === 0)
+      )
+      .forEach(([key, value]) => {
+        Array.isArray(value)
+          ? (builder = builder.query('terms', key, value))
+          : (builder = builder.query('term', key, value))
+      })
     const { body } = await client.search<SearchResponse<Page>>({
       index: INDEX_ALIAS,
-      body: params,
+      body: builder.build(),
     })
 
     if (body.hits.total.value === 0) {

packages/api/src/generated/graphql.ts

@@ -180,6 +180,7 @@ export type ArticleSavingRequestError = {
 };
 
 export enum ArticleSavingRequestErrorCode {
+  BadData = 'BAD_DATA',
   NotFound = 'NOT_FOUND',
   Unauthorized = 'UNAUTHORIZED'
 }

packages/api/src/generated/schema.graphql

@@ -145,6 +145,7 @@ type ArticleSavingRequestError {
 }
 
 enum ArticleSavingRequestErrorCode {
+  BAD_DATA
   NOT_FOUND
   UNAUTHORIZED
 }

packages/api/src/resolvers/article/index.ts

@@ -3,6 +3,26 @@
 /* eslint-disable @typescript-eslint/no-unsafe-assignment */
 /* eslint-disable @typescript-eslint/no-unsafe-member-access */
 /* eslint-disable @typescript-eslint/no-floating-promises */
+import { Readability } from '@omnivore/readability'
+import * as httpContext from 'express-http-context'
+import graphqlFields from 'graphql-fields'
+import normalizeUrl from 'normalize-url'
+import { searchHighlights } from '../../elastic/highlights'
+import {
+  createPage,
+  getPageByParam,
+  searchAsYouType,
+  searchPages,
+  updatePage,
+  updatePagesAsync,
+} from '../../elastic/pages'
+import {
+  ArticleSavingRequestStatus,
+  Page,
+  PageType,
+  SearchItem as SearchItemData,
+} from '../../elastic/types'
+import { env } from '../../env'
 import {
   Article,
   ArticleError,
@@ -51,11 +71,13 @@ import {
   UpdatesSinceErrorCode,
   UpdatesSinceSuccess,
 } from '../../generated/graphql'
+import { createPageSaveRequest } from '../../services/create_page_save_request'
+import { parsedContentToPage } from '../../services/save_page'
+import { saveSearchHistory } from '../../services/search_history'
+import { traceAs } from '../../tracing'
 import { Merge } from '../../util'
-import {
-  getStorageFileDetails,
-  makeStorageFilePublic,
-} from '../../utils/uploads'
+import { analytics } from '../../utils/analytics'
+import { isSiteBlockedForParse } from '../../utils/blocked'
 import { ContentParseError } from '../../utils/errors'
 import {
   authorized,
@@ -67,45 +89,19 @@ import {
   userDataToUser,
   validatedDate,
 } from '../../utils/helpers'
+import { createImageProxyUrl } from '../../utils/imageproxy'
 import {
   getDistillerResult,
   htmlToMarkdown,
   ParsedContentPuppeteer,
   parsePreparedContent,
 } from '../../utils/parser'
-import { isSiteBlockedForParse } from '../../utils/blocked'
-import { Readability } from '@omnivore/readability'
-import { traceAs } from '../../tracing'
-
-import { createImageProxyUrl } from '../../utils/imageproxy'
-import normalizeUrl from 'normalize-url'
-import { WithDataSourcesContext } from '../types'
-
 import { parseSearchQuery, SortBy, SortOrder } from '../../utils/search'
-import { createPageSaveRequest } from '../../services/create_page_save_request'
-import { analytics } from '../../utils/analytics'
-import { env } from '../../env'
-import graphqlFields from 'graphql-fields'
-
 import {
-  ArticleSavingRequestStatus,
-  Page,
-  PageType,
-  SearchItem as SearchItemData,
-} from '../../elastic/types'
-import {
-  createPage,
-  getPageById,
-  getPageByParam,
-  searchAsYouType,
-  searchPages,
-  updatePage,
-  updatePagesAsync,
-} from '../../elastic/pages'
-import { searchHighlights } from '../../elastic/highlights'
-import { saveSearchHistory } from '../../services/search_history'
-import { parsedContentToPage } from '../../services/save_page'
-import * as httpContext from 'express-http-context'
+  getStorageFileDetails,
+  makeStorageFilePublic,
+} from '../../utils/uploads'
+import { WithDataSourcesContext } from '../types'
 
 enum ArticleFormat {
   Markdown = 'markdown',
@@ -649,24 +645,18 @@ export const setBookmarkArticleResolver = authorized<
     { input: { articleID, bookmark } },
     { claims: { uid }, log, pubsub }
   ) => {
-    const page = await getPageById(articleID)
+    const page = await getPageByParam({
+      userId: uid,
+      _id: articleID,
+    })
     if (!page) {
       return { errorCodes: [SetBookmarkArticleErrorCode.NotFound] }
     }
 
     if (!bookmark) {
-      const pageRemoved = await getPageByParam({
-        userId: uid,
-        _id: articleID,
-      })
-
-      if (!pageRemoved) {
-        return { errorCodes: [SetBookmarkArticleErrorCode.NotFound] }
-      }
-
       // delete the page and its metadata
       const deleted = await updatePage(
-        pageRemoved.id,
+        page.id,
         {
           state: ArticleSavingRequestStatus.Deleted,
           labels: [],
@@ -684,7 +674,7 @@ export const setBookmarkArticleResolver = authorized<
         userId: uid,
         event: 'link_removed',
         properties: {
-          url: pageRemoved.url,
+          url: page.url,
           env: env.server.apiEnv,
         },
       })
@@ -704,7 +694,7 @@ export const setBookmarkArticleResolver = authorized<
       // Make sure article.id instead of userArticle.id has passed. We use it for cache updates
       return {
         bookmarkedArticle: {
-          ...pageRemoved,
+          ...page,
           isArchived: false,
           savedByViewer: false,
           postedByViewer: false,

packages/api/src/resolvers/article_saving_request/index.ts

@@ -57,22 +57,28 @@ export const articleSavingRequestResolver = authorized<
   ArticleSavingRequestError,
   QueryArticleSavingRequestArgs
 >(async (_, { id, url }, { models, claims }) => {
+  if (!id && !url) {
+    return { errorCodes: [ArticleSavingRequestErrorCode.BadData] }
+  }
+  const user = await models.user.get(claims.uid)
+  if (!user) {
+    return { errorCodes: [ArticleSavingRequestErrorCode.Unauthorized] }
+  }
   const params = {
     _id: id || undefined,
     url: url || undefined,
     userId: claims.uid,
+    state: [
+      ArticleSavingRequestStatus.Succeeded,
+      ArticleSavingRequestStatus.Processing,
+    ],
   }
   const page = await getPageByParam(params)
   if (!page) {
     return { errorCodes: [ArticleSavingRequestErrorCode.NotFound] }
   }
-  const user = await models.user.get(page.userId)
-  if (user && page) {
-    if (isParsingTimeout(page)) {
-      page.state = ArticleSavingRequestStatus.Succeeded
-    }
-    return { articleSavingRequest: pageToArticleSavingRequest(user, page) }
+  if (isParsingTimeout(page)) {
+    page.state = ArticleSavingRequestStatus.Succeeded
   }
-
-  return { errorCodes: [ArticleSavingRequestErrorCode.NotFound] }
+  return { articleSavingRequest: pageToArticleSavingRequest(user, page) }
 })

packages/api/src/resolvers/highlight/index.ts

@@ -1,7 +1,15 @@
 /* eslint-disable @typescript-eslint/no-unsafe-assignment */
 /* eslint-disable @typescript-eslint/require-await */
 /* eslint-disable @typescript-eslint/no-floating-promises */
-import { authorized, unescapeHtml } from '../../utils/helpers'
+import {
+  addHighlightToPage,
+  deleteHighlight,
+  getHighlightById,
+  updateHighlight,
+} from '../../elastic/highlights'
+import { getPageById, updatePage } from '../../elastic/pages'
+import { Highlight as HighlightData } from '../../elastic/types'
+import { env } from '../../env'
 import {
   CreateHighlightError,
   CreateHighlightErrorCode,
@@ -26,16 +34,8 @@ import {
   UpdateHighlightSuccess,
   User,
 } from '../../generated/graphql'
-import { env } from '../../env'
 import { analytics } from '../../utils/analytics'
-import { Highlight as HighlightData } from '../../elastic/types'
-import { getPageById, updatePage } from '../../elastic/pages'
-import {
-  addHighlightToPage,
-  deleteHighlight,
-  getHighlightById,
-  updateHighlight,
-} from '../../elastic/highlights'
+import { authorized, unescapeHtml } from '../../utils/helpers'
 
 const highlightDataToHighlight = (highlight: HighlightData): Highlight => ({
   ...highlight,
@@ -58,16 +58,11 @@ export const createHighlightResolver = authorized<
       errorCodes: [CreateHighlightErrorCode.NotFound],
     }
   }
-
-  analytics.track({
-    userId: claims.uid,
-    event: 'highlight_created',
-    properties: {
-      pageId,
-      env: env.server.apiEnv,
-    },
-  })
-
+  if (page.userId !== claims.uid) {
+    return {
+      errorCodes: [CreateHighlightErrorCode.Unauthorized],
+    }
+  }
   if (input.annotation && input.annotation.length > 4000) {
     return {
       errorCodes: [CreateHighlightErrorCode.BadData],
@@ -108,6 +103,15 @@ export const createHighlightResolver = authorized<
       },
     })
 
+    analytics.track({
+      userId: claims.uid,
+      event: 'highlight_created',
+      properties: {
+        pageId,
+        env: env.server.apiEnv,
+      },
+    })
+
     return { highlight: highlightDataToHighlight(highlight) }
   } catch (err) {
     log.error('Error creating highlight', err)
@@ -130,7 +134,11 @@ export const mergeHighlightResolver = authorized<
       errorCodes: [MergeHighlightErrorCode.NotFound],
     }
   }
-
+  if (page.userId !== claims.uid) {
+    return {
+      errorCodes: [MergeHighlightErrorCode.Unauthorized],
+    }
+  }
   const articleHighlights = page.highlights
 
   /* Compute merged annotation form the order of highlights appearing on page */

packages/api/src/resolvers/reminders/index.ts

@@ -1,4 +1,7 @@
-import { authorized } from '../../utils/helpers'
+import { DateTime } from 'luxon'
+import { getPageById } from '../../elastic/pages'
+import { Page } from '../../elastic/types'
+import { env } from '../../env'
 import {
   CreateReminderError,
   CreateReminderErrorCode,
@@ -17,14 +20,11 @@ import {
   UpdateReminderErrorCode,
   UpdateReminderSuccess,
 } from '../../generated/graphql'
-import { deleteTask, enqueueReminder } from '../../utils/createTask'
-import { analytics } from '../../utils/analytics'
-import { env } from '../../env'
-import { DataModels } from '../types'
-import { DateTime } from 'luxon'
 import { setLinkArchived } from '../../services/archive_link'
-import { getPageById } from '../../elastic/pages'
-import { Page } from '../../elastic/types'
+import { analytics } from '../../utils/analytics'
+import { deleteTask, enqueueReminder } from '../../utils/createTask'
+import { authorized } from '../../utils/helpers'
+import { DataModels } from '../types'
 
 const validScheduleTime = (str: string): Date | undefined => {
   const scheduleTime = DateTime.fromISO(str, { setZone: true }).set({
@@ -166,7 +166,11 @@ export const reminderResolver = authorized<
         errorCodes: [ReminderErrorCode.NotFound],
       }
     }
-
+    if (page.userId !== uid) {
+      return {
+        errorCodes: [ReminderErrorCode.Unauthorized],
+      }
+    }
     const reminder = await models.reminder.getCreatedByParameters(uid, {
       elasticPageId: page.id,
     })

packages/api/src/routers/article_router.ts

@@ -111,6 +111,13 @@ export function articleRouter() {
           if (!page) {
             return res.status(404).send('Page not found')
           }
+          if (page.userId !== uid) {
+            logger.info('User is not allowed to access speech of the article', {
+              userId: uid,
+              articleId,
+            })
+            return res.status(401).send({ errorCode: 'UNAUTHORIZED' })
+          }
           const speechFile = htmlToSpeechFile({
             title: page.title,
             content: page.content,

packages/api/src/routers/svc/integrations.ts

@@ -3,12 +3,12 @@
 /* eslint-disable @typescript-eslint/no-unsafe-member-access */
 import express from 'express'
 import { EntityType, readPushSubscription } from '../../datalayer/pubsub'
-import { getRepository } from '../../entity/utils'
-import { Integration, IntegrationType } from '../../entity/integration'
-import { buildLogger } from '../../utils/logger'
-import { syncWithIntegration } from '../../services/integrations'
 import { getPageById, searchPages } from '../../elastic/pages'
 import { Page } from '../../elastic/types'
+import { Integration, IntegrationType } from '../../entity/integration'
+import { getRepository } from '../../entity/utils'
+import { syncWithIntegration } from '../../services/integrations'
+import { buildLogger } from '../../utils/logger'
 import { DateFilter } from '../../utils/search'
 
 export interface Message {
@@ -89,6 +89,10 @@ export function integrationsServiceRouter() {
           res.status(200).send('No page found')
           return
         }
+        if (page.userId !== userId) {
+          logger.info('Page does not belong to user', { id, userId })
+          return res.status(200).send('Page does not belong to user')
+        }
         // sync updated page with integration
         logger.info('syncing updated page with integration', {
           integrationId: integration.id,

packages/api/src/routers/text_to_speech.ts

@@ -1,22 +1,22 @@
 /* eslint-disable @typescript-eslint/no-misused-promises */
 /* eslint-disable @typescript-eslint/no-unsafe-assignment */
 /* eslint-disable @typescript-eslint/explicit-module-boundary-types */
-import express from 'express'
-import cors from 'cors'
-import { corsConfig } from '../utils/corsConfig'
-import { getRepository, setClaims } from '../entity/utils'
-import { getPageById } from '../elastic/pages'
-import { Speech, SpeechState } from '../entity/speech'
-import { buildLogger } from '../utils/logger'
-import { getClaimsByToken } from '../utils/auth'
-import { shouldSynthesize } from '../services/speech'
-import { readPushSubscription } from '../datalayer/pubsub'
-import { AppDataSource } from '../server'
-import { enqueueTextToSpeech } from '../utils/createTask'
 import { htmlToSpeechFile } from '@omnivore/text-to-speech-handler'
-import { UserPersonalization } from '../entity/user_personalization'
+import cors from 'cors'
+import express from 'express'
+import { readPushSubscription } from '../datalayer/pubsub'
+import { getPageById } from '../elastic/pages'
 import { ArticleSavingRequestStatus } from '../elastic/types'
+import { Speech, SpeechState } from '../entity/speech'
+import { UserPersonalization } from '../entity/user_personalization'
+import { getRepository, setClaims } from '../entity/utils'
+import { AppDataSource } from '../server'
 import { FeatureName, getFeature } from '../services/features'
+import { shouldSynthesize } from '../services/speech'
+import { getClaimsByToken } from '../utils/auth'
+import { corsConfig } from '../utils/corsConfig'
+import { enqueueTextToSpeech } from '../utils/createTask'
+import { buildLogger } from '../utils/logger'
 
 const DEFAULT_VOICE = 'Larry'
 const DEFAULT_COMPLIMENTARY_VOICE = 'Evelyn'
@@ -59,7 +59,10 @@ export function textToSpeechRouter() {
         logger.info('No page found', { id })
         return res.status(200).send('No page found')
       }
-
+      if (page.userId !== userId) {
+        logger.info('Page does not belong to user', { id, userId })
+        return res.status(200).send('Page does not belong to user')
+      }
       if (page.state === ArticleSavingRequestStatus.Processing) {
         logger.info('Page is still processing, try again later', { id })
         return res.status(400).send('Page is still processing')

packages/api/src/schema.ts

@@ -1084,6 +1084,7 @@ const schema = gql`
   enum ArticleSavingRequestErrorCode {
     UNAUTHORIZED
     NOT_FOUND
+    BAD_DATA
   }
   type ArticleSavingRequestError {
     errorCodes: [ArticleSavingRequestErrorCode!]!

packages/api/src/services/create_page_save_request.ts

@@ -2,7 +2,12 @@ import normalizeUrl from 'normalize-url'
 import * as privateIpLib from 'private-ip'
 import { v4 as uuidv4 } from 'uuid'
 import { createPubSubClient, PubsubClient } from '../datalayer/pubsub'
-import { countByCreatedAt, createPage, getPageByParam } from '../elastic/pages'
+import {
+  countByCreatedAt,
+  createPage,
+  getPageByParam,
+  updatePage,
+} from '../elastic/pages'
 import { ArticleSavingRequestStatus, PageType } from '../elastic/types'
 import {
   ArticleSavingRequest,
@@ -87,6 +92,10 @@ export const createPageSaveRequest = async (
     stripWWW: false,
   })
 
+  const ctx = {
+    pubsub,
+    uid: userId,
+  }
   let page = await getPageByParam({
     userId,
     url: normalizedUrl,
@@ -110,7 +119,7 @@ export const createPageSaveRequest = async (
     }
 
     // create processing page
-    const pageId = await createPage(page, { pubsub, uid: userId })
+    const pageId = await createPage(page, ctx)
     if (!pageId) {
       console.log('Failed to create page', page)
       return Promise.reject({
@@ -118,7 +127,16 @@ export const createPageSaveRequest = async (
       })
     }
   }
-
+  // reset state to processing
+  if (page.state !== ArticleSavingRequestStatus.Processing) {
+    await updatePage(
+      page.id,
+      {
+        state: ArticleSavingRequestStatus.Processing,
+      },
+      ctx
+    )
+  }
   // enqueue task to parse page
   await enqueueParseRequest(url, userId, page.id, priority)
 

packages/api/src/services/save_file.ts

@@ -34,12 +34,9 @@ export const saveFile = async (
     }
   }
 
-  const uploadFileDetails = await getStorageFileDetails(
-    input.uploadFileId,
-    uploadFile.fileName
-  )
+  await getStorageFileDetails(input.uploadFileId, uploadFile.fileName)
 
-  const uploadFileData = await ctx.authTrx(async (tx) => {
+  await ctx.authTrx(async (tx) => {
     return ctx.models.uploadFile.setFileUploadComplete(input.uploadFileId, tx)
   })
 

packages/api/src/services/save_page.ts

@@ -101,20 +101,24 @@ export const savePage = async (
     originalHtml: parseResult.domContent,
     canonicalUrl: parseResult.canonicalUrl,
   })
-
+  // check if the page already exists
   const existingPage = await getPageByParam({
     userId: saver.userId,
     url: articleToSave.url,
-    state: ArticleSavingRequestStatus.Succeeded,
   })
-
   if (existingPage) {
+    pageId = existingPage.id
+    slug = existingPage.slug
     if (
       !(await updatePage(
         existingPage.id,
         {
-          savedAt: new Date(),
-          archivedAt: null,
+          // update the page with the new content
+          ...articleToSave,
+          archivedAt: null, // unarchive if it was archived
+          id: pageId, // we don't want to update the id
+          slug, // we don't want to update the slug
+          createdAt: existingPage.createdAt, // we don't want to update the createdAt
         },
         ctx
       ))
@@ -124,8 +128,6 @@ export const savePage = async (
         message: 'Failed to update existing page',
       }
     }
-    pageId = existingPage.id
-    slug = existingPage.slug
   } else if (shouldParseInBackend(input)) {
     try {
       await createPageSaveRequest(
@@ -235,7 +237,7 @@ export const parsedContentToPage = ({
     hash: uploadFileHash || stringToHash(parsedContent?.content || url),
     image: parsedContent?.previewImage ?? undefined,
     publishedAt: validatedDate(parsedContent?.publishedDate ?? undefined),
-    uploadFileId: uploadFileId,
+    uploadFileId,
     readingProgressPercent: 0,
     readingProgressAnchorIndex: 0,
     state: ArticleSavingRequestStatus.Succeeded,

packages/api/src/utils/uploads.ts

@@ -1,7 +1,7 @@
 /* eslint-disable @typescript-eslint/no-unsafe-member-access */
 /* eslint-disable @typescript-eslint/no-unsafe-assignment */
-import { env } from '../env'
 import { File, GetSignedUrlConfig, Storage } from '@google-cloud/storage'
+import { env } from '../env'
 
 /* On GAE/Prod, we shall rely on default app engine service account credentials.
  * Two changes needed: 1) add default service account to our uploads GCS Bucket
@@ -83,13 +83,6 @@ export const getStorageFileDetails = async (
   id: string,
   fileName: string
 ): Promise<{ md5Hash: string; fileUrl: string }> => {
-  // if (env.dev.isLocal) {
-  //   return {
-  //     md5Hash: 'some_md5_hash',
-  //     fileUrl: 'http://localhost:3000/public/' + id + '/' + fileName,
-  //   }
-  // }
-
   const filePathName = generateUploadFilePathName(id, fileName)
   const file = storage.bucket(bucketName).file(filePathName)
   const [metadata] = await file.getMetadata()

packages/web/lib/networking/queries/useGetArticleSavingStatus.tsx

@@ -131,7 +131,7 @@ export function useGetArticleSavingStatus({
     }
   }
 
-  if (status === 'PROCESSING' || status === 'DELETED') {
+  if (status === 'PROCESSING') {
     return {}
   }