feat: Reintroduce collaboration feature #10602

tomi · 2024-08-29T10:33:28Z

Summary

Collaboration feature was removed because it was missing authorization checks. This adds checks that user can't send workflow opened or closed message, unless they have access to that workflow. Also uses cache for storing the collaboration state, so the feature works correctly with multiple mains.

The feature is currently only enabled for the new canvas, as that should come live soon anyways.

Related Linear tickets, Github issues, and Community forum posts

CAT-79

Review / Merge checklist

PR title and summary are descriptive. (conventions)
Docs updated or follow-up ticket created.
Tests included.
PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

tomi · 2024-08-29T10:38:36Z

packages/cli/src/collaboration/collaboration.service.ts

+			try {
+				await this.handleUserMessage(event.userId, event.msg);
+			} catch (error) {
+				this.logger.error('Error handling user message', {


Should we report these to sentry? What's the correct way to do that? Throw an ApplicationError and let the top-level handler take care of it?

tomi · 2024-08-29T10:39:17Z

packages/cli/src/collaboration/collaboration.service.ts

+		const workflow = await this.sharedWorkflowRepository.findWorkflowForUser(workflowId, user, [
+			'workflow:read',
+		]);


Ideally would want to check the permissions without needing to query the entire workflow. Might do that as a follow up

tomi · 2024-08-29T10:40:26Z

packages/cli/src/push/websocket.push.ts

+
+				this.onMessageReceived(pushRef, JSON.parse(buffer.toString('utf8')));
+			} catch (error) {
+				this.logger.error("Couldn't parse message from editor-UI", {


Maybe should also report this to sentry?

tomi · 2024-08-29T10:40:50Z

packages/editor-ui/src/components/MainHeader/CollaborationPane.vue

@@ -0,0 +1,82 @@
+<script setup lang="ts">


1:1 copy from the one we had before

tomi · 2024-08-29T10:41:20Z

packages/editor-ui/src/views/NodeView.v2.vue

@@ -252,6 +254,8 @@ async function initializeData() {

 	try {
 		await Promise.all(loadPromises);
+
+		collaborationStore.notifyWorkflowOpened(workflowId.value);


Is this the correct place where this should be sent?

Maybe a better place would be the initializeWorkspaceForExistingWorkflow or openWorkflow method in the same file.

Collaboration feature was removed because it was missing authorization checks. This adds checks that user can't send workflow opened or closed message, unless they have access to that workflow. Also uses `cache` for storing the collaboration state, so the feature works correctly with multiple mains. The feature is currently only enabled for the new canvas, as that should come live soon anyways.

…mplement-access-control

despairblue

Disclaimer: I did not review the FE code.

The backend good looks good, also the permission related code. 🚀

I only made some smaller suggestions to the tests.

packages/cli/src/collaboration/collaboration.message.ts

packages/cli/src/collaboration/__tests__/collaboration.state.test.ts

packages/cli/src/push/__tests__/websocket.push.test.ts

packages/cli/test/integration/collaboration/collaboration.service.test.ts

…mplement-access-control

tomi · 2024-08-30T06:41:31Z

Thank you for a great review @despairblue ! I addressed all your comments. LMK if I missed something ✌️

despairblue

Awesome 🚀

Approving the backend changes!

cypress · 2024-08-30T08:36:30Z

n8n Run #6724

Run Properties: Passed #6724 • 9d974342b2: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 tomi 🗃️ e2e/*

Project	`n8n`
Branch Review	`cat-79-bug-collaborationservice-does-not-implement-access-control`
Run status	`Passed #6724`
Run duration	`04m 45s`
Commit	`9d974342b2: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 tomi 🗃️ e2e/*`
Committer	`Tomi Turtiainen`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`1`
Pending	`0`
Skipped	`0`
Passing	`421`
View all changes introduced in this branch ↗︎

github-actions · 2024-08-30T08:36:34Z

✅ All Cypress E2E specs passed

MiloradFilipovic

Apart from the minor comments I left, front-end part looks good to me.
However, I am seeing some unexpected behavior when switching users (the list is not always synced in both user sessions):

Screen.Recording.2024-08-30.at.18.00.39.mov

packages/editor-ui/src/stores/collaboration.store.ts

MiloradFilipovic · 2024-08-30T16:18:58Z

packages/editor-ui/src/views/NodeView.v2.vue

@@ -252,6 +254,8 @@ async function initializeData() {

 	try {
 		await Promise.all(loadPromises);
+
+		collaborationStore.notifyWorkflowOpened(workflowId.value);


Maybe a better place would be the initializeWorkspaceForExistingWorkflow or openWorkflow method in the same file.

tomi · 2024-09-03T11:13:51Z

Apart from the minor comments I left, front-end part looks good to me. However, I am seeing some unexpected behavior when switching users (the list is not always synced in both user sessions):
Screen.Recording.2024-08-30.at.18.00.39.mov

Couldn't reproduce anymore after changing the place where we notify workflow opened

MiloradFilipovic

Thanks for addressing!
Looking good with the latest changes 🙌

github-actions · 2024-09-03T14:41:05Z

✅ All Cypress E2E specs passed

* master: refactor(RabbitMQ Trigger Node): Improve type-safety, add tests, and fix issues with manual triggers (#10663) feat(editor): Add support for nodes with multiple main inputs in new canvas (no-changelog) (#10659) fix(editor): Set minimum zoom to 0 to allow fitting very large workflows in new canvas (no-changelog) (#10666) feat(editor): Change selection to be default canvas behaviour (no-changelog) (#10668) feat: More hints to nodes (#10565) fix(editor): Fix opening executions tab from a new, unsaved workflow (#10652) fix(AI Agent Node): Fix tools agent when using memory and Anthropic models (#10513) feat(editor): Make highlighted data pane floating (#10638) fix(editor): Fix workflow loading after switching to executions view in new canvas (no-changelog) (#10655) refactor(benchmark): Separate cloud env provisioning from running benchmarks (#10657) feat(core): Implement wrapping of regular nodes as AI Tools (#10641) refactor(editor): Remove Trial logic in personalization modal and port to script setup (#10649) fix(core): Declutter webhook insertion errors (#10650) feat: Reintroduce collaboration feature (#10602) feat(benchmark): Add scenario for expressions with Set node (#10647) feat(benchmark): Add benchmark scenario for binary files (#10648) build: Add `reset` script (#10627) feat(editor): Overhaul node insert position computation in new canvas (no-changelog) (#10637)

janober · 2024-09-05T10:42:53Z

Got released with [email protected]

tomi commented Aug 29, 2024

View reviewed changes

tomi force-pushed the cat-79-bug-collaborationservice-does-not-implement-access-control branch from 0a316a0 to 4427285 Compare August 29, 2024 10:57

tomi force-pushed the cat-79-bug-collaborationservice-does-not-implement-access-control branch from 4427285 to ceb0ab7 Compare August 29, 2024 10:58

n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team ui Enhancement in /editor-ui or /design-system labels Aug 29, 2024

tomi added 5 commits August 29, 2024 14:40

Fix filenames in imports

00aca7e

Fix collaboration store initialization

862e39b

Remove console.log

65d17ea

Fix typo

0c6127f

Merge branch 'master' into cat-79-bug-collaborationservice-does-not-i…

ecc77cb

…mplement-access-control

despairblue requested changes Aug 29, 2024

View reviewed changes

tomi added 4 commits August 30, 2024 08:56

Merge branch 'master' into cat-79-bug-collaborationservice-does-not-i…

9f61167

…mplement-access-control

Report unexpected errors to sentry

03de55d

Use zod to parse incoming messages

1f6aa68

Fix tests based on feedback

2d943e4

tomi requested a review from despairblue August 30, 2024 06:41

despairblue previously approved these changes Aug 30, 2024

View reviewed changes

MiloradFilipovic self-requested a review August 30, 2024 15:54

MiloradFilipovic reviewed Aug 30, 2024

View reviewed changes

Remove console.logs

32653b4

tomi dismissed despairblue’s stale review via 32653b4 September 1, 2024 10:44

tomi added 2 commits September 3, 2024 13:21

Remove failing test

0e4650b

Fix when workflow opened is notified

9d97434

tomi requested a review from MiloradFilipovic September 3, 2024 11:13

MiloradFilipovic approved these changes Sep 3, 2024

View reviewed changes

tomi merged commit 2ea2bfe into master Sep 3, 2024
32 checks passed

tomi deleted the cat-79-bug-collaborationservice-does-not-implement-access-control branch September 3, 2024 14:52

github-actions bot mentioned this pull request Sep 5, 2024

🚀 Release 1.58.0 #10680

Merged

janober added the Released label Sep 5, 2024

riascho pushed a commit that referenced this pull request Sep 23, 2024

feat: Reintroduce collaboration feature (#10602)

2a92d08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Reintroduce collaboration feature #10602

feat: Reintroduce collaboration feature #10602

tomi commented Aug 29, 2024 •

edited by despairblue

Loading

tomi Aug 29, 2024

tomi Aug 29, 2024

tomi Aug 29, 2024

tomi Aug 29, 2024

tomi Aug 29, 2024

MiloradFilipovic Aug 30, 2024

despairblue left a comment

tomi commented Aug 30, 2024

despairblue left a comment

cypress bot commented Aug 30, 2024 •

edited

Loading

github-actions bot commented Aug 30, 2024

MiloradFilipovic left a comment

MiloradFilipovic Aug 30, 2024

tomi commented Sep 3, 2024

MiloradFilipovic left a comment

github-actions bot commented Sep 3, 2024

janober commented Sep 5, 2024

feat: Reintroduce collaboration feature #10602

feat: Reintroduce collaboration feature #10602

Conversation

tomi commented Aug 29, 2024 • edited by despairblue Loading

Summary

Related Linear tickets, Github issues, and Community forum posts

Review / Merge checklist

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

despairblue left a comment

Choose a reason for hiding this comment

tomi commented Aug 30, 2024

despairblue left a comment

Choose a reason for hiding this comment

cypress bot commented Aug 30, 2024 • edited Loading

github-actions bot commented Aug 30, 2024

MiloradFilipovic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomi commented Sep 3, 2024

MiloradFilipovic left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 3, 2024

janober commented Sep 5, 2024

tomi commented Aug 29, 2024 •

edited by despairblue

Loading

cypress bot commented Aug 30, 2024 •

edited

Loading