ACL.checkPermission returning incorrect Permission when principalType and principalId provided

[supermafete]

Hi there,

I'm trying to add ACL support to loopback-graphql component. To do that I'm using ACL.checkPermission() function (http://apidocs.strongloop.com/loopback/#acl-checkpermission) whose description is 'Check if the given principal is allowed to access the model/property'.

So, for the component, we need to check the permission for every property of the model. The thing is that checkPermission() function returns the permission with high score, without regarding in principalType nor principalId parameters.

Let me introduce my model ACLs:

  "acls": [
    {
      "accessType": "READ",
      "principalType": "ROLE",
      "principalId": "$everyone",
      "permission": "DENY",
      "property": "*"
    },
    {
      "accessType": "READ",
      "principalType": "ROLE",
      "principalId": "$unauthenticated",
      "permission": "DENY",
      "property": "*"
    },
    {
      "accessType": "READ",
      "principalType": "ROLE",
      "principalId": "$authenticated",
      "permission": "ALLOW",
      "property": "id"
    }
  ],

As you can see, we are denying READ to everyone and for unauthenticated roles, and allowing READ for property 'id' to authenticated role. That works properly in REST. When I ask for Book model, even if I'm authenticated, it returns a 401 due I don't have access to other properties different than 'id'.

The ability of GraphQL is to query only for needed properties, so I want to check if the role 'authenticated' has access to 'id' and 'name' and so on... This is my code to do that:

ACL.checkPermission('ROLE', '$authenticated', 'Book', 'id', 'READ',
       (checkPermissionErr, checkPermissionRes) => {
         if (checkPermissionRes.permission === 'DENY') {
           data[property] = null;
         }
     }),

That's working fine as you can see in the debug logs:

loopback:security:acl [GraphQL] Permission for Book.id is ALLOW +0ms
loopback:security:acl [GraphQL] Permission for Book.name is DENY +7ms
loopback:security:acl [GraphQL] Permission for Book.links is DENY +0ms 

BUT, if I call:

ACL.checkPermission('ROLE', '$unauthenticated', 'Book', 'id', 'READ',
       (checkPermissionErr, checkPermissionRes) => {
         if (checkPermissionRes.permission === 'DENY') {
           data[property] = null;
         }
     }),

The results are the same, because the permission candidate that checkPermission is resolving is this one:

{ model: 'Book',
  property: 'id',
  principalType: 'ROLE',
  principalId: '$authenticated',
  accessType: 'READ',
  permission: 'ALLOW' }

Debugging more we get this in the logs:

  loopback:security:acl The following ACLs were searched:  +1ms
  loopback:security:acl ---ACL--- +0ms
  loopback:security:acl model Book +0ms
  loopback:security:acl property id +0ms
  loopback:security:acl principalType ROLE +0ms
  loopback:security:acl principalId $authenticated +0ms
  loopback:security:acl accessType READ +0ms
  loopback:security:acl permission ALLOW +0ms
  loopback:security:acl with score: 8136 +0ms
  loopback:security:acl ---ACL--- +0ms
  loopback:security:acl model Book +0ms
  loopback:security:acl property * +0ms
  loopback:security:acl principalType ROLE +0ms
  loopback:security:acl principalId $unauthenticated +0ms
  loopback:security:acl accessType READ +0ms
  loopback:security:acl permission DENY +0ms
  loopback:security:acl with score: 7627 +0ms
  loopback:security:acl ---ACL--- +0ms
  loopback:security:acl model Book +0ms
  loopback:security:acl property * +0ms
  loopback:security:acl principalType ROLE +0ms
  loopback:security:acl principalId $everyone +0ms
  loopback:security:acl accessType READ +0ms
  loopback:security:acl permission DENY +0ms
  loopback:security:acl with score: 7623 +0ms

So checkPermission is resolving the permission with highest score, no matter the principalType or principalId provided to the function.

I think this is an unnoticed (for REST) but incorrect behavior.

Probably there is a mistake in acl.js(233):

      if (!req.isWildcard()) {
        // We should stop from the first match for non-wildcard
        permission = candidate.permission;

We should NOT stop from the first match for non-wildcard. We should stop from the firstmatch that corresponds to the principalType and principalId provided.

Please, let me know your feedback. I'm dealing with this for some days to end the ACL support for loopback-graphql.

Thanks.

[supermafete]

Any news on this? I patched acl.js with this code, and it's working apparently fine:

acl.js(233)

        // We should stop from the first match for non-wildcard
        permission = candidate.permission;

        // Not really. We must find the first permission that matches our request
        var new_candidate = acls.find((acl) => {
          console.log("TYPE", principalType);
          console.log("ID", principalId);
          return ((acl.principalType === principalType) && (acl.principalId === principalId));
        });
        if (new_candidate) {
          candidate = new_candidate;
        }

        permission = candidate.permission;
        break;

[supermafete]

Seems that nobody is interested in this issue, probably because it does not affect to standard loopback configurations. That doesn't mean that still there is a bug in a sensitive loopback's function.

I'm going to fork the project and make a pull request.

Thanks.

[bajtos]

Hello, could you please check #3293 - could it be related to the problem you are experiencing?

[supermafete]

It's related, in part. But there's some more troubles with that code I resolved in #3618. Note that the issue I'm fixing has no evidence when using REST (probably that's why it goes unnoticed), because REST has not the ability to access one single property of an instance, but when you go forward REST and want to implement some more complex logic, then you must face the problem.

I already commented #3293. Hope you can do anything to rate my PR. Otherwise I'll have to maintain my own loopback fork :(

BTW #3293 only adds a necessary break that #3618 does too.

[bajtos]

@supermafete thank you for the explanation.

BTW #3293 only adds a necessary break that #3618 does too.

Great! So let's do this in baby steps: land #3293 first, then review and eventually land #3618 too.

[supermafete]

@bajtos great!

3618 has some additional commits for custom use of that fork. I don't know if you need something from my side. Anyway you can tell me whatever you need.

[princecharmx]

@bajtos @supermafete , is the fix available in latest repo? whats the status here.

[princecharmx]

@supermafete @bajtos , is loopback being actively devleoped or its getting obsolete? No responses from anyone?

[supermafete]

Hi Patrik,

Yes is actively developed, but I don't know why this issue is not being attended.

Regards,

2017-11-14 19:33 GMT+01:00 Pratik Chopra notifications@github.com:

@supermafete https://github.com/supermafete @bajtos https://github.com/bajtos , is loopback being actively devleoped or its getting obsolete? No responses from anyone?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/strongloop/loopback/issues/3518#issuecomment-344353843, or mute the thread https://github.com/notifications/unsubscribe-auth/ABtJ-_bMzxUramZ6J2siAyKpyZhW45Anks5s2dz-gaJpZM4OjpwO .

[supermafete]

BTW, no, it's not merged in the latest versions.

[princecharmx]

@supermafete , thanks for responding. If you are in direct touch with the core team, please do reach out as this is big security loophole for loopback to be termed as enterprise friendly

[supermafete]

@princecharmx I'm not with direct touch with the core team. I'm just an external developer implementing Loopback solutions. I strongly think that this issue must be reviewed asap, as it concerns to security. The fact is probably nobody are noticing it because it only happens implementing property level security, and it's unnoticed using REST.

[bajtos]

Hello, sorry for the silence. Our maintainers team is small, unfortunately we don't have capacity to address all issues ourselves. Patches are welcome, we are prioritizing our support for people contributing pull requests over people reporting issues.

As for the pull request https://github.com/strongloop/loopback/pull/3618, the comment https://github.com/strongloop/loopback/pull/3618#issuecomment-332377535 summarizes pretty well what's needed to move that work forward:

There's no test cases here. Without one, we can't see if this is making any difference or not. Also the build is not passing and commits need to be cleaned up.

[supermafete]

@bajtos All right. I will try my best to make a new PR, but I have no idea how to make a test case. Could you give me a rough guide?

[bajtos]

@supermafete

Please don't open a new pull request, just add more commits to your existing one. As for tests, take a look at https://github.com/strongloop/loopback/pull/3293, I think it's a good example you can follow.

[supermafete]

@bajtos Ok, I'll do my best on further days.

[princecharmx]

@supermafete , @bajtos , this problem is only with custom access token. I reverted to loopback AccessToken model than it started working as intended. Also, @supermafete , I tried to apply fixes you suggested but that did not solve my problem. I would like to know if you are also using custom access token in your project?

[supermafete]

@princecharmx I'm not using custom access tokens.

[stale[bot]]

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.

[joshrickert]

I'm having this issue as well, so I'll add my report so stalebot doesn't close it. I'm also implementing property-level security on my models.

Same issue as above, where with this ACL:

{
    "principalType": "ROLE",
    "principalId": "$everyone",
    "permission": "DENY",
    "property": "*"
},
{
    "principalType": "ROLE",
    "principalId": "$authenticated",
    "permission": "ALLOW",
    "accessType": "READ",
    "property": "id"
},
{
    "principalType": "ROLE",
    "principalId": "$owner",
    "permission": "ALLOW",
    "accessType": "READ",
    "property": "id"
}

and calling this in my tests:

checkPermission(ACL.ROLE, '$everyone', 'Customer', 'id', ACL.READ)

yields an ALLOW permission and the following debug output:

loopback:security:acl The following ACLs were searched:  +16s
loopback:security:acl ---ACL--- +0ms
loopback:security:acl model Customer +0ms
loopback:security:acl property id +0ms
loopback:security:acl principalType ROLE +0ms
loopback:security:acl principalId $owner +1ms
loopback:security:acl accessType READ +0ms
loopback:security:acl permission ALLOW +0ms
loopback:security:acl with score: 8144 +0ms
loopback:security:acl ---ACL--- +0ms
loopback:security:acl model Customer +0ms
loopback:security:acl property id +1ms
loopback:security:acl principalType ROLE +0ms
loopback:security:acl principalId $authenticated +0ms
loopback:security:acl accessType READ +0ms
loopback:security:acl permission ALLOW +0ms
loopback:security:acl with score: 8136 +0ms
loopback:security:acl ---ACL--- +0ms
loopback:security:acl model Customer +1ms
loopback:security:acl property * +0ms
loopback:security:acl principalType ROLE +0ms
loopback:security:acl principalId $everyone +0ms
loopback:security:acl accessType * +0ms
loopback:security:acl permission DENY +0ms
loopback:security:acl with score: 7495 +1ms

[joshrickert]

I dug around in the code and tried out some variations of @supermafete's PR. I still wasn't able to get all of my app's tests passing (although more of them passed). I noticed that ACL.checkAccessForContext suffered from a similar issue when the principal was a dynamic role.

It seems like one big thing that is missing is the idea that some roles are supersets of others. So if I grant a permission to $authenticated or $related, I would also expect $owner to pass that access check.

While reading the source, I was wondering whether the AccessRequest class ought to include the principal so that separate arguments aren't necessary. I also think the checkPermission and checkAccessForContext are eligible for some refactoring because they duplicate a lot of functionality between them. However, I experimented with these changes on a local clone of the repo and found that it opened a can of worms by causing a lot of the existing tests to start failing when I used $everyone as a safe default for the AccessRequest.

I was able to find a workaround for this whole issue, at least for my use case. Using a USER as the principalType in checkAccessForContext granted permissions correctly according to my ACLs utilizing dynamic roles. So I adapted my tests to use dummy users; I just won't check a dynamic role's access directly in my application.

[bajtos]

Thank you @joshrickert for looking into this issue. I am afraid I don't have enough knowledge about our authentication layer to be able to meaningful help you. Perhaps @raymondfeng or @ebarault could provide some guidance here?

[stale[bot]]

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[bot]]

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.