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(core): Return all team projects if the user has global project:read permissions #8983

Merged
merged 4 commits into from
Mar 28, 2024

Conversation

despairblue
Copy link
Contributor

Summary

The endpoint GET /my-projects now returns the personal project and all team projects if the user has global project:read permissions.

Related tickets and issues

https://linear.app/n8n/issue/PAY-1455/update-the-project-endpoint-to-return-all-team-projects-for

Review / Merge checklist

  • PR title and summary are descriptive. Remember, the title automatically goes into the changelog. Use (no-changelog) otherwise. (conventions)
  • Tests included.

    A bug is not considered fixed, unless a test is added to prevent it from happening again.
    A feature is not complete without tests.

@n8n-assistant n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Mar 27, 2024
@despairblue despairblue marked this pull request as ready for review March 27, 2024 11:50
return {
name: `Unclaimed Personal Project (${pr.projectId})`,
Copy link
Contributor

Choose a reason for hiding this comment

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

One consideration is that we might want to have all projects with name, including for invited but not signed up user - perhaps use just the email. But this is something to discuss later.

Copy link

cypress bot commented Mar 27, 2024

18 failed and 1 flaky tests on run #4493 ↗︎

18 189 5 0 Flakiness 1

Details:

🌳 🖥️ browsers:node18.12.0-chrome107 🤖 despairblue 🗃️ e2e/*
Project: n8n Commit: 196ee644b2
Status: Failed Duration: 05:09 💡
Started: Mar 28, 2024 8:45 AM Ended: Mar 28, 2024 8:50 AM
Failed  17-sharing.cy.ts • 8 failed tests

View Output Video

Test Artifacts
Sharing > should create C1, W1, W2, share W1 with U3, as U2 Test Replay Screenshots Video
Sharing > should create C2, share C2 with U1 and U2, as U3 Test Replay Screenshots Video
Sharing > should open W1, add node using C2 as U3 Test Replay Screenshots Video
Sharing > should open W1, add node using C2 as U2 Test Replay Screenshots Video
Sharing > should not have access to W2, as U3 Test Replay Screenshots Video
Sharing > should have access to W1, W2, as U1 Test Replay Screenshots Video
Sharing > should automatically test C2 when opened by U2 sharee Test Replay Screenshots Video
Sharing > should work for admin role on credentials created by others (also can share it with themselves) Test Replay Screenshots Video
Failed  19-execution.cy.ts • 3 failed tests

View Output Video

Test Artifacts
Execution > should send proper payload for node rerun Test Replay Screenshots Video
Execution > should send proper payload for manual node run Test Replay Screenshots Video
Execution > should successfully execute partial executions with nodes attached to the second output Test Replay Screenshots Video
Failed  30-langchain.cy.ts • 3 failed tests

View Output Video

Test Artifacts
Langchain Integration > should be able to open and execute Basic LLM Chain node Test Replay Screenshots Video
Langchain Integration > should be able to open and execute Agent node Test Replay Screenshots Video
Langchain Integration > should add and use Manual Chat Trigger node together with Agent node Test Replay Screenshots Video
Failed  30-editor-after-route-changes.cy.ts • 1 failed test

View Output Video

Test Artifacts
Editor actions should work > after switching between Editor and Debug Test Replay Screenshots Video
Failed  18-user-management.cy.ts • 1 failed test

View Output Video

Test Artifacts
User Management > should delete user and transfer their data Test Replay Screenshots Video

The first 5 failed specs are shown, see all 40 specs in Cypress Cloud.

Flakiness  cypress/e2e/5-ndv.cy.ts • 1 flaky test

View Output Video

Test Artifacts
NDV > should not retrieve remote options when required params throw errors Test Replay Screenshots Video

Review all test suite changes for PR #8983 ↗︎

Copy link
Contributor

⚠️ Some Cypress E2E specs are failing, please fix them before merging

Copy link
Contributor

@krynble krynble left a comment

Choose a reason for hiding this comment

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

Screenshot 2024-03-27 at 12 48 54

Just noticed that if I am also part of the project, it gets duplicate for whatever reason. I created a project and invited an additional user, and I can now see it twice in the response.

Copy link
Contributor

⚠️ Some Cypress E2E specs are failing, please fix them before merging

@despairblue
Copy link
Contributor Author

despairblue commented Mar 27, 2024

Screenshot 2024-03-27 at 12 48 54

Just noticed that if I am also part of the project, it gets duplicate for whatever reason. I created a project and invited an additional user, and I can now see it twice in the response.

It's fixed now. But that took some fidgeting. The problem was that it used the relationships to get all projects. Which meant it would return projects multiple times if we have an OR query like "all project relations that you are a part of OR relate to team projects".

It would also not return orphaned projects at all, because they have no project relation.

Anyways, it's both fixed now, but the controller got a bit more complicated. I amended the tests to check for the right roles and scopes as well.

Btw it will return the global role for projects for which the user has no project relationship.

I hope that makes sense. If not let me know and create some examples.

@despairblue despairblue requested a review from krynble March 28, 2024 08:22
Copy link
Contributor

⚠️ Some Cypress E2E specs are failing, please fix them before merging

@despairblue despairblue merged commit f1da02b into feature/rbac Mar 28, 2024
19 of 28 checks passed
@despairblue despairblue deleted the pay-1455-return-all-projects-to-owners branch March 28, 2024 08:51
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants