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

Permissions ignored in custom User.find #221

Open
dottodot opened this issue Feb 29, 2016 · 15 comments
Open

Permissions ignored in custom User.find #221

dottodot opened this issue Feb 29, 2016 · 15 comments

Comments

@dottodot
Copy link

If I put the following in my own UserController permissions seem to be ignored and a user making a request to /user can see all users

module.exports = {
  find: function(req, res) {
    User.find().then(function (users) {
      res.ok(users);
    });
  }
}

If I remove this they only can view their own.

@dottodot
Copy link
Author

dottodot commented Mar 3, 2016

Any chance of some help with this. It's a pretty critical issue if you can't use the UserController without exposing all users to everyone.

For reference I'm using v2.2.0

@ghost
Copy link

ghost commented Mar 11, 2016

Can u paste me your config/policies.js?

I am guessing that u did not apply a policies to your UserController and actions.

@dottodot
Copy link
Author

This is what I have at the moment

module.exports.policies = {

  '*': [
    'basicAuth',
    'passport',
    'sessionAuth',
    'ModelPolicy',
    'AuditPolicy',
    'OwnerPolicy',
    'PermissionPolicy',
    'RolePolicy',
    'CriteriaPolicy'
  ],

  UserController: {
    'create': true,
    'signup': true,
    'forgot': true,
    'reset': true
  }

};

@ghost
Copy link

ghost commented Mar 11, 2016

  1. As I thought the following lines are not restricting access to UserController.find.
 UserController: {
    'create': true,
    'signup': true,
    'forgot': true,
    'reset': true
  }

Moreso no UserController action is restricted as every action is set to 'true', meaning anybody may hit this controller's actions.

  1. As far as i understand sails-permissions this module restricts access to your models ONLY IF u access them via RESTful routes. Specifically sails-permissions uses policies and policies only restrict Controllers and their actions, nothing else.
    If someone permittedly hits one of ur controller actions and you use Waterline models to alter ur database, sails-permissions will not restrict anything as it protects the REST-API of sails, as mentioned above. So you either got to write ur own policy or handle this within your find action.

You could be doing something like:

UserController: {
    '*': [   // restrict any action of the UserController with the following policies. But if u do this there is imo no need for the find action on your UserController, as you could query your users via REST which would be authorized (or not) as intended by sails-permissions. 
    'basicAuth',
    'passport',
    'sessionAuth',
    'ModelPolicy',
    'AuditPolicy',
    'OwnerPolicy',
    'PermissionPolicy',
    'RolePolicy',
    'CriteriaPolicy'
  ]
    'find': [ // exclude the find action from the above listing and explicitly name the policies to be checked on this controller's action.
         'basicAuth', // if basic authorization is present on the http header this policy passes if auth succeeds
         'passport', // init passport and session
         'sessionAuth', // check if req.session.authenticated ==  true
         'AuditPolicy',  // write logs
     ] // now check in the find action if everything is met to authorize whatever you want to be doing
  }

Please have a read of Sails Policies and Sails Blueprint API

If I am wrong please correct me :) Improvements and/or new knowledge are always welcome!

Hoping to be helpful..

Regards,
René

@dottodot
Copy link
Author

This reason I have the find action on my controller is so I can customise the response. My original post just shows basic example as it doesn't make any difference to the problem.

So am I right in thinking Sail permissions only works if you only use the default blueprint routes?

@ghost
Copy link

ghost commented Mar 11, 2016

Not exactly but yes, kind of :)

Sails policies only work on controllers and their actions. So u can protect ur find action. But the model logic will not be touched. It basically applies the policies to the blueprint controllers (more correctly ALL controller u mention and configure in policies.js).

@dottodot
Copy link
Author

I still a bit confused. If I have the following policies

UserController: {
    '*': [
      'basicAuth',
      'passport',
      'sessionAuth',
      'ModelPolicy',
      'AuditPolicy',
      'OwnerPolicy',
      'PermissionPolicy',
      'RolePolicy',
      'CriteriaPolicy'
    ],
    'find': [
      'basicAuth',
      'passport',
      'sessionAuth',
      'ModelPolicy',
      'AuditPolicy',
      'OwnerPolicy',
      'PermissionPolicy',
      'RolePolicy',
      'CriteriaPolicy'
    ],
    'create': true,
    'signup': true
  }

I can see that the Polices are triggered but just don't have any effect on the results returned. Just seem bizarre that it can go through all the checks only to not have any outcome on the result. Surely that defeats the purpose.

@dottodot
Copy link
Author

Also as an experiment I've tried with the Role controller setting policy for read to owner returns no results, and setting it to role does, so policies seem to work sometimes.

@dottodot
Copy link
Author

Also my custom findOne for users also returns the correct response i.e users can only view themselves, so it seems it's an issue with just custom find.

@dottodot
Copy link
Author

Think this issue might be related to #212

findTargetObjects: function findTargetObjects(req) {

    // handle add/remove routes that have :parentid as the primary key field
    var originalId;
    if (req.params.parentid) {
      originalId = req.params.id;
      req.params.id = req.params.parentid;
    }

    return new Promise(function (resolve, reject) {
      sails.hooks.blueprints.middleware.find(req, {
        ok: resolve,
        serverError: reject,
        // this isn't perfect, since it returns a 500 error instead of a 404 error
        // but it is better than crashing the app when a record doesn't exist
        notFound: reject
      });
    }).then(function (result) {
      console.log(result);
      if (originalId !== undefined) {
        req.params.id = originalId;
      }
      return result;
    });
  }

Here the result is actually correct

@tjwebb are you able to provide any help with this as little unsure where to look next to find out where it's going wrong.

@tjwebb
Copy link
Member

tjwebb commented Mar 12, 2016

see #200

@dottodot
Copy link
Author

OK thanks for letting me know, massively disappointing though.

@tjwebb
Copy link
Member

tjwebb commented Mar 13, 2016

We're disappointed as well: balderdashy/sails#3429 (comment)

We are looking for active users/contributes to assist in transferring the long-term maintenance responsibility of this project to someone else.

@medisoft
Copy link

medisoft commented Apr 7, 2017

It is weird. Yesterday I took more than 5 hours reaching for the bug. Permissions seems to be working OK, because until the last policy it reaches only the correct results. But at the end the find request returns everything. I think this could be a problem between sails-permissions and the ORM.

I would like to maintain this module, as I use in many projects.

@tjwebb
Copy link
Member

tjwebb commented Apr 8, 2017

@medisoft if you're interested in maintaining, can you send me an email? [email protected]. Thanks

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

No branches or pull requests

3 participants