Secure view fixes (#10973)

* Restrict actions that are not allowed in secure view mode * Restrict keyboard actions that are not allowed in secure view mode * Add restriction for apps/viewers * Add unit tests
owncloud · Jun 3, 2024 · 36cd238 · 36cd238
1 parent 722a837
commit 36cd238
Show file tree

Hide file tree

Showing 15 changed files with 142 additions and 11 deletions.
diff --git a/packages/web-app-external/src/index.ts b/packages/web-app-external/src/index.ts
@@ -69,6 +69,7 @@ export default defineWebApplication({
                 icon: provider.icon,
                 name: provider.name,
                 mimeType: mimeType.mime_type,
+                secureView: provider.secure_view,
                 routeName: 'external-apps',
                 hasPriority: mimeType.default_application === provider.name,
                 ...(mimeType.allow_creation && { newFileMenu: { menuTitle: () => mimeType.name } }),

diff --git a/packages/web-app-external/src/schemas.ts b/packages/web-app-external/src/schemas.ts
@@ -2,7 +2,8 @@ import { z } from 'zod'
 
 const AppProviderSchema = z.object({
   icon: z.string(),
-  name: z.string()
+  name: z.string(),
+  secure_view: z.boolean().optional()
 })
 
 const MimeTypeSchema = z.object({

diff --git a/packages/web-client/src/helpers/resource/functions.ts b/packages/web-client/src/helpers/resource/functions.ts
@@ -142,7 +142,7 @@ export function buildResource(resource: WebDavResponseResource): Resource {
       return this.permissions.indexOf(DavPermission.FolderCreateable) >= 0
     },
     canDownload: function () {
-      return true
+      return this.permissions.indexOf(DavPermission.SecureView) === -1
     },
     canBeDeleted: function () {
       return this.permissions.indexOf(DavPermission.Deletable) >= 0

diff --git a/packages/web-client/src/helpers/share/constants.ts b/packages/web-client/src/helpers/share/constants.ts
@@ -15,5 +15,6 @@ export const GraphShareRoleIdMap = {
   FileEditor: '2d00ce52-1fc2-4dbc-8b95-a73b73395f5a',
   FolderEditor: 'fb6c3e19-e378-47e5-b277-9732f9de6e21',
   SpaceEditor: '58c63c02-1d89-4572-916a-870abc5a1b7d',
-  SpaceManager: '312c0871-5ef7-4b3a-85b6-0e4074c64049'
+  SpaceManager: '312c0871-5ef7-4b3a-85b6-0e4074c64049',
+  SecureViewer: 'aa97fe03-7980-45ac-9e50-b325749fd7e6'
 } as const
diff --git a/packages/web-client/src/helpers/share/functions.ts b/packages/web-client/src/helpers/share/functions.ts
@@ -13,6 +13,7 @@ import { buildWebDavSpacesPath } from '../space'
 import { DriveItem, Identity, Permission, UnifiedRoleDefinition, User } from '../../graph/generated'
 import { urlJoin } from '../../utils'
 import { uniq } from 'lodash-es'
+import { GraphShareRoleIdMap } from './constants'
 
 export const isShareResource = (resource: Resource): resource is ShareResource => {
   return Object.hasOwn(resource, 'sharedWith')
@@ -139,7 +140,9 @@ export function buildIncomingShareResource({
     sharePermissions,
     outgoing: false,
     canRename: () => driveItem['@client.synchronize'],
-    canDownload: () => sharePermissions.includes(GraphSharePermission.readBasic),
+    canDownload: () =>
+      sharePermissions.includes(GraphSharePermission.readBasic) &&
+      !shareRoles.some((shareRole) => shareRole.id === GraphShareRoleIdMap.SecureViewer),
     canUpload: () => sharePermissions.includes(GraphSharePermission.createUpload),
     canCreate: () => sharePermissions.includes(GraphSharePermission.createChildren),
     canBeDeleted: () => sharePermissions.includes(GraphSharePermission.deleteStandard),

diff --git a/packages/web-client/src/webdav/constants/dav.ts b/packages/web-client/src/webdav/constants/dav.ts
@@ -11,6 +11,7 @@ export abstract class DavPermission {
   static readonly FileUpdateable: string = 'W'
   static readonly FolderCreateable: string = 'CK'
   static readonly Deny: string = 'Z'
+  static readonly SecureView: string = 'X'
 }
 
 export enum DavMethod {

diff --git a/packages/web-pkg/src/apps/types.ts b/packages/web-pkg/src/apps/types.ts
@@ -60,6 +60,7 @@ export interface ApplicationFileExtension {
   mimeType?: string
   newFileMenu?: { menuTitle: () => string }
   routeName?: string
+  secureView?: boolean
 }
 
 /** ApplicationInformation describes required information of an application */

diff --git a/packages/web-pkg/src/composables/actions/files/useFileActions.ts b/packages/web-pkg/src/composables/actions/files/useFileActions.ts
@@ -116,6 +116,10 @@ export const useFileActions = () => {
               return false
             }
 
+            if (!resources[0].canDownload() && !fileExtension.secureView) {
+              return false
+            }
+
             if (!unref(isSearchActive) && isLocationTrashActive(router, 'files-trash-generic')) {
               return false
             }

diff --git a/packages/web-pkg/src/composables/actions/files/useFileActionsCopy.ts b/packages/web-pkg/src/composables/actions/files/useFileActionsCopy.ts
@@ -84,6 +84,9 @@ export const useFileActionsCopy = () => {
             }
           }
 
+          if (!unref(resources)[0].canDownload()) {
+            return false
+          }
           // copy can't be restricted in authenticated context, because
           // a user always has their home dir with write access
           return true

diff --git a/packages/web-pkg/src/composables/piniaStores/apps.ts b/packages/web-pkg/src/composables/piniaStores/apps.ts
@@ -49,7 +49,8 @@ export const useAppsStore = defineStore('apps', () => {
       hasPriority:
         data.hasPriority ||
         unref(externalAppConfig)?.[appId]?.priorityExtensions?.includes(data.extension) ||
-        false
+        false,
+      secureView: data.secureView || false
     })
   }
 

diff --git a/packages/web-pkg/src/composables/piniaStores/clipboard.ts b/packages/web-pkg/src/composables/piniaStores/clipboard.ts
@@ -13,13 +13,21 @@ export const useClipboardStore = defineStore('clipboard', () => {
   const resources = ref<Resource[]>([])
 
   const copyResources = (r: Resource[]) => {
+    if (!r[0].canDownload()) {
+      return
+    }
+
     action.value = ClipboardActions.Copy
     resources.value = r
 
     showMessage({ title: $gettext('Copied to clipboard!'), status: 'success' })
   }
 
   const cutResources = (r: Resource[]) => {
+    if (!r[0].canDownload()) {
+      return
+    }
+
     action.value = ClipboardActions.Cut
     resources.value = r
 

diff --git a/packages/web-pkg/src/services/preview/previewService.ts b/packages/web-pkg/src/services/preview/previewService.ts
@@ -76,7 +76,8 @@ export class PreviewService {
   ): Promise<string | undefined> {
     const { space, resource } = options
     const serverSupportsPreview = this.available && this.isMimetypeSupported(resource.mimeType)
-    const resourceSupportsPreview = resource.type !== 'folder' && resource.extension
+    const resourceSupportsPreview =
+      resource.type !== 'folder' && resource.extension && resource.canDownload()
     if (!serverSupportsPreview || !resourceSupportsPreview) {
       return undefined
     }

diff --git a/packages/web-pkg/tests/unit/composables/actions/files/useFileActions.spec.ts b/packages/web-pkg/tests/unit/composables/actions/files/useFileActions.spec.ts
@@ -2,6 +2,8 @@ import { mock } from 'vitest-mock-extended'
 import { useFileActions } from '../../../../../src/composables/actions'
 import { defaultComponentMocks, RouteLocation, getComposableWrapper } from 'web-test-helpers'
 import { computed, unref } from 'vue'
+import { describe } from 'vitest'
+import { Resource } from '@ownclouders/web-client'
 
 const mockUseEmbedMode = vi.fn().mockReturnValue({ isEnabled: computed(() => false) })
 vi.mock('../../../../../src/composables/embedMode', () => ({
@@ -13,12 +15,12 @@ describe('fileActions', () => {
     it('should provide a list of editors', () => {
       getWrapper({
         setup: ({ editorActions }) => {
-          expect(unref(editorActions).length).toBeTruthy()
+          expect(unref(editorActions).length).toEqual(2)
         }
       })
     })
     it('should provide an empty list if embed mode is enabled', () => {
-      mockUseEmbedMode.mockReturnValue({
+      mockUseEmbedMode.mockReturnValueOnce({
         isEnabled: computed(() => true)
       })
       getWrapper({
@@ -28,6 +30,30 @@ describe('fileActions', () => {
       })
     })
   })
+  describe('secure view context', () => {
+    describe('computed property "editorActions"', () => {
+      it('only displays editors that support secure view', () => {
+        getWrapper({
+          setup: ({ editorActions }) => {
+            const secureViewResource = mock<Resource>({
+              id: '1',
+              canDownload: () => false,
+              mimeType: 'text/txt',
+              extension: 'txt'
+            })
+            const actions = unref(editorActions)
+            expect(actions.length).toEqual(2)
+            expect(
+              actions[0].isVisible({ resources: [secureViewResource], space: null })
+            ).toBeFalsy()
+            expect(
+              actions[1].isVisible({ resources: [secureViewResource], space: null })
+            ).toBeTruthy()
+          }
+        })
+      })
+    })
+  })
 })
 
 function getWrapper({ setup }: { setup: (instance: ReturnType<typeof useFileActions>) => void }) {
@@ -67,13 +93,32 @@ function getWrapper({ setup }: { setup: (instance: ReturnType<typeof useFileActi
                       extension: 'txt'
                     }
                   ]
+                },
+                external: {
+                  applicationMenu: {
+                    enabled: () => true
+                  },
+                  defaultExtension: '',
+                  icon: 'check_box_outline_blank',
+                  name: 'External',
+                  id: 'external'
                 }
               },
               fileExtensions: [
                 {
                   app: 'text-editor',
                   extension: 'txt',
                   hasPriority: false
+                },
+                {
+                  app: 'external',
+                  label: 'Open in Collabora',
+                  mimeType: 'text/txt',
+                  routeName: 'external-apps',
+                  icon: 'https://host.docker.internal:9980/favicon.ico',
+                  name: 'Collabora',
+                  hasPriority: false,
+                  secureView: true
                 }
               ]
             }

diff --git a/packages/web-pkg/tests/unit/composables/actions/files/useFileActionsCopy.spec.ts b/packages/web-pkg/tests/unit/composables/actions/files/useFileActionsCopy.spec.ts
@@ -4,6 +4,7 @@ import { Resource, SpaceResource } from '@ownclouders/web-client'
 import { defaultComponentMocks, RouteLocation, getComposableWrapper } from 'web-test-helpers'
 import { useFileActionsCopy } from '../../../../../src/composables/actions/files'
 import { useClipboardStore } from '../../../../../src/composables/piniaStores'
+import { describe } from 'vitest'
 
 describe('copy', () => {
   describe('search context', () => {
@@ -38,6 +39,36 @@ describe('copy', () => {
       })
     })
   })
+  describe('computed property "actions"', () => {
+    describe('isVisible', () => {
+      it('returns true if "canDownload" is true', () => {
+        getWrapper({
+          searchLocation: false,
+          setup: ({ actions }) => {
+            expect(
+              unref(actions)[0].isVisible({
+                space: null,
+                resources: [mock<Resource>({ id: '1', canDownload: () => true })]
+              })
+            ).toBeTruthy()
+          }
+        })
+      })
+      it('returns false if "canDownload" is false', () => {
+        getWrapper({
+          searchLocation: false,
+          setup: ({ actions }) => {
+            expect(
+              unref(actions)[0].isVisible({
+                space: null,
+                resources: [mock<Resource>({ id: '1', canDownload: () => false })]
+              })
+            ).toBeFalsy()
+          }
+        })
+      })
+    })
+  })
 })
 
 function getWrapper({

diff --git a/packages/web-pkg/tests/unit/services/previewService.spec.ts b/packages/web-pkg/tests/unit/services/previewService.spec.ts
@@ -68,6 +68,25 @@ describe('PreviewService', () => {
       })
       expect(preview).toBeUndefined()
     })
+    it('does not load preview if "canDownload" is false', async () => {
+      const objectUrl = 'objectUrl'
+      const supportedMimeTypes = ['image/png']
+      const { previewService } = getWrapper({
+        supportedMimeTypes,
+        version: '1'
+      })
+      window.URL.createObjectURL = vi.fn().mockImplementation(() => objectUrl)
+      const preview = await previewService.loadPreview({
+        space: mock<SpaceResource>(),
+        resource: mock<Resource>({
+          mimeType: supportedMimeTypes[0],
+          webDavPath: '/',
+          etag: '',
+          canDownload: () => false
+        })
+      })
+      expect(preview).toEqual(undefined)
+    })
     describe('private files', () => {
       it('loads preview', async () => {
         const objectUrl = 'objectUrl'
@@ -79,7 +98,12 @@ describe('PreviewService', () => {
         window.URL.createObjectURL = vi.fn().mockImplementation(() => objectUrl)
         const preview = await previewService.loadPreview({
           space: mock<SpaceResource>(),
-          resource: mock<Resource>({ mimeType: supportedMimeTypes[0], webDavPath: '/', etag: '' })
+          resource: mock<Resource>({
+            mimeType: supportedMimeTypes[0],
+            webDavPath: '/',
+            etag: '',
+            canDownload: () => true
+          })
         })
         expect(preview).toEqual(objectUrl)
       })
@@ -94,7 +118,8 @@ describe('PreviewService', () => {
           id: '1',
           mimeType: supportedMimeTypes[0],
           webDavPath: '/',
-          etag: ''
+          etag: '',
+          canDownload: () => true
         })
         window.URL.createObjectURL = vi.fn().mockImplementation(() => objectUrl)
         const preview = await previewService.loadPreview(
@@ -121,7 +146,12 @@ describe('PreviewService', () => {
         })
         const preview = await previewService.loadPreview({
           space: mock<SpaceResource>({ driveType: 'public' }),
-          resource: mock<Resource>({ mimeType: supportedMimeTypes[0], downloadURL, etag: '' })
+          resource: mock<Resource>({
+            mimeType: supportedMimeTypes[0],
+            downloadURL,
+            etag: '',
+            canDownload: () => true
+          })
         })
         expect(preview).toEqual(`${downloadURL}?scalingup=0&preview=1&a=1`)
       })