yuqi1129 commented on code in PR #13022:
URL: https://github.com/apache/gravitino/pull/13022#discussion_r4420036023
##########
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:
[Nit] Reuse the general read authorization expression instead of introducing
an EXIST_OK-specific copy.
This value is exactly the existing READ_TABLE_AUTHORIZATION_EXPRESSION.
EXIST_OK is the request mode that selects a read check, not a distinct
permission rule. Keeping two public constants with identical logic makes later
read-policy changes easy to apply to DescribeTable but miss here.
Please reuse READ_TABLE_AUTHORIZATION_EXPRESSION directly. If a shared
namespace-read expression is needed, name/share it by that capability as well,
keeping the EXIST_OK branching in the handler. Also, the handler Javadoc says
"describe/probe", but a probe deliberately permits create-only callers and
returns no metadata; it should refer to describe/read semantics only.
##########
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:
[Important] Checking by name before invocation does not close the
concurrent-replacement disclosure in #12955.
The evaluator receives names only.
BaseMetadataAuthorizationMethodInterceptor finishes these checks and then calls
`methodInvoker.proceed()` without retaining a resolved object ID or validating
the returned object. The table dispatcher's TreeLock is acquired inside that
later operation, so it does not protect this earlier authorization either.
A caller can pass the read check on table T1 (and retain CREATE_TABLE on its
schema); another request then renames/removes T1 and creates unreadable T2 at
the same name; EXIST_OK subsequently loads T2 and returns its
location/properties/storage options using the authorization decision for T1.
The catalog/schema EXIST_OK result paths have the analogous gap. Avoiding an
existence probe does not prevent this identity change.
Please tie authorization to the exact existing object/snapshot being
returned, detecting/rechecking replacement before emitting metadata. Add a
controlled replacement regression at the operation/HTTP boundary. I verified
the call chain and used a temporary interceptor-boundary probe showing that
unreadable result metadata is returned without reauthorization; I have not
reproduced a physical database drop/recreate through HTTP.
##########
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:
[Important] Preserve creation of an absent object for create-only callers.
This read check runs unconditionally before the create operation knows
whether it will create a new object or return an existing one. A user with
USE_CATALOG + CREATE_SCHEMA on the parent catalog, but no USE_SCHEMA on the
requested schema, now receives 403 for `CreateNamespace(mode=exist_ok)` even
when the schema does not exist. The same unconditional restriction applies to
CREATE_TABLE callers without SELECT_TABLE. #12955 explicitly requires
preserving normal creation of absent objects.
I reproduced the namespace case through embedded HTTP: admin describe of a
fresh name returns 404; the writer's EXIST_OK returns 403 and leaves it absent;
the same writer's CREATE succeeds with 200 and can then describe its new schema
as owner. The current tests only assert privilege combinations/existing-object
rejection and do not protect this path.
Please authorize creation on the new-object path and require read permission
when returning existing metadata, with the check bound to the resolved object
(see F2). Add missing-table and missing-namespace HTTP regressions alongside
the existing-object denial 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
