LiJie20190102 commented on code in PR #13022:
URL: https://github.com/apache/gravitino/pull/13022#discussion_r4429842351
##########
lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/authorization/LanceMetadataAuthorizationMethodInterceptor.java:
##########
@@ -196,59 +202,82 @@ protected Object toErrorResponse(Method method, Object[]
args, Throwable throwab
}
/**
- * Authorizes a create request whose mode overwrites an object that already
exists.
+ * Authorizes a create request whose mode affects an existing object.
*
* <p>An overwrite replaces an existing namespace or table, so it is a
modification rather than a
* creation and is authorized against the modification expression for the
invoked resource instead
* of the create expression on the method. Without this, CREATE_CATALOG,
CREATE_SCHEMA, or
* CREATE_TABLE would escalate into permission to replace an object the
caller does not own.
*
+ * <p>The {@code exist_ok} mode returns the existing object's metadata
(location, properties,
+ * schema) when the object already exists. That is a read of the object, so
the caller must hold
+ * the same read privilege that a describe/probe would require. Without this
check, a caller with
+ * only CREATE_TABLE could obtain metadata that DescribeTable would deny.
+ *
* <p>The mode alone decides this, without probing whether the object
exists: an existence probe
* at authorization time would race with the create that follows it, and the
required privilege
* would then depend on that race.
*/
- private static final class OverwriteAuthzHandler implements
AuthorizationHandler {
+ private static final class CreateModeAuthzHandler implements
AuthorizationHandler {
private static final String OVERWRITE_MODE = "OVERWRITE";
+ private static final String EXIST_OK_MODE = "EXIST_OK";
private final boolean overwrite;
- private final String authorizationExpression;
+ private final boolean existOk;
+ private final String overwriteExpression;
+ private final String existOkExpression;
- private OverwriteAuthzHandler(String mode, String authorizationExpression)
{
+ private CreateModeAuthzHandler(
+ String mode, String overwriteExpression, String existOkExpression) {
// Read the mode through the same normalization the create operation
applies, so a token the
- // operation will act on as an overwrite cannot be authorized as a plain
create. Comparing the
- // raw string here would leave a gap: " overwrite " reaches the
operation as OVERWRITE but
- // would not match, and a caller holding only a create privilege could
replace an object owned
- // by somebody else.
- this.overwrite = OVERWRITE_MODE.equals(CommonUtil.normalizeToken(mode));
- this.authorizationExpression = authorizationExpression;
+ // operation will act on as an overwrite or exist_ok cannot be
authorized as a plain create.
+ String normalized = CommonUtil.normalizeToken(mode);
+ this.overwrite = OVERWRITE_MODE.equals(normalized);
+ this.existOk = EXIST_OK_MODE.equals(normalized);
+ this.overwriteExpression = overwriteExpression;
+ this.existOkExpression = existOkExpression;
}
@Override
public void process(Map<Entity.EntityType, NameIdentifier>
nameIdentifierMap) {
- if (!overwrite) {
- return;
- }
-
Entity.EntityType entityType = deepestEntityType(nameIdentifierMap);
- boolean authorized =
- new AuthorizationExpressionEvaluator(authorizationExpression)
- .evaluate(
- nameIdentifierMap,
- new HashMap<>(),
- new AuthorizationRequestContext(),
- Optional.of(entityType.name()));
- if (!authorized) {
- throw new ForbiddenException(
- "User '%s' is not authorized to overwrite '%s'",
- PrincipalUtils.getCurrentUserName(),
nameIdentifierMap.get(entityType));
+ if (overwrite) {
+ boolean authorized =
+ new AuthorizationExpressionEvaluator(overwriteExpression)
+ .evaluate(
+ nameIdentifierMap,
+ new HashMap<>(),
+ new AuthorizationRequestContext(),
+ Optional.of(entityType.name()));
+ if (!authorized) {
+ throw new ForbiddenException(
+ "User '%s' is not authorized to overwrite '%s'",
+ PrincipalUtils.getCurrentUserName(),
nameIdentifierMap.get(entityType));
+ }
+ }
+ if (existOk) {
+ boolean authorized =
+ new AuthorizationExpressionEvaluator(existOkExpression)
+ .evaluate(
+ nameIdentifierMap,
+ new HashMap<>(),
+ new AuthorizationRequestContext(),
+ Optional.of(entityType.name()));
+ if (!authorized) {
Review Comment:
Fixed in 963f7dd6c. The handler now resolves the addressed object first,
mirroring the core server's load-as-probe authorization: an exist_ok request
that resolves to an existing object is authorized against the read expression
(and the create expression is skipped, so read-only callers are not denied),
while a request that resolves to an absent object falls through to the create
expression on the method, so create-only callers keep their normal creation
path. Added the missing-object HTTP regressions:
LanceNamespaceAuthorizationIT.testExistOkPreservesCreationOfAnAbsentNamespace
and
LanceTableAuthorizationIT.testExistOkCreatesAnAbsentTableButDoesNotReadAnExistingOne,
plus unit coverage in TestLanceMetadataAuthorizationMethodInterceptor.
--
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]
