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.

Sign up for GitHub

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

Jump to bottom

feat(core): Allow user role modification #7797

Merged
merged 4 commits into from
Nov 24, 2023
Merged

feat(core): Allow user role modification #7797

merged 4 commits into from
Nov 24, 2023

Conversation

ivov
Copy link
Contributor

@ivov ivov commented Nov 23, 2023 •
edited
Loading

https://linear.app/n8n/issue/PAY-985

PATCH /users/:id/role
  unauthenticated user
    ✓ should receive 401 (349 ms)
  member
    ✓ should fail to demote owner to member (349 ms)
    ✓ should fail to demote owner to admin (359 ms)
    ✓ should fail to demote admin to member (381 ms)
    ✓ should fail to promote other member to owner (353 ms)
    ✓ should fail to promote other member to admin (377 ms)
    ✓ should fail to promote self to admin (354 ms)
    ✓ should fail to promote self to owner (371 ms)
  admin
    ✓ should receive 400 on invalid payload (351 ms)
    ✓ should receive 404 on unknown target user (351 ms)
    ✓ should fail to demote owner to admin (349 ms)
    ✓ should fail to demote owner to member (347 ms)
    ✓ should fail to promote member to owner (384 ms)
    ✓ should fail to promote admin to owner (350 ms)
    ✓ should be able to demote admin to member (354 ms)
    ✓ should be able to demote self to member (350 ms)
    ✓ should be able to promote member to admin (349 ms)
  owner
    ✓ should be able to promote member to admin (349 ms)
    ✓ should be able to demote admin to member (349 ms)
    ✓ should fail to demote self to admin (348 ms)
    ✓ should fail to demote self to member (354 ms)

@n8n-assistant n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Nov 23, 2023
ivov
ivov commented Nov 24, 2023
View reviewed changes
packages/cli/test/integration/users.api.test.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.

Tests in this controller are significantly slow. I'll look into this separately.

@ivov ivov marked this pull request as ready for review November 24, 2023 09:45
cstuncsik
cstuncsik approved these changes Nov 24, 2023
View reviewed changes
Copy link
Contributor

@cstuncsik cstuncsik left a comment

Choose a reason for hiding this comment

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

Just had a question that maybe answered to myself :)
Otherwise it looks good to me

packages/cli/src/requests.ts
export type ChangeRole = AuthenticatedRequest<
{ id: string },
{},
{ newRole?: { scope?: RoleScopes; name?: RoleNames } },
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I'm just missing something but if it's ChangeRole request what would it do without the newRole object?
Why is it optional?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or is it just to keep the contract real-life like? Like I can send any bullshit request

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or is it just to keep the contract real-life like?

Exactly! If we don't do this, when working in the controller we'll assume (based on the typing) that this property is guaranteed to be there, which is not the case and requires validation.

@ivov ivov merged commit 7a86d36 into master Nov 24, 2023
13 of 16 checks passed
@ivov ivov deleted the pay-985-add-user-role-modification-endpoint branch November 24, 2023 10:40
@ivov ivov mentioned this pull request Nov 27, 2023
Merged
ivov added a commit that referenced this pull request Nov 27, 2023
@ivov
perf(core): Make user controller tests faster (no-changelog) (#7819)
75a5807 
Before: `17.949 s`
After: `3.886 s`

Followup to:
#7797 (comment)
@github-actions github-actions bot mentioned this pull request Nov 29, 2023
Merged
ivov added a commit that referenced this pull request Nov 29, 2023
@github-actions @ivov
🚀 Release 1.19.0 (#7863)
303cf31 
#
[1.19.0](https://github.com/n8n-io/n8n/compare/n8n@1.18.0...n8n@1.19.0)
(2023-11-29)


### Bug Fixes

* **core:** Ensure member and admin cannot be promoted to owner
([#7830](#7830))
([9b87a59](9b87a59)),
closes
[/linear.app/n8n/issue/PAY-985/add-user-role-modification-endpoint#comment-62355f6](https://github.com//linear.app/n8n/issue/PAY-985/add-user-role-modification-endpoint/issues/comment-62355f6)
* **core:** Prevent error messages due to statistics about data loading
([#7824](#7824))
([847f6ac](847f6ac))
* **core:** Tighten checks for multi-main setup usage
([#7788](#7788))
([fdb2c18](fdb2c18))
* **core:** Use AbortController to notify nodes to abort execution
([#6141](#6141))
([d2c18c5](d2c18c5))
* **editor:** Add telemetry to workflow history
([#7811](#7811))
([d497041](d497041))
* **editor:** Allow owners and admins to share workflows and credentials
they don't own ([#7833](#7833))
([3ab3ec9](3ab3ec9))
* **editor:** Disable context menu actions in read-only mode
([#7789](#7789))
([902beff](902beff))
* **editor:** Fix cloud plan data loading on instance
([#7841](#7841))
([8b99384](8b99384))
* **editor:** Fix credential icon for old node type version
([#7843](#7843))
([4074107](4074107))
* **editor:** Fix icon for unknown node type
([#7842](#7842))
([28ac5a7](28ac5a7))
* **editor:** Fix mouse position in workflow previews
([#7853](#7853))
([c063398](c063398))
* **editor:** Show nice error when environment is not set up
([#7778](#7778))
([5835e05](5835e05))
* **editor:** Suppress dev server websocket messages in workflow view
([#7808](#7808))
([685ffd7](685ffd7))
* **Google Sheets Node:** Read operation execute for each item
([#7800](#7800))
([d548872](d548872))
* **HTTP Request Node:** Enable expressions for binary input data fields
([#7782](#7782))
([6208af0](6208af0))
* **Microsoft SQL Node:** Prevent double escaping table name
([#7801](#7801))
([73ec753](73ec753))


### Features

* Add AI tool building capabilities
([#7336](#7336))
([87def60](87def60))
* Add initial scope checks via decorators
([#7737](#7737))
([a37f1cb](a37f1cb))
* Ado 1296 spike credential setup in templates
([#7786](#7786))
([aae45b0](aae45b0))
* **core:** Add Support for custom CORS origins for webhooks
([#7455](#7455))
([99a9ea4](99a9ea4))
* **core:** Allow user role modification
([#7797](#7797))
([7a86d36](7a86d36))
* **core:** Set up endpoint for all existing roles with license flag
([#7834](#7834))
([2356fb0](2356fb0))
* **editor:** Add node name and version to NDV node settings
([#7731](#7731))
([da85198](da85198))
* **editor:** Add routing middleware, permission checks, RBAC store,
RBAC component ([#7702](#7702))
([67a8891](67a8891))
* **editor:** Replace middleware for Role checks with Scope checks
([#7847](#7847))
([72852a6](72852a6))
* **editor:** Show avatars for users currently working on the same
workflow ([#7763](#7763))
([77bc8ec](77bc8ec))
* **Notion Node:** Option to simplify output in getChildBlocks operation
([#7791](#7791))
([d667bca](d667bca))
* **Slack Node:** Add support for getting the profile of a user
([#7829](#7829))
([90bb6ba](90bb6ba))

Co-authored-by: ivov <ivov@users.noreply.github.com>
@janober
Copy link
Member

janober commented Dec 1, 2023

Got released with n8n@1.19.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cstuncsik cstuncsik cstuncsik approved these changes

Assignees
No one assigned
Labels
core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team Released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ivov @janober @cstuncsik