sungwy commented on PR #4356: URL: https://github.com/apache/polaris/pull/4356#issuecomment-4904750405
> Thanks for this. Read against the #3779 arc, the shape is right. The request side now says _what_ to authorize as plain intent, which is what lets an authorizer participate without inheriting Polaris RBAC. That was #3760's stated aim, and this PR takes it live at the call sites. > > The piece I'd settle before the decision path cuts over: the request became implementation-neutral, but the state object didn't. `AuthorizationState` still hands every authorizer a `PolarisResolutionManifest`, and each one just calls `resolveAll()` on it, so an authorizer that doesn't want Polaris-native resolution still has to accept and drive it to satisfy the contract. The intent half lets an authorizer bring its own machinery; the state half still requires the native one. Since the point of the series is to let the authorizer decide whether native resolution happens at all, I'd make that state opaque or optional before phases 5 and 6 lock the contract in. > > Two smaller things in the same spirit: > > * `updateTable` resolves with a placeholder operation to read catalog config, then authorizes a different set. It's fine today, but it quietly assumes resolution is operation-independent, which your own comment flags. That assumption is part of the contract now, so it belongs in the `resolveAuthorizationInputs` javadoc rather than a TODO. It's exactly what a selective authorizer would break. > * The empty-namespace rejection turns a 404 into a 400 on public catalog paths (`commitTransaction`, `renameTable` source), not just admin grants, and the new mapper applies the guard unevenly (`policy()` is missing it). The 400 is the right call; the CHANGELOG just scopes it too narrowly. > > Direction's good. The contract is easiest to get neutral now, while the new decision API is still dormant at the call sites. Hi @flyingImer - thanks for the review. Are these blocking concerns in your opinion? I'm looking to do one more pass to incorporate the consensus I've been able to reach with @dimas-b @flyrain on the `AuthorizationState`. If these aren't blocking concerns, I'd like to move this PR forward -- 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]
