Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add permission checks for creating shares and favorites #9810

Merged
merged 6 commits into from
Nov 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .drone.env
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
# The version of OCIS to use in pipelines that test against OCIS
OCIS_COMMITID=c4d3ec7e5b07abe00ce3d9c322387eba80224ecf
OCIS_COMMITID=a9366caa0cb97de0c4a68a43f05156cc584bee16
OCIS_BRANCH=master
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
Enhancement: Permission checks for shares and favorites

Permission checks for creating shares and favorites have been added.

https://github.com/owncloud/ocis/issues/7497
https://github.com/owncloud/web/pull/9810
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ export default defineComponent({
const language = useGettext()

const filteredActions = computed(() =>
pickBy(props.actions, (action) => action.displayed(props.item, store) === true)
pickBy(props.actions, (action) => action.displayed(props.item, store, ability) === true)
)

return {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ import {
import * as uuid from 'uuid'
import { defineComponent, inject, PropType, ComponentPublicInstance } from 'vue'
import {
useAbility,
useCapabilityFilesSharingAllowCustomPermissions,
useCapabilityFilesSharingResharingDefault,
useStore
Expand Down Expand Up @@ -166,7 +167,9 @@ export default defineComponent({
emits: ['optionChange'],
setup() {
const store = useStore()
const ability = useAbility()
return {
ability,
resource: inject<Resource>('resource'),
incomingParentShare: inject<Share>('incomingParentShare'),
hasRoleCustomPermissions: useCapabilityFilesSharingAllowCustomPermissions(store),
Expand Down Expand Up @@ -202,7 +205,7 @@ export default defineComponent({
return PeopleShareRoles.custom(this.resource.isFolder)
},
resourceIsSharable() {
return this.allowSharePermission && this.resource.canShare()
return this.allowSharePermission && this.resource.canShare({ ability: this.ability })
},
availableRoles() {
if (this.resourceIsSpace) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,8 @@ export default defineComponent({
},
setup() {
const store = useStore()
const { can } = useAbility()
const ability = useAbility()
const { can } = ability
kulmann marked this conversation as resolved.
Show resolved Hide resolved
const passwordPolicyService = usePasswordPolicyService()
const hasResharing = useCapabilityFilesSharingResharing()

Expand Down Expand Up @@ -206,7 +207,7 @@ export default defineComponent({
return false
}

return unref(resource).canShare({ user: store.getters.user })
return unref(resource).canShare({ user: store.getters.user, ability })
})

const canEditLink = ({ permissions }: Share) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@
<script lang="ts">
import { mapGetters, mapActions, mapState, mapMutations } from 'vuex'
import {
useAbility,
useStore,
useCapabilityProjectSpacesEnabled,
useCapabilityShareJailEnabled,
Expand Down Expand Up @@ -125,6 +126,7 @@ export default defineComponent({
},
setup() {
const store = useStore()
const ability = useAbility()
const { getMatchingSpace } = useGetMatchingSpace()

const resource = inject<Ref<Resource>>('resource')
Expand Down Expand Up @@ -164,6 +166,7 @@ export default defineComponent({

return {
...useShares(),
ability,
resource,
space: inject<Ref<SpaceResource>>('space'),
matchingSpace,
Expand Down Expand Up @@ -270,7 +273,7 @@ export default defineComponent({
if (isShareJail && !this.hasResharing) {
return false
}
return this.resource.canShare({ user: this.user })
return this.resource.canShare({ user: this.user, ability: this.ability })
},

noResharePermsMessage() {
Expand Down
2 changes: 1 addition & 1 deletion packages/web-app-files/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ export const navItems = (context): AppNavigationItem[] => {
path: `/${appInfo.id}/favorites`
},
enabled(capabilities) {
return capabilities.files?.favorites
return capabilities.files?.favorites && context.$ability.can('read-all', 'Favorite')
},
priority: 20
},
Expand Down
8 changes: 5 additions & 3 deletions packages/web-client/src/helpers/resource/functions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -155,8 +155,10 @@ export function buildResource(resource: WebDavResponseResource): Resource {
canRename: function () {
return this.permissions.indexOf(DavPermission.Renameable) >= 0
},
canShare: function () {
return this.permissions.indexOf(DavPermission.Shareable) >= 0
canShare: function ({ ability }) {
return (
ability.can('create-all', 'Share') && this.permissions.indexOf(DavPermission.Shareable) >= 0
)
},
canCreate: function () {
return this.permissions.indexOf(DavPermission.FolderCreateable) >= 0
Expand All @@ -182,7 +184,7 @@ export function buildResource(resource: WebDavResponseResource): Resource {
return this.permissions.indexOf(DavPermission.Deny) >= 0
},
getDomSelector: () => extractDomSelector(id)
}
} satisfies Resource
Object.defineProperty(r, 'nodeId', {
get() {
return extractNodeId(this.id)
Expand Down
4 changes: 3 additions & 1 deletion packages/web-client/src/helpers/resource/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,15 @@ export type AbilityActions =
export type AbilitySubjects =
| 'Account'
| 'Drive'
| 'Favorite'
| 'Group'
| 'Language'
| 'Logo'
| 'PublicLink'
| 'ReadOnlyPublicLinkPassword'
| 'Role'
| 'Setting'
| 'Share'

export type Ability = MongoAbility<[AbilityActions, AbilitySubjects]>
export type AbilityRule = SubjectRawRule<AbilityActions, AbilitySubjects, any>
Expand Down Expand Up @@ -84,7 +86,7 @@ export interface Resource {
canCreate?(): boolean
canUpload?({ user }: { user?: User }): boolean
canDownload?(): boolean
canShare?({ user }?: { user?: User }): boolean
canShare?({ user, ability }?: { user?: User; ability?: Ability }): boolean
canRename?({ user }?: { user?: User; ability?: Ability }): boolean
canBeDeleted?({ user }?: { user?: User; ability?: Ability }): boolean
canBeRestored?(): boolean
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
import { mock, mockDeep } from 'jest-mock-extended'
import {
Ability,
Resource,
WebDavResponseResource,
buildResource,
extractDomSelector,
extractExtensionFromFile,
extractNameWithoutExtension
} from '../../../../src/helpers'
import { DavPermission, DavProperty } from '../../../../src/webdav/constants'

describe('extractDomSelector', () => {
it.each([
Expand Down Expand Up @@ -75,3 +80,47 @@ describe('filterResources', () => {
})
})
})

describe('buildResource', () => {
describe('canShare', () => {
it('is true when ability and share permissions are given', () => {
const webDavResponse = mockDeep<WebDavResponseResource>({
props: {
[DavProperty.Permissions]: DavPermission.Shareable,
[DavProperty.Tags]: undefined
}
})
const resource = buildResource(webDavResponse)
const ability = mock<Ability>()
ability.can.mockReturnValue(true)
expect(resource.canShare({ ability })).toBeTruthy()
expect(ability.can).toHaveBeenCalledWith('create-all', 'Share')
})
it('is false when ability is not given', () => {
const webDavResponse = mockDeep<WebDavResponseResource>({
props: {
[DavProperty.Permissions]: DavPermission.Shareable,
[DavProperty.Tags]: undefined
}
})
const resource = buildResource(webDavResponse)
const ability = mock<Ability>()
ability.can.mockReturnValue(false)
expect(resource.canShare({ ability })).toBeFalsy()
expect(ability.can).toHaveBeenCalledWith('create-all', 'Share')
})
it('is false when share permissions are not given', () => {
const webDavResponse = mockDeep<WebDavResponseResource>({
props: {
[DavProperty.Permissions]: '',
[DavProperty.Tags]: undefined
}
})
const resource = buildResource(webDavResponse)
const ability = mock<Ability>()
ability.can.mockReturnValue(true)
expect(resource.canShare({ ability })).toBeFalsy()
expect(ability.can).toHaveBeenCalledWith('create-all', 'Share')
})
})
})
6 changes: 5 additions & 1 deletion packages/web-pkg/src/components/AppBar/AppBar.vue
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ import {
useFileActionsRestore
} from '../../composables/actions'
import {
useAbility,
useCapabilitySpacesMaxQuota,
useFileActionsToggleHideShare,
useRouteMeta,
Expand Down Expand Up @@ -165,6 +166,7 @@ export default defineComponent({
setup(props, { emit }) {
const store = useStore()
const { $gettext } = useGettext()
const { can } = useAbility()

const { actions: acceptShareActions } = useFileActionsAcceptShare({ store })
const { actions: hideShareActions } = useFileActionsToggleHideShare({ store })
Expand All @@ -188,7 +190,9 @@ export default defineComponent({
const breadcrumbMaxWidth = ref<number>(0)
const isSearchLocation = useActiveLocation(isLocationCommonActive, 'files-common-search')

const hasSharesNavigation = computed(() => useSlots().hasOwnProperty('navigation'))
const hasSharesNavigation = computed(
() => useSlots().hasOwnProperty('navigation') && can('create-all', 'Share')
)

const batchActions = computed(() => {
let actions = [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ export const useFileActionsCreateQuickLink = ({
return false
}
}
return canShare(resources[0], store)
return canShare(resources[0], store, ability)
},
componentType: 'button',
class: 'oc-files-actions-create-quicklink-trigger'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { FileAction, FileActionOptions, useIsFilesAppActive } from '../../action
import { useStore } from '../../store'
import { useRouter } from '../../router'
import { useClientService } from '../../clientService'
import { useAbility } from '../../ability'

export const useFileActionsFavorite = ({ store }: { store?: Store<any> } = {}) => {
store = store || useStore()
Expand All @@ -16,6 +17,8 @@ export const useFileActionsFavorite = ({ store }: { store?: Store<any> } = {}) =
const clientService = useClientService()
const hasFavorites = useCapabilityFilesFavorites()
const isFilesAppActive = useIsFilesAppActive()
const ability = useAbility()

const handler = ({ resources }: FileActionOptions) => {
store
.dispatch('Files/markFavorite', {
Expand Down Expand Up @@ -58,7 +61,7 @@ export const useFileActionsFavorite = ({ store }: { store?: Store<any> } = {}) =
return false
}

return unref(hasFavorites)
return unref(hasFavorites) && ability.can('create-all', 'Favorite')
},
componentType: 'button',
class: 'oc-files-actions-favorite-trigger'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,12 @@ import { useRouter } from '../../router'
import { useStore } from '../../store'
import { Store } from 'vuex'
import { FileAction, FileActionOptions } from '../types'
import { useAbility } from '../../ability'

export const useFileActionsShowShares = ({ store }: { store?: Store<any> } = {}) => {
store = store || useStore()
const router = useRouter()
const ability = useAbility()
const { $gettext } = useGettext()
const isFilesAppActive = useIsFilesAppActive()

Expand Down Expand Up @@ -46,7 +48,7 @@ export const useFileActionsShowShares = ({ store }: { store?: Store<any> } = {})
return false
}
}
return canShare(resources[0], store)
return canShare(resources[0], store, ability)
},
componentType: 'button',
class: 'oc-files-actions-show-shares-trigger'
Expand Down
4 changes: 2 additions & 2 deletions packages/web-pkg/src/quickActions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,15 @@ import { Ability } from '@ownclouders/web-client/src/helpers/resource/types'
import { Store } from 'vuex'
import { ApplicationQuickActions } from './apps'

export function canShare(item, store) {
export function canShare(item: Resource, store: Store<any>, ability: Ability) {
const { capabilities } = store.state.user
if (!capabilities.files_sharing || !capabilities.files_sharing.api_enabled) {
return false
}
if (item.isReceivedShare() && !capabilities.files_sharing.resharing) {
return false
}
return item.canShare()
return item.canShare({ ability })
}

export function showQuickLinkPasswordModal({ $gettext, store, passwordPolicyService }, onConfirm) {
Expand Down
9 changes: 9 additions & 0 deletions packages/web-runtime/src/services/auth/abilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ export const getAbilities = (
{ action: 'read-all', subject: 'Account' },
{ action: 'update-all', subject: 'Account' }
],
'Favorites.List.all': [{ action: 'read-all', subject: 'Favorite' }],
'Favorites.Write.all': [
{ action: 'create-all', subject: 'Favorite' },
{ action: 'update-all', subject: 'Favorite' }
],
'Groups.ReadWrite.all': [
{ action: 'create-all', subject: 'Group' },
{ action: 'delete-all', subject: 'Group' },
Expand All @@ -32,6 +37,10 @@ export const getAbilities = (
{ action: 'read-all', subject: 'Role' },
{ action: 'update-all', subject: 'Role' }
],
'Shares.Write.all': [
{ action: 'create-all', subject: 'Share' },
{ action: 'update-all', subject: 'Share' }
],
'Settings.ReadWrite.all': [
{ action: 'read-all', subject: 'Setting' },
{ action: 'update-all', subject: 'Setting' }
Expand Down
14 changes: 14 additions & 0 deletions packages/web-runtime/tests/unit/services/auth/abilities.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,15 @@ describe('getAbilities', () => {
const expectedActions = ['create-all', 'delete-all', 'read-all', 'update-all']
expect(abilities).toEqual(expectedActions.map((action) => ({ action, subject: 'Group' })))
})
it.each([
{ permissions: [''], expectedActions: [] },
{ permissions: ['Favorites.List.all'], expectedActions: ['read-all'] },
{ permissions: ['Favorites.Write.all'], expectedActions: ['create-all', 'update-all'] }
])('gets correct abilities for subject "Favorites"', function (data) {
const abilities = getAbilities(data.permissions)
const expectedResult = data.expectedActions.map((action) => ({ action, subject: 'Favorite' }))
expect(abilities).toEqual(expectedResult)
})
it('gets correct abilities for subject "Language"', function () {
const abilities = getAbilities(['Language.ReadWrite.all'])
const expectedActions = ['read-all', 'update-all']
Expand All @@ -35,6 +44,11 @@ describe('getAbilities', () => {
const expectedActions = ['create-all']
expect(abilities).toEqual(expectedActions.map((action) => ({ action, subject: 'PublicLink' })))
})
it('gets correct abilities for subject "Share"', function () {
const abilities = getAbilities(['Shares.Write.all'])
const expectedActions = ['create-all', 'update-all']
expect(abilities).toEqual(expectedActions.map((action) => ({ action, subject: 'Share' })))
})
it('gets correct abilities for subject "Setting"', function () {
const abilities = getAbilities(['Settings.ReadWrite.all'])
const expectedActions = ['read-all', 'update-all']
Expand Down