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

findById does not include "options", but find does? #3209

Closed
DaGaMs opened this issue Feb 17, 2017 · 5 comments
Closed

findById does not include "options", but find does? #3209

DaGaMs opened this issue Feb 17, 2017 · 5 comments
Assignees
Labels

Comments

@DaGaMs
Copy link
Contributor

DaGaMs commented Feb 17, 2017

Description/Steps to reproduce

According to the new remoting context injection #3023 I expect to always get an options property in the current context of an operation hook. I have a mixin that will restrict the requested object to the current owner (and in the future other team members, so I can't use $owner):

module.exports = function(Model, options) {
  Model.observe('access', function limitToTenant(ctx, next) {
    var userId = ctx.options.accessToken.userId;
    if (!userId) {
      console.error("No user in context ", ctx.options);
      var error = new Error("Not authorized");
      error.statusCode = 401;
      next(error);
    }
    if (_.has(ctx,'query')) {
      if (_.has(ctx.query, 'where')) {
        var old = ctx.query.where;
        ctx.query.where = {and: [old, {ownerId: userId}]};
      }
      else {
        ctx.query.where = {ownerId: userId};
      }
    }
    next();
  });
};

This works just fine when I do a request like this: /api/projects?filter[where][id]=1, but when I do a request like this: /api/projects/1, the options parameter is empty.

Expected result

I expect invocation of find and findById to both propagate the options to operation hooks.

Additional information

Here is the model definition file:

{
  "name": "project",
  "base": "PersistedModel",
  "strict": true,
  "idInjection": true,
  "replaceOnPUT": false,
  "options": {
    "validateUpsert": true
  },
  "mixins": {
    "AccessControl": {}
  },
  "mysql": {
    "schema": "testdb",
    "table": "Project"
  },
  "properties": {
    "lastUpdated": {
      "type": "date",
      "required": true,
      "length": null,
      "dataType": "timestamp",
      "nullable": "N"
    },
    "created": {
      "type": "date",
      "required": true,
      "index": -1,
      "dataType": "datetime",
      "nullable": "N" 
    },
    "name": {
      "type": "string",
      "required": false,
      "length": 256,
      "index": 1,
      "nullable": "Y"
    }
  },
  "validations": [],
  "relations": {
    "owner": {
      "model": "Person",
      "type": "belongsTo"
    }
  },
  "acls": [
    {
      "accessType": "*",
      "principalType": "ROLE",
      "principalId": "$everyone",
      "permission": "DENY"
    },
    {
      "accessType": "*",
      "principalType": "ROLE",
      "principalId": "$authenticated",
      "permission": "ALLOW",
      "property": "count"
    },
    {
      "accessType": "*",
      "principalType": "ROLE",
      "principalId": "$authenticated",
      "permission": "ALLOW",
      "property": ["all", "find", "findOne", "findById", "findOrCreate", "create", "getIdName", "getSourceId"]
    },
    {
      "accessType": "*",
      "principalType": "ROLE",
      "principalId": "$owner",
      "permission": "ALLOW"
    }
  ],
  "methods": {
  }
}

Versions

$   npm ls --prod --depth 0 | grep loopback
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected] (git://github.com/dagams/loopback-component-passport.git#db99e72a8fcf7291c6803c70b7c4ebc256c0ea3e)
├── [email protected]
├── [email protected]
@DaGaMs
Copy link
Contributor Author

DaGaMs commented Feb 18, 2017

Ok, I have narrowed this down a bit now. The problem is not actually with findById, but with the ACL settings. I must be missing some important ACL property somewhere, but I'm not sure what it is. I created a repo with a minimal project based on the Note example of strongloop 3.0. Just type mocha to run the test cases.

https://github.com/DaGaMs/loopback_context_test

@DaGaMs
Copy link
Contributor Author

DaGaMs commented Feb 18, 2017

Some more information. When I run with DEBUG="*", I can see that in the working test case, the ACL resolver seems to "do the right thing", i.e. resolve to isOwner=true, see this gist;

On the other hand, in the test case that fails, I get this output, which differs in the following two lines which don't appear in the passing test:

loopback:security:role isOwner(): Note 1 userId: 1 principalType: USER +1ms
loopback:security:role Model not found for id "1" +0ms

This does look like a bug to me, but I'm not sure where it's coming from.

@DaGaMs
Copy link
Contributor Author

DaGaMs commented Feb 18, 2017

So, simply removing the last part of the access control list makes the tests pass:

   {
      "accessType": "*",
      "principalType": "ROLE",
      "principalId": "$owner",
      "permission": "ALLOW"
    }

It seems that there's really some very peculiar bug that only strikes when the dynamic $owner role is resolved in a findById query.

@stale
Copy link

stale bot commented Aug 23, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot closed this as completed Sep 6, 2017
@0candy 0candy removed the triaging label Sep 6, 2017
@stale
Copy link

stale bot commented Sep 6, 2017

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.

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

No branches or pull requests

4 participants