You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "sungwy (via GitHub)" <gi...@apache.org> on 2026/07/30 13:01:43 UTC
[PR] Draft: update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
sungwy opened a new pull request, #5194:
URL: https://github.com/apache/polaris/pull/5194
<!--
๐ Describe what changes you're proposing, especially breaking or user-facing changes.
๐ See https://github.com/apache/polaris/blob/main/CONTRIBUTING.md for more.
-->
In Draft, update the handler call sites to use the new `authorize` method using `AuthorizationIntent`.
This will allow each `PolarisAuthorizer` implementations to decide which `PolarisEntity` needs to be resolved for its authorization decision, allowing them to ignore RBAC resolution by choice.
## Checklist
- [ ] ๐ก๏ธ Don't disclose security issues! (contact security@apache.org)
- [ ] ๐ Clearly explained why the changes are needed, or linked related issues: Fixes #
- [ ] ๐งช Added/updated tests with good coverage, or manually tested (and explained how)
- [ ] ๐ก Added comments for complex logic
- [ ] ๐งพ Updated `CHANGELOG.md` (if needed)
- [ ] ๐ Updated documentation in `site/content/in-dev/unreleased` (if needed)
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "flyingImer (via GitHub)" <gi...@apache.org>.
flyingImer commented on code in PR #5194:
URL: https://github.com/apache/polaris/pull/5194#discussion_r3882597258
##########
extensions/auth/opa/src/test/java/org/apache/polaris/extension/auth/opa/OpaPolarisAuthorizerTest.java:
##########
@@ -838,9 +864,7 @@ void authorizeResolvedCatalogTargetPreservesRootParent() throws Exception {
{
"type": "CATALOG",
"name": "catalog-1",
- "parents": [
- {"type": "ROOT", "name": "root_container", "parents": []}
- ]
+ "parents": []
}
""");
assertThat(target).isEqualTo(expectedTarget);
Review Comment:
This test makes the GET_CATALOG payload change explicit to me: the ROOT parent is no longer sent. The same migration also removes the ROOT target from LIST_CATALOGS and splits collection requests into one OPA query per intent, **but I don't see those changes in the current CHANGELOG**. Since existing Rego policies may depend on these shapes, could we document the full set under Breaking changes before merging?
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java:
##########
@@ -319,26 +300,20 @@ protected void authorizeResolvedRegisterTableOverwriteOrThrow(
ResolvedPathKey.ofTableLike(identifier), PolarisEntitySubType.ICEBERG_TABLE, true);
if (tableTarget != null) {
- authorizer()
- .authorizeOrThrow(
- polarisPrincipal(),
- resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
- overwriteOp,
- tableTarget,
- null /* secondary */);
+ authorizer().authorize(authorizationState, overwriteRequest).throwIfDenied();
} else {
PolarisResolvedPathWrapper namespaceTarget =
resolutionManifest.getResolvedPath(ResolvedPathKey.ofNamespace(namespace), true);
if (namespaceTarget == null) {
throw noSuchNamespaceException(namespace);
}
- authorizer()
- .authorizeOrThrow(
+ AuthorizationRequest fallbackRequest =
+ new AuthorizationRequest(
polarisPrincipal(),
- resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
- fallbackOp, // normal register-table operation
- namespaceTarget,
- null /* secondary */);
+ List.of(
+ new SingleTargetAuthorizationIntent(
+ fallbackOp, PolarisSecurableMapper.namespace(catalogName(), namespace))));
+ authorizer().authorize(authorizationState, fallbackRequest).throwIfDenied();
Review Comment:
I think this handler flow is inconsistent with the SPI's request-selective resolution model: T
he current resolution request contains only the table overwrite intent, while this branch authorizes a namespace fallback intent; the built-in authorizers mask the mismatch by calling resolveAll().
Could we make it an invariant that every request which may reach authorize() has its required inputs covered by the preceding resolution?
One concrete option would be to construct both intents up front, pass one AuthorizationRequest containing both to resolveAuthorizationInputs, and then authorize the corresponding single-intent request after the table-existence check. A request-selective test for the table-missing branch could verify that the namespace intent was present during resolution.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Draft: update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "flyingImer (via GitHub)" <gi...@apache.org>.
flyingImer commented on code in PR #5194:
URL: https://github.com/apache/polaris/pull/5194#discussion_r3817327357
##########
extensions/auth/opa/src/main/java/org/apache/polaris/extension/auth/opa/OpaPolarisAuthorizer.java:
##########
@@ -187,64 +182,6 @@ public AuthorizationDecision authorize(
return AuthorizationDecision.allow();
}
- /**
- * Authorizes a single target and secondary entity for the given principal and operation.
- *
- * <p>Delegates to the multi-target version for consistency.
- *
- * @param polarisPrincipal the principal requesting authorization
- * @param activatedEntities the set of activated entities (roles, etc.)
- * @param authzOp the operation to authorize
- * @param target the main target entity
- * @param secondary the secondary entity (if any)
- * @throws ForbiddenException if authorization is denied by OPA
- */
- @Override
- public void authorizeOrThrow(
- @NonNull PolarisPrincipal polarisPrincipal,
- @NonNull Set<PolarisBaseEntity> activatedEntities,
- @NonNull PolarisAuthorizableOperation authzOp,
- @Nullable PolarisResolvedPathWrapper target,
- @Nullable PolarisResolvedPathWrapper secondary) {
- authorizeOrThrow(
- polarisPrincipal,
- activatedEntities,
- authzOp,
- target == null ? null : List.of(target),
- secondary == null ? null : List.of(secondary));
- }
-
- /**
- * Authorizes one or more target and secondary entities for the given principal and operation.
- *
- * <p>Sends the authorization context to OPA and throws if not allowed.
- *
- * @param polarisPrincipal the principal requesting authorization
- * @param activatedEntities the set of activated entities (roles, etc.)
- * @param authzOp the operation to authorize
- * @param targets the list of main target entities
- * @param secondaries the list of secondary entities (if any)
- * @throws ForbiddenException if authorization is denied by OPA
- */
- @Override
- public void authorizeOrThrow(
- @NonNull PolarisPrincipal polarisPrincipal,
- @NonNull Set<PolarisBaseEntity> activatedEntities,
- @NonNull PolarisAuthorizableOperation authzOp,
- @Nullable List<PolarisResolvedPathWrapper> targets,
- @Nullable List<PolarisResolvedPathWrapper> secondaries) {
- boolean allowed =
- queryOpa(
Review Comment:
IIUC, this is the path that kept collection authorization in one OPA query with the full target set. CatalogHandler now sends the multi-intent AuthorizationRequest through authorize(), and OpaPolarisAuthorizer issues one query per evaluated intent. A set-aware Rego policy may therefore see different inputs. Is that compatibility change intended in this phase?
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationDecision.java:
##########
@@ -41,4 +42,10 @@ static AuthorizationDecision deny(@Nullable String message) {
@Value.Parameter(order = 2)
Optional<String> getMessage();
+
+ default void throwIfDenied() {
+ if (!isAllowed()) {
+ throw new ForbiddenException("%s", getMessage().orElse("Authorization denied"));
Review Comment:
This message is returned verbatim to Iceberg clients. For OPA, it includes principal and operation iirc, re-exposing fields that #4197 removed from the client response. Could throwIfDenied use a generic ForbiddenException message and log the decision message instead?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "flyingImer (via GitHub)" <gi...@apache.org>.
flyingImer commented on code in PR #5194:
URL: https://github.com/apache/polaris/pull/5194#discussion_r3909244915
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java:
##########
@@ -319,26 +300,20 @@ protected void authorizeResolvedRegisterTableOverwriteOrThrow(
ResolvedPathKey.ofTableLike(identifier), PolarisEntitySubType.ICEBERG_TABLE, true);
if (tableTarget != null) {
- authorizer()
- .authorizeOrThrow(
- polarisPrincipal(),
- resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
- overwriteOp,
- tableTarget,
- null /* secondary */);
+ authorizer().authorize(authorizationState, overwriteRequest).throwIfDenied();
} else {
PolarisResolvedPathWrapper namespaceTarget =
resolutionManifest.getResolvedPath(ResolvedPathKey.ofNamespace(namespace), true);
if (namespaceTarget == null) {
throw noSuchNamespaceException(namespace);
}
- authorizer()
- .authorizeOrThrow(
+ AuthorizationRequest fallbackRequest =
+ new AuthorizationRequest(
polarisPrincipal(),
- resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
- fallbackOp, // normal register-table operation
- namespaceTarget,
- null /* secondary */);
+ List.of(
+ new SingleTargetAuthorizationIntent(
+ fallbackOp, PolarisSecurableMapper.namespace(catalogName(), namespace))));
+ authorizer().authorize(authorizationState, fallbackRequest).throwIfDenied();
Review Comment:
Thanks, the namespace and optional-path explanation clarifies why the current built-ins are safe. This flow first resolves REGISTER_TABLE_OVERWRITE, then uses the same AuthorizationState to authorize namespace-level REGISTER_TABLE when the table is missing. May resolveAuthorizationInputs choose what to resolve from the full AuthorizationRequest, including its operation, or must it depend only on PolarisSecurable and the manifest? For example, could an implementation decide that REGISTER_TABLE_OVERWRITE needs only requested paths while REGISTER_TABLE also needs caller catalog roles; the current Javadoc reads as permitting that, so if selection is intentionally securable-only, could we state that in the SPI and test it?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Draft: update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on PR #5194:
URL: https://github.com/apache/polaris/pull/5194#issuecomment-5260934575
> @sungwy : Is this still a draft PR (title / description)?
I toggling the state while trying to figure out how to unblock the CI that seems to be stuck on this PR ๐
But thank you for the early review @dimas-b . This will help move it closer to the finishline
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Draft: update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5194:
URL: https://github.com/apache/polaris/pull/5194#discussion_r3763292816
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizer.java:
##########
@@ -56,8 +56,10 @@ void resolveAuthorizationInputs(
/**
* Convenience method that throws a {@link ForbiddenException} when authorization is denied.
*
- * <p>Implementations should provide allow/deny decisions via {@link #authorize}.
+ * @deprecated Use {@link #authorize(AuthorizationState, AuthorizationRequest)} and handle the
+ * returned {@link AuthorizationDecision}.
*/
+ @Deprecated(since = "1.2.0")
default void authorizeOrThrow(
Review Comment:
Another point: Keeping deprecated SPI methods in Polaris forces implementations to maintain "dead" code. Proactively removing that code risks that a future Polaris version may want to call the deprecated methods again, which creates uncertainly for implementors.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Draft: update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #5194:
URL: https://github.com/apache/polaris/pull/5194#issuecomment-5397860392
I think the PR is in a good sharp. I proposed to merge it today or tmr if CI is happy and no more further comments.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on PR #5194:
URL: https://github.com/apache/polaris/pull/5194#issuecomment-5574094121
sorry to ask for your approval again @dimas-b - there was a merge conflict on `CHANGELOG.md`. I'll merge it now once the CI turns green given that this PR has been open for a while with a number of approvals.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Draft: update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #5194:
URL: https://github.com/apache/polaris/pull/5194#discussion_r3849039913
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationDecision.java:
##########
@@ -41,4 +45,11 @@ static AuthorizationDecision deny(@Nullable String message) {
@Value.Parameter(order = 2)
Optional<String> getMessage();
+
+ default void throwIfDenied() {
+ if (!isAllowed()) {
+ getMessage().ifPresent(message -> LOGGER.debug("Authorization denied: {}", message));
+ throw new ForbiddenException("Authorization denied");
Review Comment:
I've actually created this [PR](https://github.com/apache/polaris/pull/5370) to hide error details for `PolarisAuthorizer` in all of its existing surface. I don't think there's a valid reason any authorization details should be leaked to the client, and it's probably safe to emit a consistent, opaque log in all of these scenarios.
Clients who receive the error should reach out to their service administrators out of band for debugging and instructions on setting up the write permissions.
cc @dimas-b
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Draft: update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "flyingImer (via GitHub)" <gi...@apache.org>.
flyingImer commented on code in PR #5194:
URL: https://github.com/apache/polaris/pull/5194#discussion_r3856274116
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationDecision.java:
##########
@@ -41,4 +45,11 @@ static AuthorizationDecision deny(@Nullable String message) {
@Value.Parameter(order = 2)
Optional<String> getMessage();
+
+ default void throwIfDenied() {
+ if (!isAllowed()) {
+ getMessage().ifPresent(message -> LOGGER.debug("Authorization denied: {}", message));
+ throw new ForbiddenException("Authorization denied");
Review Comment:
Thanks, this makes sense. I agree that authorization denials should be opaque to clients and AuthorizationDecision.message should be used only for server-side diagnostics. I'll follow up in #5370 on documenting that intent. Assuming #5370 lands before this PR, this thread is addressed from my side.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Draft: update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #5194:
URL: https://github.com/apache/polaris/pull/5194#discussion_r3849039913
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationDecision.java:
##########
@@ -41,4 +45,11 @@ static AuthorizationDecision deny(@Nullable String message) {
@Value.Parameter(order = 2)
Optional<String> getMessage();
+
+ default void throwIfDenied() {
+ if (!isAllowed()) {
+ getMessage().ifPresent(message -> LOGGER.debug("Authorization denied: {}", message));
+ throw new ForbiddenException("Authorization denied");
Review Comment:
I've actually created this PR to hide error details for `PolarisAuthorizer` in all of its existing surface. I don't think there's a valid reason any authorization details should be leaked to the client, and it's probably safe to emit a consistent, opaque log in all of these scenarios.
Clients who receive the error should reach out to their service administrators out of band for debugging and instructions on setting up the write permissions.
cc @dimas-b
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #5194:
URL: https://github.com/apache/polaris/pull/5194#discussion_r3920240674
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java:
##########
@@ -319,26 +300,20 @@ protected void authorizeResolvedRegisterTableOverwriteOrThrow(
ResolvedPathKey.ofTableLike(identifier), PolarisEntitySubType.ICEBERG_TABLE, true);
if (tableTarget != null) {
- authorizer()
- .authorizeOrThrow(
- polarisPrincipal(),
- resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
- overwriteOp,
- tableTarget,
- null /* secondary */);
+ authorizer().authorize(authorizationState, overwriteRequest).throwIfDenied();
} else {
PolarisResolvedPathWrapper namespaceTarget =
resolutionManifest.getResolvedPath(ResolvedPathKey.ofNamespace(namespace), true);
if (namespaceTarget == null) {
throw noSuchNamespaceException(namespace);
}
- authorizer()
- .authorizeOrThrow(
+ AuthorizationRequest fallbackRequest =
+ new AuthorizationRequest(
polarisPrincipal(),
- resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
- fallbackOp, // normal register-table operation
- namespaceTarget,
- null /* secondary */);
+ List.of(
+ new SingleTargetAuthorizationIntent(
+ fallbackOp, PolarisSecurableMapper.namespace(catalogName(), namespace))));
+ authorizer().authorize(authorizationState, fallbackRequest).throwIfDenied();
Review Comment:
Sounds good - I think just creating those additional AuthorizationRequests in those paths will improve legibility of the code. I've adopted the nit @flyingImer - please take another pass when you can
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Draft: update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5194:
URL: https://github.com/apache/polaris/pull/5194#discussion_r3771187311
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizerImpl.java:
##########
@@ -856,13 +856,15 @@ private PolarisResolvedPathWrapper getResolvedSecurable(
PolarisSecurable securable,
boolean prependRootContainer) {
PolarisResolvedPathWrapper resolvedSecurable =
- securable.getLeaf().entityType().isTopLevel()
- ? resolutionManifest.getResolvedTopLevelEntity(
- securable.getLeaf().name(), securable.getLeaf().entityType())
- : resolutionManifest.getResolvedPath(
- ResolvedPathKey.of(
- getPathNamesWithinCatalog(securable), securable.getLeaf().entityType()),
- prependRootContainer);
+ securable.getLeaf().entityType() == org.apache.polaris.core.entity.PolarisEntityType.CATALOG
+ ? resolutionManifest.getResolvedReferenceCatalogEntity(prependRootContainer)
+ : securable.getLeaf().entityType().isTopLevel()
+ ? resolutionManifest.getResolvedTopLevelEntity(
+ securable.getLeaf().name(), securable.getLeaf().entityType())
+ : resolutionManifest.getResolvedPath(
+ ResolvedPathKey.of(
+ getPathNamesWithinCatalog(securable), securable.getLeaf().entityType()),
Review Comment:
Ah, right... Securables are too specific to the native RBAC. I'm fine with current code "as is" :+1:
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Draft: update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on PR #5194:
URL: https://github.com/apache/polaris/pull/5194#issuecomment-5443723614
Thanks for the approval @flyrain
@flyingImer - my perspective is that it is okay to hide the authorization internals from the client error message, and make it as consistent and opaque as possible in 403. I think it makes sense to require the user to coordinate with the service administrator out of band to debug the issue like you suggest.
That being said, if the community is still divided and discussion is still in progress on what message is the right level of detail to send to the client side, I don't see a reason to rush it and pull it into the scope of this PR.
This Auth refactoring has been much needed and requested over the year, and I think it would be helpful for us to compartmentalize these two as separate efforts.
If there are no concerns, I'd like to move forward with merging this PR and follow up on #5370 separately.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Draft: update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "flyingImer (via GitHub)" <gi...@apache.org>.
flyingImer commented on code in PR #5194:
URL: https://github.com/apache/polaris/pull/5194#discussion_r3848038849
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -1009,24 +1018,58 @@ private Set<PolarisStorageActions> authorizeRegisterTable(
try {
if (overwrite) {
+ AuthorizationRequest overwriteRequest =
+ new AuthorizationRequest(
+ polarisPrincipal(),
+ List.of(
+ new SingleTargetAuthorizationIntent(
+ PolarisAuthorizableOperation
+ .REGISTER_TABLE_OVERWRITE_WITH_WRITE_DELEGATION,
+ PolarisSecurableMapper.tableLike(catalogName(), tableIdentifier))));
authorizeResolvedRegisterTableOverwriteOrThrow(
- PolarisAuthorizableOperation.REGISTER_TABLE_OVERWRITE_WITH_WRITE_DELEGATION,
+ authorizationState,
+ overwriteRequest,
PolarisAuthorizableOperation.REGISTER_TABLE_WITH_WRITE_DELEGATION,
tableIdentifier);
} else {
+ AuthorizationRequest createRequest =
+ new AuthorizationRequest(
+ polarisPrincipal(),
+ List.of(
+ new SingleTargetAuthorizationIntent(
+ PolarisAuthorizableOperation.REGISTER_TABLE_WITH_WRITE_DELEGATION,
+ PolarisSecurableMapper.namespace(
+ catalogName(), tableIdentifier.namespace()))));
authorizeResolvedCreateTableLikeUnderNamespaceOperationOrThrow(
- PolarisAuthorizableOperation.REGISTER_TABLE_WITH_WRITE_DELEGATION, tableIdentifier);
+ authorizationState, createRequest, tableIdentifier);
Review Comment:
IIUC, this flow still resolves REGISTER_TABLE_WITH_READ_DELEGATION for the table, then authorizes REGISTER_TABLE_WITH_WRITE_DELEGATION for the namespace. The built-in authorizers currently call resolveAll(), so the mismatch is masked there; however, the SPI explicitly permits request-selective resolution, and the second authorize() can receive state that never resolved its namespace. The same pattern appears in updateTable and fallback paths. I wonder if every request that may be authorized be included in resolution first?
##########
CHANGELOG.md:
##########
@@ -42,14 +42,14 @@ request adding CHANGELOG notes for breaking (!) changes and possibly other secti
### Changes
+- OPA authorization now evaluates batched collection requests as multiple intent-level OPA queries
Review Comment:
Thanks, the new CHANGELOG entry covers the collection-query split. The other OPA input changes are still missing: GET_CATALOG drops the ROOT parent and LIST_CATALOGS drops the ROOT target. Since existing Rego policies may need updates for all three, could we either preserve those two inputs in this phase or document the complete set under Breaking changes rather than Changes?
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationDecision.java:
##########
@@ -41,4 +45,11 @@ static AuthorizationDecision deny(@Nullable String message) {
@Value.Parameter(order = 2)
Optional<String> getMessage();
+
+ default void throwIfDenied() {
+ if (!isAllowed()) {
+ getMessage().ifPresent(message -> LOGGER.debug("Authorization denied: {}", message));
+ throw new ForbiddenException("Authorization denied");
Review Comment:
Thanks for addressing this. I think the issue is slightly different: the exception should keep a generic base, but AuthorizationDecision should still allow an optional client-safe reason. Dropping the reason entirely removes actionable guidance such as credential rotation. Could we define this field as client-safe, ideally name it accordingly, append it to "Authorization denied", and require every authorizer to sanitize it before returning the decision? OPA can omit the client detail while logging its full diagnostic server-side.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Draft: update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "flyingImer (via GitHub)" <gi...@apache.org>.
flyingImer commented on PR #5194:
URL: https://github.com/apache/polaris/pull/5194#issuecomment-5446911636
@sungwy I'm aligned with keeping the broader 403 policy in #5370. #5194 currently puts principal and operation into the client-facing 403, so could we keep the existing generic OPA message and log those details server-side? For the other two threads, the smallest path I see is to resolve every AuthorizationRequest before it may be authorized and document the full OPA input changes under Breaking changes. With those changes, I'd be comfortable moving this 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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Draft: update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on PR #5194:
URL: https://github.com/apache/polaris/pull/5194#issuecomment-5330186264
Thanks for the detailed reviews @dimas-b
@flyrain and @gracechen09 - could I ask for your reviews on this PR?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #5194:
URL: https://github.com/apache/polaris/pull/5194#discussion_r3884512437
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java:
##########
@@ -319,26 +300,20 @@ protected void authorizeResolvedRegisterTableOverwriteOrThrow(
ResolvedPathKey.ofTableLike(identifier), PolarisEntitySubType.ICEBERG_TABLE, true);
if (tableTarget != null) {
- authorizer()
- .authorizeOrThrow(
- polarisPrincipal(),
- resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
- overwriteOp,
- tableTarget,
- null /* secondary */);
+ authorizer().authorize(authorizationState, overwriteRequest).throwIfDenied();
} else {
PolarisResolvedPathWrapper namespaceTarget =
resolutionManifest.getResolvedPath(ResolvedPathKey.ofNamespace(namespace), true);
if (namespaceTarget == null) {
throw noSuchNamespaceException(namespace);
}
- authorizer()
- .authorizeOrThrow(
+ AuthorizationRequest fallbackRequest =
+ new AuthorizationRequest(
polarisPrincipal(),
- resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
- fallbackOp, // normal register-table operation
- namespaceTarget,
- null /* secondary */);
+ List.of(
+ new SingleTargetAuthorizationIntent(
+ fallbackOp, PolarisSecurableMapper.namespace(catalogName(), namespace))));
+ authorizer().authorize(authorizationState, fallbackRequest).throwIfDenied();
Review Comment:
Hi EJ, I agree with the principle that, long term, it would be cleaner if every request passed to `authorize()` had an explicitly corresponding request passed through `resolveAuthorizationInputs()`.
I think the current case is safe, though, and Iโd like to avoid expanding phase 5 around that invariant. The operation is part of the `AuthorizationIntent`, but today it does not drive resolution selection for this path.
Resolution is only based on `PolarisSecurable` today. For register-table overwrite, the namespace resource is already included in the manifest before resolution, and the table-like path is optional, so the fallback authorization is using the same resolved request state rather than relying on a different or missing resource.
We also deliberately have a few places where we resolve once and then authorize narrower or derived requests against the same `AuthorizationState`, because that preserves comparison against the same resolved state rather than doing a second resolution. One example is `updateTable`: it resolves once before deriving the final fine-grained table metadata operations, then authorizes each derived operation against that shared state.
So I agree this is a good SPI cleanup direction, but Iโd prefer to keep this PR focused on phase-5 feature parity and document/follow up on a stronger request-selective resolution invariant separately. I can add a clarifying comment here if that would help make the current assumption explicit.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Draft: update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #5194:
URL: https://github.com/apache/polaris/pull/5194#discussion_r3770392632
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizerImpl.java:
##########
@@ -856,13 +856,15 @@ private PolarisResolvedPathWrapper getResolvedSecurable(
PolarisSecurable securable,
boolean prependRootContainer) {
PolarisResolvedPathWrapper resolvedSecurable =
- securable.getLeaf().entityType().isTopLevel()
- ? resolutionManifest.getResolvedTopLevelEntity(
- securable.getLeaf().name(), securable.getLeaf().entityType())
- : resolutionManifest.getResolvedPath(
- ResolvedPathKey.of(
- getPathNamesWithinCatalog(securable), securable.getLeaf().entityType()),
- prependRootContainer);
+ securable.getLeaf().entityType() == org.apache.polaris.core.entity.PolarisEntityType.CATALOG
+ ? resolutionManifest.getResolvedReferenceCatalogEntity(prependRootContainer)
+ : securable.getLeaf().entityType().isTopLevel()
+ ? resolutionManifest.getResolvedTopLevelEntity(
+ securable.getLeaf().name(), securable.getLeaf().entityType())
+ : resolutionManifest.getResolvedPath(
+ ResolvedPathKey.of(
+ getPathNamesWithinCatalog(securable), securable.getLeaf().entityType()),
Review Comment:
good suggestion.
I'm unsure if we want to import auth concepts like `PolarisSecurable` into `PolarisResolutionManifest` though. would it make more sense to create a convenience method in `polaris-core/auth` instead? I'm a bit on the fence regarding which direction the import should go if we were to create the convenience method outside of `PolarisAuthorizer`
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Draft: update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5194:
URL: https://github.com/apache/polaris/pull/5194#discussion_r3762109278
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizer.java:
##########
@@ -56,8 +56,10 @@ void resolveAuthorizationInputs(
/**
* Convenience method that throws a {@link ForbiddenException} when authorization is denied.
*
- * <p>Implementations should provide allow/deny decisions via {@link #authorize}.
+ * @deprecated Use {@link #authorize(AuthorizationState, AuthorizationRequest)} and handle the
+ * returned {@link AuthorizationDecision}.
*/
+ @Deprecated(since = "1.2.0")
Review Comment:
nit: `forRemoval = true`?
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizerImpl.java:
##########
@@ -856,13 +856,15 @@ private PolarisResolvedPathWrapper getResolvedSecurable(
PolarisSecurable securable,
boolean prependRootContainer) {
PolarisResolvedPathWrapper resolvedSecurable =
- securable.getLeaf().entityType().isTopLevel()
- ? resolutionManifest.getResolvedTopLevelEntity(
- securable.getLeaf().name(), securable.getLeaf().entityType())
- : resolutionManifest.getResolvedPath(
- ResolvedPathKey.of(
- getPathNamesWithinCatalog(securable), securable.getLeaf().entityType()),
- prependRootContainer);
+ securable.getLeaf().entityType() == org.apache.polaris.core.entity.PolarisEntityType.CATALOG
+ ? resolutionManifest.getResolvedReferenceCatalogEntity(prependRootContainer)
+ : securable.getLeaf().entityType().isTopLevel()
+ ? resolutionManifest.getResolvedTopLevelEntity(
+ securable.getLeaf().name(), securable.getLeaf().entityType())
+ : resolutionManifest.getResolvedPath(
+ ResolvedPathKey.of(
+ getPathNamesWithinCatalog(securable), securable.getLeaf().entityType()),
Review Comment:
nit: maybe add this code as a convenience method to `resolutionManifest`?
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizerImpl.java:
##########
@@ -856,13 +856,15 @@ private PolarisResolvedPathWrapper getResolvedSecurable(
PolarisSecurable securable,
boolean prependRootContainer) {
PolarisResolvedPathWrapper resolvedSecurable =
- securable.getLeaf().entityType().isTopLevel()
- ? resolutionManifest.getResolvedTopLevelEntity(
- securable.getLeaf().name(), securable.getLeaf().entityType())
- : resolutionManifest.getResolvedPath(
- ResolvedPathKey.of(
- getPathNamesWithinCatalog(securable), securable.getLeaf().entityType()),
- prependRootContainer);
+ securable.getLeaf().entityType() == org.apache.polaris.core.entity.PolarisEntityType.CATALOG
Review Comment:
nit: import?
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizer.java:
##########
@@ -56,8 +56,10 @@ void resolveAuthorizationInputs(
/**
* Convenience method that throws a {@link ForbiddenException} when authorization is denied.
*
- * <p>Implementations should provide allow/deny decisions via {@link #authorize}.
+ * @deprecated Use {@link #authorize(AuthorizationState, AuthorizationRequest)} and handle the
+ * returned {@link AuthorizationDecision}.
*/
+ @Deprecated(since = "1.2.0")
default void authorizeOrThrow(
Review Comment:
Applies to all `authorizeOrThrow` methods.
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizer.java:
##########
@@ -56,8 +56,10 @@ void resolveAuthorizationInputs(
/**
* Convenience method that throws a {@link ForbiddenException} when authorization is denied.
*
- * <p>Implementations should provide allow/deny decisions via {@link #authorize}.
+ * @deprecated Use {@link #authorize(AuthorizationState, AuthorizationRequest)} and handle the
+ * returned {@link AuthorizationDecision}.
*/
+ @Deprecated(since = "1.2.0")
default void authorizeOrThrow(
Review Comment:
This is an SPI interface. IMHO, there is no point keeping uncalled methods in an SPI. Apparently after this PR Polaris will no longer call this method, so implementations do not need to worry about implementing it. Removing its impl. is a trivial task during upgrade and the need to it will normally be identified in downstream CI or local builds.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalogHandler.java:
##########
@@ -287,12 +273,16 @@ private void authorizePolicyMappingOperationOrThrow(
determinePolicyMappingOperation(target, targetWrapper, isAttach);
authorizer()
- .authorizeOrThrow(
- polarisPrincipal(),
- resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
- op,
- policyWrapper,
- targetWrapper);
+ .authorize(
+ authorizationState,
+ new AuthorizationRequest(
+ polarisPrincipal(),
Review Comment:
@sungwy : WDYT?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Draft: update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5194:
URL: https://github.com/apache/polaris/pull/5194#discussion_r3763283785
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizer.java:
##########
@@ -56,8 +56,10 @@ void resolveAuthorizationInputs(
/**
* Convenience method that throws a {@link ForbiddenException} when authorization is denied.
*
- * <p>Implementations should provide allow/deny decisions via {@link #authorize}.
+ * @deprecated Use {@link #authorize(AuthorizationState, AuthorizationRequest)} and handle the
+ * returned {@link AuthorizationDecision}.
*/
+ @Deprecated(since = "1.2.0")
default void authorizeOrThrow(
Review Comment:
Wearing my SPI implementor hat, I'd be confused as to how to implement the deprecated methods. I'd certainly want to browse the Polaris code to check whether it still calls the deprecated methods... which is a bigger overhead for implementors compared to removing code for removed interface methods.
That said, I'm ok with the deprecation approach too.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Draft: update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "flyingImer (via GitHub)" <gi...@apache.org>.
flyingImer commented on code in PR #5194:
URL: https://github.com/apache/polaris/pull/5194#discussion_r3817131224
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
Review Comment:
I don't think this is only about whether the operation affects resolution. This flow resolves READ_DELEGATION for the table, then createRequest authorizes WRITE_DELEGATION for the namespace. Since the SPI lets implementations resolve only what a request needs, reusing that state undermines request-selective resolution. Do we have any idea how to resolve it? I'm inclined if we could resolve and authorize the same AuthorizationRequest here
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Draft: update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "flyingImer (via GitHub)" <gi...@apache.org>.
flyingImer commented on PR #5194:
URL: https://github.com/apache/polaris/pull/5194#issuecomment-5415086338
> > Thanks @flyrain - I created this separate PR (#5370) to try to address the concerns around leaking error message to the client.
> > I think it my be slightly easier to get the SPI settled once that PR is merged. It'll also make our test assertions consistent across all authorization cases.
>
> An immediately followup SGTM. We will need to make sure they are released together. WDYT? @flyingImer
I think #5370 should merge first and this PR should rebase onto it, rather than relying only on same-release sequencing. #5370 owns the response-message change across the authorizers, tests, and documentation, while this branch currently carries only part of it and its CI is red. The branches also overlap, so #5194 needs conflict resolution either way. After the rebase, the request-resolution and remaining OPA payload threads still need to be addressed before I would consider #5194 merge-ready IMHO.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy merged PR #5194:
URL: https://github.com/apache/polaris/pull/5194
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Draft: update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "flyingImer (via GitHub)" <gi...@apache.org>.
flyingImer commented on code in PR #5194:
URL: https://github.com/apache/polaris/pull/5194#discussion_r3817105225
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizer.java:
##########
@@ -56,8 +56,10 @@ void resolveAuthorizationInputs(
/**
* Convenience method that throws a {@link ForbiddenException} when authorization is denied.
*
- * <p>Implementations should provide allow/deny decisions via {@link #authorize}.
+ * @deprecated Use {@link #authorize(AuthorizationState, AuthorizationRequest)} and handle the
+ * returned {@link AuthorizationDecision}.
*/
+ @Deprecated(since = "1.2.0")
default void authorizeOrThrow(
Review Comment:
IIUC, the OPA payload is already affected in this PR. authorizeCatalogTargetOmitsRootParent now expects catalog targets without the ROOT parent, and authorizeRootScopedOperationOmitsRootTarget expects LIST_CATALOGS to omit targets entirely. Since #3779 assigns the OPA breaking change to Phase 6, could we preserve the legacy payload here or explicitly move that compatibility break into this phase?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Draft: update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on PR #5194:
URL: https://github.com/apache/polaris/pull/5194#issuecomment-5404252907
Thanks @flyrain - I created this separate PR (https://github.com/apache/polaris/pull/5370) to try to address the concerns around leaking error message to the client.
I think it my be slightly easier to get the SPI settled once that PR is merged. It'll also make our test assertions consistent across all authorization cases.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Draft: update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #5194:
URL: https://github.com/apache/polaris/pull/5194#issuecomment-5414537863
> Thanks @flyrain - I created this separate PR (#5370) to try to address the concerns around leaking error message to the client.
>
> I think it my be slightly easier to get the SPI settled once that PR is merged. It'll also make our test assertions consistent across all authorization cases.
An immediately followup SGTM. We will need to make sure they are released together. WDYT? @flyingImer
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Draft: update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #5194:
URL: https://github.com/apache/polaris/pull/5194#discussion_r3825504538
##########
extensions/auth/opa/src/main/java/org/apache/polaris/extension/auth/opa/OpaPolarisAuthorizer.java:
##########
@@ -187,64 +182,6 @@ public AuthorizationDecision authorize(
return AuthorizationDecision.allow();
}
- /**
- * Authorizes a single target and secondary entity for the given principal and operation.
- *
- * <p>Delegates to the multi-target version for consistency.
- *
- * @param polarisPrincipal the principal requesting authorization
- * @param activatedEntities the set of activated entities (roles, etc.)
- * @param authzOp the operation to authorize
- * @param target the main target entity
- * @param secondary the secondary entity (if any)
- * @throws ForbiddenException if authorization is denied by OPA
- */
- @Override
- public void authorizeOrThrow(
- @NonNull PolarisPrincipal polarisPrincipal,
- @NonNull Set<PolarisBaseEntity> activatedEntities,
- @NonNull PolarisAuthorizableOperation authzOp,
- @Nullable PolarisResolvedPathWrapper target,
- @Nullable PolarisResolvedPathWrapper secondary) {
- authorizeOrThrow(
- polarisPrincipal,
- activatedEntities,
- authzOp,
- target == null ? null : List.of(target),
- secondary == null ? null : List.of(secondary));
- }
-
- /**
- * Authorizes one or more target and secondary entities for the given principal and operation.
- *
- * <p>Sends the authorization context to OPA and throws if not allowed.
- *
- * @param polarisPrincipal the principal requesting authorization
- * @param activatedEntities the set of activated entities (roles, etc.)
- * @param authzOp the operation to authorize
- * @param targets the list of main target entities
- * @param secondaries the list of secondary entities (if any)
- * @throws ForbiddenException if authorization is denied by OPA
- */
- @Override
- public void authorizeOrThrow(
- @NonNull PolarisPrincipal polarisPrincipal,
- @NonNull Set<PolarisBaseEntity> activatedEntities,
- @NonNull PolarisAuthorizableOperation authzOp,
- @Nullable List<PolarisResolvedPathWrapper> targets,
- @Nullable List<PolarisResolvedPathWrapper> secondaries) {
- boolean allowed =
- queryOpa(
Review Comment:
Hi EJ, good point regarding OPA policy compatibility. I agree this is worth documenting in our `CHANGELOG.md` as good hygiene, even if we expect it to be an uncommon case. The affected policies would need to be set-aware over `resource.targets` for collection-style operations, and Iโd be somewhat surprised if deployments rely on that type of policy for the actions we authorize today.
This change was intended and thought to be safe to introduced for the reason above. After adopting the new SPI, Polaris and OPA become a bit more chatty, but the cumulative output of authorization results is the same.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Draft: update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #5194:
URL: https://github.com/apache/polaris/pull/5194#discussion_r3770299273
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalogHandler.java:
##########
@@ -287,12 +273,16 @@ private void authorizePolicyMappingOperationOrThrow(
determinePolicyMappingOperation(target, targetWrapper, isAttach);
authorizer()
- .authorizeOrThrow(
- polarisPrincipal(),
- resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
- op,
- policyWrapper,
- targetWrapper);
+ .authorize(
+ authorizationState,
+ new AuthorizationRequest(
+ polarisPrincipal(),
Review Comment:
good question, I'm frankly a bit confused why we have both `determineRequestedPolicyMappingOperation` and `determinePolicyMappingOperation` when the resulting operation types are consistent in both methods...
Ideally, we'd reuse the same AuthorizationRequest, but I'd appreciate some more context here before making that decision.
@HonahX - do you happen to have some context on this?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Draft: update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5194:
URL: https://github.com/apache/polaris/pull/5194#discussion_r3770319386
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizer.java:
##########
@@ -56,8 +56,10 @@ void resolveAuthorizationInputs(
/**
* Convenience method that throws a {@link ForbiddenException} when authorization is denied.
*
- * <p>Implementations should provide allow/deny decisions via {@link #authorize}.
+ * @deprecated Use {@link #authorize(AuthorizationState, AuthorizationRequest)} and handle the
+ * returned {@link AuthorizationDecision}.
*/
+ @Deprecated(since = "1.2.0")
default void authorizeOrThrow(
Review Comment:
I hope our OPA protocol (request shape) is not affected by these java changes :)
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Draft: update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #5194:
URL: https://github.com/apache/polaris/pull/5194#discussion_r3762891049
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizer.java:
##########
@@ -56,8 +56,10 @@ void resolveAuthorizationInputs(
/**
* Convenience method that throws a {@link ForbiddenException} when authorization is denied.
*
- * <p>Implementations should provide allow/deny decisions via {@link #authorize}.
+ * @deprecated Use {@link #authorize(AuthorizationState, AuthorizationRequest)} and handle the
+ * returned {@link AuthorizationDecision}.
*/
+ @Deprecated(since = "1.2.0")
default void authorizeOrThrow(
Review Comment:
I see your point. I remember we didn't yet settle on a shared understanding of what the lifecycle management for SPIs look like within Polaris per this thread (https://lists.apache.org/thread/k1dtmj9kq0zscm903l3cdf4kyf5x6nfw).
I erred on the side of caution in marking the methods for deprecation, knowing that this topic is still in discussion for now. But could be convinced otherwise :)
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Draft: update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #5194:
URL: https://github.com/apache/polaris/pull/5194#issuecomment-5261670910
@sungwy : as for CI, a rebase or simple force push should help
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Draft: update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #5194:
URL: https://github.com/apache/polaris/pull/5194#discussion_r3769471692
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizer.java:
##########
@@ -56,8 +56,10 @@ void resolveAuthorizationInputs(
/**
* Convenience method that throws a {@link ForbiddenException} when authorization is denied.
*
- * <p>Implementations should provide allow/deny decisions via {@link #authorize}.
+ * @deprecated Use {@link #authorize(AuthorizationState, AuthorizationRequest)} and handle the
+ * returned {@link AuthorizationDecision}.
*/
+ @Deprecated(since = "1.2.0")
default void authorizeOrThrow(
Review Comment:
> Keeping deprecated SPI methods in Polaris forces implementations to maintain "dead" code. Proactively removing that code risks that a future Polaris version may want to call the deprecated methods again, which creates uncertainly for implementors.
That's a good point. I'm convinced we should just remove it, unless the change creates a downstream impact to an outside contract (e.g. OPA input shape). And even in that case, Polaris can take internal measures to introduce the breaking change in a staged manner.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Draft: update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5194:
URL: https://github.com/apache/polaris/pull/5194#discussion_r3776481366
##########
extensions/auth/opa/src/main/java/org/apache/polaris/extension/auth/opa/OpaPolarisAuthorizer.java:
##########
@@ -192,13 +192,9 @@ public AuthorizationDecision authorize(
*
* <p>Delegates to the multi-target version for consistency.
*
- * @param polarisPrincipal the principal requesting authorization
- * @param activatedEntities the set of activated entities (roles, etc.)
- * @param authzOp the operation to authorize
- * @param target the main target entity
- * @param secondary the secondary entity (if any)
- * @throws ForbiddenException if authorization is denied by OPA
+ * @deprecated Use intent-based {@link #authorize(AuthorizationState, AuthorizationRequest)}.
*/
+ @Deprecated(since = "1.2.0", forRemoval = true)
@Override
public void authorizeOrThrow(
Review Comment:
nit: would you suppress (expected, I assume) javac warnings (comments below)?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Draft: update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #5194:
URL: https://github.com/apache/polaris/pull/5194#discussion_r3796503409
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizer.java:
##########
@@ -56,8 +56,10 @@ void resolveAuthorizationInputs(
/**
* Convenience method that throws a {@link ForbiddenException} when authorization is denied.
*
- * <p>Implementations should provide allow/deny decisions via {@link #authorize}.
+ * @deprecated Use {@link #authorize(AuthorizationState, AuthorizationRequest)} and handle the
+ * returned {@link AuthorizationDecision}.
*/
+ @Deprecated(since = "1.2.0")
default void authorizeOrThrow(
Review Comment:
It isn't affected yet - but the phase 6 of the migration suggests making those breaking changes in the OPA protocol
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Draft: update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #5194:
URL: https://github.com/apache/polaris/pull/5194#issuecomment-5331275058
> Thanks for the detailed reviews @dimas-b
>
> @flyrain and @gracechen09 - could I ask for your reviews on this PR?
@sungwy , just back from the vacation. Sorry for the delay. Will take a look soon.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Draft: update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5194:
URL: https://github.com/apache/polaris/pull/5194#discussion_r3797269812
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
Review Comment:
It feels like the "operation" is not supposed to affect the actions performed by `resolveAuthorizationInputs()` ๐ค
If so, could / should we refactor the authZ classes a bit more to make it explicit?
This can be done, later. I do not think it's a blocker for this PR.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #5194:
URL: https://github.com/apache/polaris/pull/5194#discussion_r3884446970
##########
extensions/auth/opa/src/test/java/org/apache/polaris/extension/auth/opa/OpaPolarisAuthorizerTest.java:
##########
@@ -838,9 +864,7 @@ void authorizeResolvedCatalogTargetPreservesRootParent() throws Exception {
{
"type": "CATALOG",
"name": "catalog-1",
- "parents": [
- {"type": "ROOT", "name": "root_container", "parents": []}
- ]
+ "parents": []
}
""");
assertThat(target).isEqualTo(expectedTarget);
Review Comment:
Yes, let's add this to the CHANGELOG as well ๐
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Draft: update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #5194:
URL: https://github.com/apache/polaris/pull/5194#issuecomment-5259461144
@sungwy : Is this still a draft PR (title / description)?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #5194:
URL: https://github.com/apache/polaris/pull/5194#discussion_r3925660855
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -935,18 +935,20 @@ private Set<PolarisStorageActions> authorizeLoadTable(
PolarisAuthorizableOperation write =
PolarisAuthorizableOperation.LOAD_TABLE_WITH_WRITE_DELEGATION;
- resolveBasicTableLikeTargetOrThrow(write, tableIdentifier);
+ ensureResolutionManifestForTable(tableIdentifier);
+ AuthorizationState authorizationState = new AuthorizationState(resolutionManifest);
+ resolveBasicTableLikeTargetOrThrow(authorizationState, write, tableIdentifier);
Set<PolarisStorageActions> actionsRequested =
new HashSet<>(Set.of(PolarisStorageActions.READ, PolarisStorageActions.LIST));
try {
authorizeResolvedBasicTableLikeOperationOrThrow(
- write, PolarisEntitySubType.ICEBERG_TABLE, tableIdentifier);
+ authorizationState, write, PolarisEntitySubType.ICEBERG_TABLE, tableIdentifier);
actionsRequested.add(PolarisStorageActions.WRITE);
} catch (ForbiddenException e) {
// Reuse the already-resolved table view for the read-delegation fallback.
authorizeResolvedBasicTableLikeOperationOrThrow(
- read, PolarisEntitySubType.ICEBERG_TABLE, tableIdentifier);
+ authorizationState, read, PolarisEntitySubType.ICEBERG_TABLE, tableIdentifier);
Review Comment:
yes, that's right and I think unlike the AuthorizationRequest, it's important that we share the `AuthorizationState`, since we want to read or write on the same resolved `PolarisSecurable`.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #5194:
URL: https://github.com/apache/polaris/pull/5194#discussion_r3925660855
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -935,18 +935,20 @@ private Set<PolarisStorageActions> authorizeLoadTable(
PolarisAuthorizableOperation write =
PolarisAuthorizableOperation.LOAD_TABLE_WITH_WRITE_DELEGATION;
- resolveBasicTableLikeTargetOrThrow(write, tableIdentifier);
+ ensureResolutionManifestForTable(tableIdentifier);
+ AuthorizationState authorizationState = new AuthorizationState(resolutionManifest);
+ resolveBasicTableLikeTargetOrThrow(authorizationState, write, tableIdentifier);
Set<PolarisStorageActions> actionsRequested =
new HashSet<>(Set.of(PolarisStorageActions.READ, PolarisStorageActions.LIST));
try {
authorizeResolvedBasicTableLikeOperationOrThrow(
- write, PolarisEntitySubType.ICEBERG_TABLE, tableIdentifier);
+ authorizationState, write, PolarisEntitySubType.ICEBERG_TABLE, tableIdentifier);
actionsRequested.add(PolarisStorageActions.WRITE);
} catch (ForbiddenException e) {
// Reuse the already-resolved table view for the read-delegation fallback.
authorizeResolvedBasicTableLikeOperationOrThrow(
- read, PolarisEntitySubType.ICEBERG_TABLE, tableIdentifier);
+ authorizationState, read, PolarisEntitySubType.ICEBERG_TABLE, tableIdentifier);
Review Comment:
yes, that's right and I think unlike the AuthorizationRequest, it's important that we share the `AuthorizationState`, since we want to read or write on the same `PolarisSecurable`.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #5194:
URL: https://github.com/apache/polaris/pull/5194#issuecomment-5605206929
Thanks a lot for the changes, @sungwy !
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on PR #5194:
URL: https://github.com/apache/polaris/pull/5194#issuecomment-5603569450
Awesome. thanks for the reviews @dimas-b @flyingImer and @flyrain
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5194:
URL: https://github.com/apache/polaris/pull/5194#discussion_r3921693354
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -935,18 +935,20 @@ private Set<PolarisStorageActions> authorizeLoadTable(
PolarisAuthorizableOperation write =
PolarisAuthorizableOperation.LOAD_TABLE_WITH_WRITE_DELEGATION;
- resolveBasicTableLikeTargetOrThrow(write, tableIdentifier);
+ ensureResolutionManifestForTable(tableIdentifier);
+ AuthorizationState authorizationState = new AuthorizationState(resolutionManifest);
+ resolveBasicTableLikeTargetOrThrow(authorizationState, write, tableIdentifier);
Set<PolarisStorageActions> actionsRequested =
new HashSet<>(Set.of(PolarisStorageActions.READ, PolarisStorageActions.LIST));
try {
authorizeResolvedBasicTableLikeOperationOrThrow(
- write, PolarisEntitySubType.ICEBERG_TABLE, tableIdentifier);
+ authorizationState, write, PolarisEntitySubType.ICEBERG_TABLE, tableIdentifier);
actionsRequested.add(PolarisStorageActions.WRITE);
} catch (ForbiddenException e) {
// Reuse the already-resolved table view for the read-delegation fallback.
authorizeResolvedBasicTableLikeOperationOrThrow(
- read, PolarisEntitySubType.ICEBERG_TABLE, tableIdentifier);
+ authorizationState, read, PolarisEntitySubType.ICEBERG_TABLE, tableIdentifier);
Review Comment:
We authorize "read" here but the resolution was done on a "write" request. Practically, this is not significant, since only entity references matter for resolution, but it would be nice to make it explicit in the AuthZ SPI.... meaning that resolution should not have the operation itself as a formal parameter.
This is just a point for follow-up thinking / PR. Not a blocker ๐
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Draft: update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #5194:
URL: https://github.com/apache/polaris/pull/5194#discussion_r3805386254
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
Review Comment:
I always found that to be awkward as well, but for the interim, we landed on an SPI shape that makes use of an `AuthorizationRequest` which includes the operation (as a part of the `AuthorizationIntent`). This was a hard earned compromise and an acceptable stopping point, so I agree with punting on this for now, but definitely worth revisiting in the future.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Draft: update `CatalogHandler` and `PolarisAdminServier` to `authorize` using `AuthorizationIntent` [polaris]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #5194:
URL: https://github.com/apache/polaris/pull/5194#discussion_r3760691965
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java:
##########
@@ -225,39 +210,35 @@ protected void authorizeCreateTableLikeUnderNamespaceOperationOrThrow(
PolarisCatalogHelpers.tableIdentifierToList(identifier),
PolarisEntityType.TABLE_LIKE,
true /* optional */));
+ AuthorizationRequest authorizationRequest =
+ new AuthorizationRequest(
+ polarisPrincipal(),
+ List.of(
+ new SingleTargetAuthorizationIntent(
+ op, PolarisSecurableMapper.namespace(catalogName(), namespace))));
AuthorizationState authorizationState = new AuthorizationState(resolutionManifest);
- authorizer()
- .resolveAuthorizationInputs(
- authorizationState,
- new AuthorizationRequest(
- polarisPrincipal(),
- List.of(
- new SingleTargetAuthorizationIntent(
- op, PolarisSecurableMapper.namespace(catalogName(), namespace)))));
- authorizeResolvedCreateTableLikeUnderNamespaceOperationOrThrow(op, identifier);
+ authorizer().resolveAuthorizationInputs(authorizationState, authorizationRequest);
+ authorizeResolvedCreateTableLikeUnderNamespaceOperationOrThrow(
+ authorizationState, authorizationRequest, identifier);
}
protected void authorizeResolvedCreateTableLikeUnderNamespaceOperationOrThrow(
- PolarisAuthorizableOperation op, TableIdentifier identifier) {
+ AuthorizationState authorizationState,
+ AuthorizationRequest authorizationRequest,
+ TableIdentifier identifier) {
if (resolutionManifest == null) {
throw new IllegalStateException(
"Resolved authorization requires resolveAuthorizationInputs to run first");
}
- Namespace namespace = identifier.namespace();
PolarisResolvedPathWrapper target =
- resolutionManifest.getResolvedPath(ResolvedPathKey.ofNamespace(namespace), true);
+ resolutionManifest.getResolvedPath(
+ ResolvedPathKey.ofNamespace(identifier.namespace()), true);
if (target == null) {
- throw noSuchNamespaceException(namespace);
+ throw noSuchNamespaceException(identifier.namespace());
}
- authorizer()
- .authorizeOrThrow(
- polarisPrincipal(),
- resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
- op,
- target,
- null /* secondary */);
+ authorizer().authorize(authorizationState, authorizationRequest).throwIfDenied();
Review Comment:
authorizeResolvedCreateTableLikeUnderNamespaceOperationOrThrow assumes resolveAuthorizationInputs already ran for the passed AuthorizationRequest, but some call sites (e.g., delegation flows) pass a fresh request and only resolved a different one. To avoid authorizer implementations depending on request-specific state, resolve with the same AuthorizationRequest immediately before calling authorize().
This issue also appears in the following locations of the same file:
- line 302
- line 366
- line 402
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalogHandler.java:
##########
@@ -287,12 +273,16 @@ private void authorizePolicyMappingOperationOrThrow(
determinePolicyMappingOperation(target, targetWrapper, isAttach);
authorizer()
- .authorizeOrThrow(
- polarisPrincipal(),
- resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
- op,
- policyWrapper,
- targetWrapper);
+ .authorize(
+ authorizationState,
+ new AuthorizationRequest(
+ polarisPrincipal(),
Review Comment:
authorizePolicyMappingOperationOrThrow resolves one AuthorizationRequest (requestedOp) but then authorizes a different freshly-constructed AuthorizationRequest (op) without resolving it. If an authorizer stores request-scoped state during resolveAuthorizationInputs, authorize() may see incomplete state for the second request.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org