From 9fa00ae3d226f8031f35b1956e205451d892c445 Mon Sep 17 00:00:00 2001 From: Florian Schade Date: Mon, 23 May 2022 10:10:56 +0200 Subject: [PATCH] use ocis storageId whenever possible use roles whenever possible refactor share loading out of link and share panel component into parent component and load the data only once use fileId for public link sharing cleanup expected failures get rid of bug demonstration test add gherkin debug helpers bump sdk bump ocis --- .../unreleased/enhancement-ocis-resharing | 6 + .../SideBar/Details/FileDetails.vue | 2 +- .../InviteCollaboratorForm.vue | 13 +- .../SideBar/Shares/Collaborators/ListItem.vue | 3 +- .../components/SideBar/Shares/FileLinks.vue | 25 +--- .../components/SideBar/Shares/FileShares.vue | 47 ++---- .../components/SideBar/Shares/SharesPanel.vue | 55 +++++++ packages/web-app-files/src/store/actions.js | 136 +++++++----------- .../SideBar/Shares/FileShares.spec.js | 12 -- .../SideBar/Shares/SharesPanel.spec.js | 49 +++++++ .../tests/unit/store/actions.spec.js | 6 +- .../sharePermissionsUsers.feature | 9 ++ .../stepDefinitions/debugContext.js | 5 + .../e2e/cucumber/steps/app-files/resource.ts | 9 +- tests/e2e/support/api/graph/user.ts | 1 + tests/e2e/support/environment/actor/shared.ts | 3 +- 16 files changed, 209 insertions(+), 172 deletions(-) create mode 100644 changelog/unreleased/enhancement-ocis-resharing create mode 100644 packages/web-app-files/tests/unit/components/SideBar/Shares/SharesPanel.spec.js create mode 100644 tests/acceptance/stepDefinitions/debugContext.js diff --git a/changelog/unreleased/enhancement-ocis-resharing b/changelog/unreleased/enhancement-ocis-resharing new file mode 100644 index 00000000000..e369f600382 --- /dev/null +++ b/changelog/unreleased/enhancement-ocis-resharing @@ -0,0 +1,6 @@ +Enhancement: Re-sharing for ocis + +We've enhanced web to be able to re-share resources for ocis, which works from sharing jails, project and personal spaces. +Beside that we also send roles, space-ref and path as separate values to the sharing api which simplifies the usage of it. + +https://github.com/owncloud/web/pull/7086 diff --git a/packages/web-app-files/src/components/SideBar/Details/FileDetails.vue b/packages/web-app-files/src/components/SideBar/Details/FileDetails.vue index a82e561adc2..0addd1de11c 100644 --- a/packages/web-app-files/src/components/SideBar/Details/FileDetails.vue +++ b/packages/web-app-files/src/components/SideBar/Details/FileDetails.vue @@ -393,7 +393,7 @@ export default defineComponent({ client: this.$client, path: this.file.path, $gettext: this.$gettext, - ...(this.currentStorageId && { storageId: this.currentStorageId }) + storageId: this.file.fileId }) this.shareIndicators = getIndicators(this.file, this.sharesTree) }, diff --git a/packages/web-app-files/src/components/SideBar/Shares/Collaborators/InviteCollaborator/InviteCollaboratorForm.vue b/packages/web-app-files/src/components/SideBar/Shares/Collaborators/InviteCollaborator/InviteCollaboratorForm.vue index 71ae9934cd7..7ba0d2fe65a 100644 --- a/packages/web-app-files/src/components/SideBar/Shares/Collaborators/InviteCollaborator/InviteCollaboratorForm.vue +++ b/packages/web-app-files/src/components/SideBar/Shares/Collaborators/InviteCollaborator/InviteCollaboratorForm.vue @@ -186,9 +186,6 @@ export default { return this.$gettext('Invite') }, - currentStorageId() { - return this.$route.params.storageId - }, resourceIsSpace() { return this.highlightedFile.type === 'space' }, @@ -326,13 +323,6 @@ export default { this.selectedRole.permissions(this.hasResharing || this.resourceIsSpace) ) - let storageId - if (this.resourceIsSpace) { - storageId = this.highlightedFile.id - } else if (this.currentStorageId) { - storageId = this.currentStorageId - } - this.addShare({ client: this.$client, graphClient: this.graphClient, @@ -342,8 +332,9 @@ export default { displayName: collaborator.label, shareType: collaborator.value.shareType, permissions: bitmask, + role: this.selectedRole, expirationDate: this.expirationDate, - storageId + storageId: this.highlightedFile.fileId }) }) ) 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 index 1e6c4636827..710d100d453 100644 --- a/packages/web-app-files/src/components/SideBar/Shares/Collaborators/ListItem.vue +++ b/packages/web-app-files/src/components/SideBar/Shares/Collaborators/ListItem.vue @@ -309,7 +309,8 @@ export default { graphClient: this.graphClient, share: this.share, permissions: bitmask, - expirationDate: expirationDate || '' + expirationDate: expirationDate || '', + role }) } } diff --git a/packages/web-app-files/src/components/SideBar/Shares/FileLinks.vue b/packages/web-app-files/src/components/SideBar/Shares/FileLinks.vue index a32c84dcaea..10403c7eed4 100644 --- a/packages/web-app-files/src/components/SideBar/Shares/FileLinks.vue +++ b/packages/web-app-files/src/components/SideBar/Shares/FileLinks.vue @@ -118,9 +118,9 @@ import { getParentPaths } from '../../../helpers/path' import { ShareTypes, LinkShareRoles, SharePermissions } from '../../../helpers/share' import { cloneStateObject } from '../../../helpers/store' import { showQuickLinkPasswordModal } from '../../../quickActions' -import CreateQuickLink from './Links/CreateQuickLink.vue' import DetailsAndEdit from './Links/DetailsAndEdit.vue' import NameAndCopy from './Links/NameAndCopy.vue' +import CreateQuickLink from './Links/CreateQuickLink.vue' export default defineComponent({ name: 'FileLinks', @@ -336,25 +336,8 @@ export default defineComponent({ return this.$route.params.storageId || null } }, - watch: { - highlightedFile(newItem, oldItem) { - if (oldItem !== newItem) { - this.reloadLinks() - } - } - }, - mounted() { - this.reloadLinks() - }, - methods: { - ...mapActions('Files', [ - 'addLink', - 'updateLink', - 'removeLink', - 'loadSharesTree', - 'loadCurrentFileOutgoingShares' - ]), + ...mapActions('Files', ['addLink', 'updateLink', 'removeLink']), ...mapActions(['showMessage', 'createModal', 'hideModal']), toggleLinkListCollapsed() { @@ -485,9 +468,9 @@ export default defineComponent({ permissions: link.permissions, quicklink: link.quicklink, name: link.name, + spaceRef: this.highlightedFile.fileId, ...(this.currentStorageId && { - storageId: this.currentStorageId, - spaceRef: `${this.currentStorageId}${this.highlightedFile.path}` + storageId: this.currentStorageId }) } }, diff --git a/packages/web-app-files/src/components/SideBar/Shares/FileShares.vue b/packages/web-app-files/src/components/SideBar/Shares/FileShares.vue index 97c8fcd2d75..1969eeebb90 100644 --- a/packages/web-app-files/src/components/SideBar/Shares/FileShares.vue +++ b/packages/web-app-files/src/components/SideBar/Shares/FileShares.vue @@ -61,7 +61,6 @@ diff --git a/packages/web-app-files/src/store/actions.js b/packages/web-app-files/src/store/actions.js index fc3385a210e..1dea3bf503c 100644 --- a/packages/web-app-files/src/store/actions.js +++ b/packages/web-app-files/src/store/actions.js @@ -14,7 +14,7 @@ import { $gettext, $gettextInterpolate } from '../gettext' import { loadPreview, move, copy } from '../helpers/resource' import { avatarUrl } from '../helpers/user' import { has } from 'lodash-es' -import { ShareTypes, SpacePeopleShareRoles } from '../helpers/share' +import { ShareTypes } from '../helpers/share' import { sortSpaceMembers } from '../helpers/space' import get from 'lodash-es/get' import { ClipboardActions } from '../helpers/clipboardActions' @@ -243,11 +243,6 @@ export default { context.commit('CURRENT_FILE_OUTGOING_SHARES_ERROR', null) context.commit('CURRENT_FILE_OUTGOING_SHARES_LOADING', true) - let spaceRef - if (storageId) { - spaceRef = `${storageId}${path}` - } - if (resource?.type === 'space') { const promises = [] const spaceMembers = [] @@ -264,7 +259,7 @@ export default { } promises.push( - client.shares.getShares(path, { reshares: true, spaceRef }).then((data) => { + client.shares.getShares(path, { reshares: true, spaceRef: storageId }).then((data) => { for (const element of data) { spaceLinks.push( buildShare( @@ -294,7 +289,7 @@ export default { // see https://owncloud.dev/owncloud-sdk/Shares.html client.shares - .getShares(path, { reshares: true, spaceRef }) + .getShares(path, { reshares: true, spaceRef: storageId }) .then((data) => { context.commit( 'CURRENT_FILE_OUTGOING_SHARES_SET', @@ -342,70 +337,56 @@ export default { context.commit('INCOMING_SHARES_LOADING', false) }) }, - changeShare( + async changeShare( { commit, getters, rootGetters }, - { client, graphClient, share, permissions, expirationDate } + { client, graphClient, share, permissions, expirationDate, role } ) { - const params = { - permissions: permissions, - expireDate: expirationDate + if (!permissions && !role) { + throw new Error('Nothing changed') } - if (!params.permissions) { - return new Promise((resolve, reject) => { - reject(new Error('Nothing changed')) + if (share.shareType === ShareTypes.space.value) { + await client.shares.shareSpaceWithUser('', share.collaborator.name, share.id, { + permissions, + role: role.name }) - } - if (share.shareType === ShareTypes.space.value) { - return new Promise((resolve, reject) => { - client.shares - .shareSpaceWithUser('', share.collaborator.name, share.id, { - permissions - }) - .then(() => { - const role = SpacePeopleShareRoles.getByBitmask(permissions) - const shareObj = { - role: role.name, - onPremisesSamAccountName: share.collaborator.name, - displayName: share.collaborator.displayName - } - const updatedShare = buildSpaceShare(shareObj, share.id) - commit('CURRENT_FILE_OUTGOING_SHARES_UPSERT', updatedShare) - - graphClient.drives.getDrive(share.id).then((response) => { - const space = buildSpace(response.data) - commit('UPDATE_RESOURCE_FIELD', { - id: share.id, - field: 'spaceRoles', - value: space.spaceRoles - }) - }) + const updatedShare = buildSpaceShare( + { + role: role.name, + onPremisesSamAccountName: share.collaborator.name, + displayName: share.collaborator.displayName + }, + share.id + ) - resolve(updatedShare) - }) - .catch((e) => { - reject(e) - }) + commit('CURRENT_FILE_OUTGOING_SHARES_UPSERT', updatedShare) + + const { data: drive } = await graphClient.drives.getDrive(share.id) + const space = buildSpace(drive) + commit('UPDATE_RESOURCE_FIELD', { + id: share.id, + field: 'spaceRoles', + value: space.spaceRoles }) + + return } - return new Promise((resolve, reject) => { - client.shares - .updateShare(share.id, params) - .then((updatedShare) => { - const share = buildCollaboratorShare( - updatedShare.shareInfo, - getters.highlightedFile, - allowSharePermissions(rootGetters) - ) - commit('CURRENT_FILE_OUTGOING_SHARES_UPSERT', share) - resolve(share) - }) - .catch((e) => { - reject(e) - }) + const updatedShare = await client.shares.updateShare(share.id, { + role: role.name, + permissions, + expireDate: expirationDate }) + + commit( + 'CURRENT_FILE_OUTGOING_SHARES_UPSERT', + buildCollaboratorShare( + updatedShare.shareInfo, + getters.highlightedFile, + allowSharePermissions(rootGetters) + ) + ) }, addShare( context, @@ -416,22 +397,19 @@ export default { shareWith, shareType, permissions, + role, expirationDate, storageId, displayName } ) { - let spaceRef - if (storageId) { - spaceRef = `${storageId}${path}` - } - if (shareType === ShareTypes.group.value) { client.shares .shareFileWithGroup(path, shareWith, { - permissions: permissions, - expirationDate: expirationDate, - spaceRef + permissions, + role: role.name, + expirationDate, + spaceRef: storageId }) .then((share) => { context.commit( @@ -462,10 +440,10 @@ export default { if (shareType === ShareTypes.space.value) { client.shares .shareSpaceWithUser(path, shareWith, storageId, { - permissions + permissions, + role: role.name }) .then(() => { - const role = SpacePeopleShareRoles.getByBitmask(permissions) const shareObj = { role: role.name, onPremisesSamAccountName: shareWith, @@ -505,10 +483,11 @@ export default { const remoteShare = shareType === ShareTypes.remote.value client.shares .shareFileWithUser(path, shareWith, { - permissions: permissions, remoteUser: remoteShare, - expirationDate: expirationDate, - spaceRef + permissions, + role: role.name, + expirationDate, + spaceRef: storageId }) .then((share) => { context.commit( @@ -591,11 +570,6 @@ export default { return Promise.resolve() } - let spaceRef - if (storageId) { - spaceRef = `${storageId}${path}` - } - // remove last entry which is the root folder parentPaths.pop() @@ -613,7 +587,7 @@ export default { shareQueriesPromises.push( shareQueriesQueue.add(() => client.shares - .getShares(queryPath, { reshares: true, spaceRef }) + .getShares(queryPath, { reshares: true, spaceRef: storageId }) .then((data) => { data.forEach((element) => { sharesTree[queryPath].push({ @@ -638,7 +612,7 @@ export default { shareQueriesPromises.push( shareQueriesQueue.add(() => client.shares - .getShares(queryPath, { shared_with_me: true, spaceRef }) + .getShares(queryPath, { shared_with_me: true, spaceRef: storageId }) .then((data) => { data.forEach((element) => { sharesTree[queryPath].push({ diff --git a/packages/web-app-files/tests/unit/components/SideBar/Shares/FileShares.spec.js b/packages/web-app-files/tests/unit/components/SideBar/Shares/FileShares.spec.js index 8a8e5fd6e9a..a78caebd9f4 100644 --- a/packages/web-app-files/tests/unit/components/SideBar/Shares/FileShares.spec.js +++ b/packages/web-app-files/tests/unit/components/SideBar/Shares/FileShares.spec.js @@ -93,18 +93,6 @@ describe('FileShares', () => { await wrapper.vm.$nextTick() expect(spyOnCollaboratorDeleteTrigger).toHaveBeenCalledTimes(1) }) - it('reloads shares if highlighted file is changed', async () => { - const spyOnReloadShares = jest - .spyOn(FileShares.methods, '$_reloadShares') - .mockImplementation() - const wrapper = getMountedWrapper({ - user, - outgoingCollaborators: collaborators - }) - wrapper.vm.$store.commit('Files/SET_HIGHLIGHTED_FILE', { name: 'testfile2' }) - await wrapper.vm.$nextTick() - expect(spyOnReloadShares).toHaveBeenCalledTimes(1) - }) it('correctly passes the shared parent route to the collaborator list item', () => { const wrapper = getShallowMountedWrapper({ user, diff --git a/packages/web-app-files/tests/unit/components/SideBar/Shares/SharesPanel.spec.js b/packages/web-app-files/tests/unit/components/SideBar/Shares/SharesPanel.spec.js new file mode 100644 index 00000000000..c31f11c18d9 --- /dev/null +++ b/packages/web-app-files/tests/unit/components/SideBar/Shares/SharesPanel.spec.js @@ -0,0 +1,49 @@ +import { createLocalVue, mount } from '@vue/test-utils' +import GetTextPlugin from 'vue-gettext' +import Vuex from 'vuex' +import DesignSystem from 'owncloud-design-system' +import SharesPanel from '@files/src/components/SideBar/Shares/SharesPanel.vue' + +const localVue = createLocalVue() +localVue.use(DesignSystem) +localVue.use(Vuex) +localVue.use(GetTextPlugin, { + translations: 'does-not-matter.json', + silent: true +}) + +jest.mock('web-pkg/src/composables/reactivity') +describe('SharesPanel', () => { + it('reloads shares if highlighted file is changed', async () => { + const spyOnReloadShares = jest.spyOn(SharesPanel.methods, '$_reloadShares').mockImplementation() + const wrapper = mount(SharesPanel, { + localVue, + store: new Vuex.Store({ + modules: { + Files: { + namespaced: true, + state: { + highlightedFile: { name: '1' } + }, + getters: { + highlightedFile: (state) => { + return state.highlightedFile + } + }, + mutations: { + SET_HIGHLIGHTED_FILE(state, file) { + state.highlightedFile = file + } + } + } + } + }), + stubs: { + 'file-shares': true + } + }) + wrapper.vm.$store.commit('Files/SET_HIGHLIGHTED_FILE', { name: '2' }) + await wrapper.vm.$nextTick() + expect(spyOnReloadShares).toHaveBeenCalledTimes(2) + }) +}) diff --git a/packages/web-app-files/tests/unit/store/actions.spec.js b/packages/web-app-files/tests/unit/store/actions.spec.js index 1dcffb8e5ce..63685553ea8 100644 --- a/packages/web-app-files/tests/unit/store/actions.spec.js +++ b/packages/web-app-files/tests/unit/store/actions.spec.js @@ -108,7 +108,8 @@ describe('vuex store actions', () => { client: clientMock, graphClient: graphClientMock, share: dataSet.share, - permissions: 1, + permissions: spaceRoleManager.bitmask(false), + role: spaceRoleManager, expirationDate: null }) @@ -128,7 +129,8 @@ describe('vuex store actions', () => { graphClient: graphClientMock, shareType: dataSet.shareType, storageId: dataSet.storageId, - permissions: 1, + permissions: spaceRoleManager.bitmask(false), + role: spaceRoleManager, expirationDate: null }) diff --git a/tests/acceptance/features/webUISharingPermissionsUsers/sharePermissionsUsers.feature b/tests/acceptance/features/webUISharingPermissionsUsers/sharePermissionsUsers.feature index 41ba92b0d95..55fb0d69018 100644 --- a/tests/acceptance/features/webUISharingPermissionsUsers/sharePermissionsUsers.feature +++ b/tests/acceptance/features/webUISharingPermissionsUsers/sharePermissionsUsers.feature @@ -166,6 +166,15 @@ Feature: Sharing files and folders with internal users with different permission And no custom permissions should be set for collaborator "Alice Hansen" for folder "simple-folder" on the webUI + + + + + + + + + Scenario: Resource owner upgrades share permissions of a re-share Given user "Carol" has been created with default attributes and without skeleton files in the server And user "Brian" has shared folder "simple-folder" with user "Alice" with "read, share, delete" permissions in the server diff --git a/tests/acceptance/stepDefinitions/debugContext.js b/tests/acceptance/stepDefinitions/debugContext.js new file mode 100644 index 00000000000..45974137941 --- /dev/null +++ b/tests/acceptance/stepDefinitions/debugContext.js @@ -0,0 +1,5 @@ +const { When } = require('@cucumber/cucumber') + +When('pause for {int}', async function (duration) { + await new Promise((resolve) => setTimeout(resolve, duration)) +}) diff --git a/tests/e2e/cucumber/steps/app-files/resource.ts b/tests/e2e/cucumber/steps/app-files/resource.ts index 87970eaffcb..e2af2b4e69a 100644 --- a/tests/e2e/cucumber/steps/app-files/resource.ts +++ b/tests/e2e/cucumber/steps/app-files/resource.ts @@ -1,4 +1,6 @@ import { DataTable, When, Then } from '@cucumber/cucumber' +import path from 'path' +import { DataTable, When } from '@cucumber/cucumber' import { World } from '../../environment' import { objects } from '../../../support' import { expect } from '@playwright/test' @@ -223,13 +225,14 @@ export const processDownload = async ( } else { expect(downloads.length).toBe(1) downloads.forEach((download) => { + const { name } = path.parse(download.suggestedFilename()) if (config.ocis) { - expect(download.suggestedFilename()).toBe('download.tar') + expect(name).toBe('download') } else { if (parentFolder) { - expect(download.suggestedFilename()).toBe(parentFolder + '.zip') + expect(name).toBe(parentFolder) } else { - expect(download.suggestedFilename()).toBe('download.zip') + expect(name).toBe('download') } } }) diff --git a/tests/e2e/support/api/graph/user.ts b/tests/e2e/support/api/graph/user.ts index be3bbb5d90e..500023588cf 100644 --- a/tests/e2e/support/api/graph/user.ts +++ b/tests/e2e/support/api/graph/user.ts @@ -26,6 +26,7 @@ export const createUser = async ({ user, admin }: { user: User; admin: User }): body, user: admin }) + checkResponseStatus(response, 'Failed while creating user') return user } diff --git a/tests/e2e/support/environment/actor/shared.ts b/tests/e2e/support/environment/actor/shared.ts index 67720f87083..2ecd519d4fb 100644 --- a/tests/e2e/support/environment/actor/shared.ts +++ b/tests/e2e/support/environment/actor/shared.ts @@ -20,7 +20,8 @@ export interface ActorOptions extends ActorsOptions { export const buildBrowserContextOptions = (options: ActorOptions): BrowserContextOptions => { const contextOptions: BrowserContextOptions = { acceptDownloads: options.context.acceptDownloads, - ignoreHTTPSErrors: true + ignoreHTTPSErrors: true, + locale: 'en-US' } if (options.context.reportVideo) {