Release 2.3.2 #1705
Closed
Release 2.3.2 #1705
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
# Description Generic events on the following API routes were handled by a default document extractor, providing incorrect results to plugin developers: `document:search`, `document:deleteByQuery`, `document:updateByQuery` # Changes * Remove the `default` document extractor. Instead, if Kuzzle is tasked to extract documents from an unknown request action, it throws an `InternalError` with this new error code: `core.fatal.assertion_failed`. This new error is meant to be used for runtime assertions, making sure that the code performs according to specifications. Here, a `default` document extractor is clearly not what we want, as it hides potential errors or omissions (and it did exactly that here) * Add the missing document extractors for `document:search`, `document:deleteByQuery`, `document:updateByQuery` * Blacklist `document:updateByQuery` from "before" document events, as documents cannot be extracted from requests made to that API route * Update the documentation accordingly * Reorganize the document extractor unit tests, for better maintenability * Add a lot of missing unit tests
## What does this PR do ? Use the new [AsyncLocalStorage](https://nodejs.org/api/async_hooks.html#async_hooks_class_asynclocalstorage) of Node.js 14 to share a context between all the asynchronous calls for processing an API request. If Kuzzle run with Node.js 12, a no-op class is used instead. The performance overhead is 8% | ---- | Log with AsyncLocalStorage | Log classic | difference | |:-----:|:-----:|:----:|:---:| | req/s | 2613 | 2842 | 8% | <details><summary>Complete benchmark details</summary> ```js # server:now /** * @returns {Promise<Object>} */ async now () { this.kuzzle.log.info('ASL FTW'); return { now: Date.now() }; } ``` Benchmark with wrk and `NODE_ENV=production` ``` # With AsyncLocalStorage $ wrk -t12 -c300 -d30s http://localhost:7512/_now Running 30s test @ http://localhost:7512/_now 12 threads and 300 connections Thread Stats Avg Stdev Max +/- Stdev Latency 119.06ms 53.96ms 659.90ms 94.74% Req/Sec 230.42 51.60 300.00 87.08% 78675 requests in 30.10s, 46.67MB read Requests/sec: 2613.86 Transfer/sec: 1.55MB # Without AsyncLocalStorage $ wrk -t12 -c300 -d30s http://localhost:7512/_now Running 30s test @ http://localhost:7512/_now 12 threads and 300 connections Thread Stats Avg Stdev Max +/- Stdev Latency 105.60ms 30.22ms 301.36ms 85.67% Req/Sec 244.67 84.32 505.00 74.93% 85509 requests in 30.08s, 50.72MB read Requests/sec: 2842.67 Transfer/sec: 1.69MB ``` </details>
# Description Loosely couple the security module to any other objects. Also makes the security module entirely responsible for managing its stored documents and cache: * all cache changes (storage or invalidation) are now made by the security module itself, by listening to the relevant Kuzzle events. * metadata changes in security documents are handled by the security module. They were previously handled by API methods, and there were a few omissions because of that * implementation details on repository objects are now handled by the security module objects, instead of being handled by API methods (expect a much lighter and more readable code, especially in the security controller) * Exceptions related to security documents (e.g. `security.[role|profile|user].not_found`) are now handled by the security module, preventing duplicated code in API methods # Changes / Fixes related to security API methods * When finding a user without profiles associated to it, we "autofixed" it by associating the restricted profiles to it. I consider this a security issue, as there is no telling from where this user document comes from. If that happens, this is either a Kuzzle bug (shouldn't be the case though), a database manipulation gone wrong, or a malicious user document insertion. Either way, we should refuse to load the user and throw an error instead of trying to "make things better" * The `body.content` argument is no longer required for `security:createRestrictedUser`: it's required (for now) for `security:createUser` because we need the `profileIds` argument in it, but createRestrictedUser forbids this argument... and the remaining user content is completely optional, so the whole object itself should be so as well * `security:updateUser`, `security:updateProfile`, `security:updateRole`: the `retryOnConflict` option was accepted but undocumented (fixed). Also, the default value for `retryOnConflict` has been set to `10` (instead of `0`) to prevent common support tickets * `security:createFirstAdmin`: * (fix) the error thrown when an admin user already exists didn't have an error id, and wasn't a KuzzleError either * (fix) that API method used hardcoded content to reset profiles, instead of the configuration stored in kuzzlerc (this means that profiles loaded that way didn't have rate limits, contrary to the ones defined in our default configuration file) * For the same reasons than for `security:createRestrictedUser`, the `content` argument is no longer required * Now rejects if a `profileIds` property is found in the body content (not a fix: previous versions of kuzzle silently overwrite this property) * `auth:updateSelf` now accepts the following options: `refresh`, `retryOnConflict` (behavior harmonized with `security:updateUser`) * (fix) `security:mGetRoles` now throws an exception if there is at least 1 role that doesn't exist. Before, empty objects were returned for non existing roles (this made a functional test return a false-positive success on an erroneous argument provided to that route) * remove the now unused `funnel:mExecute` method: `security:m*` were the only ones using it. It's ugly and inefficient: now security repositories handle m* methods themselves, without duplicating API requests * `security:search[Roles|Profiles|Users]`: the `size` option is now, by default, set to the maximum number of documents that a single request can fetch (taken from the kuzzlerc configuration) * security user creation (createUser, createRestrictedUser, createFirstAdmin): fix the rollback sequence when a user has multiple strategies (already created strategy credentials weren't rollbacked) * truncating roles, profiles and users now also clear the RAM cache (even if it fails, because it might have deleted a few objects before the failure occurs) * (fix) fix redundant (because erroneous) unit tests on `roleRepository` methods `checkRoleNativeRights` and `checkRolePluginsRights` * Process the `content` and `profileIds` arguments separately in the `core:security:user` API, laying the groundwork for future Kuzzle API changes, and to store user documents differently / in a safer way. # Other changes * Asyncify functions whenever possible * Ask events without answerers now throw a `core.fatal.assertion_failed` exception. This change was made because otherwise, debugging simple typos in event names is a nightmare :expressionless: * Make the security controller use the `NativeController` secure getters instead of methods in `util/requestAssertions` as much as possible. It should soon be possible to get rid of requestAssertions (will need to move a few methods in NativeController beforehand though) * Modify the `getId` method in the NativeController class: * add a `ifMissing` option: `error` (default: throws if id is missing), `generate` (returns a UUIDv4 id if missing), `ignore` (returns `null` if missing) * The option `resetCache` has been removed from `roleRepository.loadRoles` and `profileRepository.loadProfiles`. It was only used by `admin:resetSecurity` to preload reinitialized roles and security, and it has been replaced by a global cache invalidation, letting the security module lazily load resetted roles/profiles when they are requested
* Fix dumpGenerator call * Update test/mocks/kuzzle.mock.js Co-authored-by: Leonardo D'Aubeterre <bluedraggy@gmail.com> * fix tests Co-authored-by: Leonardo D'Aubeterre <bluedraggy@gmail.com>
# Description Fix #1603: The error loader now makes the difference between native and plugin errors, and applies fewer checks to the latter. # Changes * add unit tests to the errors loader * enforce non-empty strings, when applicable * fix a few error messages * check property types correctly (e.g. does not handle false booleans or nul numbers as non-existing properties) * does not allow subdomain codes to be equal to 0, except for plugin errors (0 is the default plugin subdomain, if none is set) # Other changes * update eslint to v7 to allow comments when disabling rules
Codecov Report
@@ Coverage Diff @@
## master #1705 +/- ##
==========================================
+ Coverage 93.52% 93.70% +0.17%
==========================================
Files 113 115 +2
Lines 7200 7228 +28
==========================================
+ Hits 6734 6773 +39
+ Misses 466 455 -11
Continue to review full report at Codecov.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
2.3.2 (2020-07-07)
Bug fixes
Enhancements