Skip to content

Commit 62579fa

Browse files
LukasHirtLukasHirt
authored
Merge pull request #12780 from owncloud/fix/admin-settings-direct-access
fix(web-app-admin-settings): [OCISDEV-138] handle direct admin settings access
2 parents 496b891 + c416e52 commit 62579fa

File tree

3 files changed

+92
-59
lines changed

3 files changed

+92
-59
lines changed

‎changelog/unreleased/bugfix-handle-direct-admin-settings-access.md‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
Bugfix: Handle direct admin settings access
2+
3+
Opening the admin settings directly by pasting the URL in the browser address bar or opening the app in a new tab now works correctly.
4+
The redirect was not correctly using the navigation guard leading to a mismatch in users permissions.
5+
We now redirect to the first available route and leave the route itself to handle the permissions navigation guard.
6+
7+
https://github.com/owncloud/web/pull/12780

‎packages/web-app-admin-settings/src/index.ts‎

Lines changed: 25 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -22,23 +22,31 @@ function $gettext(msg: string) {
2222

2323
const appId = 'admin-settings'
2424

25+
function getAvailableRoute(ability: Ability) {
26+
if (ability.can('read-all', 'Setting')) {
27+
return { name: 'admin-settings-general' }
28+
}
29+
30+
if (ability.can('read-all', 'Account')) {
31+
return { name: 'admin-settings-users' }
32+
}
33+
34+
if (ability.can('read-all', 'Group')) {
35+
return { name: 'admin-settings-groups' }
36+
}
37+
38+
if (ability.can('read-all', 'Drive')) {
39+
return { name: 'admin-settings-spaces' }
40+
}
41+
42+
throw Error('Insufficient permissions')
43+
}
44+
2545
export const routes = ({ $ability }: { $ability: Ability }): RouteRecordRaw[] => [
2646
{
2747
path: '/',
2848
redirect: () => {
29-
if ($ability.can('read-all', 'Setting')) {
30-
return { name: 'admin-settings-general' }
31-
}
32-
if ($ability.can('read-all', 'Account')) {
33-
return { name: 'admin-settings-users' }
34-
}
35-
if ($ability.can('read-all', 'Group')) {
36-
return { name: 'admin-settings-groups' }
37-
}
38-
if ($ability.can('read-all', 'Drive')) {
39-
return { name: 'admin-settings-spaces' }
40-
}
41-
throw Error('Insufficient permissions')
49+
return { name: 'admin-settings-general' }
4250
}
4351
},
4452
{
@@ -47,7 +55,7 @@ export const routes = ({ $ability }: { $ability: Ability }): RouteRecordRaw[] =>
4755
component: General,
4856
beforeEnter: (to, from, next) => {
4957
if (!$ability.can('read-all', 'Setting')) {
50-
next({ path: '/' })
58+
return next(getAvailableRoute($ability))
5159
}
5260
next()
5361
},
@@ -62,7 +70,7 @@ export const routes = ({ $ability }: { $ability: Ability }): RouteRecordRaw[] =>
6270
component: Users,
6371
beforeEnter: (to, from, next) => {
6472
if (!$ability.can('read-all', 'Account')) {
65-
next({ path: '/' })
73+
return next(getAvailableRoute($ability))
6674
}
6775
next()
6876
},
@@ -77,7 +85,7 @@ export const routes = ({ $ability }: { $ability: Ability }): RouteRecordRaw[] =>
7785
component: Groups,
7886
beforeEnter: (to, from, next) => {
7987
if (!$ability.can('read-all', 'Group')) {
80-
next({ path: '/' })
88+
return next(getAvailableRoute($ability))
8189
}
8290
next()
8391
},
@@ -92,7 +100,7 @@ export const routes = ({ $ability }: { $ability: Ability }): RouteRecordRaw[] =>
92100
component: Spaces,
93101
beforeEnter: (to, from, next) => {
94102
if (!$ability.can('read-all', 'Drive')) {
95-
next({ path: '/' })
103+
return next(getAvailableRoute($ability))
96104
}
97105
next()
98106
},

‎packages/web-app-admin-settings/tests/unit/index.spec.ts‎

Lines changed: 60 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -45,85 +45,103 @@ describe('admin settings index', () => {
4545
})
4646
describe('routes', () => {
4747
describe('default-route "/"', () => {
48-
it('should redirect to general if permission given', () => {
48+
it('should redirect to general', () => {
4949
const ability = mock<Ability>()
5050
ability.can.mockReturnValueOnce(true)
5151
const route = routes({ $ability: ability }).find((n) => n.path === '/')
5252
expect((route.redirect as any)().name).toEqual('admin-settings-general')
5353
})
54-
it('should redirect to user management if permission given', () => {
55-
const ability = mock<Ability>()
56-
ability.can.mockReturnValueOnce(false)
57-
ability.can.mockReturnValueOnce(true)
58-
const route = routes({ $ability: ability }).find((n) => n.path === '/')
59-
expect((route.redirect as any)().name).toEqual('admin-settings-users')
60-
})
61-
it('should redirect to group management if permission given', () => {
62-
const ability = mock<Ability>()
63-
ability.can.mockReturnValueOnce(false)
64-
ability.can.mockReturnValueOnce(false)
65-
ability.can.mockReturnValueOnce(true)
66-
const route = routes({ $ability: ability }).find((n) => n.path === '/')
67-
expect((route.redirect as any)().name).toEqual('admin-settings-groups')
68-
})
69-
it('should redirect to space management if permission given', () => {
70-
const ability = mock<Ability>()
71-
ability.can.mockReturnValueOnce(false)
72-
ability.can.mockReturnValueOnce(false)
73-
ability.can.mockReturnValueOnce(false)
74-
ability.can.mockReturnValueOnce(true)
75-
const route = routes({ $ability: ability }).find((n) => n.path === '/')
76-
expect((route.redirect as any)().name).toEqual('admin-settings-spaces')
77-
})
78-
it('should throw an error if permissions are insufficient', () => {
79-
const ability = mock<Ability>()
80-
ability.can.mockReturnValue(false)
81-
expect(routes({ $ability: ability }).find((n) => n.path === '/').redirect).toThrow()
82-
})
8354
})
8455
it.each([
85-
{ can: true, redirect: null },
86-
{ can: false, redirect: { path: '/' } }
56+
{ can: vi.fn(() => true), redirect: null },
57+
{
58+
can: vi.fn((_, subject) => {
59+
if (subject === 'Group') {
60+
return true
61+
}
62+
63+
return false
64+
}),
65+
redirect: { name: 'admin-settings-groups' }
66+
}
8767
])('redirects "/general" with sufficient permissions', ({ can, redirect }) => {
88-
const ability = mock<Ability>({ can: vi.fn(() => can) })
68+
const ability = mock<Ability>({ can })
8969
const route = routes({ $ability: ability }).find((n) => n.path === '/general')
9070
const nextMock = vi.fn()
9171
;(route.beforeEnter as any)({}, {}, nextMock)
9272
const args = [...(redirect ? [redirect] : [])]
9373
expect(nextMock).toHaveBeenCalledWith(...args)
9474
})
9575
it.each([
96-
{ can: true, redirect: null },
97-
{ can: false, redirect: { path: '/' } }
76+
{ can: vi.fn(() => true), redirect: null },
77+
{
78+
can: vi.fn((_, subject) => {
79+
if (subject === 'Drive') {
80+
return true
81+
}
82+
83+
return false
84+
}),
85+
redirect: { name: 'admin-settings-spaces' }
86+
}
9887
])('redirects "/users" with sufficient permissions', ({ can, redirect }) => {
99-
const ability = mock<Ability>({ can: vi.fn(() => can) })
88+
const ability = mock<Ability>({ can })
10089
const route = routes({ $ability: ability }).find((n) => n.path === '/users')
10190
const nextMock = vi.fn()
10291
;(route.beforeEnter as any)({}, {}, nextMock)
10392
const args = [...(redirect ? [redirect] : [])]
10493
expect(nextMock).toHaveBeenCalledWith(...args)
10594
})
10695
it.each([
107-
{ can: true, redirect: null },
108-
{ can: false, redirect: { path: '/' } }
96+
{ can: vi.fn(() => true), redirect: null },
97+
{
98+
can: vi.fn((_, subject) => {
99+
if (subject === 'Setting') {
100+
return true
101+
}
102+
103+
return false
104+
}),
105+
redirect: { name: 'admin-settings-general' }
106+
}
109107
])('redirects "/groups" with sufficient permissions', ({ can, redirect }) => {
110-
const ability = mock<Ability>({ can: vi.fn(() => can) })
108+
const ability = mock<Ability>({ can })
111109
const route = routes({ $ability: ability }).find((n) => n.path === '/groups')
112110
const nextMock = vi.fn()
113111
;(route.beforeEnter as any)({}, {}, nextMock)
114112
const args = [...(redirect ? [redirect] : [])]
115113
expect(nextMock).toHaveBeenCalledWith(...args)
116114
})
117115
it.each([
118-
{ can: true, redirect: null },
119-
{ can: false, redirect: { path: '/' } }
116+
{ can: vi.fn(() => true), redirect: null },
117+
{
118+
can: vi.fn((_, subject) => {
119+
if (subject === 'Account') {
120+
return true
121+
}
122+
123+
return false
124+
}),
125+
redirect: { name: 'admin-settings-users' }
126+
}
120127
])('redirects "/spaces" with sufficient permissions', ({ can, redirect }) => {
121-
const ability = mock<Ability>({ can: vi.fn(() => can) })
128+
const ability = mock<Ability>({ can })
122129
const route = routes({ $ability: ability }).find((n) => n.path === '/spaces')
123130
const nextMock = vi.fn()
124131
;(route.beforeEnter as any)({}, {}, nextMock)
125132
const args = [...(redirect ? [redirect] : [])]
126133
expect(nextMock).toHaveBeenCalledWith(...args)
127134
})
135+
it.each(['/general', '/users', '/groups', '/spaces'])(
136+
'should throw an error if permissions are insufficient',
137+
(path) => {
138+
const ability = mock<Ability>({ can: vi.fn(() => false) })
139+
const route = routes({ $ability: ability }).find((n) => n.path === path)
140+
const nextMock = vi.fn()
141+
expect(() => {
142+
;(route.beforeEnter as any)({}, {}, nextMock)
143+
}).toThrowError('Insufficient permissions')
144+
}
145+
)
128146
})
129147
})

0 commit comments

Comments
 (0)