yuqi1129 commented on code in PR #13105:
URL: https://github.com/apache/gravitino/pull/13105#discussion_r4004807575
##########
docs/lance-rest-service.md:
##########
@@ -172,6 +172,21 @@ gravitino.lance-rest.gravitino-metalake = my_metalake
In auxiliary mode, `gravitino.lance-rest.gravitino-uri` is not required
because the Lance
REST service uses Gravitino's internal API.
+**Authorization and Anonymous Requests**
+
+The `simple` authenticator performs no real authentication. It accepts any
request and produces an
+`anonymous` principal when no credentials are provided. The behavior for
anonymous requests depends
+on whether authorization is enabled:
+
+- **Authorization disabled (default):** The `LanceServiceIdentityFilter` is
installed. Anonymous
+ requests run as the configured service user
+ (`gravitino.lance-rest.gravitino-simple.user-name`, default
`lance-rest-server`). This preserves
+ backward compatibility.
+- **Authorization enabled (`gravitino.authorization.enable = true`):** The
filter is **not**
+ installed. Anonymous requests are rejected by the authorization interceptor
with HTTP 403,
+ provided `gravitino.lance-rest.gravitino-metalake` is configured. Every
caller must present
Review Comment:
Please update the existing **Authentication and deployment modes** section
as part of this fix, rather than leaving two conflicting descriptions. Its
auxiliary-mode row still says that anonymous requests fall back to the service
user without limiting that behavior to authorization-disabled deployments.
Also, `Every caller must present valid credentials` is misleading for
`simple`: it does not validate the password, and any supplied Basic user name
becomes the caller identity. Please say that anonymous requests are rejected,
while `simple` is not a security boundary and deployments that need verified
identities must use a credential-validating authenticator.
##########
lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/TestLanceRESTService.java:
##########
@@ -18,22 +18,112 @@
*/
package org.apache.gravitino.lance;
+import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
import java.util.Set;
+import org.apache.commons.lang3.reflect.FieldUtils;
+import org.apache.gravitino.Config;
+import org.apache.gravitino.Configs;
+import org.apache.gravitino.GravitinoEnv;
import org.apache.gravitino.lance.common.config.LanceConfig;
+import org.apache.gravitino.lance.service.LanceServiceIdentityFilter;
import org.apache.gravitino.listener.EventBus;
+import org.apache.gravitino.metrics.MetricsSystem;
import org.apache.gravitino.server.web.HttpAuditFilter;
import org.apache.gravitino.server.web.JettyServer;
import org.apache.gravitino.server.web.JettyServerConfig;
import org.apache.gravitino.server.web.JettyServerTestUtils;
import org.apache.gravitino.server.web.RequestContextFilter;
import org.eclipse.jetty.servlet.ServletHandler;
+import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Test;
public class TestLanceRESTService {
+ private Config previousConfig;
+ private MetricsSystem previousMetricsSystem;
+ private EventBus previousEventBus;
+
+ @AfterEach
+ public void tearDown() throws Exception {
+ if (previousConfig != null) {
+ FieldUtils.writeField(GravitinoEnv.getInstance(), "config",
previousConfig, true);
+ }
+ if (previousMetricsSystem != null) {
+ FieldUtils.writeField(
+ GravitinoEnv.getInstance(), "metricsSystem", previousMetricsSystem,
true);
+ }
+ if (previousEventBus != null) {
+ FieldUtils.writeField(GravitinoEnv.getInstance(), "eventBus",
previousEventBus, true);
+ }
+ }
+
+ private void injectGravitinoEnv(boolean authorizationEnabled) throws
Exception {
+ previousConfig = (Config) FieldUtils.readField(GravitinoEnv.getInstance(),
"config", true);
+ previousMetricsSystem =
+ (MetricsSystem) FieldUtils.readField(GravitinoEnv.getInstance(),
"metricsSystem", true);
+ previousEventBus =
+ (EventBus) FieldUtils.readField(GravitinoEnv.getInstance(),
"eventBus", true);
+
+ Config mockConfig = mock(Config.class);
+
when(mockConfig.get(Configs.ENABLE_AUTHORIZATION)).thenReturn(authorizationEnabled);
+ FieldUtils.writeField(GravitinoEnv.getInstance(), "config", mockConfig,
true);
+ FieldUtils.writeField(GravitinoEnv.getInstance(), "metricsSystem", new
MetricsSystem(), true);
+ FieldUtils.writeField(
+ GravitinoEnv.getInstance(), "eventBus", new
EventBus(Collections.emptyList()), true);
+ }
+
+ private boolean hasServiceIdentityFilter(LanceRESTService service) throws
Exception {
+ JettyServer server = (JettyServer) FieldUtils.readField(service, "server",
true);
+ ServletHandler servletHandler =
+
JettyServerTestUtils.getServletContextHandler(server).getServletHandler();
+ Set<String> filterPathSpecs =
+ JettyServerTestUtils.filterPathSpecsFor(servletHandler,
LanceServiceIdentityFilter.class);
+ return !filterPathSpecs.isEmpty();
+ }
+
+ /** See GH-13093. The filter must not be registered when authorization is
enabled. */
+ @Test
+ public void testServiceIdentityFilterNotRegisteredWhenAuthorizationEnabled()
throws Exception {
Review Comment:
Please add an end-to-end HTTP regression test to an authorization IT (for
example, `LanceNamespaceAuthorizationIT`). This unit test verifies the filter
mapping, but it does not prove the intended externally visible result: an
anonymous request receives 403 when authorization is enabled.
The IT should configure the fallback service identity as an existing
privileged user (for example, set it to the test admin), omit the Authorization
header, and assert 403. That detail is important: if the fallback user is not
in the metalake, the pre-fix code can also return 403 during user validation,
causing a false-positive regression test. The proposed test should return 200
before this fix and 403 after 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
