meteor / meteor-feature-requests

A tracker for Meteor issues that are requests for new functionality, not bugs.
Other
89 stars 3 forks source link

Add middlewares support for Methods and Publications #395

Closed diavrank closed 4 years ago

diavrank commented 4 years ago

It would be great to add one or more middlewares to methods and publications, because currently I have to repeat code in each Method and Publication in order to add security about permissions of each user. I use the alanning:roles package and I did a function which validates if a user has the permission to execute a certain action (method or publication), however, I have to invoke this function in each Meteor method/publication and this provoke to repeat code.

The following image shows the problem:

codeRepeated

I found a package which solve this problem, its name is konecty:methods-middleware , but it has some deficiencies:

  1. It only works for methods, not for publications.
  2. You can add only one middleware by method, this is, you cannot add multiple middlewares by method.

I know that Meteor isn't a Rest API, however, I consider that middlewares are essential for web applications development. So, I would love if you could add this functionality to core of Meteor. Maybe you can copy the way Laravel implements it, since it's very simple how to manage middlewares to your routes (but in the Meteor case would be for methods and publications).

The idea is to able to access to the context variable "this" inside of the middlewares too, in order to get data such as this.userId to implement security about user permissions. Moreover, for other kind of middlewares like a binnacle, it would be great that middlewares (and methods and publications) are able to know the method name or publication name, and know if it is a method or a publication, because currently the context variable "this" only has the following data:

contextMethod

Notes: Tested on Meteor 1.10.1

hexsprite commented 4 years ago

ValidatedMethod has support for middleware via Mixins. Take a look at that. Would it meet your requirements?

https://github.com/meteor/validated-method

mitar commented 4 years ago

I made https://github.com/peerlibrary/meteor-middleware some time ago.

diavrank commented 4 years ago

ValidatedMethod has support for middleware via Mixins. Take a look at that. Would it meet your requirements?

https://github.com/meteor/validated-method

I guess for publications I would have to use gnil:validated-publish. I will check them,

diavrank commented 4 years ago

I made https://github.com/peerlibrary/meteor-middleware some time ago.

This package is good, but I look for a package simpler and it can work with meteor methods too.

I look for the same principle: publicationAuthGuard

mitar commented 4 years ago

Do note that your permission check will get done only at the beginning of the publish, so if it is a long-lasting publish permissions used might get out of sync with the database.

diavrank commented 4 years ago

ValidatedMethod has support for middleware via Mixins. Take a look at that. Would it meet your requirements? https://github.com/meteor/validated-method

I guess for publications I would have to use gnil:validated-publish. I will check them,

Hi @hexsprite , I have checked it and it does solved my problem perfectly for methods:

validatedMethods

I used the MethodHooks dependency from lacosta:method-hooks through Mixins for the middlewares that I needed.

I also realized that using ValidatedMethod promotes test my code.

My middleware function is the following:

AuthGuard.checkPermission

const checkPermission = function (methodArgs, methodOptions) {
    const idUser = this.userId;
    const permissions = methodOptions.permissions;
    let hasPermission = false;
    if (idUser !== null) {
        let group = Roles.getGroupsForUser(idUser)[0];
        hasPermission = Roles.userIsInRole(idUser, permissions, group);
    }
    if (!hasPermission) {
        throw new Meteor.Error('403', "Access denied",
            "You do not have permission to use this action");
    }
    return methodArgs;
};

For publications, I had to use peerlibrary:middleware from @mitar , because gnil:validated-publish doesn't provide the Hooks mixin like ValidatedMethods.

diavrank commented 4 years ago

Do note that your permission check will get done only at the beginning of the publish, so if it is a long-lasting publish permissions used might get out of sync with the database.

Hi @mitar , your peerlibrary:middleware package worked quite well for me. I used javascript (no coffee script) so I'm not sure if I implemented it well (I used a website to decaffeinate it ) but it works. My middleware class is the following:

PermissionMiddlware.js

export class PermissionMiddleware extends PublishMiddleware {
    constructor(permissions) {
        super();
        this._permissions=permissions;
    }

    added(publish, collection, id, fields) {
        return super.added(...arguments);
    }

    changed(publish, collection, id, fields) {
        if(this.checkPermission(publish.userId)){
            return super.changed(...arguments);
        }
    }

    removed(publish, collection, id) {
        if(this.checkPermission(publish.userId)){
            return super.removed(...arguments);
        }
    }

    onReady(publish) {
        if(this.checkPermission(publish.userId)){
            return super.onReady(...arguments);
        }
    }

    onStop(publish) {
        return super.onStop(...arguments);
    }

    onError(publish, error) {
        return super.onError(...arguments);
    }

    checkPermission(idUser){
        const group = Roles.getGroupsForUser(idUser)[0];
        return  Roles.userIsInRole(idUser, this._permissions, group);
    }
}

UsersPubs.js

import {Meteor} from 'meteor/meteor';
import {PermissionMiddleware} from './../../middlewares/PermissionMiddleware';
import Permissions from "../../startup/server/Permissions";

const usersPublication = new PublishEndpoint('users', function () {
    return Meteor.users.find({});
});

usersPublication.use(new PermissionMiddleware([Permissions.USERS.LIST]));

Notes: I had problems to install the latest version of peerlibrary:middleware (0.3.0). I used the 0.1.1 version because I also use Restivus package which use coffeescript 1.0.2 . So, I don't know if the latest version has great changes against the 0.1.1 version, but it solved my problem anyway. This is the error that I got:

C:\Users\Iván\Documents\WEB\scaffold-meteor-vue>meteor add peerlibrary:middleware@0.3.0
 => Errors while adding packages:

While selecting package versions:
error: Conflict: Constraint coffeescript@2.4.1 is not satisfied by coffeescript 1.0.17.
Constraints on package "coffeescript":
* coffeescript@1.0.2 <- nimble:restivus 0.8.12
* coffeescript@2.4.1 <- peerlibrary:middleware 0.3.0

So , I only have a question, my middleware class is correct?

mitar commented 4 years ago

There is nothing new in the new version, just support for CoffeeScript 2.

I do not think your middleware is correct. You are not checking permissions in added. But also, you should just not execute the whole query if permission check fails. No point in checking at every change callback.

On the side, I personally prefer simply rewriting queries to include permission checks, over checking permissions in JavaScript and then deciding to do the query or not.

diavrank commented 4 years ago

There is nothing new in the new version, just support for CoffeeScript 2.

I do not think your middleware is correct. You are not checking permissions in added. But also, you should just not execute the whole query if permission check fails. No point in checking at every change callback.

On the side, I personally prefer simply rewriting queries to include permission checks, over checking permissions in JavaScript and then deciding to do the query or not.

Thank you, yes the reason why I don't check permissions on the added method is because I realized that when I subscribe to the publication and the users are displayed in the table, for each user who is in the table the method added is called, this is, if 10 users are displayed, 10 times is called the added method (I verify that through console.logs). So, I didn't want to include it in order to optimize performance. Moreover, I realized that change method execute when there is an update of a user, remove when I delete an user and ready when a client subscription is started, so those methods are perfect to check permissions, because even I did a test having an user A with that permission and suscribed to that publication, and in other browser window I have another user B (with the same permission) who removes the permission of the other user A and indeed when the user B modify or remove users, the user A doesn't see the changes until the user B restore the permissions of user A.

In the other hand, I put the if condition in the return statement in order to block the publication execution if permission check fails, but specifically in that part is where I have doubt because I don't know if exists another way to block the publication execution, or that is the correct way to do it?

mitar commented 4 years ago

I think this is getting very off topic and becoming a support channel for you. Please use forum for that.