diff --git a/packages/backend/server/src/core/permission/__tests__/__snapshots__/actions.spec.ts.md b/packages/backend/server/src/core/permission/__tests__/__snapshots__/actions.spec.ts.md index 6e5bfd57c..88767c3c0 100644 --- a/packages/backend/server/src/core/permission/__tests__/__snapshots__/actions.spec.ts.md +++ b/packages/backend/server/src/core/permission/__tests__/__snapshots__/actions.spec.ts.md @@ -6,6 +6,10 @@ Generated by [AVA](https://avajs.dev). ## should be able to fixup doc role from workspace role and doc role +> WorkspaceRole: External, DocRole: None + + 'None' + > WorkspaceRole: External, DocRole: External 'External' @@ -26,6 +30,10 @@ Generated by [AVA](https://avajs.dev). 'Editor' +> WorkspaceRole: Collaborator, DocRole: None + + 'None' + > WorkspaceRole: Collaborator, DocRole: External 'External' @@ -46,6 +54,10 @@ Generated by [AVA](https://avajs.dev). 'Owner' +> WorkspaceRole: Admin, DocRole: None + + 'Manager' + > WorkspaceRole: Admin, DocRole: External 'Manager' @@ -66,6 +78,10 @@ Generated by [AVA](https://avajs.dev). 'Owner' +> WorkspaceRole: Owner, DocRole: None + + 'Owner' + > WorkspaceRole: Owner, DocRole: External 'Owner' @@ -190,6 +206,24 @@ Generated by [AVA](https://avajs.dev). ## should be able to get correct permissions from DocRole +> DocRole: None + + { + 'Doc.Copy': false, + 'Doc.Delete': false, + 'Doc.Duplicate': false, + 'Doc.Properties.Read': false, + 'Doc.Properties.Update': false, + 'Doc.Publish': false, + 'Doc.Read': false, + 'Doc.Restore': false, + 'Doc.TransferOwner': false, + 'Doc.Trash': false, + 'Doc.Update': false, + 'Doc.Users.Manage': false, + 'Doc.Users.Read': false, + } + > DocRole: External { diff --git a/packages/backend/server/src/core/permission/__tests__/__snapshots__/actions.spec.ts.snap b/packages/backend/server/src/core/permission/__tests__/__snapshots__/actions.spec.ts.snap index 8d00ba829..e319b6c8d 100644 Binary files a/packages/backend/server/src/core/permission/__tests__/__snapshots__/actions.spec.ts.snap and b/packages/backend/server/src/core/permission/__tests__/__snapshots__/actions.spec.ts.snap differ diff --git a/packages/backend/server/src/core/permission/__tests__/actions.spec.ts b/packages/backend/server/src/core/permission/__tests__/actions.spec.ts index f04bd2407..adbfd3197 100644 --- a/packages/backend/server/src/core/permission/__tests__/actions.spec.ts +++ b/packages/backend/server/src/core/permission/__tests__/actions.spec.ts @@ -36,8 +36,9 @@ const docRoles = Object.values(DocRole).filter( test(`should be able to fixup doc role from workspace role and doc role`, t => { for (const workspaceRole of workspaceRoles) { for (const docRole of docRoles) { + const fixedDocRole = fixupDocRole(workspaceRole, docRole); t.snapshot( - DocRole[fixupDocRole(workspaceRole, docRole)!], + fixedDocRole === null ? null : DocRole[fixedDocRole], `WorkspaceRole: ${WorkspaceRole[workspaceRole]}, DocRole: ${DocRole[docRole]}` ); } diff --git a/packages/backend/server/src/core/permission/__tests__/doc.spec.ts b/packages/backend/server/src/core/permission/__tests__/doc.spec.ts index f9e1eb959..1359c6ef6 100644 --- a/packages/backend/server/src/core/permission/__tests__/doc.spec.ts +++ b/packages/backend/server/src/core/permission/__tests__/doc.spec.ts @@ -111,6 +111,44 @@ test('should return [External] if doc is public', async t => { t.is(role, DocRole.External); }); +test('should return null if doc role is [None]', async t => { + await models.doc.setDefaultRole(ws.id, 'doc1', DocRole.None); + + await models.workspaceUser.set( + ws.id, + user.id, + WorkspaceRole.Collaborator, + WorkspaceMemberStatus.Accepted + ); + + const role = await ac.getRole({ + workspaceId: ws.id, + docId: 'doc1', + userId: user.id, + }); + + t.is(role, null); +}); + +test('should return [External] if doc role is [None] but doc is public', async t => { + await models.doc.setDefaultRole(ws.id, 'doc1', DocRole.None); + await models.workspaceUser.set( + ws.id, + user.id, + WorkspaceRole.Collaborator, + WorkspaceMemberStatus.Accepted + ); + await models.doc.publish(ws.id, 'doc1'); + + const role = await ac.getRole({ + workspaceId: ws.id, + docId: 'doc1', + userId: 'random-user-id', + }); + + t.is(role, DocRole.External); +}); + test('should return mapped permissions', async t => { const { permissions } = await ac.role({ workspaceId: ws.id, diff --git a/packages/backend/server/src/core/permission/doc.ts b/packages/backend/server/src/core/permission/doc.ts index 1d2723c70..8e478c9f8 100644 --- a/packages/backend/server/src/core/permission/doc.ts +++ b/packages/backend/server/src/core/permission/doc.ts @@ -81,8 +81,12 @@ export class DocAccessController extends AccessController<'doc'> { ); // if user is in workspace but doc role is not set, fallback to default doc role - if (workspaceRole && workspaceRole !== WorkspaceRole.External) { - docRole = defaultDocRole.workspace; + if (workspaceRole !== null && workspaceRole !== WorkspaceRole.External) { + docRole = + defaultDocRole.external !== null + ? // edgecase: when doc role set to [None] for workspace member, but doc is public, we should fallback to external role + Math.max(defaultDocRole.workspace, defaultDocRole.external) + : defaultDocRole.workspace; } else { // else fallback to external doc role docRole = defaultDocRole.external; @@ -92,7 +96,10 @@ export class DocAccessController extends AccessController<'doc'> { // we need to fixup doc role to make sure it's not miss set // for example: workspace owner will have doc owner role // workspace external will not have role higher than editor - return fixupDocRole(workspaceRole, docRole); + const role = fixupDocRole(workspaceRole, docRole); + + // never return [None] + return role === DocRole.None ? null : role; } private async defaultDocRole(workspaceId: string, docId: string) { diff --git a/packages/backend/server/src/core/permission/types.ts b/packages/backend/server/src/core/permission/types.ts index ddec8648d..8934ddb37 100644 --- a/packages/backend/server/src/core/permission/types.ts +++ b/packages/backend/server/src/core/permission/types.ts @@ -222,7 +222,7 @@ export function mapDocRoleToPermissions(docRole: DocRole | null) { {} as Record ); - if (docRole === null) { + if (docRole === null || docRole === DocRole.None) { return permissions; } @@ -249,7 +249,10 @@ export function fixupDocRole( workspaceRole: WorkspaceRole | null, docRole: DocRole | null ): DocRole | null { - if (workspaceRole === null && docRole === null) { + if ( + workspaceRole === null && + (docRole === null || docRole === DocRole.None) + ) { return null; } @@ -343,26 +346,6 @@ export function docActionRequiredRole(action: DocAction): DocRole { ); } -/** - * Useful when a workspace member doesn't have a specified role in the doc, but want to check the permission of the action - */ -export function docActionRequiredWorkspaceRole( - action: DocAction -): WorkspaceRole { - const docRole = docActionRequiredRole(action); - - switch (docRole) { - case DocRole.Owner: - return WorkspaceRole.Owner; - case DocRole.Manager: - return WorkspaceRole.Admin; - case DocRole.Editor: - case DocRole.Reader: - case DocRole.External: - return WorkspaceRole.Collaborator; - } -} - export function workspaceActionRequiredRole( action: WorkspaceAction ): WorkspaceRole { diff --git a/packages/backend/server/src/models/common/role.ts b/packages/backend/server/src/models/common/role.ts index 0239ffa4f..f2cb86870 100644 --- a/packages/backend/server/src/models/common/role.ts +++ b/packages/backend/server/src/models/common/role.ts @@ -1,4 +1,8 @@ export enum DocRole { + /** + * `None` equals to `role = null`, it only exists to give a value that API can use + */ + None = -(1 << 15), External = 0, Reader = 10, Editor = 20,