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

Add more security actions logs #1563

Open
wants to merge 17 commits into
base: 2-dev
from

Conversation

@Yoann-Abbes
Copy link
Contributor

Yoann-Abbes commented Jan 24, 2020

What does this PR do ?

This PR adds logs on security actions who modify either a role, a profile or a user.

Jira Ticket

How should this be manually tested?

  • Step 1 :
  • Step 2 :
  • Step 3 :
    ...

Other changes

Add a new getUser getter in the base controller that returns the context user object

Boyscout

Yoann-Abbes added 2 commits Jan 24, 2020
nit
@Yoann-Abbes Yoann-Abbes added the 2.x label Jan 24, 2020
@Yoann-Abbes Yoann-Abbes self-assigned this Jan 24, 2020
@@ -167,7 +167,7 @@ class Funnel {
// resolves the callback immediately if a slot is available
if (this.concurrentRequests < this.kuzzle.config.limits.concurrentRequests) {
if (this.requestsCacheById[request.internalId]) {
delete this.requestsCacheById[request.internalId];
this.requestsCacheById[request.internalId] = undefined;

This comment has been minimized.

Copy link
@Aschen

Aschen Jan 24, 2020

Contributor

We need to keep the delete keyword here since the requestsCacheById will continue to grow with properties set to undefined.

    const user = {
      name: 'aschen',
      age: 26
    };
    user.age = undefined;
    Object.keys(user) //=> ['name', 'age']

This comment has been minimized.

Copy link
@scottinet

scottinet Jan 28, 2020

Member

or we can use a Map instead
either way that object will end up being treated as a b-tree, which is exactly what we want here

@@ -187,7 +187,7 @@ class MqttProtocol extends Protocol {
const connection = this.connections.get(client);

this.connections.delete(client);
delete this.connectionsById[connection.id];
this.connectionsById[connection.id] = undefined;

This comment has been minimized.

Copy link
@Aschen

Aschen Jan 24, 2020

Contributor

Same remark about value = undefined vs delete value

@@ -142,7 +142,7 @@ class StatisticsController {
}

if (this.currentStats.connections[requestContext.connection.protocol] === 1) {
delete this.currentStats.connections[requestContext.connection.protocol];
this.currentStats.connections[requestContext.connection.protocol] = undefined;

This comment has been minimized.

Copy link
@Aschen

Aschen Jan 24, 2020

Contributor

Same remark about value = undefined vs delete value

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 27, 2020

Codecov Report

Merging #1563 into 2-dev will decrease coverage by 0.12%.
The diff coverage is 88.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##            2-dev    #1563      +/-   ##
==========================================
- Coverage   93.66%   93.53%   -0.13%     
==========================================
  Files         102      102              
  Lines        6899     6964      +65     
==========================================
+ Hits         6462     6514      +52     
- Misses        437      450      +13
Impacted Files Coverage Δ
lib/api/controllers/index.js 100% <ø> (ø) ⬆️
lib/core/storage/indexStorage.js 100% <ø> (ø) ⬆️
lib/api/controllers/bulk.js 96.87% <ø> (ø) ⬆️
lib/core/entrypoints/service/httpFormDataStream.js 100% <ø> (ø) ⬆️
lib/core/entrypoints/protocols/http.js 99.4% <ø> (ø) ⬆️
lib/api/funnel.js 96.61% <ø> (ø) ⬆️
lib/core/router.js 95.34% <ø> (ø) ⬆️
lib/config/httpRoutes.js 100% <ø> (ø) ⬆️
lib/core/models/repositories/repository.js 97.69% <ø> (ø) ⬆️
lib/core/httpRouter/routeHandler.js 100% <ø> (ø) ⬆️
... and 55 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b1f6f2...36a80cd. Read the comment docs.

lib/api/controllers/security.js Outdated Show resolved Hide resolved
@Aschen

This comment has been minimized.

Copy link
Contributor

Aschen commented Jan 28, 2020

Also, you should add a PR label

Copy link
Member

scottinet left a comment

To prevent subtle bugs to occur (see @Aschen comments, and mines too), I'll prefer if you replace only "delete" with "= undefined" for objects created on the fly. For RAM caches, you should instead replace JSON objects with Maps (which are designed to contain volatile data).

lib/api/controllers/security.js Outdated Show resolved Hide resolved
lib/api/controllers/security.js Outdated Show resolved Hide resolved
lib/core/plugins/manager.js Outdated Show resolved Hide resolved
Yoann-Abbes and others added 5 commits Jan 28, 2020
Co-Authored-By: Alexandre Bouthinon <bouthinon.alexandre@gmail.com>
Co-Authored-By: Sébastien Cottinet <scottinet@protonmail.com>
…leio/kuzzle into KZL-1357-more-security-actions-logs
@Aschen

This comment has been minimized.

Copy link
Contributor

Aschen commented Jan 29, 2020

Considering the work necessary to remove the delete keyword (use Map instead), I advise to make these change in a separate PR

@@ -304,6 +304,7 @@ class SecurityController extends NativeController {
description,
{ creatorId, refresh, apiKeyId });

this.kuzzle.log.info(`[SECURITY] User "${request.context.user}" applied action "${request.input.action}" on user "${userId}."`);

This comment has been minimized.

Copy link
@Aschen

Aschen Feb 1, 2020

Contributor

Here request.context.user is an object, IMHO it's better to print only the ID
(This remark apply everywhere)

Yoann-Abbes added 5 commits Feb 5, 2020
fix
@Yoann-Abbes

This comment has been minimized.

Copy link
Contributor Author

Yoann-Abbes commented Feb 5, 2020

Considering the work necessary to remove the delete keyword (use Map instead), I advise to make these change in a separate PR

#1568

@Yoann-Abbes Yoann-Abbes requested a review from Aschen Feb 5, 2020
@Aschen
Aschen approved these changes Feb 5, 2020
@@ -50,12 +50,12 @@ class Janitor {
* @param {object} securities
* @returns {Promise}
*/
async loadSecurities (securities = {}) {
async loadSecurities (securities = {}, userId = '-1') {

This comment has been minimized.

Copy link
@scottinet

scottinet Feb 7, 2020

Member

This parameter is not currently used, as this.createSecurity still takes only 2 arguments.

Also, I'm not sure about this default value. The "-1" kuid is reserved for the anonymous user, i.e. a non-authenticated user invoking the API. I'm not sure that this should be the default value when loadSecurities is invoked by Kuzzle itself upon startup.

Last remark: you should probably receive a nullable User object instead of a user kuid, for the same reason as the one stated in this remark: if admin:loadSecurities is executed by a plugin, you won't get any user object at all, so you cannot have a valid user id.

@@ -305,6 +305,7 @@ class SecurityController extends NativeController {
description,
{ apiKeyId, creatorId, refresh });

this.kuzzle.log.info(`[SECURITY] User "${request.context.user._id}" applied action "${request.input.action}" on user "${userId}."`);

This comment has been minimized.

Copy link
@scottinet

scottinet Feb 7, 2020

Member

⚠️ request.context.user can be null, for instance if the request has been instantiated by a plugin (either with the SDK or with the execute method).

After giving it some thoughts, my guess is that you should create a dedicated getUserId getter in the BaseController class.
This getter should return <null> or something like that (to be debated) if the user object is null.
It should return neither -1 nor anonymous because we don't know if the request is not a consequence of some other API call made by an authenticated user.

That new getter should be used everywhere.

@@ -374,7 +375,7 @@ class Janitor {
body: objects[_id],
controller: 'security',
refresh: 'wait_for'
});
}, {user: { _id: 'User' } });

This comment has been minimized.

Copy link
@scottinet

scottinet Feb 7, 2020

Member

"User" is not a valid kuid. You should revert that change and instead let the new getter I propose above handle the null user object.

@@ -392,7 +393,7 @@ class Janitor {
body: { ids },
controller: 'security',
refresh: 'wait_for'
});
}, { user: { _id: 'User' } });

This comment has been minimized.

Copy link
@scottinet
Yoann-Abbes added 3 commits Feb 13, 2020
@@ -1505,11 +1505,15 @@ function mDelete (kuzzle, type, request) {
errorsManager.get('services', 'storage', 'incomplete_delete', errors));
}

const userId = request.context && request.context.user && request.context.user._id

This comment has been minimized.

Copy link
@Aschen

Aschen Feb 13, 2020

Contributor

Why don't use getUserId here?

This comment has been minimized.

Copy link
@Yoann-Abbes

Yoann-Abbes Feb 13, 2020

Author Contributor

Because this function is outside the class

This comment has been minimized.

Copy link
@Aschen

Aschen Feb 13, 2020

Contributor

Oh yes you're right!
This function should be a method of the controller instead so you won't need to pass the kuzzle object to it and you can use the getUserId method

lib/core/janitor.js Show resolved Hide resolved
@Yoann-Abbes Yoann-Abbes requested review from Aschen and scottinet Feb 13, 2020
@sonarcloud

This comment has been minimized.

Copy link

sonarcloud bot commented Feb 17, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.1% 0.1% Duplication

@Aschen
Aschen approved these changes Feb 19, 2020

const promise = this.kuzzle.janitor.loadSecurities(securities, request.context.user._id);
const user = this.getUser(request);
const promise = this.kuzzle.janitor.loadSecurities(securities, { user });

This comment has been minimized.

Copy link
@scottinet

scottinet Feb 19, 2020

Member

janitor.loadSecurities seems to expect a User object, not an anonymous {user: <User>} object

@@ -305,7 +305,7 @@ class SecurityController extends NativeController {
description,
{ apiKeyId, creatorId, refresh });

this.kuzzle.log.info(`[SECURITY] User "${request.context.user._id}" applied action "${request.input.action}" on user "${userId}."`);
this.kuzzle.log.info(`[SECURITY] User "${this.getUserId(request)}" applied action "${request.input.action}" on user "${userId}."`);

This comment has been minimized.

Copy link
@scottinet

scottinet Feb 19, 2020

Member

you should use the creatorId variable instead (which should use that new getUserId function to prevent an error if invoked by a plugin)

@@ -1010,7 +1010,7 @@ class SecurityController extends NativeController {
updatedUser,
options);

this.kuzzle.log.info(`[SECURITY] User "${request.context.user._id}" applied action "${request.input.action}" on user "${createdUser._id}."`);
this.kuzzle.log.info(`[SECURITY] User "${this.getUserId(request)}" applied action "${request.input.action}" on user "${createdUser._id}."`);

This comment has been minimized.

Copy link
@scottinet

scottinet Feb 19, 2020

Member

I know this isn't your code, but the metadata object built by this function (l:983) should use the new, safer method.

Same goes for the updateMetadata method

return Bluebird.promisify(kuzzle.funnel.mExecute, {context: kuzzle.funnel})(deleteRequest);
});

return Bluebird.all(promises)

This comment has been minimized.

Copy link
@scottinet

scottinet Feb 19, 2020

Member

(nitpicking) would benefit from being converted to async/await calls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.