From cf298994a8aee8565dd9563d7a3fa0e834dcae45 Mon Sep 17 00:00:00 2001 From: Kristaps Fabians Geikins Date: Wed, 20 Aug 2025 13:06:17 +0300 Subject: [PATCH] fix(server): viewerResources failing w/ nonexistent view (#5271) --- .../viewer/graph/resolvers/viewerResources.ts | 3 +- .../viewer/services/viewerResources.ts | 7 ++--- .../tests/integration/viewerResources.spec.ts | 28 +++++++++---------- .../unit/services/viewerResources.spec.ts | 0 .../src/authz/fragments/savedViews.spec.ts | 18 ++++++++++++ .../shared/src/authz/fragments/savedViews.ts | 11 ++++++-- .../policies/project/savedViews/canRead.ts | 14 ++++++++-- 7 files changed, 56 insertions(+), 25 deletions(-) delete mode 100644 packages/server/modules/viewer/tests/unit/services/viewerResources.spec.ts diff --git a/packages/server/modules/viewer/graph/resolvers/viewerResources.ts b/packages/server/modules/viewer/graph/resolvers/viewerResources.ts index 18f854158..8fc9a1c3f 100644 --- a/packages/server/modules/viewer/graph/resolvers/viewerResources.ts +++ b/packages/server/modules/viewer/graph/resolvers/viewerResources.ts @@ -40,7 +40,8 @@ const extendedViewerResourcesResolver = async ( const canRead = await ctx.authPolicies.project.savedViews.canRead({ userId: ctx.userId, projectId, - savedViewId + savedViewId, + allowNonExistent: true // ignore missing view }) throwIfAuthNotOk(canRead) } diff --git a/packages/server/modules/viewer/services/viewerResources.ts b/packages/server/modules/viewer/services/viewerResources.ts index 851cc91ee..4f668cf92 100644 --- a/packages/server/modules/viewer/services/viewerResources.ts +++ b/packages/server/modules/viewer/services/viewerResources.ts @@ -14,7 +14,6 @@ import type { ViewerResourceItem } from '@/modules/core/graph/generated/graphql' import type { CommitRecord } from '@/modules/core/helpers/types' -import { NotFoundError } from '@/modules/shared/errors' import { getFeatureFlags } from '@/modules/shared/helpers/envHelper' import type { DependenciesOf } from '@/modules/shared/helpers/factory' import type { @@ -370,9 +369,9 @@ const adjustResourceIdStringWithSpecificSavedViewSettingsFactory = : params.savedViewId if (!savedView) { - throw new NotFoundError( - `Saved view with ID ${params.savedViewId} not found in project ${projectId}` - ) + // We don't want this to fail, cause this would break the app when nonexistant views are referenced + // Just ignore the view + return emptyReturn } const savedViewResources = resourceBuilder().addFromString(savedView.resourceIds) diff --git a/packages/server/modules/viewer/tests/integration/viewerResources.spec.ts b/packages/server/modules/viewer/tests/integration/viewerResources.spec.ts index 2d22ae7cf..a2cebdeab 100644 --- a/packages/server/modules/viewer/tests/integration/viewerResources.spec.ts +++ b/packages/server/modules/viewer/tests/integration/viewerResources.spec.ts @@ -10,7 +10,6 @@ import { } from '@/modules/core/repositories/commits' import { getStreamObjectsFactory } from '@/modules/core/repositories/objects' import { buildBasicTestProject } from '@/modules/core/tests/helpers/creation' -import { NotFoundError } from '@/modules/shared/errors' import { getFeatureFlags } from '@/modules/shared/helpers/envHelper' import type { ViewerResourceGroup } from '@/modules/viewer/domain/types/resources' import type { SavedView } from '@/modules/viewer/domain/types/savedViews' @@ -25,7 +24,7 @@ import { viewerResourcesToString } from '@/modules/viewer/services/viewerResources' import { createTestSavedView } from '@/modules/viewer/tests/helpers/savedViews' -import { expectToThrow, itEach } from '@/test/assertionHelper' +import { itEach } from '@/test/assertionHelper' import type { BasicTestUser } from '@/test/authHelper' import { buildBasicTestUser, createTestUser } from '@/test/authHelper' import { @@ -323,21 +322,20 @@ describe('Viewer Resources Collection Service', () => { secondModelBasicView = views[1] }) - it('throws if setting nonexistant view', async () => { + it('should not throw if setting nonexistant view', async () => { const sut = buildSUT() - const err = await expectToThrow( - async () => - await sut({ - projectId: myProject.id, - resourceIdString: resourceBuilder() - .addResources(firstModel().id) - .toString(), - savedViewId: 'aaa' - }) - ) - expect(err instanceof NotFoundError).to.be.true - expect(err.message).to.include('Saved view') + // shouldnt throw so that deleting a view while its referred to in the URL doesn't + // cause the entire app to break - just ignore the value then + const res = await sut({ + projectId: myProject.id, + resourceIdString: resourceBuilder() + .addResources(firstModel().id) + .toString(), + savedViewId: 'aaa' + }) + + expect(res).to.be.ok }) itEach( diff --git a/packages/server/modules/viewer/tests/unit/services/viewerResources.spec.ts b/packages/server/modules/viewer/tests/unit/services/viewerResources.spec.ts deleted file mode 100644 index e69de29bb..000000000 diff --git a/packages/shared/src/authz/fragments/savedViews.spec.ts b/packages/shared/src/authz/fragments/savedViews.spec.ts index 111ddd0a0..68684726f 100644 --- a/packages/shared/src/authz/fragments/savedViews.spec.ts +++ b/packages/shared/src/authz/fragments/savedViews.spec.ts @@ -229,6 +229,24 @@ describe('ensureCanAccessSavedViewFragment', () => { }) } ) + + it.each(['read', 'write'])( + `doesn't fail if view doesnt exist and allowNonExistent set (%s)`, + async (access) => { + const sut = buildWorkspaceSUT({ + getSavedView: async () => null + }) + + const result = await sut({ + userId, + projectId, + savedViewId, + access, + allowNonExistent: true + }) + expect(result).toBeAuthOKResult() + } + ) }) }) diff --git a/packages/shared/src/authz/fragments/savedViews.ts b/packages/shared/src/authz/fragments/savedViews.ts index e8a93dd22..9eecf059e 100644 --- a/packages/shared/src/authz/fragments/savedViews.ts +++ b/packages/shared/src/authz/fragments/savedViews.ts @@ -52,6 +52,10 @@ export const ensureCanAccessSavedViewFragment: AuthPolicyEnsureFragment< ProjectContext & SavedViewContext & { access: 'read' | 'write' + /** + * In some cases we want to just ignore a view being non-existant, instead of throwing + */ + allowNonExistent?: boolean }, InstanceType< | typeof SavedViewNotFoundError @@ -71,7 +75,7 @@ export const ensureCanAccessSavedViewFragment: AuthPolicyEnsureFragment< > > = (loaders) => - async ({ userId, projectId, savedViewId, access }) => { + async ({ userId, projectId, savedViewId, access, allowNonExistent }) => { const canUseSavedViews = await ensureCanUseProjectWorkspacePlanFeatureFragment( loaders )({ @@ -81,7 +85,10 @@ export const ensureCanAccessSavedViewFragment: AuthPolicyEnsureFragment< if (canUseSavedViews.isErr) return err(canUseSavedViews.error) const savedView = await loaders.getSavedView({ projectId, savedViewId }) - if (!savedView) return err(new SavedViewNotFoundError()) + if (!savedView) { + if (allowNonExistent) return ok() + return err(new SavedViewNotFoundError()) + } const isPublic = savedView.visibility === SavedViewVisibility.public if (isPublic && access === 'read') { diff --git a/packages/shared/src/authz/policies/project/savedViews/canRead.ts b/packages/shared/src/authz/policies/project/savedViews/canRead.ts index f4e1d9f07..49fd17154 100644 --- a/packages/shared/src/authz/policies/project/savedViews/canRead.ts +++ b/packages/shared/src/authz/policies/project/savedViews/canRead.ts @@ -34,7 +34,14 @@ export const canReadSavedViewPolicy: AuthPolicy< | typeof Loaders.getWorkspaceSsoProvider | typeof Loaders.getWorkspaceSsoSession | typeof Loaders.getProjectRole, - MaybeUserContext & ProjectContext & SavedViewContext, + MaybeUserContext & + ProjectContext & + SavedViewContext & { + /** + * In some cases we want to just ignore a view being non-existant, instead of throwing + */ + allowNonExistent?: boolean + }, InstanceType< | typeof SavedViewNotFoundError | typeof SavedViewNoAccessError @@ -53,11 +60,12 @@ export const canReadSavedViewPolicy: AuthPolicy< > > = (loaders) => - async ({ userId, projectId, savedViewId }) => { + async ({ userId, projectId, savedViewId, allowNonExistent }) => { return await ensureCanAccessSavedViewFragment(loaders)({ userId, projectId, savedViewId, - access: 'read' + access: 'read', + allowNonExistent }) }