loopback-graphql icon indicating copy to clipboard operation
loopback-graphql copied to clipboard

ACL Support

Open LancelotP opened this issue 8 years ago • 8 comments

Hey !

I'm very interested in your project, I myself did an implementation of graphql inside loopback about 6 month ago on loopback 2.0 version. I've had troubles dealing with ACLs. I know it's in your roadmap, but do you have any ideas on how to implement them inside loopback-graphql ?

This project looks very promising !

LancelotP avatar Feb 13 '17 09:02 LancelotP

I am looking into it. The one approach that might work is using strong remoting to access the data, hence ACL will work. WDYT?

Tallyb avatar Feb 17 '17 06:02 Tallyb

Yup that's the one approach I was looking into earlier this week but was lacking time to test it correctly and ended up using something like this:

function checkACL({accessToken, id, model, method}) {
  return new Promise((resolve, reject) => {
    ACL.checkAccessForContext({
      accessToken,
      model,
      property: method,
      method, method,
      modelId: id,
    }, (err, res) => {
      if (err)
        reject(err);
      else if (res.permission !== ACL.DENY)
        resolve(res);
      else
        reject('Authorization Required');
    });
  });

One resolver amongst others :

return checkACL({accessToken: req.accessToken, model: 'map', method: '__get__markers', id: map.id})
      .then(() => app.models.map.findById(map.id))
      .then(map => map.markers.getAsync());

This was a prototype project, it works but not the best solution at all since this logic is one layer too high. It should be in the logic layer, not the graphql resolver.

LancelotP avatar Feb 17 '17 09:02 LancelotP

Any update on this? This project looks amazing but I don't think I would be able to use it without ACL support :/ Happy to help out if required :)

TimPerry avatar Apr 06 '17 13:04 TimPerry

Feel free to suggest a PR

On Thu, Apr 6, 2017, 16:12 Tim Perry [email protected] wrote:

Any update on this? This project looks amazing but I don't think I would be able to use it without ACL support :/ Happy to help out if required :)

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Tallyb/loopback-graphql/issues/34#issuecomment-292169896, or mute the thread https://github.com/notifications/unsubscribe-auth/AHC0jyze5ixmsw_3YajDkrsneU5-7Irpks5rtOTXgaJpZM4L--G9 .

Tallyb avatar Apr 06 '17 13:04 Tallyb

Hi there!

I was working on implementing ACLs in loopback-graphql the last two days. Finally I almost have a working thing. Probably is not the best solution, but @LancelotP give me the idea. Please, let me know what do you think.

I'm working only with findOne execution by the moment. I wrote this checkACL function for testing:

function checkACL(params, modelObject) {
  const AccessToken = modelObject.app.models.AccessToken;

  const ACL = modelObject.app.models.ACL;
  return new Promise((resolve, reject) => {
    AccessToken.resolve(params.accessToken, (err, res) => {
      const at = res;
      if (!at) {
        resolve(false);
      } else {
        ACL.checkAccessForToken(at, params.model,  params.modelId, params.method, (err2, res2) => {
          console.log("ACL: ", res2);
          if (err2) {
            reject(err2);
          } else if (res2) {
            resolve(modelObject.findById(params.modelId));
          } else {
            reject();
          }
        });
      }
    });
  });

This function resolves a promise with the result of findById if the token is allowed and rejects with an error or null when not allowed or in case of error.

Then I've modified the findOne function:

function findOne(model, obj, args, context) {
  const accessToken = context.query.access_token;

  let id = obj ? obj[model.getIdName()] : args.id;

  return checkACL({
    accessToken: accessToken,
    model: model.definition.name,
    modelId: id,
    method: 'title',
  }, model);

}

As you can see I'm getting the access token from the request, then I return the checkACL promise. checkACL can validate if a token is allowed to access the resource. The fact is when the token is allowed the query is executed correctly.

HTTP Status: 200
{
  "data": {
    "Book": {
      "title": "Libro 1"
    }
  }
}

But, in the case that the token is not allowed, proper response would be a 403 forbidden. Now I just get this:

HTTP Status: 200
{
  "data": {
    "Book": null
  },
  "errors": [
    {
      "message": "Cannot return null for non-nullable field Book.title.",
      "locations": [
        {
          "line": 3,
          "column": 5
        }
      ],
      "path": [
        "Book",
        "title"
      ]
    }
  ]
}

I don't have any idea on how to change the statusCode from loopback-graphql. DOn't know even if it's possible.

Do you have any idea on how to manage this?

Thank you in advance.

supermafete avatar Jul 19 '17 11:07 supermafete

I am looking into another (simpler) solution. Possibly calling the REST APIs which will automatically implement the ACL

Tallyb avatar Jul 21 '17 08:07 Tallyb

I already tried that @Tallyb ;)

Same results. The REST promise rejects and the result of the findOne Promise does not fit with what is expected, so that's not the solution, unless I implemented it wrong (thing that is perfectly possible :)

I'm dressing my code now. I will make a fork today or monday with my progress.

El vie., 21 jul. 2017 a las 10:51, Tally Barak ([email protected]) escribió:

I am looking into another (simpler) solution. Possibly calling the REST APIs which will automatically implement the ACL

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Tallyb/loopback-graphql/issues/34#issuecomment-316944628, or mute the thread https://github.com/notifications/unsubscribe-auth/ABtJ-8BPTitom5WKWGJ0ZCS2PKvDnXnCks5sQGajgaJpZM4L--G9 .

supermafete avatar Jul 21 '17 09:07 supermafete

It's almost working, but I have a strange behavior checking permissions. I just opened a Issue in loopback:

https://github.com/strongloop/loopback/issues/3518

supermafete avatar Jul 26 '17 13:07 supermafete