Skip to content

Commit c793284

Browse files
jerryshaojerryshao
committed
Fix the 404 issue
1 parent 1be3aee commit c793284

File tree

5 files changed

+278
-51
lines changed

5 files changed

+278
-51
lines changed

‎clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/TagOperationsAuthorizationIT.java‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
import com.google.common.collect.ImmutableSet;
2626
import com.google.common.collect.Maps;
2727
import java.io.IOException;
28+
import java.lang.reflect.Method;
2829
import java.util.ArrayList;
2930
import java.util.Arrays;
3031
import java.util.HashMap;
@@ -38,6 +39,7 @@
3839
import org.apache.gravitino.authorization.SecurableObject;
3940
import org.apache.gravitino.authorization.SecurableObjects;
4041
import org.apache.gravitino.client.GravitinoMetalake;
42+
import org.apache.gravitino.dto.MetalakeDTO;
4143
import org.apache.gravitino.exceptions.ForbiddenException;
4244
import org.apache.gravitino.integration.test.container.ContainerSuite;
4345
import org.apache.gravitino.integration.test.container.HiveContainer;
@@ -294,6 +296,82 @@ public void testDropTag() {
294296
gravitinoMetalake.deleteTag("tag1");
295297
}
296298

299+
@Test
300+
@Order(9)
301+
public void testTagOperationsWithNonExistentMetalake() throws Exception {
302+
// Test that all tag operations with @AuthorizationExpression return 403 Forbidden
303+
// when the metalake doesn't exist, instead of inconsistent 404 responses
304+
String nonExistentMetalake = "nonExistentMetalake";
305+
306+
// Access the restClient from normalUserClient using reflection
307+
Method restClientMethod =
308+
normalUserClient.getClass().getSuperclass().getDeclaredMethod("restClient");
309+
restClientMethod.setAccessible(true);
310+
Object restClient = restClientMethod.invoke(normalUserClient);
311+
312+
// Create a MetalakeDTO for the non-existent metalake
313+
MetalakeDTO metalakeDTO =
314+
MetalakeDTO.builder()
315+
.withName(nonExistentMetalake)
316+
.withComment("test")
317+
.withProperties(Maps.newHashMap())
318+
.withAudit(
319+
org.apache.gravitino.dto.AuditDTO.builder()
320+
.withCreator("test")
321+
.withCreateTime(java.time.Instant.now())
322+
.build())
323+
.build();
324+
325+
// Use DTOConverters.toMetaLake() via reflection to create GravitinoMetalake
326+
Class<?> dtoConvertersClass = Class.forName("org.apache.gravitino.client.DTOConverters");
327+
Method toMetaLakeMethod =
328+
dtoConvertersClass.getDeclaredMethod(
329+
"toMetaLake",
330+
MetalakeDTO.class,
331+
Class.forName("org.apache.gravitino.client.RESTClient"));
332+
toMetaLakeMethod.setAccessible(true);
333+
GravitinoMetalake nonExistentMetalakeObj =
334+
(GravitinoMetalake) toMetaLakeMethod.invoke(null, metalakeDTO, restClient);
335+
336+
// Test createTag - should return 403 ForbiddenException
337+
assertThrows(
338+
ForbiddenException.class,
339+
() -> nonExistentMetalakeObj.createTag("testTag", "comment", Maps.newHashMap()));
340+
341+
// Test getTag - should return 403 ForbiddenException
342+
assertThrows(ForbiddenException.class, () -> nonExistentMetalakeObj.getTag("testTag"));
343+
344+
// Test alterTag - should return 403 ForbiddenException
345+
assertThrows(
346+
ForbiddenException.class,
347+
() -> nonExistentMetalakeObj.alterTag("testTag", TagChange.rename("newName")));
348+
349+
// Test deleteTag - should return 403 ForbiddenException
350+
assertThrows(
351+
ForbiddenException.class,
352+
() -> {
353+
nonExistentMetalakeObj.deleteTag("testTag");
354+
});
355+
356+
// Test listTags - now has @AuthorizationExpression with empty expression,
357+
// should return 403 ForbiddenException
358+
assertThrows(ForbiddenException.class, nonExistentMetalakeObj::listTags);
359+
360+
// Test listTagsInfo - now has @AuthorizationExpression with empty expression,
361+
// should return 403 ForbiddenException
362+
assertThrows(ForbiddenException.class, nonExistentMetalakeObj::listTagsInfo);
363+
364+
// Test associateTags for metadata object - should return 403 ForbiddenException
365+
// when trying to associate tags to a catalog in a non-existent metalake
366+
assertThrows(
367+
ForbiddenException.class,
368+
() ->
369+
nonExistentMetalakeObj
370+
.loadCatalog(CATALOG)
371+
.supportsTags()
372+
.associateTags(new String[] {"testTag"}, null));
373+
}
374+
297375
private Column[] createColumns() {
298376
return new Column[] {Column.of("col1", Types.StringType.get())};
299377
}

‎gradle.properties‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ defaultScalaVersion = 2.12
3636
pythonVersion = 3.9
3737

3838
# skipDockerTests is used to skip the tests that require Docker to be running.
39-
skipDockerTests = true
39+
skipDockerTests = false
4040

4141
# enableFuse is used to enable the fuse module in the build.
4242
enableFuse = false

‎server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java‎

Lines changed: 56 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -37,9 +37,14 @@
3737
import org.aopalliance.intercept.MethodInvocation;
3838
import org.apache.commons.lang3.StringUtils;
3939
import org.apache.gravitino.Entity;
40+
import org.apache.gravitino.GravitinoEnv;
4041
import org.apache.gravitino.MetadataObject;
4142
import org.apache.gravitino.NameIdentifier;
4243
import org.apache.gravitino.authorization.AuthorizationUtils;
44+
import org.apache.gravitino.exceptions.ForbiddenException;
45+
import org.apache.gravitino.exceptions.MetalakeNotInUseException;
46+
import org.apache.gravitino.exceptions.NoSuchMetalakeException;
47+
import org.apache.gravitino.metalake.MetalakeManager;
4348
import org.apache.gravitino.server.authorization.annotations.AuthorizationExpression;
4449
import org.apache.gravitino.server.authorization.annotations.AuthorizationRequest;
4550
import org.apache.gravitino.server.authorization.expression.AuthorizationExpressionEvaluator;
@@ -131,37 +136,6 @@ public Object invoke(MethodInvocation methodInvocation) throws Throwable {
131136
AuthorizationExpression expressionAnnotation =
132137
method.getAnnotation(AuthorizationExpression.class);
133138

134-
// Check current user exists in metalake before authorization
135-
if (expressionAnnotation != null) {
136-
Object[] args = methodInvocation.getArguments();
137-
Map<Entity.EntityType, NameIdentifier> metadataContext =
138-
extractNameIdentifierFromParameters(parameters, args);
139-
140-
// Check if current user exists in the metalake.
141-
NameIdentifier metalakeIdent = metadataContext.get(Entity.EntityType.METALAKE);
142-
143-
if (metalakeIdent != null) {
144-
String currentUser = PrincipalUtils.getCurrentUserName();
145-
try {
146-
AuthorizationUtils.checkCurrentUser(metalakeIdent.name(), currentUser);
147-
} catch (org.apache.gravitino.exceptions.ForbiddenException ex) {
148-
LOG.warn(
149-
"User validation failed - User: {}, Metalake: {}, Reason: {}",
150-
currentUser,
151-
metalakeIdent.name(),
152-
ex.getMessage());
153-
return Utils.forbidden(ex.getMessage(), ex);
154-
} catch (Exception ex) {
155-
LOG.error(
156-
"Unexpected error during user validation - User: {}, Metalake: {}",
157-
currentUser,
158-
metalakeIdent.name(),
159-
ex);
160-
return Utils.internalError("Failed to validate user", ex);
161-
}
162-
}
163-
}
164-
165139
try {
166140
AuthorizationExecutor executor;
167141
if (expressionAnnotation != null) {
@@ -172,22 +146,57 @@ public Object invoke(MethodInvocation methodInvocation) throws Throwable {
172146
extractNameIdentifierFromParameters(parameters, args);
173147

174148
Map<String, Object> pathParams = Utils.extractPathParamsFromParameters(parameters, args);
175-
AuthorizationExpressionEvaluator authorizationExpressionEvaluator =
176-
new AuthorizationExpressionEvaluator(expression);
177-
AuthorizationRequest.RequestType requestType =
178-
extractAuthorizationRequestTypeFromParameters(parameters);
179-
executor =
180-
AuthorizeExecutorFactory.create(
181-
requestType,
182-
metadataContext,
183-
authorizationExpressionEvaluator,
184-
pathParams,
185-
entityType,
186-
parameters,
187-
args);
188-
boolean authorizeResult = executor.execute();
189-
if (!authorizeResult) {
190-
return buildNoAuthResponse(expressionAnnotation, metadataContext, method, expression);
149+
150+
// Check metalake and user existence before authorization
151+
NameIdentifier metalakeIdent = metadataContext.get(Entity.EntityType.METALAKE);
152+
if (metalakeIdent != null) {
153+
try {
154+
MetalakeManager.checkMetalake(
155+
metalakeIdent, GravitinoEnv.getInstance().entityStore());
156+
} catch (NoSuchMetalakeException | MetalakeNotInUseException ex) {
157+
// If metalake doesn't exist or is not in use, return no auth response
158+
return buildNoAuthResponse(expressionAnnotation, metadataContext, method, expression);
159+
}
160+
161+
String currentUser = PrincipalUtils.getCurrentUserName();
162+
try {
163+
AuthorizationUtils.checkCurrentUser(metalakeIdent.name(), currentUser);
164+
} catch (ForbiddenException ex) {
165+
LOG.warn(
166+
"User validation failed - User: {}, Metalake: {}, Reason: {}",
167+
currentUser,
168+
metalakeIdent.name(),
169+
ex.getMessage());
170+
return Utils.forbidden(ex.getMessage(), ex);
171+
} catch (Exception ex) {
172+
LOG.error(
173+
"Unexpected error during user validation - User: {}, Metalake: {}",
174+
currentUser,
175+
metalakeIdent.name(),
176+
ex);
177+
return Utils.internalError("Failed to validate user", ex);
178+
}
179+
}
180+
181+
// If expression is empty, skip authorization check (method handles its own filtering)
182+
if (StringUtils.isNotBlank(expression)) {
183+
AuthorizationExpressionEvaluator authorizationExpressionEvaluator =
184+
new AuthorizationExpressionEvaluator(expression);
185+
AuthorizationRequest.RequestType requestType =
186+
extractAuthorizationRequestTypeFromParameters(parameters);
187+
executor =
188+
AuthorizeExecutorFactory.create(
189+
requestType,
190+
metadataContext,
191+
authorizationExpressionEvaluator,
192+
pathParams,
193+
entityType,
194+
parameters,
195+
args);
196+
boolean authorizeResult = executor.execute();
197+
if (!authorizeResult) {
198+
return buildNoAuthResponse(expressionAnnotation, metadataContext, method, expression);
199+
}
191200
}
192201
}
193202
return methodInvocation.proceed();

‎server/src/main/java/org/apache/gravitino/server/web/rest/TagOperations.java‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -87,8 +87,10 @@ public TagOperations(TagDispatcher tagDispatcher) {
8787
@Produces("application/vnd.gravitino.v1+json")
8888
@Timed(name = "list-tags." + MetricNames.HTTP_PROCESS_DURATION, absolute = true)
8989
@ResponseMetered(name = "list-tags", absolute = true)
90+
@AuthorizationExpression(expression = "", accessMetadataType = MetadataObject.Type.METALAKE)
9091
public Response listTags(
91-
@PathParam("metalake") String metalake,
92+
@PathParam("metalake") @AuthorizationMetadata(type = Entity.EntityType.METALAKE)
93+
String metalake,
9294
@QueryParam("details") @DefaultValue("false") boolean verbose) {
9395
LOG.info(
9496
"Received list tag {} request for metalake: {}", verbose ? "infos" : "names", metalake);
@@ -261,8 +263,11 @@ public Response deleteTag(
261263
@Produces("application/vnd.gravitino.v1+json")
262264
@Timed(name = "list-objects-for-tag." + MetricNames.HTTP_PROCESS_DURATION, absolute = true)
263265
@ResponseMetered(name = "list-objects-for-tag", absolute = true)
266+
@AuthorizationExpression(expression = "", accessMetadataType = MetadataObject.Type.TAG)
264267
public Response listMetadataObjectsForTag(
265-
@PathParam("metalake") String metalake, @PathParam("tag") String tagName) {
268+
@PathParam("metalake") @AuthorizationMetadata(type = Entity.EntityType.METALAKE)
269+
String metalake,
270+
@PathParam("tag") @AuthorizationMetadata(type = Entity.EntityType.TAG) String tagName) {
266271
LOG.info("Received list objects for tag: {} under metalake: {}", tagName, metalake);
267272

268273
try {

0 commit comments

Comments
 (0)