snazy commented on PR #3228: URL: https://github.com/apache/polaris/pull/3228#issuecomment-3714586128
> 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 `Supplier`s 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 I think that different authorizers have different needs, hence I added the 3 flags there. * "requires resolved entities" - that's purely for `PolarisAuthorizerImpl` * "requires principal roles" + "requires catalog roles" are things that other authorizer implementations may or may not need, one may only require the prinicipal roles while another also the catalog roles. I didn't intent to this approach "into stone", but rather as a stop-gap solution until we've got something to isolate authorizer concerns (aka: moved the authorization logic in the "resolver" to the authorizer implementation(s)). Lazy suppĺiers could make the logic more complex, and potentially slower when multiple entities are involved (multiple distinct invocations preventing "batch" reads?). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
