LiJie20190102 commented on code in PR #13022:
URL: https://github.com/apache/gravitino/pull/13022#discussion_r4429845987
##########
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()));
Review Comment:
The interceptor now re-checks the read privilege after the operation has
resolved its result and before the response is returned
(invokeWithExistOkRecheck), so when the addressed object is replaced between
the pre-check and the load, the request is denied against the object the
response actually describes instead of leaking its metadata. A caller that just
created the object owns it and passes the re-check. Added a controlled
replacement regression at the operation boundary
(testExistOkRechecksReadWhenTheObjectAppearsConcurrently, scripting
absent-at-authorization / existing-at-response); a physically concurrent
drop/recreate cannot be reproduced deterministically over HTTP in the IT
framework, so the regression stays at the interceptor boundary.
##########
lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/authorization/LanceAuthorizationExpressions.java:
##########
@@ -129,6 +129,29 @@ public final class LanceAuthorizationExpressions {
+
AuthorizationExpressionConstants.MODIFY_TABLE_AUTHORIZATION_EXPRESSION
+ ")";
+ /**
+ * Authorizes reading an existing object when a create request uses {@code
exist_ok} mode and the
+ * object already exists. The response returns the existing object's
metadata, so the caller must
+ * hold the same read privilege that a describe would require. Without this
check, a caller with
+ * only {@code CREATE_TABLE} or {@code CREATE_SCHEMA} could obtain metadata
that describe would
+ * deny.
+ *
+ * <p>For tables, the expression requires the table read privilege. For
namespaces, it requires
+ * catalog access (at catalog level) or schema read (at schema level); the
entity type is already
+ * resolved by the interceptor, so only the matching branch is evaluated.
+ */
+ public static final String EXIST_OK_TABLE_AUTHORIZATION_EXPRESSION =
Review Comment:
The EXIST_OK_TABLE/NAMESPACE_AUTHORIZATION_EXPRESSION constants are removed.
The handler reuses READ_TABLE_AUTHORIZATION_EXPRESSION for tables and
CAN_ACCESS_METADATA — the same expression DescribeNamespace uses — for
namespaces, with the exist_ok branching kept in the handler. The Javadoc now
refers to describe/read semantics only.
--
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]
