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.

Sign up for GitHub

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 promise support #23

Closed
wants to merge 4 commits into from
Closed

add promise support #23

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d0c0837
add promise support
brysgo May 19, 2018
5d0d837
some fixes from code review
brysgo May 23, 2018
31f27eb
a dumb implementation of auth level caching
brysgo May 23, 2018
846c775
switch to es6 WeakMap
brysgo May 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 52 additions & 38 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
const _ = require('lodash');
const {
resetCache,
getAuthorizedFields,
hasPermission,
authIsDisabled,
Expand All @@ -10,69 +11,82 @@ const {
const PermissionDeniedError = require('./lib/PermissionDeniedError');

module.exports = (schema) => {
function save(doc, options, next) {
if (doc.isNew && !hasPermission(schema, options, 'create', doc)) {
return next(new PermissionDeniedError('create'));
async function save(doc, options, next) {
resetCache();
if (doc.isNew && !await hasPermission(schema, options, 'create', doc)) {
next(new PermissionDeniedError('create'));
return;
}

const authorizedFields = getAuthorizedFields(schema, options, 'write', doc);
const authorizedFields = await getAuthorizedFields(schema, options, 'write', doc);
const modifiedPaths = doc.modifiedPaths();
const discrepancies = _.difference(modifiedPaths, authorizedFields);

if (discrepancies.length > 0) {
return next(new PermissionDeniedError('write', discrepancies));
next(new PermissionDeniedError('write', discrepancies));
return;
}

return next();
next();
}

function removeQuery(query, next) {
if (!hasPermission(schema, query.options, 'remove')) {
return next(new PermissionDeniedError('remove'));
async function removeQuery(query, next) {
resetCache();
if (!await hasPermission(schema, query.options, 'remove')) {
next(new PermissionDeniedError('remove'));
return;
}

return next();
next();
return;
}

function removeDoc(doc, options, next) {
if (!hasPermission(schema, options, 'remove', doc)) {
return next(new PermissionDeniedError('remove'));
async function removeDoc(doc, options, next) {
resetCache();
if (!await hasPermission(schema, options, 'remove', doc)) {
next(new PermissionDeniedError('remove'));
return;
}

return next();
next();
return;
}

function find(query, docs, next) {
const sanitizedResult = sanitizeDocumentList(schema, query.options, docs);
async function find(query, docs, next) {
resetCache();
const sanitizedResult = await sanitizeDocumentList(schema, query.options, docs);

return next(null, sanitizedResult);
next(null, sanitizedResult);
}

function update(query, next) {
async function update(query, next) {
resetCache();
// If this is an upsert, you'll need the create permission
// TODO add some tests for the upset case
if (
query.options
&& query.options.upsert
&& !hasPermission(schema, query.options, 'create')
&& !await hasPermission(schema, query.options, 'create')
) {
return next(new PermissionDeniedError('create'));
next(new PermissionDeniedError('create'));
return;
}

const authorizedFields = getAuthorizedFields(schema, query.options, 'write');
const authorizedFields = await getAuthorizedFields(schema, query.options, 'write');

// check to see if the group is trying to update a field it does not have permission to
const modifiedPaths = getUpdatePaths(query._update);
const discrepancies = _.difference(modifiedPaths, authorizedFields);
if (discrepancies.length > 0) {
return next(new PermissionDeniedError('write', discrepancies));
next(new PermissionDeniedError('write', discrepancies));
return;
}

// TODO handle the overwrite option
// TODO handle Model.updateMany

// Detect which fields can be returned if 'new: true' is set
const authorizedReturnFields = getAuthorizedFields(schema, query.options, 'read');
const authorizedReturnFields = await getAuthorizedFields(schema, query.options, 'read');

// create a sanitizedReturnFields object that will be used to return only the fields that a
// group has access to read
Expand All @@ -84,7 +98,7 @@ module.exports = (schema) => {
});
query._fields = sanitizedReturnFields;

return next();
next();
}

// Find paths with permissioned schemas and store those so deep checks can be done
Expand All @@ -98,34 +112,34 @@ module.exports = (schema) => {
});

schema.pre('findOneAndRemove', function preFindOneAndRemove(next) {
Copy link
Contributor

Choose a reason for hiding this comment

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

These functions would all need to async (and use await) as well, right? This is a little off since they use the next() function as well. Mongoose 5.x (I think) will see that a promise is returned and move along when the promise is resolved. But the code is written to wait for next() to be called. And Mongoose 4.x doesn't support promises from middleware, so there might be some inconsistent behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should work as is, because we are just forwarding next it doesn't care about the promises.

Copy link
Contributor

Choose a reason for hiding this comment

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

You've got me thinking on overdrive here. I think the issue is that if one of the lower level functions throws an exception, then we'll get an UnresolvedPromiseRejection since the promise isn't tied all the way up. removeQuery is an async function that will return a promise, and that promise isn't handled.

You could poke at this and test it. I suspect you might need to do

removeQuery(this, next).catch(err => next(err));

It's a little jank since the non-error path passes the next function down, but the error path is handled up here as a promise. This actually might be a problem in the existing code, so I'm open to you highlighting that and us creating a new task to properly handle exceptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really appreciate the thought around the error path here. I think it is important and it is something I hadn't considered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I fully understand the problem you are outlining, but I would love to learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I see what you are saying, does mongoose do .catch() on returned promises?

Copy link
Contributor

Choose a reason for hiding this comment

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

The issue is that removeQuery returns a promise, right? It is an async function. If that promise rejects (let's say because an error gets thrown), node will stop because the promise rejects, but there's no .catch() on the promise. Try it out. just insert a throw in one of the helper functions. You'll get the following when you run npm test.

helpers.test.js
(node:46340) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 2): fpp
(node:46340) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

This sorta the challenge of this change. By adding an async function at the root, we're forced to change every function in the library to handle promises properly when the old code was mostly synchronous.

if (authIsDisabled(this.options)) { return next(); }
return removeQuery(this, next);
if (authIsDisabled(this.options)) { next(); return; }
removeQuery(this, next);
});
// TODO, WTF, how to prevent someone from Model.find().remove().exec(); That doesn't
// fire any remove hooks. Does it fire a find hook?
schema.pre('remove', function preRemove(next, options) {
if (authIsDisabled(options)) { return next(); }
return removeDoc(this, options, next);
if (authIsDisabled(options)) { next(); return; }
removeDoc(this, options, next);
});
schema.pre('save', function preSave(next, options) {
if (authIsDisabled(options)) { return next(); }
return save(this, options, next);
if (authIsDisabled(options)) { next(); return; }
save(this, options, next);
});
schema.post('find', function postFind(doc, next) {
if (authIsDisabled(this.options)) { return next(); }
return find(this, doc, next);
if (authIsDisabled(this.options)) { next(); return; }
find(this, doc, next);
});
schema.post('findOne', function postFindOne(doc, next) {
if (authIsDisabled(this.options)) { return next(); }
return find(this, doc, next);
if (authIsDisabled(this.options)) { next(); return; }
find(this, doc, next);
});
schema.pre('update', function preUpdate(next) {
if (authIsDisabled(this.options)) { return next(); }
return update(this, next);
if (authIsDisabled(this.options)) { next(); return; }
update(this, next);
});
schema.pre('findOneAndUpdate', function preFindOneAndUpdate(next) {
if (authIsDisabled(this.options)) { return next(); }
return update(this, next);
if (authIsDisabled(this.options)) { next(); return; }
update(this, next);
});

schema.query.setAuthLevel = function setAuthLevel(authLevel) {
Expand Down
76 changes: 47 additions & 29 deletions lib/helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
const _ = require('lodash');

function resolveAuthLevel(schema, options, doc) {
let authLevelCache;

function resetCache() {
authLevelCache = new Map();
}

async function resolveAuthLevel(schema, options, doc) {
// Look into options the options and try to find authLevels. Always prefer to take
// authLevels from the direct authLevel option as opposed to the computed
// ones from getAuthLevel in the schema object.
Expand All @@ -9,9 +15,21 @@ function resolveAuthLevel(schema, options, doc) {
if (options.authLevel) {
authLevels = _.castArray(options.authLevel);
} else if (typeof schema.getAuthLevel === 'function') {
authLevels = _.castArray(schema.getAuthLevel(options.authPayload, doc));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the approach of changes lines 7-17 as opposed to adding await just to this line?

Copy link
Contributor

Choose a reason for hiding this comment

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

otherwise it's ambiguous what exactly is in the authLevelsIn variable. it could be an array, or it could be a promise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A few reasons, I'm casting it right after and throwing away authLevelsIn, adding await Promise.resolve(...) to each line is not super easy to read, and last but not least, the first iteration was using .then() so this would have been a lot more challenging.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Two things. You don't need an await in the first case. authLevels is just looking up a key in an object. Second, you don't need await Promise.resolve(foo)k...you can just write await foo;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@makinde learned something new! await works on non-promise values

let arg2Map = authLevelCache.get(options.authLevel)
if (!arg2Map) {
arg2Map = new Map();
authLevelCache.set(options.authLevel, arg2Map);
}
const cachedValue = arg2Map.get(doc);
if (cachedValue) {
authLevels = cachedValue;
} else {
authLevels = _.castArray(await schema.getAuthLevel(options.authPayload, doc));
arg2Map.set(doc, authLevels);
}
}
}

// Add `defaults` to the list of levels since you should always be able to do what's specified
// in defaults.
authLevels.push('defaults');
Expand All @@ -23,8 +41,8 @@ function resolveAuthLevel(schema, options, doc) {
.value();
}

function getAuthorizedFields(schema, options, action, doc) {
const authLevels = resolveAuthLevel(schema, options, doc);
async function getAuthorizedFields(schema, options, action, doc) {
const authLevels = await resolveAuthLevel(schema, options, doc);

return _.chain(authLevels)
.flatMap(level => schema.permissions[level][action])
Expand All @@ -33,8 +51,8 @@ function getAuthorizedFields(schema, options, action, doc) {
.value();
}

function hasPermission(schema, options, action, doc) {
const authLevels = resolveAuthLevel(schema, options, doc);
async function hasPermission(schema, options, action, doc) {
const authLevels = await resolveAuthLevel(schema, options, doc);
const perms = schema.permissions || {};

// look for any permissions setting for this action that is set to true (for these authLevels)
Expand All @@ -45,21 +63,21 @@ function authIsDisabled(options) {
return options && options.authLevel === false;
}

function embedPermissions(schema, options, doc) {
async function embedPermissions(schema, options, doc) {
if (!options || !options.permissions) { return; }

const permsKey = options.permissions === true ? 'permissions' : options.permissions;
doc[permsKey] = {
read: getAuthorizedFields(schema, options, 'read', doc),
write: getAuthorizedFields(schema, options, 'write', doc),
remove: hasPermission(schema, options, 'remove', doc),
read: await getAuthorizedFields(schema, options, 'read', doc),
write: await getAuthorizedFields(schema, options, 'write', doc),
remove: await hasPermission(schema, options, 'remove', doc),
};
}

function sanitizeDocument(schema, options, doc) {
const authorizedFields = getAuthorizedFields(schema, options, 'read', doc);
async function sanitizeDocument(schema, options, doc) {
const authorizedFields = await getAuthorizedFields(schema, options, 'read', doc);

if (!doc || getAuthorizedFields.length === 0) { return false; }
if (!doc || authorizedFields.length === 0) { return false; }

// Check to see if group has the permission to see the fields that came back.
// We must edit the document in place to maintain the right reference
Expand Down Expand Up @@ -97,36 +115,35 @@ function sanitizeDocument(schema, options, doc) {

// Check to see if we're going to be inserting the permissions info
if (options.permissions) {
embedPermissions(schema, options, doc);
await embedPermissions(schema, options, doc);
}

// Apply the rules down one level if there are any path specific permissions
_.each(schema.pathsWithPermissionedSchemas, (path, subSchema) => {
await Promise.all(_.map(schema.pathsWithPermissionedSchemas, async (path, subSchema) => {
if (innerDoc[path]) {
// eslint-disable-next-line no-use-before-define
innerDoc[path] = sanitizeDocumentList(subSchema, options, innerDoc[path]);
innerDoc[path] = await sanitizeDocumentList(subSchema, options, innerDoc[path]);
}
});
}));

return doc;
}

function sanitizeDocumentList(schema, options, docs) {
async function sanitizeDocumentList(schema, options, docs) {
const multi = _.isArrayLike(docs);
const docList = _.castArray(docs);
const sanitizeAndAddOptions = (doc) => {
const upgradedOptions = _.isEmpty(schema.pathsWithPermissionedSchemas)
? options
: _.merge({}, options, { authPayload: { originalDoc: doc } });
return sanitizeDocument(schema, upgradedOptions, doc);
};

const filteredResult = _.chain(docList)
.map((doc) => {
const upgradedOptions = _.isEmpty(schema.pathsWithPermissionedSchemas)
? options
: _.merge({}, options, { authPayload: { originalDoc: doc } });

return sanitizeDocument(schema, upgradedOptions, doc);
})
.filter(docList)
.value();
const filteredResult = (
await Promise.all(docList.map(sanitizeAndAddOptions))
).filter(doc => !doc);

return multi ? filteredResult : filteredResult[0];
return multi ? (filteredResult) : filteredResult[0];
}

function getUpdatePaths(updates) {
Expand All @@ -143,6 +160,7 @@ function getUpdatePaths(updates) {
}

module.exports = {
resetCache,
resolveAuthLevel,
getAuthorizedFields,
hasPermission,
Expand Down
Loading