OPL: Incorrect Evaluation of Permissions in Ory Keto

[LukaGiorgadze]

Preflight checklist

• I could not find a solution in the existing issues, docs, nor discussions.
• I agree to follow this project's Code of Conduct.
• I have read and am following this repository's Contribution Guidelines.
• I have joined the Ory Community Slack.
• I am signed up to the Ory Security Patch Newsletter.

Ory Network Project

No response

Describe the bug

When evaluating user permissions in Ory Keto, a discrepancy occurs based on the order of user group checks. When a user is only in the "owners" group, the expected behavior is to return True for the itemView perm. However, when the line this.related.owners.includes(ctx.subject) is moved up in the code block, while keeping the rest of the logic identical, the function incorrectly returns False. This issue arises despite the fact that the code remains technically the same.

This discrepancy manifests when incorporating a check for blocked users, denoted by !this.related.blockedUsers.includes(ctx.subject) && (). The unexpected behavior undermines the intended functionality of permission evaluation and could potentially lead to incorrect access control decisions.

class User implements Namespace {}

class LegalEntityUserGroup implements Namespace {
  related: {
    // predefined roles
    owners: User[];
    administrators: User[];
    creators: User[];
    viewers: User[];
    members: User[];
  };
}

class BlockedUserGroup implements Namespace {
  related: {
    blockedUsers: User[];
  };
}

class LegalEntity implements Namespace {
  related: {
    // pre-deifned roles
    owners: SubjectSet<LegalEntityUserGroup, "owners">[];
    administrators: SubjectSet<LegalEntityUserGroup, "administrators">[];
    creators: SubjectSet<LegalEntityUserGroup, "creators">[];
    viewers: SubjectSet<LegalEntityUserGroup, "viewers">[];
    members: SubjectSet<LegalEntityUserGroup, "members">[];
    blockedUsers: SubjectSet<BlockedUserGroup, "blockedUsers">[];
  };

  permits = {
    own: (ctx: Context): boolean =>
        !this.related.blockedUsers.includes(ctx.subject) &&
        this.related.owners.includes(ctx.subject),

    itemView: (ctx: Context): boolean =>
      !this.related.blockedUsers.includes(ctx.subject) &&
      (this.related.administrators.includes(ctx.subject) ||
      this.related.creators.includes(ctx.subject) ||
      this.related.viewers.includes(ctx.subject) ||
      this.related.owners.includes(ctx.subject) || // ⚠️ move this line up and it it won't work.
      this.related.members.includes(ctx.subject)),

  };
}

Reproducing the bug

1. Add User in LegalEntityUserGroup as owner use object legalentity_123;
2. Relate LegalEntityUserGroup to LegalEntity
3. Send request:

./check
?namespace=LegalEntity
&relation=itemView
&object=legalentity_123
&subject_set.namespace=User
&subject_set.object=user_123
&subject_set.relation=

Relevant log output

No response

Relevant configuration

No response

Version

v0.11.1-alpha.0

On which operating system are you observing this issue?

macOS

In which environment are you deploying?

Docker Compose

Additional Context

No response

[rbnbr]

I encountered a similar problem when using hierarchical checks (a if permit c, c if permit d, d if ID in owners).
My hot-fix solution for now was to remove all hierarchical permits and replace them: (a if ID in owners, c if ID in owners, d if ID in owners).
Definitely not pretty but now it's working as expected.

FYI: There was no '!' in any of the OPL rules, so my issue was not related to that.

[hperl]

Hi, and thanks for the report. Unfortunately I could not reproduce this yet. Please see my attempt here:

f430843

I get the correct check result as per the instructions you posted, but I failed to rearrange the expressions such that the check would be false.

Can you post exactly how you modified the OPL?

[hperl]

@rbnbr please also provide a reproducer of the bug you discovered. A PR with a failing test would be highly appreciated (they are quite nice to write if you know Go), but I can also follow your instructions if you post the OPL, tuples and checks. Big thanks in advance!

[rbnbr]

@hperl I'll take a look after work and try to produce the bug again and come up with the PR as requested.
I hope it wasn't a failed configuration from my side.
Anyway, thanks for looking into that.

Also since you're here, I'd like to note that the "!" Is not officially supported by the documentation afaik https://www.ory.sh/docs/keto/reference/ory-permission-language
It only states the "&&" and "||" operators. Did I miss it?

[hperl]

Also since you're here, I'd like to note that the "!" Is not officially supported by the documentation afaik https://www.ory.sh/docs/keto/reference/ory-permission-language It only states the "&&" and "||" operators. Did I miss it?

You're right! Fix is here: #1530

[rbnbr]

So I was able to recreate the (what I believe to be) bug.
It doesn't seem to be related to the expression ordering stated above but is rather some traversal/transitive permissions issue?

I added a pull request but the tests couldn't be properly executed on my machine. Maybe I used the wrong commands?

Using the OPL as described in this file with the following tuples:

NAMESPACE       OBJECT  RELATION NAME   SUBJECT
SuperUsers      super   admins          Role:admin#members
Group           g       supers          SuperUsers:super
Role            admin   members         User:u
Comment         c       parents         Group:g

The following is returned when checked with the CLI:

> keto check User:u update Comment c --insecure-disable-transport-security
Denied

> keto check User:u delete Comment c --insecure-disable-transport-security 
Allowed

Which is (imo) weird since the update is defined as being permitted if the delete is permitted too see:

permits = {
        delete: (ctx: Context): boolean => this.related.parents.traverse((group) => group.permits.update(ctx)),
        update: (ctx: Context): boolean => this.permits.delete(ctx),
    }

[hperl]

Thanks @rbnbr for the testcase! The behaviour you encountered comes from the fact that the default depth (the number of hops we take to check access) is 5, which is exactly one too short for correctly checking delete. I updated the test case in your PR to assert that the check is indeed the correct result if the depth is >= 6.

@zepatrik maybe we should handle "depth depletion" in a more obvious way, i.e. by returning an error, or even better by returning the queries where the check stopped, so that the check can be resumed from those queries. This would then work like tree traversal pagination. WDYT?

@LukaGiorgadze, is your issue maybe also related to the read depth limit?

[rbnbr]

Thank you, @hperl, for looking into that. I also thought the depth might be the issue, though; when I added the -d parameter with, e.g., -d 30 to the CLI call, it did not change the result for me. Is the CLI parameter not parsed correctly?

So, the results with the CLI do not change if I add the depth parameter:

keto check User:u update Comment c --insecure-disable-transport-security -d 30
Denied

keto check User:u delete Comment c --insecure-disable-transport-security -d 30
Allowed

[hperl]

You also need to set the max in the configuration.

[rbnbr]

Oh, right. Great! Thanks for pointing it out, I missed that part. So it's no bug after all then :)