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

[full-ci] Implement permission concept for handling user abilities #8431

Merged
merged 20 commits into from
Feb 17, 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=3e3d3f9de2919368f5849061e9c3ea6894c6457c
OCIS_COMMITID=359509dcfe0bd8bf23c71c7aee5fdd27faa7675a
OCIS_BRANCH=master
5 changes: 5 additions & 0 deletions changelog/unreleased/change-remove-permission-manager
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Change: Remove permission manager

BREAKING CHANGE for developers: The `PermissionManager` has been removed. Permission management is now being handled by `CASL`. For more details on how it works please see the linked PR down below.

https://github.com/owncloud/web/pull/8431
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
<div class="oc-flex oc-flex-middle">
<h3 v-text="$gettext('Logo')" />
<oc-button
v-if="menuItems.length"
:id="`logo-context-btn`"
v-oc-tooltip="$gettext('Show context menu')"
:aria-label="$gettext('Show context menu')"
Expand Down Expand Up @@ -59,10 +60,9 @@ export default defineComponent({
const store = useStore()
const instance = getCurrentInstance().proxy as any

const menuItems = computed(() => [
...instance.$_uploadLogo_items,
...instance.$_resetLogo_items
])
const menuItems = computed(() =>
[...instance.$_uploadLogo_items, ...instance.$_resetLogo_items].filter((i) => i.isEnabled())
)
const logo = computed(() => store.getters.configuration.currentTheme.logo.topbar)
const menuSections = computed(() => [
{
Expand Down
27 changes: 19 additions & 8 deletions packages/web-app-admin-settings/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import General from './views/General.vue'
import Users from './views/Users.vue'
import Groups from './views/Groups.vue'
import Spaces from './views/Spaces.vue'
import { Ability } from 'web-pkg'

// just a dummy function to trick gettext tools
function $gettext(msg) {
return msg
Expand All @@ -15,14 +17,23 @@ const appInfo = {
isFileEditor: false
}

const routes = ({ $permissionManager }) => [
const routes = ({ $ability }: { $ability: Ability }) => [
{
path: '/',
redirect: () => {
if ($permissionManager.hasSystemManagement()) {
if ($ability.can('read-all', 'Setting')) {
return { name: 'admin-settings-general' }
}
return { name: 'admin-settings-spaces' }
if ($ability.can('read-all', 'Account')) {
return { name: 'admin-settings-users' }
}
if ($ability.can('read-all', 'Group')) {
return { name: 'admin-settings-groups' }
}
if ($ability.can('read-all', 'Space')) {
return { name: 'admin-settings-spaces' }
}
throw Error('Insufficient permissions')
kulmann marked this conversation as resolved.
Show resolved Hide resolved
}
},
{
Expand Down Expand Up @@ -63,15 +74,15 @@ const routes = ({ $permissionManager }) => [
}
]

const navItems = ({ $permissionManager }) => [
const navItems = ({ $ability }: { $ability: Ability }) => [
{
name: $gettext('General'),
icon: 'settings-4',
route: {
path: `/${appInfo.id}/general?`
},
enabled: () => {
return $permissionManager.hasSystemManagement()
return $ability.can('read-all', 'Setting')
}
},
{
Expand All @@ -81,7 +92,7 @@ const navItems = ({ $permissionManager }) => [
path: `/${appInfo.id}/users?`
},
enabled: () => {
return $permissionManager.hasUserManagement()
return $ability.can('read-all', 'Account')
}
},
{
Expand All @@ -91,7 +102,7 @@ const navItems = ({ $permissionManager }) => [
path: `/${appInfo.id}/groups?`
},
enabled: () => {
return $permissionManager.hasUserManagement()
return $ability.can('read-all', 'Group')
}
},
{
Expand All @@ -101,7 +112,7 @@ const navItems = ({ $permissionManager }) => [
path: `/${appInfo.id}/spaces?`
},
enabled: () => {
return $permissionManager.hasSpaceManagement()
return $ability.can('read-all', 'Space')
}
}
]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ export default {
label: () => {
return this.$gettext('Reset logo')
},
isEnabled: () => {
return this.$can('update-all', 'Logo')
},
handler: this.$_resetLogo_trigger,
componentType: 'button',
class: 'oc-general-actions-reset-logo-trigger'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ export default {
label: () => {
return this.$gettext('Upload logo')
},
isEnabled: () => {
return this.$can('update-all', 'Logo')
},
handler: this.$_uploadLogo_trigger,
componentType: 'button',
class: 'oc-general-actions-upload-logo-trigger'
Expand Down
59 changes: 46 additions & 13 deletions packages/web-app-admin-settings/tests/unit/index.spec.ts
Original file line number Diff line number Diff line change
@@ -1,46 +1,46 @@
import index from '../../src/index'
import { Ability } from 'web-pkg'
import { mock } from 'jest-mock-extended'

const getAbilityMock = (hasPermission) => mock<Ability>({ can: () => hasPermission })

describe('admin settings index', () => {
describe('navItems', () => {
describe('general', () => {
it.each([true, false])('should be enabled according to the permissions', (enabled) => {
const $permissionManager = { hasSystemManagement: () => enabled }
expect(
index
.navItems({ $permissionManager })
.navItems({ $ability: getAbilityMock(enabled) })
.find((n) => n.name === 'General')
.enabled()
).toBe(enabled)
})
})
describe('user management', () => {
it.each([true, false])('should be enabled according to the permissions', (enabled) => {
const $permissionManager = { hasUserManagement: () => enabled }
expect(
index
.navItems({ $permissionManager })
.navItems({ $ability: getAbilityMock(enabled) })
.find((n) => n.name === 'Users')
.enabled()
).toBe(enabled)
})
})
describe('group management', () => {
it.each([true, false])('should be enabled according to the permissions', (enabled) => {
const $permissionManager = { hasUserManagement: () => enabled }
expect(
index
.navItems({ $permissionManager })
.navItems({ $ability: getAbilityMock(enabled) })
.find((n) => n.name === 'Groups')
.enabled()
).toBe(enabled)
})
})
describe('space management', () => {
it.each([true, false])('should be enabled according to the permissions', (enabled) => {
const $permissionManager = { hasSpaceManagement: () => enabled }
expect(
index
.navItems({ $permissionManager })
.navItems({ $ability: getAbilityMock(enabled) })
.find((n) => n.name === 'Spaces')
.enabled()
).toBe(enabled)
Expand All @@ -50,23 +50,56 @@ describe('admin settings index', () => {
describe('routes', () => {
describe('default-route "/"', () => {
it('should redirect to general if permission given', () => {
const $permissionManager = { hasSystemManagement: () => true }
const ability = mock<Ability>()
ability.can.mockReturnValueOnce(true)
expect(
index
.routes({ $permissionManager })
.routes({ $ability: ability })
.find((n) => n.path === '/')
.redirect().name
).toEqual('admin-settings-general')
})
it('should redirect to space management if no system management permission given', () => {
const $permissionManager = { hasSystemManagement: () => false }
it('should redirect to user management if permission given', () => {
const ability = mock<Ability>()
ability.can.mockReturnValueOnce(false)
ability.can.mockReturnValueOnce(true)
expect(
index
.routes({ $ability: ability })
.find((n) => n.path === '/')
.redirect().name
).toEqual('admin-settings-users')
})
it('should redirect to group management if permission given', () => {
const ability = mock<Ability>()
ability.can.mockReturnValueOnce(false)
ability.can.mockReturnValueOnce(false)
ability.can.mockReturnValueOnce(true)
expect(
index
.routes({ $permissionManager })
.routes({ $ability: ability })
.find((n) => n.path === '/')
.redirect().name
).toEqual('admin-settings-groups')
})
it('should redirect to space management if permission given', () => {
const ability = mock<Ability>()
ability.can.mockReturnValueOnce(false)
ability.can.mockReturnValueOnce(false)
ability.can.mockReturnValueOnce(false)
ability.can.mockReturnValueOnce(true)
expect(
index
.routes({ $ability: ability })
.find((n) => n.path === '/')
.redirect().name
).toEqual('admin-settings-spaces')
})
it('should throw an error if permissions are insufficient', () => {
const ability = mock<Ability>()
ability.can.mockReturnValue(false)
expect(index.routes({ $ability: ability }).find((n) => n.path === '/').redirect).toThrow()
})
})
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,6 @@ import {
} from 'web-test-helpers'
import Spaces from '../../../src/views/Spaces.vue'

window.ResizeObserver =
window.ResizeObserver ||
jest.fn().mockImplementation(() => ({
disconnect: jest.fn(),
observe: jest.fn(),
unobserve: jest.fn()
}))

kulmann marked this conversation as resolved.
Show resolved Hide resolved
const selectors = {
loadingSpinnerStub: 'app-loading-spinner-stub',
spacesListStub: 'spaces-list-stub',
Expand Down
11 changes: 6 additions & 5 deletions packages/web-app-files/src/views/spaces/Projects.vue
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ import AppLoadingSpinner from 'web-pkg/src/components/AppLoadingSpinner.vue'

import AppBar from '../../components/AppBar/AppBar.vue'
import CreateSpace from '../../components/AppBar/CreateSpace.vue'
import { useAccessToken, useStore, useGraphClient } from 'web-pkg/src/composables'
import { useAbility, useAccessToken, useStore, useGraphClient } from 'web-pkg/src/composables'
import { loadPreview } from 'web-pkg/src/helpers/preview'
import { ImageDimension } from 'web-pkg/src/constants'
import SpaceContextActions from '../../components/Spaces/SpaceContextActions.vue'
Expand Down Expand Up @@ -103,6 +103,7 @@ export default defineComponent({
setup() {
const store = useStore()
const { selectedResourcesIds } = useSelectedResources({ store })
const { can } = useAbility()

const runtimeSpaces = computed(
() => store.getters['runtime/spaces/spaces'].filter((s) => isProjectSpaceResource(s)) || []
Expand Down Expand Up @@ -135,6 +136,8 @@ export default defineComponent({
return loadResourcesTask.isRunning || !loadResourcesTask.last
})

const hasCreatePermission = computed(() => can('create-all', 'Space'))

return {
...useSideBar(),
...useScrollTo(),
Expand All @@ -147,7 +150,8 @@ export default defineComponent({
handleSort,
sortBy,
sortDir,
sortFields
sortFields,
hasCreatePermission
}
},
data: function () {
Expand All @@ -168,9 +172,6 @@ export default defineComponent({
},
showSpaceMemberLabel() {
return this.$gettext('Show members')
},
hasCreatePermission() {
return this.$permissionManager.hasSpaceManagement()
}
},
watch: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,6 @@ const breadCrumbItemWithContextActionAllowed = {
allowContextActions: true
}

window.ResizeObserver =
window.ResizeObserver ||
jest.fn().mockImplementation(() => ({
disconnect: jest.fn(),
observe: jest.fn(),
unobserve: jest.fn()
}))

describe('AppBar component', () => {
describe('renders', () => {
it('by default no breadcrumbs, no bulkactions, no sharesnavigation but viewoptions and sidebartoggle', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,7 @@ function getWrapper(space) {
global: {
mocks: {
...defaultComponentMocks({ currentRoute: mock<RouteLocation>({ path: '/files' }) }),
$permissionManager: {
canEditSpaceQuota: () => true
}
$can: () => true
},
plugins: [...defaultPlugins(), store]
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,6 @@ import {
defaultComponentMocks
} from 'web-test-helpers'

window.ResizeObserver =
window.ResizeObserver ||
jest.fn().mockImplementation(() => ({
disconnect: jest.fn(),
observe: jest.fn(),
unobserve: jest.fn()
}))

afterEach(() => jest.clearAllMocks())

describe('SpaceHeader', () => {
it('should add the "squashed"-class when the sidebar is opened', () => {
const wrapper = getWrapper({ space: buildSpace({ id: 1 }), sideBarOpen: true })
Expand All @@ -42,12 +32,7 @@ describe('SpaceHeader', () => {
})

function getWrapper({ space = {}, sideBarOpen = false }) {
const mocks = {
...defaultComponentMocks(),
$permissionManager: {
canEditSpaceQuota: () => true
}
}
const mocks = defaultComponentMocks()
const store = createStore(defaultStoreMockOptions)
return mount(SpaceHeader, {
props: {
Expand All @@ -58,7 +43,8 @@ function getWrapper({ space = {}, sideBarOpen = false }) {
mocks,
plugins: [...defaultPlugins(), store],
stubs: {
'quota-modal': true
'quota-modal': true,
'space-context-actions': true
}
}
})
Expand Down
Loading