feat(API): Implement users account quota guards #6434

OlegIvaniv · 2023-06-14T14:48:41Z

Github issue / Community forum post (link here to close automatically):

Signed-off-by: Oleg Ivaniv <me@olegivaniv.com>

github-actions · 2023-06-14T14:49:15Z

Great PR! Please pay attention to the following items before merging:

Files matching packages/**:

If fixing bug, added test to cover scenario.
If addressing forum or Github issue, added link to description.

Files matching packages/**/*.ts:

Added unit tests to cover new or updated functionality.

Make sure to check off this list before asking for review.

packages/cli/src/UserManagement/UserManagementHelper.ts

ivov

General comments:

We are aiming to have a 1-user flavor of UM, rather than to check whether UM is enabled or disabled. Also remember that in v1 there will be no explicit checks for UM as it will always be enabled. Hence we should focus the checks on the users quota.
Let's aim to add functionality to services or repositories instead of UserManagementHelper. The UserManagement dir was created when UM was only a feature, but by now UM has become an integral part of the BE - so over time that dir and its helpers should be removed. isInstanceOwner in particular could be isOwner is the UserRepository.
Can we centralize the check by adding it to resolveJwtContent instead? Or was there a reason that this didn't work or shouldn't be done?
We are throwing 400 but I wonder if 401 would be more appropriate, since members are unauthorized in 1-user UM.
For endpoints protected by this new check, I wonder if we could create a Nest-style decorator auth guard. We should have most of the setup for this already.

ivov · 2023-06-19T14:57:30Z

packages/cli/src/License.ts

+	getUsersLimit(): number {
+		const limit = this.getFeatureValue(LICENSE_QUOTAS.USERS_LIMIT);
+
+		return (limit ?? -1) as number;
+	}


An overload on getFeatureValue would be nice, to have proper typing.

Thanks @ivov! I've addressed all your points but 5. Moving the check to central place(resolveJwtContent) simplified the logic so IMO there's no need for the decorator. Can you have another look, please?

- Getting `usersQuota` from `Settings` repo - Revert `isUserManagementEnabled` helper - Fix FE listing of users Signed-off-by: Oleg Ivaniv <me@olegivaniv.com>

Signed-off-by: Oleg Ivaniv <me@olegivaniv.com>

…s-having-1-as-users-quota

codecov · 2023-06-22T13:21:52Z

Codecov Report

Patch coverage: 75.47% and project coverage change: -0.06 ⚠️

Comparison is base (20737b5) 28.92% compared to head (04db6a8) 28.87%.

❗ Current head 04db6a8 differs from pull request most recent head 9ed555d. Consider uploading reports for the commit 9ed555d to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6434      +/-   ##
==========================================
- Coverage   28.92%   28.87%   -0.06%     
==========================================
  Files        3069     3070       +1     
  Lines      188355   188417      +62     
  Branches    20892    20899       +7     
==========================================
- Hits        54488    54399      -89     
- Misses     132987   133135     +148     
- Partials      880      883       +3

Impacted Files	Coverage Δ
...blicApi/v1/handlers/workflows/workflows.service.ts	`92.15% <ø> (-0.16%)`	⬇️
packages/cli/src/Server.ts	`0.00% <0.00%> (ø)`
packages/cli/src/push/index.ts	`23.63% <ø> (ø)`
packages/workflow/src/Interfaces.ts	`57.14% <ø> (ø)`
packages/cli/src/controllers/users.controller.ts	`73.27% <45.45%> (-1.61%)`	⬇️
packages/cli/src/controllers/auth.controller.ts	`74.71% <50.00%> (-1.20%)`	⬇️
packages/cli/src/License.ts	`55.55% <54.54%> (+1.11%)`	⬆️
...es/cli/src/controllers/passwordReset.controller.ts	`82.22% <60.00%> (-3.32%)`	⬇️
packages/editor-ui/src/views/SettingsUsersView.vue	`47.92% <82.50%> (+4.28%)`	⬆️
packages/editor-ui/src/stores/settings.store.ts	`73.17% <85.71%> (+0.26%)`	⬆️
... and 10 more

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

packages/cli/src/License.ts

packages/cli/test/integration/shared/testDb.ts

packages/cli/test/unit/License.test.ts

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

packages/editor-ui/src/views/SettingsUsersView.vue

Signed-off-by: Oleg Ivaniv <me@olegivaniv.com>

OlegIvaniv · 2023-07-04T15:32:45Z

@ivov Addressed your comments. Can you re-review, please?

cypress · 2023-07-05T07:19:09Z

1 flaky tests on run #1424 ↗︎

0	219	0	0	1

Details:

🌳 ado-762-in-app-deny-member-logins-for-plans-having-1-as-users-quota 🖥️ brows...
Project: n8n	Commit: `9ed555dc89`
Status: Passed	Duration: 07:45 💡
Started: Jul 12, 2023 7:29 AM	Ended: Jul 12, 2023 7:37 AM

cypress/e2e/24-ndv-paired-item.cy.ts • 1 flaky test

View Output Video

Test		Artifacts
NDV > can pair items between input and output across branches and runs		`Output` `Screenshots` `Video`

_{This comment has been generated by cypress-bot as a result of this
project's GitHub integration settings.}

…s-having-1-as-users-quota Signed-off-by: Oleg Ivaniv <me@olegivaniv.com> # Conflicts: # packages/cli/src/Ldap/helpers.ts # packages/cli/src/Server.ts # packages/cli/src/middlewares/auth.ts # packages/cli/src/push/index.ts # packages/cli/src/sso/saml/samlHelpers.ts # packages/editor-ui/src/router.ts # packages/editor-ui/src/stores/settings.store.ts # packages/editor-ui/src/views/SettingsUsersView.vue # packages/workflow/src/Interfaces.ts

Signed-off-by: Oleg Ivaniv <me@olegivaniv.com>

ivov

Still need to test it locally.

packages/cli/src/Ldap/helpers.ts

packages/cli/src/License.ts

packages/cli/src/auth/jwt.ts

packages/cli/src/constants.ts

packages/cli/test/integration/auth.api.test.ts

packages/cli/test/integration/shared/utils.ts

packages/cli/test/integration/variables.test.ts

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

refactor: License typings suggestions

Co-authored-by: Iván Ovejero <ivov.src@gmail.com>

Signed-off-by: Oleg Ivaniv <me@olegivaniv.com>

ivov

Thanks for addressing everything! I'll test it out later today :)

OlegIvaniv · 2023-07-12T07:24:24Z

Thanks for addressing everything! I'll test it out later today :)

Thanks for the thorough review <3 Let me know if I can help with anything

github-actions · 2023-07-12T07:37:18Z

✅ All Cypress E2E specs passed

ivov · 2023-07-12T12:09:46Z

@OlegIvaniv

Tested these cases, let me know if I'm missing any, else let's merge!

When having 1 as user quota:

Owner can log in
Owner cannot invite users, sees upgrade path
Owner can delete existing users, with transfer
Already logged-in member remains logged in
Logged out member cannot log in, sees quota message

Regular case:

Owner can log in
Owner can invite users
Member can log in

* master: ci!: Drop support for Node.js 16. Add initial support for Node.js 20 (#6649) feat(API): Implement users account quota guards (#6434) feat(OpenAI Node): Update max token limit to support newer model limits (#6644) ci: Run checklist job as a separate workflow (no-changelog) (#6645) fix(HTTP Request Node): Cleanup circular references in response (#6590) fix(Postgres Node): Upsert does not fetch columns when schema other then public (#6643) fix(core): Update docker compose setup for V1 images (#6642) refactor: Delete unnecessary interface re-exports from core, and delete unused code in nodes-base (no-changelog) (#6631) fix(core): Reduce memory consumption on `BinaryDataManager.init` (#6633) fix(editor): Implement simple view for templates experiment (no-changelog) (#6637) # Conflicts: # packages/editor-ui/src/stores/settings.store.ts

* ADO-814-trial-banner: ✔️ Updated callout component snapshots 👌 Moving DOM logic from the store 👌 Addressing more points from code review 👌 Refactoring trial banner component and ui store 👌 Updating permanent banner dismiss endpoint and back-end logic ⚡ Updating the migrations syntax ci!: Drop support for Node.js 16. Add initial support for Node.js 20 (#6649) feat(API): Implement users account quota guards (#6434) feat(OpenAI Node): Update max token limit to support newer model limits (#6644) ci: Run checklist job as a separate workflow (no-changelog) (#6645) fix(HTTP Request Node): Cleanup circular references in response (#6590) fix(Postgres Node): Upsert does not fetch columns when schema other then public (#6643) fix(core): Update docker compose setup for V1 images (#6642) refactor: Delete unnecessary interface re-exports from core, and delete unused code in nodes-base (no-changelog) (#6631) fix(core): Reduce memory consumption on `BinaryDataManager.init` (#6633) ⚡ Updating upgrade plan button click 👌 Addressing design review feedback ⚡ Using query parameters in migrations fix(editor): Implement simple view for templates experiment (no-changelog) (#6637) ⚡ Updating migration files

* feat(cli): Implement users account quota guards Signed-off-by: Oleg Ivaniv <me@olegivaniv.com> * Remove comment Signed-off-by: Oleg Ivaniv <me@olegivaniv.com> * Address PR comments - Getting `usersQuota` from `Settings` repo - Revert `isUserManagementEnabled` helper - Fix FE listing of users Signed-off-by: Oleg Ivaniv <me@olegivaniv.com> * Refactor isWithinUserQuota getter and fix tests Signed-off-by: Oleg Ivaniv <me@olegivaniv.com> * Revert testDb.ts changes Signed-off-by: Oleg Ivaniv <me@olegivaniv.com> * Cleanup & improve types Signed-off-by: Oleg Ivaniv <me@olegivaniv.com> * Fix duplicated method * Fix failing test * Remove `isUserManagementEnabled` completely Signed-off-by: Oleg Ivaniv <me@olegivaniv.com> * Check for globalRole.name to determine if user is owner Signed-off-by: Oleg Ivaniv <me@olegivaniv.com> * Fix unit tests Signed-off-by: Oleg Ivaniv <me@olegivaniv.com> * Set isInstanceOwnerSetUp in specs * Fix SettingsUserView UM Signed-off-by: Oleg Ivaniv <me@olegivaniv.com> * refactor: License typings suggestions for users quota guards (#6636) refactor: License typings suggestions * Update packages/cli/src/Ldap/helpers.ts Co-authored-by: Iván Ovejero <ivov.src@gmail.com> * Update packages/cli/test/integration/shared/utils.ts Co-authored-by: Iván Ovejero <ivov.src@gmail.com> * Address PR comments Signed-off-by: Oleg Ivaniv <me@olegivaniv.com> * Use 403 for all user quota related errors Signed-off-by: Oleg Ivaniv <me@olegivaniv.com> --------- Signed-off-by: Oleg Ivaniv <me@olegivaniv.com> Co-authored-by: Iván Ovejero <ivov.src@gmail.com>

janober · 2023-07-14T12:16:46Z

Got released with n8n@1.0.3

OlegIvaniv added 2 commits June 14, 2023 16:40

feat(cli): Implement users account quota guards

68447a2

Signed-off-by: Oleg Ivaniv <me@olegivaniv.com>

Remove comment

dd7b2f8

Signed-off-by: Oleg Ivaniv <me@olegivaniv.com>

OlegIvaniv changed the title ~~feat(cli): Implement users account quota guards~~ feat(API): Implement users account quota guards Jun 14, 2023

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 Jun 14, 2023

OlegIvaniv marked this pull request as ready for review June 19, 2023 07:30

mutdmour reviewed Jun 19, 2023

View reviewed changes

packages/cli/src/UserManagement/UserManagementHelper.ts Outdated Show resolved Hide resolved

packages/cli/src/UserManagement/UserManagementHelper.ts Outdated Show resolved Hide resolved

ivov requested changes Jun 19, 2023

View reviewed changes

OlegIvaniv added 6 commits June 20, 2023 15:01

Address PR comments

0387b58

- Getting `usersQuota` from `Settings` repo - Revert `isUserManagementEnabled` helper - Fix FE listing of users Signed-off-by: Oleg Ivaniv <me@olegivaniv.com>

Refactor isWithinUserQuota getter and fix tests

569c0a5

Signed-off-by: Oleg Ivaniv <me@olegivaniv.com>

Revert testDb.ts changes

152cc30

Signed-off-by: Oleg Ivaniv <me@olegivaniv.com>

Cleanup & improve types

84aab51

Signed-off-by: Oleg Ivaniv <me@olegivaniv.com>

Merge branch 'master' into ado-762-in-app-deny-member-logins-for-plan…

c174647

…s-having-1-as-users-quota

Fix duplicated method

d6238b4

OlegIvaniv requested a review from ivov June 22, 2023 12:45

Fix failing test

851917a

ivov reviewed Jun 26, 2023

View reviewed changes

OlegIvaniv added 4 commits July 4, 2023 10:17

Remove isUserManagementEnabled completely

c5f1556

Signed-off-by: Oleg Ivaniv <me@olegivaniv.com>

Check for globalRole.name to determine if user is owner

0f4b2b2

Signed-off-by: Oleg Ivaniv <me@olegivaniv.com>

Fix unit tests

312b3cc

Signed-off-by: Oleg Ivaniv <me@olegivaniv.com>

Set isInstanceOwnerSetUp in specs

f52d4be

OlegIvaniv added 2 commits July 7, 2023 15:32

Fix SettingsUserView UM

04db6a8

Signed-off-by: Oleg Ivaniv <me@olegivaniv.com>

ivov reviewed Jul 11, 2023

View reviewed changes

refactor: License typings suggestions for users quota guards (#6636)

db63828

refactor: License typings suggestions

OlegIvaniv and others added 4 commits July 11, 2023 11:18

Update packages/cli/src/Ldap/helpers.ts

3ad4163

Co-authored-by: Iván Ovejero <ivov.src@gmail.com>

Update packages/cli/test/integration/shared/utils.ts

7ad3399

Co-authored-by: Iván Ovejero <ivov.src@gmail.com>

Address PR comments

ed9588b

Signed-off-by: Oleg Ivaniv <me@olegivaniv.com>

Use 403 for all user quota related errors

9ed555d

Signed-off-by: Oleg Ivaniv <me@olegivaniv.com>

ivov approved these changes Jul 12, 2023

View reviewed changes

OlegIvaniv merged commit e5620ab into master Jul 12, 2023

OlegIvaniv deleted the ado-762-in-app-deny-member-logins-for-plans-having-1-as-users-quota branch July 12, 2023 12:11

github-actions bot mentioned this pull request Jul 13, 2023

🚀 Release 1.0.3 #6664

Merged

janober added the Released label Jul 14, 2023

github-actions bot mentioned this pull request Jul 26, 2023

🚀 Release 1.1.0 #6746

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(API): Implement users account quota guards #6434

feat(API): Implement users account quota guards #6434

OlegIvaniv commented Jun 14, 2023

github-actions bot commented Jun 14, 2023 •

edited

Loading

ivov left a comment

ivov Jun 19, 2023

OlegIvaniv Jun 22, 2023

codecov bot commented Jun 22, 2023 •

edited

Loading

OlegIvaniv commented Jul 4, 2023

cypress bot commented Jul 5, 2023 •

edited

Loading

ivov left a comment

ivov left a comment

OlegIvaniv commented Jul 12, 2023

github-actions bot commented Jul 12, 2023

ivov commented Jul 12, 2023

janober commented Jul 14, 2023

feat(API): Implement users account quota guards #6434

feat(API): Implement users account quota guards #6434

Conversation

OlegIvaniv commented Jun 14, 2023

github-actions bot commented Jun 14, 2023 • edited Loading

ivov left a comment

Choose a reason for hiding this comment

ivov Jun 19, 2023

Choose a reason for hiding this comment

OlegIvaniv Jun 22, 2023

Choose a reason for hiding this comment

codecov bot commented Jun 22, 2023 • edited Loading

Codecov Report

OlegIvaniv commented Jul 4, 2023

cypress bot commented Jul 5, 2023 • edited Loading

1 flaky tests on run #1424 ↗︎

ivov left a comment

Choose a reason for hiding this comment

ivov left a comment

Choose a reason for hiding this comment

OlegIvaniv commented Jul 12, 2023

github-actions bot commented Jul 12, 2023

ivov commented Jul 12, 2023

janober commented Jul 14, 2023

github-actions bot commented Jun 14, 2023 •

edited

Loading

codecov bot commented Jun 22, 2023 •

edited

Loading

cypress bot commented Jul 5, 2023 •

edited

Loading