fix: redirect unauthorized dataset access to /datasets for knowledge editors (#36073)

Co-authored-by: yyh <92089059+lyzno1@users.noreply.github.com>

web/__tests__/apps/create-app-flow.test.tsx

@@ -79,6 +79,9 @@ vi.mock('@tanstack/react-query', async (importOriginal) => {
   const actual = await importOriginal<typeof import('@tanstack/react-query')>()
   return {
     ...actual,
+    useQuery: () => ({
+      data: [],
+    }),
     useInfiniteQuery: () => ({
       data: { pages: mockPages },
       isLoading: mockIsLoading,

web/app/(commonLayout)/datasets/(datasetDetailLayout)/[datasetId]/__tests__/layout-main.spec.tsx

@@ -0,0 +1,151 @@
+import { render, screen, waitFor } from '@testing-library/react'
+import { usePathname, useRouter } from '@/next/navigation'
+import { useDatasetDetail, useDatasetRelatedApps } from '@/service/knowledge/use-dataset'
+import DatasetDetailLayout from '../layout-main'
+
+const mockReplace = vi.fn()
+const mockSetAppSidebarExpand = vi.fn()
+
+vi.mock('@/next/navigation', () => ({
+  usePathname: vi.fn(),
+  useRouter: vi.fn(),
+}))
+
+vi.mock('@/service/knowledge/use-dataset', () => ({
+  useDatasetDetail: vi.fn(),
+  useDatasetRelatedApps: vi.fn(),
+}))
+
+vi.mock('@/app/components/app/store', () => ({
+  useStore: (selector: (state: { setAppSidebarExpand: typeof mockSetAppSidebarExpand }) => unknown) => selector({
+    setAppSidebarExpand: mockSetAppSidebarExpand,
+  }),
+}))
+
+vi.mock('@/context/app-context', () => ({
+  useAppContext: () => ({
+    isCurrentWorkspaceDatasetOperator: false,
+  }),
+}))
+
+vi.mock('@/context/event-emitter', () => ({
+  useEventEmitterContextContext: () => ({
+    eventEmitter: undefined,
+  }),
+}))
+
+vi.mock('@/hooks/use-breakpoints', () => ({
+  default: () => 'desktop',
+  MediaType: {
+    mobile: 'mobile',
+  },
+}))
+
+vi.mock('@/hooks/use-document-title', () => ({
+  default: vi.fn(),
+}))
+
+vi.mock('@/app/components/app-sidebar', () => ({
+  default: () => <aside aria-label="dataset navigation" />,
+}))
+
+vi.mock('@/app/components/datasets/extra-info', () => ({
+  default: () => <div />,
+}))
+
+const mockUsePathname = vi.mocked(usePathname)
+const mockUseRouter = vi.mocked(useRouter)
+const mockUseDatasetDetail = vi.mocked(useDatasetDetail)
+const mockUseDatasetRelatedApps = vi.mocked(useDatasetRelatedApps)
+
+describe('DatasetDetailLayout', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+    mockUsePathname.mockReturnValue('/datasets/dataset-1/pipeline')
+    mockUseRouter.mockReturnValue({
+      back: vi.fn(),
+      forward: vi.fn(),
+      refresh: vi.fn(),
+      push: vi.fn(),
+      replace: mockReplace,
+      prefetch: vi.fn(),
+    })
+    mockUseDatasetRelatedApps.mockReturnValue({ data: undefined } as ReturnType<typeof useDatasetRelatedApps>)
+  })
+
+  describe('Access Errors', () => {
+    it.each([403, 404])('should redirect to datasets page when dataset detail returns %s', async (status) => {
+      // Arrange
+      mockUseDatasetDetail.mockReturnValue({
+        data: undefined,
+        error: new Response(null, { status }),
+        refetch: vi.fn(),
+      } as unknown as ReturnType<typeof useDatasetDetail>)
+
+      // Act
+      render(
+        <DatasetDetailLayout datasetId="dataset-1">
+          <div>Pipeline content</div>
+        </DatasetDetailLayout>,
+      )
+
+      // Assert
+      await waitFor(() => {
+        expect(mockReplace).toHaveBeenCalledWith('/datasets')
+      })
+      expect(mockUseDatasetRelatedApps).toHaveBeenCalledWith('dataset-1', { enabled: false })
+      expect(screen.queryByText('Pipeline content')).not.toBeInTheDocument()
+    })
+
+    it('should redirect when the dataset detail error exposes status without being a Response', async () => {
+      // Arrange
+      mockUseDatasetDetail.mockReturnValue({
+        data: undefined,
+        error: { status: 403 },
+        refetch: vi.fn(),
+      } as unknown as ReturnType<typeof useDatasetDetail>)
+
+      // Act
+      render(
+        <DatasetDetailLayout datasetId="dataset-1">
+          <div>Pipeline content</div>
+        </DatasetDetailLayout>,
+      )
+
+      // Assert
+      await waitFor(() => {
+        expect(mockReplace).toHaveBeenCalledWith('/datasets')
+      })
+      expect(screen.queryByText('Pipeline content')).not.toBeInTheDocument()
+    })
+  })
+
+  describe('Rendering', () => {
+    it('should render children when dataset detail is available', () => {
+      // Arrange
+      mockUseDatasetDetail.mockReturnValue({
+        data: {
+          id: 'dataset-1',
+          name: 'Dataset 1',
+          provider: 'vendor',
+          runtime_mode: 'rag_pipeline',
+          is_published: true,
+        },
+        error: null,
+        refetch: vi.fn(),
+      } as unknown as ReturnType<typeof useDatasetDetail>)
+
+      // Act
+      render(
+        <DatasetDetailLayout datasetId="dataset-1">
+          <div>Pipeline content</div>
+        </DatasetDetailLayout>,
+      )
+
+      // Assert
+      expect(screen.getByText('Pipeline content')).toBeInTheDocument()
+      expect(mockUseDatasetRelatedApps).toHaveBeenCalledWith('dataset-1', { enabled: true })
+      expect(mockReplace).not.toHaveBeenCalled()
+    })
+  })
+})

web/app/(commonLayout)/datasets/(datasetDetailLayout)/[datasetId]/layout-main.tsx

@@ -23,7 +23,7 @@ import DatasetDetailContext from '@/context/dataset-detail'
 import { useEventEmitterContextContext } from '@/context/event-emitter'
 import useBreakpoints, { MediaType } from '@/hooks/use-breakpoints'
 import useDocumentTitle from '@/hooks/use-document-title'
-import { usePathname } from '@/next/navigation'
+import { usePathname, useRouter } from '@/next/navigation'
 import { useDatasetDetail, useDatasetRelatedApps } from '@/service/knowledge/use-dataset'
 
 type IAppDetailLayoutProps = {
@@ -31,12 +31,26 @@ type IAppDetailLayoutProps = {
   datasetId: string
 }
 
+const getResponseStatus = (error: unknown) => {
+  if (error instanceof Response)
+    return error.status
+
+  if (typeof error === 'object' && error && 'status' in error && typeof error.status === 'number')
+    return error.status
+}
+
+const shouldRedirectToDatasetList = (error: unknown) => {
+  const status = getResponseStatus(error)
+  return status === 403 || status === 404
+}
+
 const DatasetDetailLayout: FC<IAppDetailLayoutProps> = (props) => {
   const {
     children,
     datasetId,
   } = props
   const { t } = useTranslation()
+  const router = useRouter()
   const pathname = usePathname()
   const hideSideBar = pathname.endsWith('documents/create') || pathname.endsWith('documents/create-from-pipeline')
   const isPipelineCanvas = pathname.endsWith('/pipeline')
@@ -54,8 +68,9 @@ const DatasetDetailLayout: FC<IAppDetailLayoutProps> = (props) => {
   const isMobile = media === MediaType.mobile
 
   const { data: datasetRes, error, refetch: mutateDatasetRes } = useDatasetDetail(datasetId)
+  const shouldRedirect = shouldRedirectToDatasetList(error)
 
-  const { data: relatedApps } = useDatasetRelatedApps(datasetId)
+  const { data: relatedApps } = useDatasetRelatedApps(datasetId, { enabled: !!datasetRes && !shouldRedirect })
 
   const isButtonDisabledWithPipeline = useMemo(() => {
     if (!datasetRes)
@@ -115,9 +130,17 @@ const DatasetDetailLayout: FC<IAppDetailLayoutProps> = (props) => {
     setAppSidebarExpand(isMobile ? mode : localeMode)
   }, [isMobile, setAppSidebarExpand])
 
+  useEffect(() => {
+    if (shouldRedirect)
+      router.replace('/datasets')
+  }, [router, shouldRedirect])
+
   if (!datasetRes && !error)
     return <Loading type="app" />
 
+  if (shouldRedirect)
+    return <Loading type="app" />
+
   return (
     <div
       className={cn(

web/service/knowledge/use-dataset.spec.ts

@@ -0,0 +1,94 @@
+import { useQuery } from '@tanstack/react-query'
+import { get } from '../base'
+import { useDatasetDetail, useDatasetRelatedApps } from './use-dataset'
+
+vi.mock('@tanstack/react-query', () => ({
+  keepPreviousData: Symbol('keepPreviousData'),
+  useInfiniteQuery: vi.fn(),
+  useMutation: vi.fn(),
+  useQuery: vi.fn(),
+  useQueryClient: vi.fn(),
+}))
+
+vi.mock('../base', () => ({
+  get: vi.fn(),
+  post: vi.fn(),
+}))
+
+vi.mock('../use-base', () => ({
+  useInvalid: vi.fn(),
+}))
+
+const mockUseQuery = vi.mocked(useQuery)
+const mockGet = vi.mocked(get)
+
+type QueryOptions = Parameters<typeof useQuery>[0]
+type RetryFn = (failureCount: number, error: unknown) => boolean
+
+const getLastQueryOptions = () => {
+  return mockUseQuery.mock.calls.at(-1)?.[0] as QueryOptions
+}
+
+const getRetryFn = () => {
+  return getLastQueryOptions().retry as RetryFn
+}
+
+describe('knowledge dataset hooks', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+    mockUseQuery.mockReturnValue({} as ReturnType<typeof useQuery>)
+  })
+
+  describe('useDatasetDetail', () => {
+    it('should not retry forbidden or missing dataset detail errors', () => {
+      // Arrange & Act
+      useDatasetDetail('dataset-1')
+      const retry = getRetryFn()
+
+      // Assert
+      expect(retry(0, new Response(null, { status: 403 }))).toBe(false)
+      expect(retry(0, new Response(null, { status: 404 }))).toBe(false)
+    })
+
+    it('should retry other dataset detail errors fewer than three times', () => {
+      // Arrange & Act
+      useDatasetDetail('dataset-1')
+      const retry = getRetryFn()
+
+      // Assert
+      expect(retry(2, new Error('temporary failure'))).toBe(true)
+      expect(retry(3, new Error('temporary failure'))).toBe(false)
+    })
+
+    it('should fetch dataset detail without silent mode', () => {
+      // Arrange
+      mockGet.mockResolvedValue({ id: 'dataset-1' })
+
+      // Act
+      useDatasetDetail('dataset-1')
+      const queryFn = getLastQueryOptions().queryFn as () => unknown
+      queryFn()
+
+      // Assert
+      expect(mockGet).toHaveBeenCalledWith('/datasets/dataset-1')
+    })
+  })
+
+  describe('useDatasetRelatedApps', () => {
+    it('should use explicit enabled option when provided', () => {
+      // Arrange & Act
+      useDatasetRelatedApps('dataset-1', { enabled: false })
+
+      // Assert
+      expect(getLastQueryOptions().enabled).toBe(false)
+    })
+
+    it('should enable related apps query when dataset id exists and no option is provided', () => {
+      // Arrange & Act
+      useDatasetRelatedApps('dataset-1')
+
+      // Assert
+      expect(getLastQueryOptions().enabled).toBe(true)
+    })
+  })
+})

web/service/knowledge/use-dataset.ts

@@ -110,13 +110,20 @@ export const useDatasetDetail = (datasetId: string) => {
     queryKey: [...datasetDetailQueryKeyPrefix, datasetId],
     queryFn: () => get<DataSet>(`/datasets/${datasetId}`),
     enabled: !!datasetId,
+    retry: (failureCount, error) => {
+      if (error instanceof Response && [403, 404].includes(error.status))
+        return false
+
+      return failureCount < 3
+    },
   })
 }
 
-export const useDatasetRelatedApps = (datasetId: string) => {
+export const useDatasetRelatedApps = (datasetId: string, options?: { enabled?: boolean }) => {
   return useQuery({
     queryKey: [NAME_SPACE, 'related-apps', datasetId],
     queryFn: () => get<RelatedAppResponse>(`/datasets/${datasetId}/related-apps`),
+    enabled: options?.enabled ?? !!datasetId,
   })
 }