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): Add filtering, selection and pagination to users #6994

Merged
merged 21 commits into from
Aug 28, 2023

Conversation

ivov
Copy link
Contributor

@ivov ivov commented Aug 23, 2023

@github-actions
Copy link
Contributor

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.

@n8n-assistant n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Aug 23, 2023
@ivov ivov marked this pull request as ready for review August 24, 2023 15:47
@cypress
Copy link

cypress bot commented Aug 24, 2023

Passing run #2018 ↗︎

0 232 0 0 Flakiness 0

Details:

🌳 pay-646-allow-filtering-and-querying-for-users 🖥️ browsers:node18.12.0-chrom...
Project: n8n Commit: 2aad14c9ad
Status: Passed Duration: 07:56 💡
Started: Aug 28, 2023 2:01 PM Ended: Aug 28, 2023 2:09 PM

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

@codecov
Copy link

codecov bot commented Aug 24, 2023

Codecov Report

Patch coverage: 70.64% and no project coverage change.

Comparison is base (07d3633) 32.13% compared to head (2aad14c) 32.13%.
Report is 9 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master    #6994    +/-   ##
========================================
  Coverage   32.13%   32.13%            
========================================
  Files        3183     3187     +4     
  Lines      195611   195758   +147     
  Branches    21339    21390    +51     
========================================
+ Hits        62850    62914    +64     
- Misses     131772   131820    +48     
- Partials      989     1024    +35     
Files Changed Coverage Δ
packages/cli/src/Interfaces.ts 100.00% <ø> (ø)
packages/cli/src/Server.ts 0.00% <0.00%> (ø)
...ges/cli/src/UserManagement/UserManagementHelper.ts 84.84% <ø> (+5.95%) ⬆️
packages/cli/src/requests.ts 100.00% <ø> (ø)
packages/cli/src/services/user.service.ts 57.89% <26.92%> (-26.49%) ⬇️
...ckages/cli/src/controllers/nodeTypes.controller.ts 25.80% <40.00%> (+5.11%) ⬆️
...es/cli/src/controllers/passwordReset.controller.ts 74.13% <50.00%> (-5.15%) ⬇️
packages/cli/src/controllers/owner.controller.ts 77.35% <64.28%> (-8.36%) ⬇️
packages/cli/src/middlewares/listQuery/filter.ts 83.33% <66.66%> (+3.33%) ⬆️
packages/cli/src/middlewares/listQuery/select.ts 83.33% <66.66%> (+3.33%) ⬆️
... and 10 more

... and 22 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

I've found an odd behavior: if the select contains only ["firstName"] the query fails but if I add id to the query it always works.

{
    "code": 0,
    "message": "SQLITE_ERROR: no such column: distinctAlias.User_id",
    "stacktrace": "QueryFailedError: SQLITE_ERROR: no such column: distinctAlias.User_id\n    at handler (/Users/omarajoue/Workspace/Projects/n8n/node_modules/.pnpm/[email protected][email protected][email protected][email protected][email protected]/src/driver/sqlite/SqliteQueryRunner.ts:113:26)\n    at replacement (/Users/omarajoue/Workspace/Projects/n8n/node_modules/.pnpm/[email protected]/node_modules/sqlite3/lib/trace.js:25:27)\n    at Statement.errBack (/Users/omarajoue/Workspace/Projects/n8n/node_modules/.pnpm/[email protected]/node_modules/sqlite3/lib/sqlite3.js:15:21)"
}
Screenshot 2023-08-25 at 11 30 37

import { Expose } from 'class-transformer';
import { BaseFilter } from './base.filter.dto';

export class UserFilter extends BaseFilter {
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have this use case for now but it might be worth filtering by name.

packages/cli/src/controllers/users.controller.ts Outdated Show resolved Hide resolved
@krynble
Copy link
Contributor

krynble commented Aug 25, 2023

Also one odd behavior: I have a user whose email is "[email protected]" and this doesn't work when filtering

@ivov
Copy link
Contributor Author

ivov commented Aug 25, 2023

Also one odd behavior: I have a user whose email is "[email protected]" and this doesn't work when filtering

You're not using other options? Maybe a screenshot? Unable to reproduce for now:

Capture 2023-08-25 at 11 56 39@2x

@ivov
Copy link
Contributor Author

ivov commented Aug 25, 2023

I've found an odd behavior: if the select contains only ["firstName"] the query fails but if I add id to the query it always works.

This is caused by taking more entities than there are available.

Should take=n fail if there are not enough n? Or return as many as there are?

Never mind, the root cause is another, will fix.

@krynble
Copy link
Contributor

krynble commented Aug 25, 2023

Also one odd behavior: I have a user whose email is "[email protected]" and this doesn't work when filtering

You're not using other options? Maybe a screenshot? Unable to reproduce for now:

Capture 2023-08-25 at 11 56 39@2x

Hum odd, does not seem to work for me, see the screenshots below

Screenshot 2023-08-25 at 12 12 14
Above you can see that there's a user with [email protected] and no filters in the query

Screenshot 2023-08-25 at 12 13 32
Filtering for [email protected] works

Screenshot 2023-08-25 at 12 13 51
Filtering by [email protected] is not working

@ivov
Copy link
Contributor Author

ivov commented Aug 25, 2023

Re: [email protected] not being found with filter

Very odd, cannot reproduce yet. Tested with a DB from scratch and recreated users and all working:

@krynble
Copy link
Contributor

krynble commented Aug 25, 2023

I've found an odd behavior: if the select contains only ["firstName"] the query fails but if I add id to the query it always works.

{
    "code": 0,
    "message": "SQLITE_ERROR: no such column: distinctAlias.User_id",
    "stacktrace": "QueryFailedError: SQLITE_ERROR: no such column: distinctAlias.User_id\n    at handler (/Users/omarajoue/Workspace/Projects/n8n/node_modules/.pnpm/[email protected][email protected][email protected][email protected][email protected]/src/driver/sqlite/SqliteQueryRunner.ts:113:26)\n    at replacement (/Users/omarajoue/Workspace/Projects/n8n/node_modules/.pnpm/[email protected]/node_modules/sqlite3/lib/trace.js:25:27)\n    at Statement.errBack (/Users/omarajoue/Workspace/Projects/n8n/node_modules/.pnpm/[email protected]/node_modules/sqlite3/lib/sqlite3.js:15:21)"
}
Screenshot 2023-08-25 at 11 30 37

This is caused by taking more entities than there are available.

Should take=n fail if there are not enough n? Or return as many as there are?

I'd say yes, that's what usually pagination systems should do IMO. If your search doesn't contain more results, it just won't return any

@ivov
Copy link
Contributor Author

ivov commented Aug 25, 2023

I've found an odd behavior: if the select contains only ["firstName"] the query fails but if I add id to the query it always works.

Fixed in 8ae8522, added test.

@ivov
Copy link
Contributor Author

ivov commented Aug 25, 2023

Re: [email protected] not being found with filter

As discussed the root cause was that the server expects URI-encoded JSON, so the + was being parsed as whitespace, so no result was being found. Postman and the browser do not auto-encode, but Insomnia does.

@krynble
Copy link
Contributor

krynble commented Aug 25, 2023

Thanks for the fixes. Found another weird issue here.

Screenshot 2023-08-25 at 14 25 32

Using only take now results in some weird result

@ivov
Copy link
Contributor Author

ivov commented Aug 25, 2023

Found another weird issue here.

Thanks for rechecking. Fixed and added more tests.

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.

One thing that seems to be a bit odd is that using only skip without take does not seem to work, not sure if intended.

Other than this, I think it's worth adding that last test before merging.

};

describe('GET /users', () => {
test('should return all users', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing that might be worth adding is a test for retrieving users with non-owners so we can make sure it always returns, since the GET endpoint should be public.

@ivov
Copy link
Contributor Author

ivov commented Aug 28, 2023

One thing that seems to be a bit odd is that using only skip without take does not seem to work, not sure if intended.

That's intended, skip requires take. I'll throw an error if it's clearer.

Capture 2023-08-28 at 15 10 30@2x

One thing that might be worth adding is a test for retrieving users with non-owners so we can make sure it always returns, since the GET endpoint should be public.

That case is covered here, or do you mean something else?

Capture 2023-08-28 at 15 05 24@2x

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.

LGTM!

@github-actions
Copy link
Contributor

✅ All Cypress E2E specs passed

@ivov ivov merged commit b716241 into master Aug 28, 2023
@ivov ivov deleted the pay-646-allow-filtering-and-querying-for-users branch August 28, 2023 14:13
@github-actions github-actions bot mentioned this pull request Aug 31, 2023
netroy added a commit that referenced this pull request Aug 31, 2023
# [1.5.0](https://github.com/n8n-io/n8n/compare/[email protected]@1.5.0)
(2023-08-31)


### Bug Fixes

* **Agile CRM Node:** Fix issue with company address not working
([#6997](#6997))
([2f81652](2f81652))
* **Code Node:** Switch over to vm2 fork
([#7018](#7018))
([dfe0fa6](dfe0fa6))
* **core:** Invalid NODES_INCLUDE should not crash the app
([#7038](#7038))
([04e3178](04e3178)),
closes [#6683](#6683)
* **core:** Setup websocket keep-live messages
([#6866](#6866))
([8bdb07d](8bdb07d)),
closes [#6757](#6757)
* **core:** Throw `NodeSSLError` only for nodes that allow ignoring SSL
issues ([#6928](#6928))
([a01c3fb](a01c3fb))
* **Date & Time Node:** Dont parse date if it's not set (null or
undefined) ([#7050](#7050))
([d72f79f](d72f79f))
* **editor:** Fix sending of Ask AI tracking events
([#7002](#7002))
([fb05afa](fb05afa))
* **Microsoft Excel 365 Node:** Support for more extensions in workbook
rlc ([#7020](#7020))
([d6e1cf2](d6e1cf2))
* **MongoDB Node:** Stringify response ObjectIDs
([#6990](#6990))
([9ca990b](9ca990b))
* **MongoDB Node:** Upgrade mongodb package to address CVE-2021-32050
([#7054](#7054))
([d3f6356](d3f6356))
* **Postgres Node:** Empty return data fix for Postgres and MySQL
([#7016](#7016))
([176ccd6](176ccd6))
* **Webhook Node:** Fix URL params for webhooks
([#6986](#6986))
([596b569](596b569))


### Features

* **core:** External Secrets storage for credentials
([#6477](#6477))
([ed927d3](ed927d3))
* **core:** Add MFA ([#4767](#4767))
([2b7ba6f](2b7ba6f))
* **core:** Add filtering, selection and pagination to users
([#6994](#6994))
([b716241](b716241))
* **editor:** Debug executions in the editor
([#6834](#6834))
([c833078](c833078))
* **RSS Read Node:** Add support for self signed certificates
([#7039](#7039))
([3b9f0fe](3b9f0fe))

Co-authored-by: netroy <[email protected]>
@janober
Copy link
Member

janober commented Aug 31, 2023

Got released with [email protected]

ivov pushed a commit that referenced this pull request Dec 6, 2023
…n login (#7936)

## Summary

What: Fix issue of login endpoint returning secret and recovery codes
when MFA is enabled. Bug was introduced in this
[PR](#6994), specifically in this
[line](https://github.com/n8n-io/n8n/pull/6994/files#diff-95a87cb029a3d26e6722df2e68132453fc254fc1f4540cbdaa95cfdbda1893deL91).

Why: We should not be filtering the secret and recovery codes

Same PR caused the issues on ticket ->
https://linear.app/n8n/issue/ADO-1494/on-user-list-copy-password-reset-link-and-copy-invite-link-are-broken

## Review / Merge checklist
- [x] PR title and summary are descriptive. **Remember, the title
automatically goes into the changelog. Use `(no-changelog)` otherwise.**
([conventions](https://github.com/n8n-io/n8n/blob/master/.github/pull_request_title_conventions.md))
- [x] [Docs updated](https://github.com/n8n-io/n8n-docs) or follow-up
ticket created.
- [x] 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.
  >
> *(internal)* You can use Slack commands to trigger [e2e
tests](https://www.notion.so/n8n/How-to-use-Test-Instances-d65f49dfc51f441ea44367fb6f67eb0a?pvs=4#a39f9e5ba64a48b58a71d81c837e8227)
or [deploy test
instance](https://www.notion.so/n8n/How-to-use-Test-Instances-d65f49dfc51f441ea44367fb6f67eb0a?pvs=4#f6a177d32bde4b57ae2da0b8e454bfce)
or [deploy early access version on
Cloud](https://www.notion.so/n8n/Cloudbot-3dbe779836004972b7057bc989526998?pvs=4#fef2d36ab02247e1a0f65a74f6fb534e).
cstuncsik pushed a commit that referenced this pull request Dec 6, 2023
…n login (#7936)

## Summary

What: Fix issue of login endpoint returning secret and recovery codes
when MFA is enabled. Bug was introduced in this
[PR](#6994), specifically in this
[line](https://github.com/n8n-io/n8n/pull/6994/files#diff-95a87cb029a3d26e6722df2e68132453fc254fc1f4540cbdaa95cfdbda1893deL91).

Why: We should not be filtering the secret and recovery codes

Same PR caused the issues on ticket ->
https://linear.app/n8n/issue/ADO-1494/on-user-list-copy-password-reset-link-and-copy-invite-link-are-broken

## Review / Merge checklist
- [x] PR title and summary are descriptive. **Remember, the title
automatically goes into the changelog. Use `(no-changelog)` otherwise.**
([conventions](https://github.com/n8n-io/n8n/blob/master/.github/pull_request_title_conventions.md))
- [x] [Docs updated](https://github.com/n8n-io/n8n-docs) or follow-up
ticket created.
- [x] 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.
  >
> *(internal)* You can use Slack commands to trigger [e2e
tests](https://www.notion.so/n8n/How-to-use-Test-Instances-d65f49dfc51f441ea44367fb6f67eb0a?pvs=4#a39f9e5ba64a48b58a71d81c837e8227)
or [deploy test
instance](https://www.notion.so/n8n/How-to-use-Test-Instances-d65f49dfc51f441ea44367fb6f67eb0a?pvs=4#f6a177d32bde4b57ae2da0b8e454bfce)
or [deploy early access version on
Cloud](https://www.notion.so/n8n/Cloudbot-3dbe779836004972b7057bc989526998?pvs=4#fef2d36ab02247e1a0f65a74f6fb534e).
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants