Authorizer: expose the required level of detail

[snazy]

Adds informative functions for PolarisAuthorizer call sites whether principal roles, catalog roles and resolved entities are required.

This change allows call sites to skip certain lookups against the backend database for information that's not needed for authorizers.

For example the OPA authorizer neither needs roles nor any grant information from ResolvedPolarisEntity.

This change only adds the informative functions to Authorizer but does not add any optimization to the call sites.

[singhpk234]

can we have public docs for this ?

Also to avoid breaking down stream service can we have default values as true ? downstream impl of this interface can override it

[snazy]

I think the whole interface could need some more docs. It took me quite a long time to understand that e.g. Set<PolarisBaseEntity> activatedEntities is a set of principal and catalog roles. Historically, that parameter was Set<Long> activatedGranteeIds.

Can you clarify what docs you'd like to see? Mean, "requiresXyz" pretty much tells what the implementation expects/needs in the calls to authorizeOrThrow.

I suspected that it's clear now, at least for the custom downstream implementations, that activatedEntities is

Honestly, I think this interface should be entirely replaced to untangle the hard coupling with Polaris internal roles and Polaris internal RBAC.

[dimas-b]

upd: default methods are in the latest commit on this PR

[singhpk234]

I wonder this comes under the use case Delegated AuthZ, should we have another interface for it entirely to undertand whats required for delegation ?

[snazy]

Nothing to do with delegation. Just to get rid of unnecessary work to get data (principal/catalog roles, grant records) that some implementations just do not need.

[dimas-b]

@singhpk234 : I'm not sure what you mean by "delegated" AuthZ 🤔 The PolarisAuthorizer interface has multiple implementations, but I do not think this is "delegation" per se... Maybe I'm missing some context.

As far as the different (two in OSS) implementations of PolarisAuthorizer are concerns, they have different requirements on the amount of input data they require for making AuthZ decisions. These new interface methods are intended (as far as I understand) to allow other Polaris classes to perform less work, if some input data is not needed by a particular authorizer.

I suppose there will be other related code changes that expose the benefits of the new methods added in this PR.

[singhpk234]

I'm not sure what you mean by "delegated" AuthZ 🤔 The PolarisAuthorizer interface has multiple implementations, but I do not think this is "delegation" per se... Maybe I'm missing some context

I see, my understanding the reason why we don't use the principalRoles / catalogRoles etc as the grants etc are managed by external system which is OPA here which gives us an AuthZ decision (YES | NO) hence the Authorization is delegated here as otherwise PolarisAuthZ would have required these stuff as grants etc are done based on that. This is what i had in mind, in context of above so here is what my thought process was considering the code change :

public interface SupportsDelegatedAuthZ { 
  default boolean requiresPrincipalRoles() {
    return false;
  }

  default boolean requiresCatalogRoles() {
    return false;
  }

   default boolean requiresResolvedEntities() {
    return false;
  } 
 }

OPAAuthZ implements PolarisAuthZ, SupportsDelegateAuthZ {
  // overrides
}

THis way persistence who wanna optimize to not look up prinicipal role and catalog role can first check SupportsDelegateAuthZ and skip the look ups accordingly.

Again i am thinking out loud here !

[snazy]

Short and precise: instanceof is a no-go in CDI as it doesn't work. It would prevent making PolarisAuthorizer request scoped in the future.

The long-term solution IMO is to have a different API/SPI structure for these things, but I do not have thought about it at all.

[dimas-b]

@singhpk234 : Thanks for clarifying the meaning of "delegated authZ" in this context. I think I understand what you mean, although, I would not personally use the term "delegation" here as in the content of Auth N/Z it has a different meaning usually (acting on behalf of a different identity).

Re: SupportsDelegatedAuthZ the implementation you propose is mostly equivalent to the state of the code in this PR, IMHO.

However, there is a downside - as @snazy mentions, the callers would have to perform instanceof checks on the PolarisAuthorizer instances, which will complicate the code on the caller side. Plus, CDI will become much harder, because injection normally guarantees only the declared type of the variable in runtime. Sub-classing is not guaranteed to be exposed in injected objects in CDI... Sub-classes may be exposes "as is" or they may not be (e.g. proxies may be injected). All in all, this approach increases code complexity.

If you have strong reasons to avoid introducing even byte-code compatible changes to PolarisAuthorizer (as in this PR). I'd proposed to move the methods from SupportsDelegatedAuthZ into a (new) AuthorizationHints interface to be produced from PolarisAuthorizerFactory and used at call sites that can optimize execution based on the information the new interface provides.

@snazy @singhpk234 : WDYT?

[singhpk234]

strong reasons to avoid introducing even byte-code compatible changes

TBH i don't have strong concerns for introducing byte-code compatibility changes, my main consideration were the version upgrade should be smooth and the API should well documented so the downstream users can override it accordingly which i think is already addressed in the recent commit so I am good from that POV as this is definitely an integration point.

I'd proposed to move the methods from SupportsDelegatedAuthZ into a (new) AuthorizationHints interface to be produced from PolarisAuthorizerFactory and used at call sites that can optimize execution based on the information the new interface provides.

I leave this to the your better judgement ! I don't have a strong for or against opinion on this ! since the concerns i had are already addressed.

cc @HonahX can you also please weigh in on this discussion

[dimas-b]

If adding default methods to the existing interface is not a concern, my preference would be to go with the current state of this PR 🙂

[dimas-b]

@snazy : WDYT about holding this PR until some progress is made on #3250 ?

[snazy]

@snazy : WDYT about holding this PR until some progress is made on #3250 ?

No strong objections. However, given that a bunch of code (mostly Resolver and the resolution-manifest and all the use/call sites) rely on e.g. ResolvedPolarisEntity I'm not sure whether we can go with just one approach. Mean, we do not need entities and with that no "resolved entities" (aka entities plus grant records) in the "external" cases. The idea here was to allow "declarative skipping" of the unnecessary invocations as a short-term workaround before doing a bigger refactoring.

[dimas-b]

@snazy : I totally agree with your assessment. I just wanted to coordinate with @sungwy 's work since some call paths in Resolver related to this PR are probably related to #3250 too... and I already asked @sungwy to try and avoid producing (resolving) entities for principals when not required 😅

[snazy]

Yup, the idea is that the Resolver checks the attributes introduced by this PR.

[sungwy]

Hi @snazy - I took a look at this PR again after yesterday's discussion, and I now fully understand your point 😃

I'm completely in agreement with your approach. I think the question of whether the additional ResolvedPolarisEntity grant information needs to be fetched and used in a authorization request should be coupled together with the mode of authorization (PolarisAuthorizer) instead of relying on a separate parameter.

I am curious to hear your thoughts on the following two questions:

1. Do we think there's a need to introduce three separate flags for requiresPrincipalRoles, requiresCatalogRoles, requiresResolvedEntities, as opposed to having a single one like: requiresResolvedPolarisEntity - in the existing use cases, they seem to be coupled together
2. What are your thoughts on an alternate approach of pushing the Suppliers of these entities into the PolarisAuthorizer for lazy evaluation instead, as opposed to exposing public boolean flags? This way, we'd remove a conditional evaluation within the Handlers, and instead push the responsibility of deciding what context is needed and whether we should fetch it only into the PolarisAuthorizer. It'll be a more complicated refactoring than what's proposed in the PR, but I wanted to hear your thoughts on it

[dimas-b]

Based on the discussion about "detached" principals, from my POV I guess one coarse flag should be sufficient. That is, if Polaris is the RBAC engine (default), then all Principal Entities, Principal Roles, Catalog Roles need to be resolved. Otherwise, none of them need to be resolved.

@snazy @sungwy @collado-mike : WDYT?