Loose coupling: security module #1630

scottinet · 2020-05-16T14:20:43Z

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 😑
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

…entials' into loose-coupling-security

…-before-credentials

…entials' into loose-coupling-security

…ectory' into loose-coupling-security

…tor-core-directory

…ectory' into loose-coupling-security

…context.kerror

…ectory' into loose-coupling-security

…type

…an_ask_event error

Aschen

Few minor things I spotted, otherwise LGTM 👍

Aschen · 2020-05-21T07:53:40Z

lib/core/auth/tokenManager.js

@@ -67,6 +69,11 @@ class TokenManager {
    this.timer = null;
  }

+  async init () {
+    const anonymous = this.kuzzle.ask('core:security:user:anonymous');


Missing await?

It's there 🤔
You might be reviewing an older version, I caught this one recently when I completed the unit tests suite.

Possible because I started this review a long time ago 😅

Aschen · 2020-07-03T10:12:12Z

lib/api/controller/auth.js

-      apiKeyId = request.input.resource._id || null,
-      description = this.getBodyString(request, 'description');
+    const expiresIn = request.input.args.expiresIn || -1;
+    const refresh = this.getRefresh(request);


Suggested change

const refresh = this.getRefresh(request);

const refresh = this.getRefresh(request, 'wait_for');

Aschen · 2020-07-03T10:14:48Z

lib/api/controller/auth.js

+      null,
+      body,
+      {
+        refresh: this.getRefresh(request),


Should be wait_for by default

Suggested change

refresh: this.getRefresh(request),

refresh: this.getRefresh(request, 'wait_for'),

Aschen · 2020-07-03T10:16:34Z

lib/api/controller/document.js

-    const id = request.input.resource._id;
-    const content = this.getBody(request);
+  async create (request) {
+    const id = this.getId(request, ifMissingEnum.IGNORE);
    const userId = this.getUserId(request);
    const refresh = this.getString(request, 'refresh', 'false');


Suggested change

const refresh = this.getString(request, 'refresh', 'false');

const refresh = this.getRefresh(request);

Aschen · 2020-07-03T10:38:39Z

lib/core/plugin/manager.js

+    const opts = Object.assign({}, strategy.config.strategyOptions, {
+      passReqToCallback: true
+    });


Nitpicking

Suggested change

const opts = Object.assign({}, strategy.config.strategyOptions, {

passReqToCallback: true

});

const opts = { ...strategy.config.strategyOptions, passReqToCallback: true };

Aschen · 2020-07-03T10:49:35Z

lib/core/security/securityLoader.js

+      promises.push(this.kuzzle.funnel.processRequest(request));
+    }
+
+    await Promise.all(promises);


Bluebird is much faster for // execution

Suggested change

await Promise.all(promises);

await Bluebird.all(promises);

Changed for the sake of uniformity, but performances are not a concern in the security loader, which should be at most called 1 time during the life of a kuzzle instance.

Aschen · 2020-07-03T10:50:12Z

lib/core/security/securityLoader.js

+'use strict';
+
+const { isEmpty } = require('lodash');
+const { Request } = require('kuzzle-common-objects');


Bluebird is much faster for // execution

Suggested change

const { Request } = require('kuzzle-common-objects');

const { Request } = require('kuzzle-common-objects');

const Bluebird = require('bluebird');

Aschen · 2020-07-03T10:58:02Z

lib/core/security/roleRepository.js

-        if ( action !== '*'
-          && !this.kuzzle.pluginsManager.isAction(roleController, action)
-        ) {
+        if ( action !== '*' && !plugins.isAction(roleController, action)) {


Suggested change

if ( action !== '*' && !plugins.isAction(roleController, action)) {

if (action !== '*' && !plugins.isAction(roleController, action)) {

scottinet · 2020-07-07T07:37:33Z

@Aschen > suggestions applied.

sonarqubecloud · 2020-07-07T09:17:11Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

0.0% Coverage
0.2% Duplication

# [2.3.2](/~https://github.com/kuzzleio/kuzzle/releases/tag/2.3.2) (2020-07-07) #### Bug fixes - [ [#1701](#1701) ] Fix: description should not be required for plugin custom errors ([scottinet](/~https://github.com/scottinet)) - [ [#1695](#1695) ] Fix dump generation ([Aschen](/~https://github.com/Aschen)) - [ [#1698](#1698) ] Fix generic events on search queries ([scottinet](/~https://github.com/scottinet)) #### Enhancements - [ [#1702](#1702) ] Update search indexes only when the "dynamic" setting changes from false to true/strict ([Aschen](/~https://github.com/Aschen)) - [ [#1630](#1630) ] Loose coupling: security module ([scottinet](/~https://github.com/scottinet)) - [ [#1604](#1604) ] Add request ID in log ([Aschen](/~https://github.com/Aschen)) ---

vidiben and others added 30 commits April 28, 2020 17:02

always create user before credentials

a5a6bb3

[wip] loose coupling: security module

09444a5

[wip] loose coupling: security module

fa5c561

Merge remote-tracking branch 'origin/security-create-user-before-cred…

37cb022

…entials' into loose-coupling-security

[refactor] replace kuzzle arg with kuzzle property

807747b

Merge remote-tracking branch 'origin/security-create-user-before-cred…

3ae7403

…entials' into loose-coupling-security

[wip] loose coupling: security module

16202e1

Merge remote-tracking branch 'origin/2-dev' into security-create-user…

e526c45

…-before-credentials

[test] update unit tests

cb79770

Merge remote-tracking branch 'origin/security-create-user-before-cred…

c6d12f5

…entials' into loose-coupling-security

[wip] loose coupling: security module

6916566

[wip] loose coupling: security module

e5b48f8

Merge remote-tracking branch 'origin/loose-coupling-refactor-core-dir…

9de12cc

…ectory' into loose-coupling-security

[wip] loose coupling: security module

8e03a3a

[fix] forgot to add the user identifier to the update role call

d425abc

Merge remote-tracking branch 'origin/loose-coupling-refactor-core-dir…

f04cce5

…ectory' into loose-coupling-security

[wip] loose coupling: security module

dc45c5c

[refactor] use singular form for directory names

dd6cfee

Merge remote-tracking branch 'origin/2-dev' into loose-coupling-refac…

656f6e7

…tor-core-directory

[refactor] update the error code documentation builder

80ef4a1

Merge remote-tracking branch 'origin/2-dev' into loose-coupling-refac…

7d084bd

…tor-core-directory

Merge remote-tracking branch 'origin/loose-coupling-refactor-core-dir…

48413b2

…ectory' into loose-coupling-security

[doc] deprecated plugin.context.errorsManager and alias it to plugin.…

7b7abcc

…context.kerror

Merge remote-tracking branch 'origin/loose-coupling-refactor-core-dir…

88e1447

…ectory' into loose-coupling-security

[wip] loose coupling: security module

59f3d28

[wip] loose coupling: security module

f137379

[wip] loose coupling: security module

4e4cfc5

Merge remote-tracking branch 'origin/2-dev' into loose-coupling-security

1f0f8d0

[wip] loose coupling: security module

3da7209

Merge remote-tracking branch 'origin/2-dev' into loose-coupling-security

0e6e105

scottinet added 10 commits June 20, 2020 15:30

[test] remove debug opts

ce41e43

[tests] fix warning on a non-error promise rejection

3a3a04e

[tests] unit tests: userRepository (part 1)

6564035

Merge remote-tracking branch 'origin/2-dev' into loose-coupling-security

155ecd9

[tests] unit tests: userRepository (end)

4495ad0

[tests] unit tests: tokenRepository

635b1ab

[tests] unit tests: securityLoader

8f74f4a

[tests] unit tests: securityLoader

d4e3989

[fix] remove limitation on ids with a leading underscore

bc50bba

[fix] forgot to update auth:updateSelf with the new user:update proto…

f13958d

…type

scottinet marked this pull request as ready for review June 30, 2020 15:15

scottinet added 6 commits June 30, 2020 17:23

[fix] this is the very last commit. I promise.

f170430

[fix] ok I lied. THIS is the last one

e2d273c

Merge remote-tracking branch 'origin/2-dev' into loose-coupling-security

70254ba

Merge remote-tracking branch 'origin/2-dev' into loose-coupling-security

9d70e3b

[errors] throw a runtime assertion failed instead of a dedicated orph…

edd30fa

…an_ask_event error

[errors] update error documentation

9ec2753

Aschen suggested changes Jul 6, 2020

View reviewed changes

Leodau approved these changes Jul 6, 2020

View reviewed changes

[fix] apply @Aschen's suggestions

1aaa9a5

Aschen approved these changes Jul 7, 2020

View reviewed changes

Merge remote-tracking branch 'origin/2-dev' into loose-coupling-security

55cd119

scottinet merged commit ccb73a4 into 2-dev Jul 7, 2020

scottinet deleted the loose-coupling-security branch July 7, 2020 09:42

This was referenced Jul 7, 2020

Release 2.3.3 #1704

Closed

Release 2.3.2 #1705

Closed

Release 2.3.2 #1706

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loose coupling: security module #1630

Loose coupling: security module #1630

scottinet commented May 16, 2020 •

edited

Loading

Aschen left a comment

Aschen May 21, 2020

scottinet Jul 7, 2020 •

edited

Loading

Aschen Jul 7, 2020

Aschen Jul 3, 2020

Aschen Jul 3, 2020

Aschen Jul 3, 2020

Aschen Jul 3, 2020

Aschen Jul 3, 2020

scottinet Jul 7, 2020

Aschen Jul 3, 2020

Aschen Jul 3, 2020

scottinet commented Jul 7, 2020

sonarqubecloud bot commented Jul 7, 2020

	const refresh = this.getRefresh(request);
	const refresh = this.getRefresh(request, 'wait_for');

	refresh: this.getRefresh(request),
	refresh: this.getRefresh(request, 'wait_for'),

	const refresh = this.getString(request, 'refresh', 'false');
	const refresh = this.getRefresh(request);

	if ( action !== '*' && !plugins.isAction(roleController, action)) {
	if (action !== '*' && !plugins.isAction(roleController, action)) {

Loose coupling: security module #1630

Loose coupling: security module #1630

Conversation

scottinet commented May 16, 2020 • edited Loading

Description

Changes / Fixes related to security API methods

Other changes

Aschen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottinet Jul 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottinet commented Jul 7, 2020

sonarqubecloud bot commented Jul 7, 2020

scottinet commented May 16, 2020 •

edited

Loading

scottinet Jul 7, 2020 •

edited

Loading