Implement Finer Grained Operations and Privileges For Update Table

[travis-bowen]

This implements finer grained operations and privileges for update table in a backwards compatible way as discussed on the mailing list.

(Seems the mailing list split the thread in two but it's here and here)

The design doc is here

The idea is that all the existing privileges and operations will work and continue to work even after this change. (i.e. TABLE_WRITE_PROPERTIES will still ensure update table is authorized even after these changes).

However, because Polaris will now be able to identify each operation within an UpdateTable request and has a privilege model with inheritance that maps to each operation, users will now have the option of restricting permissions at a finer level if desired.

[HonahX]

Thanks for working on this! @travis-bowen

I am +1 on the idea of more fine grained commit actions. Left some initial comments. Let me know what you think!

[HonahX]

Overall LGTM! Thanks for adding this and updating! I have some small comments about the spec and test, please let me know WDYT?

[collado-mike]

We should verify this with @dennishuo . This is really only so we can control the feature flag at the catalog-level, right? If we control it at a system level, we don't need this? Personally, I think it's fine to control at the realm level and if it means we don't have to muck with the entity resolution for authz, I think that would be better.

[travis-bowen]

So yes - it is only to be able to control at the catalog level. It was a suggestion from Dmitri here - #2697 (comment)

I think it's okay - if not better performant to do it this way. Before we would have created and resolved the manifest for each operation, which is already required. Now, we just resolve it once here and then get the benefit of being able to have this param be controllable at the catalog level (although it's hard to know why someone would want this controllable at the catalog level).

But happy to review this with @dennishuo as well as I already was going to get him to review.

[HonahX]

How about we just creating another helper in icebergCatalogHandler, like

authorizeUpdateTableOperationOrThrow(
UpdateTableRequest request,
PolarisEntitySubType subType,
TableIdentifier identifier,
) {
// add path, resolve like normal

// get catalog entity and verify the config

getUpdateTableAuthorizableOperations(request);

for (PolarisAuthorizableOperation op : ops) {
      authorizer.authorizeOrThrow(
          polarisPrincipal,
          resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
          op,
          target,
          null /* secondary */);
    }
}

So we do not have to resolve twice. It will indeed create some duplicated code, but that could be solved in future refactoring and is IMHO preferred than resolve path twice (assuming in some case EntityCache is not available)?

[travis-bowen]

So, went ahead and kept things as they are currently.
There shouldn't be any risk of resolving the path twice because I updated even the CatalogHandler to use ensureResolutionManifestForTable which only resolves if it's null.

Let me know if I might have missed something though.

[dennishuo]

Forgot to respond here. Yeah the control flow extracting the ResolutionManifest initialization looks good to me, and I agree it looks like it won't do any extra persistence resolution calls.

I guess conceptually the generation of the ResolutionManifest doesn't need to be strictly inlined in the authorize helper, even though it's intentionally coupled a bit because the intent is that at least the filled-in ResolutionManifest shouldn't "escape" to be used for any real actions before authz has basically "approved" the manifest.

At some point we might want to attach some counters to our ResolutionManifests and/or persistence impls so that in tests we can easily add regression tests to ensure we don't accidentally add extraneous persistence calls.

[dennishuo]

Overall LGTM, just a comment on a test case where the comment didn't seem to match the intent, but I could've been reading it wrong if it's indeed working as intended.

[HonahX]

Seems all comments have been addressed and there's also no objection in the dev mailing list. Merging

Thanks @dimas-b @collado-mike @dennishuo for reviewing!

[singhpk234]

Thank you for the change @travis-bowen ! would be really nice to add this in the CHANGELOG.md file too, to help users try this out :)
Good news this will be part of 1.2 ! was just about to do rc cut today

[sungwy]

I was just thinking about this feature today - thank you so much for working on this PR @travis-bowen ! This is such an awesome enhancement to the list of operation types that can be used for authorization decisions on Polaris.