WEB-1140 manage user workspace membership services #2460

cdriesler · 2024-07-01T18:15:03Z

Description & motivation

We need repo/service functions for basic workspace role operations:
- Create
- Update
- Remove
We need to protect against the final admin of a workspace being removed
We need to make sure relevant events are emitted by the service functions

Changes:

Adds repo functions and relevant integration tests
Adds workspace role events and calls them in service functions

…mentations

linear · 2024-07-01T18:15:07Z

WEB-1140 Manage user workspace membership services

…hip-services

iainsproat · 2024-07-05T17:46:02Z

packages/server/modules/workspaces/repositories/workspaces.ts

+    const workspacesAclTable = tables.workspacesAcl(db)
+
+    // Get current role
+    const currentRoleQuery = workspacesAclTable


should this be in a separate function getCurrentRoleForUserInWorkspace or similar? I assume we'll need to undertake this similar query in other functions?

Should we not return all roles for a user, and then we can check in that list of all roles if they are an admin without making a further db query?

I think that's fair. Based on the roles listed I assumed a user would only ever have one role in a workspace.

Am I reading it right that you're suggesting dropping the .where('role', 'workspace:admin') bit and inspecting the list? That makes sense to me as well even if there's only ever one role. Is it strictly worse (performance wise?) to include this extra clause?

iainsproat · 2024-07-05T17:47:21Z

packages/server/modules/workspaces/repositories/workspaces.ts

+    }
+
+    // Protect against removing last admin in workspace
+    const isUserLastAdmin = await isUserLastAdminFactory({ db })({


See note above. Can we retrieve all roles for a user in the workspace above, then do an roles.includes('workspace:admin') here?

I think it's also important to check, not just that the user is an admin, but that there are no other admins in the workspace. Is that maybe implied in another way from checking a given user's roles?

iainsproat · 2024-07-05T17:54:49Z

packages/server/modules/workspaces/services/workspaceRoleCreation.ts

+  async ({ userId, workspaceId, role }: WorkspaceAcl): Promise<void> => {
+    await upsertWorkspaceRole({ userId, workspaceId, role })
+
+    // TODO: Should we return the final record from `upsert`, or `get`, instead of emitting args directly?


From upsert, if it returns the values in the row as this should reflect what is stored. If it doesn't return the value, then using the args is ok.

This is something I plan to ask about next time we're all around. It looks like, in general, we don't return the final result from upsert functions but this is a case where I'd find it useful.

For me, an upsert is a complex conditional operation, cause if the value doesn't exist, you need to insert the full object. So allowing partial upsert needs a lot of guards to get right. I like full upserts more for that reason. If that is true, there must not be any differences between the args object and the stored value.

packages/server/modules/workspaces/tests/integration/repositories.spec.ts

cdriesler · 2024-07-07T18:13:47Z

@iainsproat took another swing, given your comments. I do like this much better lmk what you think

gjedlicska · 2024-07-08T05:10:21Z

packages/server/modules/workspaces/repositories/workspaces.ts

+    }
+
+    // Bail early if user has no role
+    const currentRole = workspaceRoles.find((role) => role.userId === userId)


instead of running a sql query against the db, inspecting the return value and based on a condition maybe run a second, you can issue a delete command with a returning '*' statement, to get back the deleted rows.

This way you are guaranteed to only run one sql statement.

https://knexjs.org/guide/query-builder.html#returning

gjedlicska · 2024-07-08T05:14:18Z

packages/server/modules/workspaces/repositories/workspaces.ts

    const validRoles = Object.values(Roles.Workspace)
    if (!validRoles.includes(role)) {
      throw new Error(`Unexpected workspace role provided: ${role}`)
    }

+    // Protect against removing last admin in workspace
+    const workspaceRoles = await getWorkspaceRolesFactory({ db })({ workspaceId })


Now this the name of the function is lying to me. This func is more like ensuringAnAdminRemainsAndUpsertsTheRoleFactory

I think this is a lot cleaner as service level guarantee, its an application logic decision, that we do not allow workspaces without admin roles. The repo itself should not implement these guards. If we want that to be a strict dataschema guard, we should use database level guards instead..

gjedlicska · 2024-07-08T05:19:20Z

packages/server/modules/workspaces/repositories/workspaces.ts

+    // Protect against removing last admin in workspace
+    const workspaceRoles = await getWorkspaceRolesFactory({ db })({ workspaceId })
+
+    if (isUserLastWorkspaceAdmin(workspaceRoles, userId)) {


There could be cases, where we want to delete the role, even if the user is the last admin ie workspace deletion. ( If its not done via cascading deletes for some reason ). That way, this function becomes even more complex, cause based on an external logic, we might want to allow deletion after all. Its a lot better to have this check in the service implementation.

gjedlicska · 2024-07-08T05:24:54Z

packages/server/modules/workspaces/services/workspaceRoleCreation.ts

+  async ({ userId, workspaceId, role }: WorkspaceAcl): Promise<void> => {
+    await upsertWorkspaceRole({ userId, workspaceId, role })
+
+    // TODO: Should we return the final record from `upsert`, or `get`, instead of emitting args directly?


For me, an upsert is a complex conditional operation, cause if the value doesn't exist, you need to insert the full object. So allowing partial upsert needs a lot of guards to get right. I like full upserts more for that reason. If that is true, there must not be any differences between the args object and the stored value.

…and repo-level guarantees

cdriesler · 2024-07-08T15:54:31Z

@gjedlicska understood and agreed on all counts. Repo functions are even less opinionated/think-y than I originally thought. Changes are in

gjedlicska · 2024-07-09T08:38:49Z

packages/server/modules/workspaces/services/workspaceRoleCreation.ts

+    const workspaceRoles = await getWorkspaceRoles({ workspaceId })
+
+    if (isUserLastWorkspaceAdmin(workspaceRoles, userId)) {
+      throw new Error('Cannot remove last admin from a workspace.')


We need a specific error type for this. If we're throwing a generic error, the top level error middleware is the one that will catch it. That in turn will make the server respond with a 500 error code, meaning an internal server error. This needs to be a 400-ish error code.

Bad example in many ways (doing it in the repo function, and we're not returning a proper status code etc) but, here's how we're doing it when changing project roles

speckle-server/packages/server/modules/core/repositories/streams.ts

Line 990 in a9e6dc5

throw new StreamAccessUpdateError(

cc @fabis94 this is why i like the results monoid patter. You cannot fuck these up.

You can derive a specific error from BaseError and set the status code on it to 400 as a constant. That will make sure the server response code is properly set

gjedlicska · 2024-07-09T09:01:18Z

packages/server/modules/workspaces/utils/isUserLastWorkspaceAdmin.ts

@@ -0,0 +1,13 @@
+import { WorkspaceAcl } from '@/modules/workspaces/domain/types'
+
+export const isUserLastWorkspaceAdmin = (


This is missing unit tests. I know the functionality is somewhat tested in the services, but in these cases i think it is required to fully test all the edge cases of this functionality.

added, skipped on technically-valid but not possible situations (workspace with no users, workspace with no admins, etc)

happy to add them in if we want to be extra-exhaustive

packages/server/modules/workspaces/repositories/workspaces.ts

gjedlicska and others added 9 commits June 28, 2024 13:54

feat(workspaces): drop createdByUserId from the dataschema

0f5a965

feat(workspaces): repositories WIP

745ab70

merge

0dde440

Merge branch 'main' into gergo/web-1178-add-repository-function-imple…

767a5b0

…mentations

protect against removing last admin in workspace

8ec9338

quick impl and stub tests

5ee124f

add tests

2f29398

services

2b5b209

unit tests for role services

4f237d7

cdriesler added 7 commits July 2, 2024 18:26

fix(workspaces): maybe tests work like this

64e2f35

Merge branch 'main' into chuck/web-1140-manage-user-workspace-members…

358908b

…hip-services

fix(workspaces): dry

e9c3e77

fix(workspaces): initialize tests better

4665746

fix(workspaces): so true

1d1cba1

fix(workspaces): right

d74d67e

fix(workspaces): self nit

d2798ca

cdriesler requested review from iainsproat, fabis94 and alemagio July 5, 2024 13:46

iainsproat reviewed Jul 5, 2024

View reviewed changes

cdriesler marked this pull request as ready for review July 5, 2024 18:34

cdriesler added 3 commits July 7, 2024 18:29

fix(workspaces): better repository structure

a79a2e7

fix(workspaces): repair tests, use example.org

42782cc

fix(workspaces): add tests for new repo functions, repair other tests

bb3c353

gjedlicska requested changes Jul 8, 2024

View reviewed changes

fix(workspaces): better distinction between service-level guarantees …

6c12b80

…and repo-level guarantees

gjedlicska requested changes Jul 9, 2024

View reviewed changes

cdriesler added 3 commits July 9, 2024 13:37

fix(workspaces): review comments and stencil tests

e3690e9

fix(workspaces): add tests

833d88d

fix(workspaces): tests work

9c709bb

gjedlicska self-requested a review July 9, 2024 16:26

gjedlicska approved these changes Jul 9, 2024

View reviewed changes

gjedlicska merged commit e703bb7 into main Jul 9, 2024
21 of 23 checks passed

gjedlicska deleted the chuck/web-1140-manage-user-workspace-membership-services branch July 9, 2024 16:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WEB-1140 manage user workspace membership services #2460

WEB-1140 manage user workspace membership services #2460

cdriesler commented Jul 1, 2024 •

edited

Loading

linear bot commented Jul 1, 2024

iainsproat Jul 5, 2024

cdriesler Jul 6, 2024

iainsproat Jul 5, 2024

cdriesler Jul 6, 2024

iainsproat Jul 5, 2024

cdriesler Jul 6, 2024

gjedlicska Jul 8, 2024

cdriesler commented Jul 7, 2024

gjedlicska Jul 8, 2024

gjedlicska Jul 8, 2024

gjedlicska Jul 8, 2024

gjedlicska Jul 8, 2024

cdriesler commented Jul 8, 2024

gjedlicska Jul 9, 2024

gjedlicska Jul 9, 2024

cdriesler Jul 9, 2024

gjedlicska Jul 9, 2024

cdriesler Jul 9, 2024

		@@ -0,0 +1,13 @@
		import { WorkspaceAcl } from '@/modules/workspaces/domain/types'

		export const isUserLastWorkspaceAdmin = (

WEB-1140 manage user workspace membership services #2460

WEB-1140 manage user workspace membership services #2460

Conversation

cdriesler commented Jul 1, 2024 • edited Loading

Description & motivation

Changes:

linear bot commented Jul 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdriesler commented Jul 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdriesler commented Jul 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdriesler commented Jul 1, 2024 •

edited

Loading