Merge pull request #6848 from owncloud/bufix-spaces-contextmenu-id

[full-ci] Bugfix: Spaces Contextmenu trigger id isn't valid
owncloud · May 2, 2022 · fa7ab3f · fa7ab3f
2 parents 5bbd582 + 7bedd3f
commit fa7ab3f
Show file tree

Hide file tree

Showing 16 changed files with 95 additions and 35 deletions.
diff --git a/changelog/unreleased/bugfix-spaces-triggerid-invalid b/changelog/unreleased/bugfix-spaces-triggerid-invalid
@@ -0,0 +1,7 @@
+Bugfix: Spaces Contextmenu trigger id isn't valid
+
+We've fixed a bug which resulted in spaces having an
+invalid trigger id for the contextmenu.
+
+https://github.com/owncloud/web/issues/6845
+https://github.com/owncloud/web/pull/6848
diff --git a/packages/web-app-files/src/components/FilesList/ResourceTable.vue b/packages/web-app-files/src/components/FilesList/ResourceTable.vue
@@ -174,7 +174,7 @@ import { determineSortFields } from '../../helpers/ui/resourceTable'
 import { useCapabilitySpacesEnabled } from 'web-pkg/src/composables'
 import Rename from '../../mixins/actions/rename'
 import { defineComponent, PropType } from '@vue/composition-api'
-import { Resource } from '../../helpers/resource'
+import { extractDomSelector, Resource } from '../../helpers/resource'
 import { ShareTypes } from '../../helpers/share'
 
 export default defineComponent({
@@ -209,7 +209,7 @@ export default defineComponent({
     resourceDomSelector: {
       type: Function,
       required: false,
-      default: (resource) => resource.id.replace(/[^A-Za-z0-9\-_]/g, '')
+      default: (resource) => extractDomSelector(resource.id)
     },
     /**
      * Asserts whether resources path should be shown in the resource name

diff --git a/packages/web-app-files/src/components/SideBar/Shares/Collaborators/ListItem.vue b/packages/web-app-files/src/components/SideBar/Shares/Collaborators/ListItem.vue
@@ -75,7 +75,7 @@
     >
       <role-dropdown
         :resource="highlightedFile"
-        :share-id="share.id"
+        :dom-selector="shareDomSelector"
         :existing-permissions="share.customPermissions"
         :existing-role="share.role"
         :allow-share-permission="hasResharing || isSpace"
@@ -107,6 +107,7 @@ import RoleDropdown from './RoleDropdown.vue'
 import { SharePermissions, ShareTypes } from '../../../../helpers/share'
 import { clientService } from 'web-pkg/src/services'
 import { useCapabilityFilesSharingResharing } from 'web-pkg/src/composables'
+import { extractDomSelector } from '../../../../helpers/resource'
 
 export default {
   name: 'ListItem',
@@ -151,6 +152,13 @@ export default {
       return this.shareType.key
     },
 
+    shareDomSelector() {
+      if (!this.share.id) {
+        return undefined
+      }
+      return extractDomSelector(this.share.id)
+    },
+
     isGroup() {
       return this.shareType === ShareTypes.group
     },

diff --git a/packages/web-app-files/src/components/SideBar/Shares/Collaborators/RoleDropdown.vue b/packages/web-app-files/src/components/SideBar/Shares/Collaborators/RoleDropdown.vue
@@ -104,7 +104,7 @@ export default {
       required: false,
       default: () => []
     },
-    shareId: {
+    domSelector: {
       type: String,
       required: false,
       default: undefined
@@ -124,8 +124,8 @@ export default {
     ...mapGetters(['capabilities']),
 
     roleButtonId() {
-      if (this.shareId) {
-        return `files-collaborators-role-button-${this.shareId}-${uuid.v4()}`
+      if (this.domSelector) {
+        return `files-collaborators-role-button-${this.domSelector}-${uuid.v4()}`
       }
       return 'files-collaborators-role-button-new'
     },

diff --git a/packages/web-app-files/src/helpers/resource/resource.ts b/packages/web-app-files/src/helpers/resource/resource.ts
@@ -38,6 +38,8 @@ export interface Resource {
   isReceivedShare?(): boolean
   isMounted?(): boolean
 
+  getDomSelector?(): string
+
   resourceOwner?: User
   owner?: User[]
   ownerDisplayName?: string
@@ -59,6 +61,10 @@ export const extractStorageId = (id?: string): string => {
   return id.indexOf('!') >= 0 ? id.split('!')[0] : ''
 }
 
+export const extractDomSelector = (str: string): string => {
+  return str.replace(/[^A-Za-z0-9\-_]/g, '')
+}
+
 export const extractNameWithoutExtension = (resource?: Resource): string => {
   const extension = resource.extension || ''
   const name = resource.name || ''

diff --git a/packages/web-app-files/src/helpers/resources.ts b/packages/web-app-files/src/helpers/resources.ts
@@ -15,7 +15,12 @@ import {
   spaceRoleManager,
   spaceRoleViewer
 } from './share'
-import { extractExtensionFromFile, extractStorageId, Resource } from './resource'
+import {
+  extractDomSelector,
+  extractExtensionFromFile,
+  extractStorageId,
+  Resource
+} from './resource'
 import { User } from './user'
 
 export function renameResource(resource, newName, newPath) {
@@ -47,8 +52,10 @@ export function buildResource(resource): Resource {
     resourcePath = `/${resourcePath}`
   }
 
+  const id = resource.fileInfo[DavProperty.FileId]
+
   return {
-    id: resource.fileInfo[DavProperty.FileId],
+    id,
     fileId: resource.fileInfo[DavProperty.FileId],
     storageId: extractStorageId(resource.fileInfo[DavProperty.FileId]),
     mimeType: resource.fileInfo[DavProperty.MimeType],
@@ -100,7 +107,8 @@ export function buildResource(resource): Resource {
     },
     isReceivedShare: function () {
       return this.permissions.indexOf(DavPermission.Shared) >= 0
-    }
+    },
+    getDomSelector: () => extractDomSelector(id)
   }
 }
 
@@ -220,7 +228,8 @@ export function buildSpace(space) {
     },
     isReceivedShare: function () {
       return false
-    }
+    },
+    getDomSelector: () => extractDomSelector(space.id)
   }
 }
 
@@ -379,6 +388,7 @@ export function buildSharedResource(share, incomingShares = false, allowSharePer
   resource.canUpload = () => true
   resource.isMounted = () => false
   resource.share = buildShare(share, resource, allowSharePermission)
+  resource.getDomSelector = () => extractDomSelector(share.id)
 
   return resource
 }
@@ -515,14 +525,15 @@ export function buildDeletedResource(resource): Resource {
   const isFolder = resource.type === 'dir' || resource.type === 'folder'
   const fullName = resource.fileInfo[DavProperty.TrashbinOriginalFilename]
   const extension = extractExtensionFromFile({ name: fullName, type: resource.type } as Resource)
+  const id = path.basename(resource.name)
   return {
     type: isFolder ? 'folder' : resource.type,
     isFolder,
     ddate: resource.fileInfo[DavProperty.TrashbinDeletedDate],
     name: path.basename(fullName),
     extension,
     path: resource.fileInfo[DavProperty.TrashbinOriginalLocation],
-    id: path.basename(resource.name),
+    id,
     indicators: [],
     canUpload: () => false,
     canDownload: () => false,
@@ -546,6 +557,7 @@ export function buildDeletedResource(resource): Resource {
     canShare: () => false,
     canCreate: () => false,
     isMounted: () => false,
-    isReceivedShare: () => false
+    isReceivedShare: () => false,
+    getDomSelector: () => extractDomSelector(id)
   }
 }
diff --git a/packages/web-app-files/src/helpers/statusIndicators.js b/packages/web-app-files/src/helpers/statusIndicators.js
@@ -91,7 +91,7 @@ const shareTypesIndirect = (path, sharesTree) => {
 export const getIndicators = (resource, sharesTree) => {
   const indicators = [
     {
-      id: `files-sharing-${resource.id.replaceAll('=', '')}`,
+      id: `files-sharing-${resource.getDomSelector()}`,
       accessibleDescription: shareUserIconDescribedBy(resource, sharesTree),
       label: $gettext('Show invited people'),
       visible: isUserShare(resource, sharesTree),
@@ -101,7 +101,7 @@ export const getIndicators = (resource, sharesTree) => {
       handler: indicatorHandler
     },
     {
-      id: `file-link-${resource.id.replaceAll('=', '')}`,
+      id: `file-link-${resource.getDomSelector()}`,
       accessibleDescription: shareLinkDescribedBy(resource, sharesTree),
       label: $gettext('Show links'),
       visible: isLinkShare(resource, sharesTree),

diff --git a/packages/web-app-files/src/mixins/filesListScrolling.js b/packages/web-app-files/src/mixins/filesListScrolling.js
@@ -7,7 +7,7 @@ export default {
       const tableHeader = document.querySelector('.files-table thead')
       offset += tableHeader.getBoundingClientRect().height
 
-      this.$scrollTo(`.oc-tbody-tr-${resource.id}`, 300, { offset: -offset })
+      this.$scrollTo(`.oc-tbody-tr-${resource.getDomSelector()}`, 300, { offset: -offset })
     }
   }
 }
diff --git a/packages/web-app-files/src/views/shares/SharedWithMe.vue b/packages/web-app-files/src/views/shares/SharedWithMe.vue
@@ -27,7 +27,7 @@
         >
           <template #status="{ resource }">
             <div
-              :key="resource.id + resource.status"
+              :key="resource.getDomSelector() + resource.status"
               class="oc-text-nowrap oc-flex oc-flex-middle oc-flex-right"
             >
               <oc-button
@@ -122,7 +122,7 @@
       >
         <template #status="{ resource }">
           <div
-            :key="resource.id + resource.status"
+            :key="resource.getDomSelector() + resource.status"
             class="oc-text-nowrap oc-flex oc-flex-middle oc-flex-right"
           >
             <oc-button

diff --git a/packages/web-app-files/src/views/spaces/Projects.vue b/packages/web-app-files/src/views/spaces/Projects.vue
@@ -38,7 +38,7 @@
             oc-child-width-1-5@xl
           "
         >
-          <li v-for="space in spaces" :key="space.id" class="oc-mb-m">
+          <li v-for="space in spaces" :key="space.getDomSelector()" class="oc-mb-m">
             <div
               class="spaces-list-card oc-card oc-card-default oc-rounded"
               :data-space-id="space.id"
@@ -88,16 +88,16 @@
                     </div>
                     <div>
                       <oc-button
-                        :id="`space-context-btn-${sanitizeStorageId(space.id)}`"
+                        :id="`space-context-btn-${space.getDomSelector()}`"
                         v-oc-tooltip="$gettext('Show context menu')"
                         :aria-label="$gettext('Show context menu')"
                         appearance="raw"
                       >
                         <oc-icon name="more-2" />
                       </oc-button>
                       <oc-drop
-                        :drop-id="`space-context-drop-${space.id}`"
-                        :toggle="`#space-context-btn-${sanitizeStorageId(space.id)}`"
+                        :drop-id="`space-context-drop-${space.getDomSelector()}`"
+                        :toggle="`#space-context-btn-${space.getDomSelector()}`"
                         mode="click"
                         close-on-click
                         :options="{ delayHide: 0 }"
@@ -250,10 +250,6 @@ export default defineComponent({
       })
     },
 
-    sanitizeStorageId(id) {
-      return id.replace('!', '\\!').split('.')[0]
-    },
-
     getSpaceCardAdditionalClass(space) {
       if (space.disabled) {
         return 'state-trashed'

diff --git a/packages/web-app-files/tests/unit/components/FilesList/ResourceTable.spec.js b/packages/web-app-files/tests/unit/components/FilesList/ResourceTable.spec.js
@@ -6,6 +6,7 @@ import VueCompositionAPI from '@vue/composition-api'
 import ResourceTable from '../../../../src/components/FilesList/ResourceTable.vue'
 import { createStore } from 'vuex-extensions'
 import Vuex from 'vuex'
+import { extractDomSelector } from '../../../../src/helpers/resource'
 
 const router = {
   push: jest.fn(),
@@ -90,7 +91,8 @@ const resourcesWithAllFields = [
     ddate: getCurrentDate(),
     owner,
     sharedWith,
-    canRename: jest.fn
+    canRename: jest.fn,
+    getDomSelector: () => extractDomSelector('forest')
   },
   {
     id: 'notes',
@@ -106,7 +108,8 @@ const resourcesWithAllFields = [
     ddate: getCurrentDate(),
     sharedWith,
     owner,
-    canRename: jest.fn
+    canRename: jest.fn,
+    getDomSelector: () => extractDomSelector('notes')
   },
   {
     id: 'documents',
@@ -121,7 +124,8 @@ const resourcesWithAllFields = [
     ddate: getCurrentDate(),
     sharedWith,
     owner,
-    canRename: jest.fn
+    canRename: jest.fn,
+    getDomSelector: () => extractDomSelector('documents')
   },
   {
     id: 'another-one==',
@@ -136,7 +140,8 @@ const resourcesWithAllFields = [
     ddate: getCurrentDate(),
     sharedWith,
     owner,
-    canRename: jest.fn
+    canRename: jest.fn,
+    getDomSelector: () => extractDomSelector('another-one==')
   }
 ]
 
@@ -164,13 +169,13 @@ describe('ResourceTable', () => {
   it('accepts resourceDomId closure', () => {
     const wrapper = getMountedWrapper({
       propsData: {
-        resourceDomSelector: (resource) => ['custom', resource.id.replace(/=+/, '')].join('-')
+        resourceDomSelector: (resource) => ['custom', resource.getDomSelector()].join('-')
       }
     })
     resourcesWithAllFields.forEach((resource) => {
       ;['.oc-tbody-tr', '#resource-table-select', '#context-menu-drop'].forEach((baseSelector) => {
         expect(
-          wrapper.find([baseSelector, 'custom', resource.id.replace(/=+/, '')].join('-')).exists()
+          wrapper.find([baseSelector, 'custom', resource.getDomSelector()].join('-')).exists()
         ).toBeTruthy()
       })
     })

diff --git a/packages/web-app-files/tests/unit/components/SideBar/Details/FileDetails.spec.js b/packages/web-app-files/tests/unit/components/SideBar/Details/FileDetails.spec.js
@@ -53,7 +53,8 @@ const sharedFile = {
   thumbnail: 'example.com/image',
   mdate: 'Tue, 20 Oct 2015 06:15:00 GMT',
   size: '740',
-  shareTypes: [ShareTypes.user.value]
+  shareTypes: [ShareTypes.user.value],
+  getDomSelector: () => '4'
 }
 
 const formDateFromJSDate = jest.fn().mockImplementation(() => 'ABSOLUTE_TIME')

diff --git a/packages/web-app-files/tests/unit/components/SideBar/Shares/Collaborators/ListItem.spec.js b/packages/web-app-files/tests/unit/components/SideBar/Shares/Collaborators/ListItem.spec.js
@@ -133,6 +133,7 @@ function createWrapper({
     }),
     propsData: {
       share: {
+        id: 'asdf',
         collaborator,
         shareType,
         role

diff --git a/...es/tests/unit/components/SideBar/Shares/Collaborators/__snapshots__/ListItem.spec.js.snap b/...es/tests/unit/components/SideBar/Shares/Collaborators/__snapshots__/ListItem.spec.js.snap
@@ -16,7 +16,7 @@ exports[`Collaborator ListItem component share inheritance indicators show when
     </div>
   </div>
   <div class="oc-width-1-3 oc-flex oc-flex-nowrap oc-flex-right oc-flex-middle">
-    <role-dropdown-stub resource="[object Object]" existingrole="[object Object]" existingpermissions="" class="files-collaborators-collaborator-role"></role-dropdown-stub>
+    <role-dropdown-stub resource="[object Object]" existingrole="[object Object]" existingpermissions="" domselector="asdf" class="files-collaborators-collaborator-role"></role-dropdown-stub>
     <edit-dropdown-stub sharecategory="user" data-testid="collaborator-edit" class="files-collaborators-collaborator-edit"></edit-dropdown-stub>
   </div>
 </div>

diff --git a/packages/web-app-files/tests/unit/helpers/resource/resource.spec.ts b/packages/web-app-files/tests/unit/helpers/resource/resource.spec.ts
@@ -1,4 +1,5 @@
 import {
+  extractDomSelector,
   extractExtensionFromFile,
   extractNameWithoutExtension,
   Resource
@@ -54,3 +55,23 @@ describe('filterResources', () => {
     })
   })
 })
+
+describe('extractDomSelector', () => {
+  it.each([
+    { input: '', expected: '' },
+    { input: '1', expected: '1' },
+    { input: 'a', expected: 'a' },
+    { input: '!=?', expected: '' },
+    { input: '-_', expected: '-_' },
+    { input: '1a!=?-_', expected: '1a-_' },
+    {
+      input: 'f2dc18fa-ca05-11ec-8c55-0f9df469d22f',
+      expected: 'f2dc18fa-ca05-11ec-8c55-0f9df469d22f'
+    }
+  ])(
+    'creates a string that does not break when being used as query selector',
+    ({ input, expected }) => {
+      expect(extractDomSelector(input)).toBe(expected)
+    }
+  )
+})