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

OPL: support type unions with traversal #1319

Open
3 of 6 tasks
zepatrik opened this issue May 3, 2023 · 4 comments
Open
3 of 6 tasks

OPL: support type unions with traversal #1319

zepatrik opened this issue May 3, 2023 · 4 comments
Labels
feat New feature or request.

Comments

@zepatrik
Copy link
Member

zepatrik commented May 3, 2023

Preflight checklist

Describe your problem

class File implements Namespace {
  related: {
    owners: (User | Group)[];
  };  permits = {
    view: (ctx: Context): boolean =>
      this.related.owners.includes(ctx.subject) ||
      this.related.owners.traverse((p) => p.permits.view(ctx)) // error throws here because user doesn't have a permit
  };
}

errors with

Failed to parse OPL config files at target file:///etc/config/keto/keto_namespaces.ts. audience=application error=map[message:error from 90:19 to 90:25: relation "view" was not declared in namespace "User"

Describe your ideal solution

We could either add a type-guard to the namespaces, or support instanceof with either the ternary operator or just with boolean branching.

Workarounds or alternatives

As a workaround one can split the relation:

class File implements Namespace {
  related: {
    owners_user: User[]
    owners_group: Group[]
  };
  permits = {
    view: (ctx: Context): boolean =>
      this.related.owners_user.includes(ctx.subject) ||
      this.related.owners_group.traverse((p) => p.permits.view(ctx)) // error throws here because user doesn't have a permit
  };
}

Version

lastest

Additional Context

No response

@zepatrik zepatrik added the feat New feature or request. label May 3, 2023
@bendoerr
Copy link

Agree, I just ran into this as well.

@PrimeDominus
Copy link

Would be useful to have this fixed to make things much less confusing.

@davidspek
Copy link

That error looks correct to me. Shouldn’t the config be owners: (User | SubjectSet<Group, "members">)[] and then the relation can simply be this.related.owners_user.includes(ctx.subject).

Unless I’m misunderstanding something here (since the Group namespace is undefined and relevant here), when you traverse the parent you can use permissions or relations defined on the parent.

@cmmoran
Copy link

cmmoran commented Jan 10, 2024

What if made a slight change to the OPL definition to include type intersections? It may sound silly at first, but this isn't real typescript; not really real typescript anyway. Something like this:

class File implements Namespace {
  related: {
    owners: (User & Group)[];
  }

  permits = {
    view: (ctx: Context): boolean =>
      this.related.owners.traverse((p) =>
        p.equals(ctx.subject) ||
        p.permits.view(ctx)
      )
  }
}

That would solve the visual aspect of the OPL showing an error where one does not necessarily exist. In order to get this to work under-the-hood, the parser would need to support it. But even before that a decision would need to be made:

  • Can type intersections and unions exist for a particular relation?

I feel that, while it would work and the parser could be updated to support this, it would get complicated very quickly. Every mixed type union/intersection would need some fairly complicated typechecking logic to properly check that the type(s) fulfill their end of the contract. The other option is to disallow it. Part of me feels like that wouldn't be right either as it would mean the parser would have to track another state for each namespace relation and if the relation is a type union or type intersection, mark the relation accordingly. Non union/intersection relations wouldn't be affected.

If type unions and type intersections are made mutually exclusive, the parser typechecks would also need to change. Right now, under the union model, a typecheck ensures that the relation exists on all namespaces for a type union. This is necessary since we can look at the type union as if it were "implementations of an interface"; all members of the type union must uphold the "contract". This would change for type intersections because only one of the namespaces of the type intersection would need to uphold the contract in order for the check to be resolved.

If type unions and type intersections can exist on a single relation, the typechecks would still need to change but it would be more complicated because the how of the relation checks would depend on which namespaces are part of a type intersection and which namespaces are part of the type union for every relation.

This leads to another question:

  • What are the side-effects of introducing this?

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

No branches or pull requests

5 participants