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

Simplify logic for revealing Tenants that a Principal has access to #1162

Open
Snarr opened this issue Nov 25, 2024 · 3 comments
Open

Simplify logic for revealing Tenants that a Principal has access to #1162

Snarr opened this issue Nov 25, 2024 · 3 comments
Assignees
Labels
clients Affects all LH clients and the public API. May be used in conjunction with `server`. controversial Requires discussion from the community. For proposals, API changes, and big features. feature Issue that denotes a new feature, request, or performance improvement. server Affects the core LH Server code.

Comments

@Snarr
Copy link
Member

Snarr commented Nov 25, 2024

Currently, revealing the Tenants that a Principal has access to requires parsing through the Principal object and conditionally performing an additional RPC request. This issue documents that complex flow and proposes possible solutions to simplify the user experience.

Current Flow

  1. Perform RPC WhoAmI or RPC GetPrincipal request
  2. Parse through the Principal object returned:
  3. If Principal has GLOBAL_ACLS to perform READ action over TENANT resource:
    (3.5): Perform RPC SearchTenant request and use all TenantIds returned in request
  4. Else if Principal has PER_TENANT_ACLS:
    (4.5): Parse through the keys of the PER_TENANT_ACLS in the object and use the TenantIds returned in request

Alternatives

If LittleHorse users find themselves implementing this flow very often, we should be considerate and supply an out-of-the-box solution as an alternative to this complex flow.

Here are some alternative ideas:

1. Add an RPC ListTenantsForPrincipal

We add an RPC that takes in a PrincipalId and returns a list of TenantIds. The server handles all of this logic internally for discovering what Tenants a Principal has access to.

Pros:

  • Reduces the flow from 6 steps to 2 steps on the client's side
  • Reduces the number of RPC calls being made to the server, as the server can handle step 4 internally

Cons:

  • Replaces a client-side solution with a server-side solution, adding complexity to the server and another RPC that we would need to maintain

This could be reduced to 1 step if the RPC behaves as RPC ListTenantsForCurrentPrincipal and infers Principal similar to RPC Whoami

2. Add an SDK method ListTenantsForPrincipal

We add an SDK method to each of our SDK libraries that takes in a PrincipalId and returns a list of TenantIds. The SDK method bundles the same steps listed above into a single method, abstracting away the messy flow.

New Flow:

  1. Call ListTenantsForPrincipal(principalId) method

Pros:

  • Reduces the flow from 6 steps to 1 step in the user's codebase
  • Does not increase complexity on the server side

Cons:

  • Still uses two RPC calls
  • Adds complexity for maintainers of the SDKs, as the same logic would be written across many different codebases. Much more room for error.

3. Change the implementation of RPC SearchTenant

Currently, RPC SearchTenant can only be called by Principals with GLOBAL_ACLS to perform theREAD action over the TENANT resource (or higher permissions).

This solution proposes that we refactor RPC SearchTenant to be used by any Principal that wants to know what Tenants it has access to.

Principals would perform an RPC SearchTenant request and the server would return all of the Tenants that Principal has permissions over.

Pros:

  • Most intuitive functionality for users
  • Does not add any RPCs or SDK methods

Cons:

  • Changes our entire policy for restricting Cluster-Scoped Resource access to admin Principals
  • Adds complexity to the server by requiring Tenants to be filtered by Principal
@Snarr Snarr added feature Issue that denotes a new feature, request, or performance improvement. server Affects the core LH Server code. controversial Requires discussion from the community. For proposals, API changes, and big features. clients Affects all LH clients and the public API. May be used in conjunction with `server`. labels Nov 25, 2024
@hazimoarafa
Copy link
Member

This is going to be required for the dashboard to have 100% accurate info on which tenants the principal has access to.

@Snarr
Copy link
Member Author

Snarr commented Nov 26, 2024

@HazimAr can the dashboard not follow the current 6-step flow outlined above? Or would that flow not respond back with accurate info? Curious to hear more about how this current flow affects the dashboard, because that may make this issue go from low to high priority.

@coltmcnealy-lh coltmcnealy-lh added this to the LittleHorse 1.0 milestone Feb 19, 2025
@coltmcnealy-lh
Copy link
Member

My vote here is for option 3. If a Principal alice has access to tenant-1 and tenant-2, there's no reason why they should have to "guess" what Tenants are available. rpc SearchTenant should show that information to them. The alice Principal doesn't need to know that tenant-3 and tenant-4 and so on even exist.

If I have global acl's over the ACL_TENANT resource, then I should (of course) be able to see all tenants.

Basically, what I'm proposing is that we make a special exception for rpc SearchTenant and do the following:

  • Every Principal can do rpc SearchTenant.
  • If you don't have ACL_TENANT global acl's, it returns only the Tenants that you explicitly have access to.
  • If you do have ACL_TENANT global acl's, it returns all the Tenants.

There might be a good reason not to do it this way though. Let's see if the community has any feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clients Affects all LH clients and the public API. May be used in conjunction with `server`. controversial Requires discussion from the community. For proposals, API changes, and big features. feature Issue that denotes a new feature, request, or performance improvement. server Affects the core LH Server code.
Projects
None yet
Development

No branches or pull requests

4 participants