yuqi1129 commented on code in PR #13481:
URL: https://github.com/apache/gravitino/pull/13481#discussion_r4094953098
##########
core/src/main/java/org/apache/gravitino/catalog/TableNormalizeDispatcher.java:
##########
@@ -62,7 +66,7 @@ public NameIdentifier[] listTables(Namespace namespace)
throws NoSuchSchemaExcep
public Table loadTable(NameIdentifier ident) throws NoSuchTableException {
// The constraints of the name spec may be more strict than underlying
catalog,
// and for compatibility reasons, we only apply case-sensitive
capabilities here.
- return dispatcher.loadTable(normalizeCaseSensitive(ident));
+ return dispatcher.loadTable(resolvePhysicalName(ident,
normalizeCaseSensitive(ident)));
Review Comment:
Authorization for this request already ran in the REST layer against
`normalizeCaseSensitive(ident)` (via `MetadataIdConverter.getID`), not against
the identifier resolved here. When the two differ and the normalized name has
an entity, the privilege check and the operation target different tables. The
same applies to alter/drop/purge/exists below. See the review summary for two
concrete timelines.
##########
core/src/main/java/org/apache/gravitino/connector/SupportsTableNameResolution.java:
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.connector;
+
+import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.annotation.Evolving;
+import org.apache.gravitino.rel.TableCatalog;
+
+/**
+ * A server-internal, connector-side capability that maps a table identifier
to the identifier under
+ * which the table is physically stored by the underlying source, for backends
whose name
+ * normalization is not reversible.
+ *
+ * <p>Most catalogs store a table under exactly the name Gravitino normalized
it to, so they do not
+ * implement this. A catalog whose {@link
org.apache.gravitino.connector.capability.Capability}
+ * folds an unquoted name to a fixed case while the source also keeps
case-sensitive names created
+ * with a different case (so a name returned by {@link
TableCatalog#listTables} may not equal the
+ * normalized name) may implement this so that a name returned by list
round-trips through
+ * load/alter/drop.
+ *
+ * <p>This is a {@link CatalogOperations} mixin, not part of the user-facing
{@link TableCatalog}
+ * API: it is only consulted by the server on the load/alter/drop path and is
never exposed to
+ * clients. The server resolves the name before the operation runs, so the
resolved identifier
+ * drives the downstream authorization hooks, the underlying catalog call and
the Gravitino entity
+ * store key consistently.
+ *
+ * <p><b>Resolution contract.</b> Implementations receive both the identifier
the caller requested
+ * and the identifier after Gravitino's case normalization, and must:
+ *
+ * <ul>
+ * <li>prefer an object whose stored name equals {@code requestedIdent}'s
name exactly, so a
+ * case-sensitive name the caller supplied verbatim is honored even when
a differently-cased
+ * sibling exists;
+ * <li>otherwise use an object whose stored name equals {@code
normalizedIdent}'s name exactly;
+ * <li>otherwise, if exactly one stored name matches {@code normalizedIdent}
case-insensitively,
Review Comment:
Even without the rule above, this fallback can diverge from the authorized
name. The authorizer looks up the entity *store* by the normalized name, while
this rule looks at the *source*. With a stale `ORDERS` registration in the
store and only `orders` in the source, the request is authorized by the stale
entity and then acts on `orders`. The resolved name has to be the one that gets
authorized.
##########
core/src/main/java/org/apache/gravitino/connector/SupportsTableNameResolution.java:
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.connector;
+
+import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.annotation.Evolving;
+import org.apache.gravitino.rel.TableCatalog;
+
+/**
+ * A server-internal, connector-side capability that maps a table identifier
to the identifier under
+ * which the table is physically stored by the underlying source, for backends
whose name
+ * normalization is not reversible.
+ *
+ * <p>Most catalogs store a table under exactly the name Gravitino normalized
it to, so they do not
+ * implement this. A catalog whose {@link
org.apache.gravitino.connector.capability.Capability}
+ * folds an unquoted name to a fixed case while the source also keeps
case-sensitive names created
+ * with a different case (so a name returned by {@link
TableCatalog#listTables} may not equal the
+ * normalized name) may implement this so that a name returned by list
round-trips through
+ * load/alter/drop.
+ *
+ * <p>This is a {@link CatalogOperations} mixin, not part of the user-facing
{@link TableCatalog}
+ * API: it is only consulted by the server on the load/alter/drop path and is
never exposed to
+ * clients. The server resolves the name before the operation runs, so the
resolved identifier
+ * drives the downstream authorization hooks, the underlying catalog call and
the Gravitino entity
+ * store key consistently.
+ *
+ * <p><b>Resolution contract.</b> Implementations receive both the identifier
the caller requested
+ * and the identifier after Gravitino's case normalization, and must:
+ *
+ * <ul>
+ * <li>prefer an object whose stored name equals {@code requestedIdent}'s
name exactly, so a
Review Comment:
This rule makes the authorization mismatch directly reachable. With `ORDERS`
and a quoted `orders` both present, an unquoted request for `orders` is
authorized as `ORDERS` but resolves to `orders`. It also contradicts a folding
capability: unquoted `orders` means `ORDERS`. I'd drop this rule and the
`requestedIdent` parameter, since quoted input is already preserved by the
normalization.
##########
core/src/test/java/org/apache/gravitino/catalog/TestTableNormalizeDispatcher.java:
##########
@@ -295,4 +298,179 @@ private void assertTableCaseInsensitive(
Assertions.assertEquals(
expectedColumns[0].name().toLowerCase(),
table.index()[0].fieldNames()[0][0].toLowerCase());
}
+
+ @Test
+ public void testPhysicalNameResolutionDrivesDownstreamIdentifier() throws
Exception {
Review Comment:
These tests mock the resolver and the downstream dispatcher, so they can't
catch the authorization mismatch. Please add a test showing that, for one
request, the metadata id used for authorization and the identifier handed to
the dispatcher come from the same resolved name (for example,
`MetadataIdConverter.getID` with a resolving catalog).
--
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]
