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

Merged
merged 20 commits into from Mar 6, 2020
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/api/controllers/admin.js
Expand Up @@ -172,8 +172,8 @@ class AdminController extends NativeController {

loadSecurities (request) {
const securities = this.getBody(request);

const promise = this.kuzzle.janitor.loadSecurities(securities);
const user = this.getUser(request);
const promise = this.kuzzle.janitor.loadSecurities(securities, { user });
Copy link
Contributor

Choose a reason for hiding this comment

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

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


return this._waitForAction(request, promise, { acknowledge: true });
}
Expand Down
15 changes: 15 additions & 0 deletions lib/api/controllers/base.js
Expand Up @@ -618,6 +618,21 @@ class NativeController extends BaseController {
return null;
}

/**
* Returns the current user
*
* @param {Request} request
*
* @returns {Object}
*/
getUser (request) {
if (request.context && request.context.user) {
return request.context.user;
}

return null;
}

getSearchParams (request) {
const
from = this.getInteger(request, 'from', 0),
Expand Down
69 changes: 58 additions & 11 deletions lib/api/controllers/security.js
Expand Up @@ -305,6 +305,7 @@ class SecurityController extends NativeController {
description,
{ apiKeyId, creatorId, refresh });

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

Choose a reason for hiding this comment

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

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

return apiKey.serialize({ includeToken: true });
}

Expand Down Expand Up @@ -536,7 +537,10 @@ class SecurityController extends NativeController {
refresh: getRefresh(request)
}
)
.then(role => formatProcessing.serializeRole(role));
.then(role => {
this.kuzzle.log.info(`[SECURITY] User "${this.getUserId(request)}" applied action "${request.input.action}" on role "${role._id}."`);
return formatProcessing.serializeRole(role);
});
}

/**
Expand All @@ -559,7 +563,10 @@ class SecurityController extends NativeController {
refresh: getRefresh(request)
}
)
.then(role => formatProcessing.serializeRole(role));
.then(role => {
this.kuzzle.log.info(`[SECURITY] User "${this.getUserId(request)}" applied action "${request.input.action}" on role "${role._id}."`);
return formatProcessing.serializeRole(role);
});
}

/**
Expand All @@ -574,7 +581,11 @@ class SecurityController extends NativeController {
const options = { refresh: getRefresh(request) };

return this.kuzzle.repositories.role.load(request.input.resource._id)
.then(role => this.kuzzle.repositories.role.delete(role, options));
.then(role =>
{
this.kuzzle.log.info(`[SECURITY] User "${this.getUserId(request)}" applied action "${request.input.action} on role "${role._id}."`);
return this.kuzzle.repositories.role.delete(role, options);
});
}

/**
Expand Down Expand Up @@ -643,7 +654,9 @@ class SecurityController extends NativeController {
refresh: getRefresh(request)
}
)
.then(profile => formatProcessing.serializeProfile(profile)
.then(profile => {
this.kuzzle.log.info(`[SECURITY] User "${this.getUserId(request)}" applied action "${request.input.action}" on profile "${profile._id}."`);
return formatProcessing.serializeProfile(profile);}
);
}

Expand All @@ -668,8 +681,10 @@ class SecurityController extends NativeController {
refresh: getRefresh(request)
}
)
.then(profile => formatProcessing.serializeProfile(profile)
);
.then(profile => {
this.kuzzle.log.info(`[SECURITY] User "${this.getUserId(request)}" applied action "${request.input.action}" on profile "${profile._id}."`);
return formatProcessing.serializeProfile(profile);
});
}

/**
Expand All @@ -684,7 +699,10 @@ class SecurityController extends NativeController {
const options = { refresh: getRefresh(request) };

return this.kuzzle.repositories.profile.load(request.input.resource._id)
.then(profile => this.kuzzle.repositories.profile.delete(profile, options));
.then(profile => {
this.kuzzle.log.info(`[SECURITY] User "${this.getUserId(request)}" applied action "${request.input.action}" on profile "${profile._id}."`);
return this.kuzzle.repositories.profile.delete(profile, options);
});
}

/**
Expand Down Expand Up @@ -854,6 +872,7 @@ class SecurityController extends NativeController {

await this.kuzzle.repositories.user.delete(user, options);

this.kuzzle.log.info(`[SECURITY] User "${this.getUserId(request)}" applied action "${request.input.action}" on user "${userId}."`);
return {
_id: userId
};
Expand All @@ -880,6 +899,7 @@ class SecurityController extends NativeController {
const pojoUser = request.input.body.content;
pojoUser._id = request.input.resource._id || uuid();

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

Expand All @@ -905,6 +925,7 @@ class SecurityController extends NativeController {

pojoUser.profileIds = this.kuzzle.config.security.restrictedProfileIds;

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

Expand Down Expand Up @@ -937,7 +958,10 @@ class SecurityController extends NativeController {
Object.assign(currentUserPojo, pojo));
})
.then(user => this.kuzzle.repositories.user.persist(user, options))
.then(updatedUser => formatProcessing.serializeUser(updatedUser));
.then(updatedUser => {
this.kuzzle.log.info(`[SECURITY] User "${this.getUserId(request)}" applied action "${request.input.action}" on user "${updatedUser._id}."`);
return formatProcessing.serializeUser(updatedUser);
});
}

/**
Expand Down Expand Up @@ -986,6 +1010,7 @@ class SecurityController extends NativeController {
updatedUser,
options);

this.kuzzle.log.info(`[SECURITY] User "${this.getUserId(request)}" applied action "${request.input.action}" on user "${createdUser._id}."`);
scottinet marked this conversation as resolved.
Show resolved Hide resolved
return formatProcessing.serializeUser(createdUser);
}

Expand All @@ -1010,7 +1035,10 @@ class SecurityController extends NativeController {
.then(profile => this.kuzzle.repositories.profile.validateAndSaveProfile(
_.extend(profile, request.input.body),
options))
.then(updatedProfile => formatProcessing.serializeProfile(updatedProfile));
.then(updatedProfile => {
this.kuzzle.log.info(`[SECURITY] User "${this.getUserId(request)}" applied action "${request.input.action}" on profile "${updatedProfile._id}."`);
return formatProcessing.serializeProfile(updatedProfile);
});
}

/**
Expand All @@ -1035,7 +1063,10 @@ class SecurityController extends NativeController {
.then(role => this.kuzzle.repositories.role.validateAndSaveRole(
_.extend(role, request.input.body),
options))
.then(updatedRole => formatProcessing.serializeRole(updatedRole));
.then(updatedRole => {
this.kuzzle.log.info(`[SECURITY] User "${this.getUserId(request)}" applied action "${request.input.action}" on role "${updatedRole._id}."`);
return formatProcessing.serializeRole(updatedRole);
});
}

/**
Expand Down Expand Up @@ -1075,6 +1106,7 @@ class SecurityController extends NativeController {
.then(() => response);
}

this.kuzzle.log.info(`[SECURITY] User "${this.getUserId(request)}" applied action "${request.input.action}".`);
return response;
});
}
Expand Down Expand Up @@ -1179,6 +1211,7 @@ class SecurityController extends NativeController {
strategy,
'create');

this.kuzzle.log.info(`[SECURITY] User "${this.getUserId(request)}" applied action "${request.input.action}" on user "${id}."`);
return createMethod(request, request.input.body, id, strategy);
});
}
Expand Down Expand Up @@ -1214,6 +1247,7 @@ class SecurityController extends NativeController {
strategy,
'update');

this.kuzzle.log.info(`[SECURITY] User "${this.getUserId(request)}" applied action "${request.input.action}" on user "${id}."`);
return updateMethod(request, request.input.body, id, strategy);
});
}
Expand Down Expand Up @@ -1273,7 +1307,10 @@ class SecurityController extends NativeController {
'delete');

return deleteMethod(request, request.input.resource._id, request.input.args.strategy)
.then(() => ({acknowledged: true}));
.then(() => {
this.kuzzle.log.info(`[SECURITY] User "${this.getUserId(request)}" applied action "${request.input.action}" on user "${request.input.resource._id}."`);
return {acknowledged: true};
});
}

/**
Expand Down Expand Up @@ -1468,6 +1505,16 @@ function mDelete (kuzzle, type, request) {
errorsManager.get('services', 'storage', 'incomplete_delete', errors));
}

const userId = request.context && request.context.user && request.context.user._id
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't use getUserId here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because this function is outside the class

Copy link
Contributor

Choose a reason for hiding this comment

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

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

? request.context.user._id
: null;

if (ids.length > 1000) {
kuzzle.log.info(`[SECURITY] User "${userId}" deleted the following ${type}s ${ids.slice(0, 1000).join(', ')}... (${ids.length - 1000} more users deleted)."`);
}
else {
kuzzle.log.info(`[SECURITY] User "${userId}" deleted the following ${type}s ${ids.join(', ')}."`);
}
return ids;
});
}
Expand Down
11 changes: 7 additions & 4 deletions lib/api/funnel.js
Expand Up @@ -673,7 +673,8 @@ class Funnel {
if (asError) {
callback(error, request);
this.handleErrorDump(error);
} else {
}
else {
callback(null, request);
}

Expand All @@ -698,20 +699,22 @@ class Funnel {
if (this[cachedItem.executor](cachedItem.request, cachedItem.callback) === -1) {
// no slot found again. We stop here and try next time
break;
} else {
}
else {
this.requestsCacheQueue.shift();
}
}
}

if (this.requestsCacheQueue.length > 0) {
setTimeout(() => this._playCachedRequests(), 0);
} else {
}
else {
const now = Date.now();
// No request remaining in cache => stop the background task and return to normal behavior
this.overloaded = false;

if (this.overloadWarned
if ( this.overloadWarned
&& (this.lastOverloadTime === 0 || this.lastOverloadTime < now - 500)
) {
this.overloadWarned = false;
Expand Down
23 changes: 12 additions & 11 deletions lib/core/janitor.js
Expand Up @@ -50,18 +50,18 @@ class Janitor {
* @param {object} securities
* @returns {Promise}
*/
async loadSecurities (securities = {}) {
async loadSecurities (securities = {}, user = null) {
Aschen marked this conversation as resolved.
Show resolved Hide resolved
assertIsObject(securities);
await this._createSecurity('createOrReplaceRole', securities.roles);
await this._createSecurity('createOrReplaceProfile', securities.profiles);
await this._deleteUsers(securities.users);
await this._createSecurity('createUser', securities.users);
await this._createSecurity('createOrReplaceRole', securities.roles, user);
await this._createSecurity('createOrReplaceProfile', securities.profiles, user);
await this._deleteUsers(securities.users, user);
await this._createSecurity('createUser', securities.users, user);
}

/**
* Load database fixtures into Kuzzle
*
* @param {String} fixtures
* @param {String} fixturesId
* @returns {Promise}
*/
loadFixtures (fixtures = {}) {
Expand Down Expand Up @@ -354,14 +354,15 @@ class Janitor {
}
}

_createSecurity (action, objects) {
_createSecurity (action, objects, user) {
if (! objects) {
return Bluebird.resolve();
}

try {
assertIsObject(objects);
} catch (e) {
}
catch (e) {
return Bluebird.reject(e);
}

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

return this.kuzzle.funnel.processRequest(request);
});
}

async _deleteUsers (users) {
async _deleteUsers (users, user) {
if (! users) {
return;
}
Expand All @@ -392,7 +393,7 @@ class Janitor {
body: { ids },
controller: 'security',
refresh: 'wait_for'
});
}, { user });

try {
await this.kuzzle.funnel.processRequest(request);
Expand Down
5 changes: 3 additions & 2 deletions test/api/controllers/admin.test.js
Expand Up @@ -23,7 +23,7 @@ describe('AdminController', () => {

adminController = new AdminController(kuzzle);

request = new Request({ controller: 'admin' });
request = new Request({ controller: 'admin' }, {user: {_id: 'test-user'}});

request.input.args.refresh = 'wait_for';
});
Expand Down Expand Up @@ -234,14 +234,15 @@ describe('AdminController', () => {
beforeEach(() => {
request.input.action = 'loadSecurities';
request.input.body = { gordon: { freeman: [] } };
request.context.user = {_id: 'test-user'};
});

it('should call Janitor.loadSecurities', () => {
return adminController.loadSecurities(request)
.then(() => {
should(kuzzle.janitor.loadSecurities)
.be.calledOnce()
.be.calledWith({ gordon: { freeman: [] } });
.be.calledWith({ gordon: { freeman: [] } }, {user: { _id: 'test-user'}} );
});
});
});
Expand Down
2 changes: 1 addition & 1 deletion test/api/controllers/security/createFirstAdmin.test.js
Expand Up @@ -63,7 +63,7 @@ describe('Test: security controller - createFirstAdmin', () => {
action: 'createFirstAdmin',
_id: 'toto',
body: {content: {password: 'pwd'}}
}))
}, { user: { _id: 'User' } }))
.then(() => {
should(createUser).be.calledOnce();
should(createUser.firstCall.args[0]).be.instanceOf(Request);
Expand Down