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

feat: Reintroduce collaboration feature #10602

Merged

Conversation

tomi
Copy link
Contributor

@tomi tomi commented Aug 29, 2024

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)

Sorry, something went wrong.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment on lines +109 to +111
const workflow = await this.sharedWorkflowRepository.findWorkflowForUser(workflowId, user, [
'workflow:read',
]);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe should also report this to sentry?

@@ -0,0 +1,82 @@
<script setup lang="ts">
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1:1 copy from the one we had before

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

try {
await Promise.all(loadPromises);

collaborationStore.notifyWorkflowOpened(workflowId.value);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the correct place where this should be sent?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Verified

This commit was signed with the committer’s verified signature.
tomi Tomi Turtiainen
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.
@tomi 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 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

Verified

This commit was signed with the committer’s verified signature.
tomi Tomi Turtiainen

Verified

This commit was signed with the committer’s verified signature.
tomi Tomi Turtiainen

Verified

This commit was signed with the committer’s verified signature.
tomi Tomi Turtiainen

Verified

This commit was signed with the committer’s verified signature.
tomi Tomi Turtiainen

Verified

This commit was signed with the committer’s verified signature.
tomi Tomi Turtiainen
…mplement-access-control
Copy link
Contributor

@despairblue despairblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

tomi added 4 commits August 30, 2024 08:56

Verified

This commit was signed with the committer’s verified signature.
tomi Tomi Turtiainen
…mplement-access-control

Verified

This commit was signed with the committer’s verified signature.
tomi Tomi Turtiainen

Verified

This commit was signed with the committer’s verified signature.
tomi Tomi Turtiainen

Verified

This commit was signed with the committer’s verified signature.
tomi Tomi Turtiainen
@tomi
Copy link
Contributor Author

tomi commented Aug 30, 2024

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

@tomi tomi requested a review from despairblue August 30, 2024 06:41
despairblue
despairblue previously approved these changes Aug 30, 2024
Copy link
Contributor

@despairblue despairblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome 🚀

Approving the backend changes!

Copy link

cypress bot commented Aug 30, 2024

n8n    Run #6724

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

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 1
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 421
View all changes introduced in this branch ↗︎

Copy link
Contributor

✅ All Cypress E2E specs passed

@MiloradFilipovic MiloradFilipovic self-requested a review August 30, 2024 15:54
Copy link
Contributor

@MiloradFilipovic MiloradFilipovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
@@ -252,6 +254,8 @@ async function initializeData() {

try {
await Promise.all(loadPromises);

collaborationStore.notifyWorkflowOpened(workflowId.value);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Verified

This commit was signed with the committer’s verified signature.
tomi Tomi Turtiainen
tomi added 2 commits September 3, 2024 13:21

Verified

This commit was signed with the committer’s verified signature.
tomi Tomi Turtiainen

Verified

This commit was signed with the committer’s verified signature.
tomi Tomi Turtiainen
@tomi
Copy link
Contributor Author

tomi commented Sep 3, 2024

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

Copy link
Contributor

@MiloradFilipovic MiloradFilipovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing!
Looking good with the latest changes 🙌

Copy link
Contributor

github-actions bot commented Sep 3, 2024

✅ All Cypress E2E specs passed

@tomi tomi merged commit 2ea2bfe into master Sep 3, 2024
32 checks passed
@tomi tomi deleted the cat-79-bug-collaborationservice-does-not-implement-access-control branch September 3, 2024 14:52
MiloradFilipovic added a commit that referenced this pull request Sep 5, 2024

Verified

This commit was signed with the committer’s verified signature.
MiloradFilipovic Milorad FIlipović
* 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)
@github-actions github-actions bot mentioned this pull request Sep 5, 2024
@janober
Copy link
Member

janober commented Sep 5, 2024

Got released with n8n@1.58.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team Released ui Enhancement in /editor-ui or /design-system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants