This is an automated email from the ASF dual-hosted git repository.
jackie pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git
The following commit(s) were added to refs/heads/master by this push:
new fed2d5f1b6 update access control check error handling to catch
throwable and log errors (#13209)
fed2d5f1b6 is described below
commit fed2d5f1b613371237b5a29348f0c043200671ad
Author: dang-stripe <50119799+dang-str...@users.noreply.github.com>
AuthorDate: Fri May 24 14:15:51 2024 -0700
update access control check error handling to catch throwable and log
errors (#13209)
---
.../controller/api/access/AccessControlUtils.java | 8 +-
.../api/access/AccessControlUtilsTest.java | 75 +++++++++++++++++
.../pinot/core/auth/FineGrainedAuthUtils.java | 19 ++++-
.../pinot/core/auth/FineGrainedAuthUtilsTest.java | 94 ++++++++++++++++++++++
4 files changed, 192 insertions(+), 4 deletions(-)
diff --git
a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControlUtils.java
b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControlUtils.java
index 4e56339a4c..825776e38f 100644
---
a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControlUtils.java
+++
b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControlUtils.java
@@ -67,9 +67,11 @@ public final class AccessControlUtils {
} catch (WebApplicationException exception) {
// throwing the exception if it's WebApplicationException
throw exception;
- } catch (Exception e) {
- throw new ControllerApplicationException(LOGGER, "Caught exception while
validating permission for "
- + userMessage, Response.Status.INTERNAL_SERVER_ERROR, e);
+ } catch (Throwable t) {
+ // catch and log Throwable for NoSuchMethodError which can happen when
there are classpath conflicts
+ // otherwise, grizzly will return a 500 without any logs or indication
of what failed
+ throw new ControllerApplicationException(LOGGER,
+ "Caught exception while validating permission for " + userMessage,
Response.Status.INTERNAL_SERVER_ERROR, t);
}
throw new ControllerApplicationException(LOGGER, "Permission is denied for
" + userMessage,
diff --git
a/pinot-controller/src/test/java/org/apache/pinot/controller/api/access/AccessControlUtilsTest.java
b/pinot-controller/src/test/java/org/apache/pinot/controller/api/access/AccessControlUtilsTest.java
new file mode 100644
index 0000000000..5270414d34
--- /dev/null
+++
b/pinot-controller/src/test/java/org/apache/pinot/controller/api/access/AccessControlUtilsTest.java
@@ -0,0 +1,75 @@
+/**
+ * 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.pinot.controller.api.access;
+
+import javax.ws.rs.core.HttpHeaders;
+import javax.ws.rs.core.Response;
+import
org.apache.pinot.controller.api.exception.ControllerApplicationException;
+import org.mockito.Mockito;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+
+public class AccessControlUtilsTest {
+
+ private final String _table = "testTable";
+ private final String _endpoint = "/testEndpoint";
+
+ @Test
+ public void testValidatePermissionAllowed() {
+ AccessControl ac = Mockito.mock(AccessControl.class);
+ HttpHeaders mockHttpHeaders = Mockito.mock(HttpHeaders.class);
+
+ Mockito.when(ac.hasAccess(_table, AccessType.READ, mockHttpHeaders,
_endpoint)).thenReturn(true);
+
+ AccessControlUtils.validatePermission(_table, AccessType.READ,
mockHttpHeaders, _endpoint, ac);
+ }
+
+ @Test
+ public void testValidatePermissionDenied() {
+ AccessControl ac = Mockito.mock(AccessControl.class);
+ HttpHeaders mockHttpHeaders = Mockito.mock(HttpHeaders.class);
+
+ Mockito.when(ac.hasAccess(_table, AccessType.READ, mockHttpHeaders,
_endpoint)).thenReturn(false);
+
+ try {
+ AccessControlUtils.validatePermission(_table, AccessType.READ,
mockHttpHeaders, _endpoint, ac);
+ Assert.fail("Expected ControllerApplicationException");
+ } catch (ControllerApplicationException e) {
+ Assert.assertTrue(e.getMessage().contains("Permission is denied"));
+ Assert.assertEquals(e.getResponse().getStatus(),
Response.Status.FORBIDDEN.getStatusCode());
+ }
+ }
+
+ @Test
+ public void testValidatePermissionWithNoSuchMethodError() {
+ AccessControl ac = Mockito.mock(AccessControl.class);
+ HttpHeaders mockHttpHeaders = Mockito.mock(HttpHeaders.class);
+
+ Mockito.when(ac.hasAccess(_table, AccessType.READ, mockHttpHeaders,
_endpoint))
+ .thenThrow(new NoSuchMethodError("Method not found"));
+
+ try {
+ AccessControlUtils.validatePermission(_table, AccessType.READ,
mockHttpHeaders, _endpoint, ac);
+ } catch (ControllerApplicationException e) {
+ Assert.assertTrue(e.getMessage().contains("Caught exception while
validating permission"));
+ Assert.assertEquals(e.getResponse().getStatus(),
Response.Status.INTERNAL_SERVER_ERROR.getStatusCode());
+ }
+ }
+}
diff --git
a/pinot-core/src/main/java/org/apache/pinot/core/auth/FineGrainedAuthUtils.java
b/pinot-core/src/main/java/org/apache/pinot/core/auth/FineGrainedAuthUtils.java
index 4a93bc3f10..ba8854818c 100644
---
a/pinot-core/src/main/java/org/apache/pinot/core/auth/FineGrainedAuthUtils.java
+++
b/pinot-core/src/main/java/org/apache/pinot/core/auth/FineGrainedAuthUtils.java
@@ -28,12 +28,17 @@ import javax.ws.rs.core.UriInfo;
import org.apache.commons.lang.StringUtils;
import org.apache.pinot.common.utils.DatabaseUtils;
import org.apache.pinot.spi.utils.builder.TableNameBuilder;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* Utility methods to share in Broker and Controller request filters related
to fine grain authorization.
*/
public class FineGrainedAuthUtils {
+
+ private static final Logger LOGGER =
LoggerFactory.getLogger(FineGrainedAuthUtils.class);
+
private FineGrainedAuthUtils() {
}
@@ -106,8 +111,20 @@ public class FineGrainedAuthUtils {
Response.Status.INTERNAL_SERVER_ERROR);
}
+ boolean hasAccess;
+ try {
+ hasAccess = accessControl.hasAccess(httpHeaders, auth.targetType(),
targetId, auth.action());
+ } catch (Throwable t) {
+ // catch and log Throwable for NoSuchMethodError which can happen when
there are classpath conflicts
+ // otherwise, grizzly will return a 500 without any logs or indication
of what failed
+ String errorMsg = String.format("Failed to check for access for target
type %s and target ID %s with action %s",
+ auth.targetType(), targetId, auth.action());
+ LOGGER.error(errorMsg, t);
+ throw new WebApplicationException(errorMsg, t,
Response.Status.INTERNAL_SERVER_ERROR);
+ }
+
// Check for access now
- if (!accessControl.hasAccess(httpHeaders, auth.targetType(), targetId,
auth.action())) {
+ if (!hasAccess) {
throw new WebApplicationException(accessDeniedMsg,
Response.Status.FORBIDDEN);
}
} else if (!accessControl.defaultAccess(httpHeaders)) {
diff --git
a/pinot-core/src/test/java/org/apache/pinot/core/auth/FineGrainedAuthUtilsTest.java
b/pinot-core/src/test/java/org/apache/pinot/core/auth/FineGrainedAuthUtilsTest.java
new file mode 100644
index 0000000000..c18f30ecf6
--- /dev/null
+++
b/pinot-core/src/test/java/org/apache/pinot/core/auth/FineGrainedAuthUtilsTest.java
@@ -0,0 +1,94 @@
+/**
+ * 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.pinot.core.auth;
+
+import java.lang.reflect.Method;
+import javax.ws.rs.WebApplicationException;
+import javax.ws.rs.core.HttpHeaders;
+import javax.ws.rs.core.Response;
+import javax.ws.rs.core.UriInfo;
+import org.mockito.Mockito;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+
+public class FineGrainedAuthUtilsTest {
+
+ @Test
+ public void testValidateFineGrainedAuthAllowed() {
+ FineGrainedAccessControl ac = Mockito.mock(FineGrainedAccessControl.class);
+ Mockito.when(ac.hasAccess(Mockito.any(HttpHeaders.class), Mockito.any(),
Mockito.any(), Mockito.any()))
+ .thenReturn(true);
+
+ UriInfo mockUriInfo = Mockito.mock(UriInfo.class);
+ HttpHeaders mockHttpHeaders = Mockito.mock(HttpHeaders.class);
+
+ FineGrainedAuthUtils.validateFineGrainedAuth(getAnnotatedMethod(),
mockUriInfo, mockHttpHeaders, ac);
+ }
+
+ @Test
+ public void testValidateFineGrainedAuthDenied() {
+ FineGrainedAccessControl ac = Mockito.mock(FineGrainedAccessControl.class);
+ Mockito.when(ac.hasAccess(Mockito.any(HttpHeaders.class), Mockito.any(),
Mockito.any(), Mockito.any()))
+ .thenReturn(false);
+
+ UriInfo mockUriInfo = Mockito.mock(UriInfo.class);
+ HttpHeaders mockHttpHeaders = Mockito.mock(HttpHeaders.class);
+
+ try {
+ FineGrainedAuthUtils.validateFineGrainedAuth(getAnnotatedMethod(),
mockUriInfo, mockHttpHeaders, ac);
+ Assert.fail("Expected WebApplicationException");
+ } catch (WebApplicationException e) {
+ Assert.assertTrue(e.getMessage().contains("Access denied to getCluster
in the cluster"));
+ Assert.assertEquals(e.getResponse().getStatus(),
Response.Status.FORBIDDEN.getStatusCode());
+ }
+ }
+
+ @Test
+ public void testValidateFineGrainedAuthWithNoSuchMethodError() {
+ FineGrainedAccessControl ac = Mockito.mock(FineGrainedAccessControl.class);
+ Mockito.when(ac.hasAccess(Mockito.any(HttpHeaders.class), Mockito.any(),
Mockito.any(), Mockito.any()))
+ .thenThrow(new NoSuchMethodError("Method not found"));
+
+ UriInfo mockUriInfo = Mockito.mock(UriInfo.class);
+ HttpHeaders mockHttpHeaders = Mockito.mock(HttpHeaders.class);
+
+ try {
+ FineGrainedAuthUtils.validateFineGrainedAuth(getAnnotatedMethod(),
mockUriInfo, mockHttpHeaders, ac);
+ Assert.fail("Expected WebApplicationException");
+ } catch (WebApplicationException e) {
+ Assert.assertTrue(e.getMessage().contains("Failed to check for access"));
+ Assert.assertEquals(e.getResponse().getStatus(),
Response.Status.INTERNAL_SERVER_ERROR.getStatusCode());
+ }
+ }
+
+ static class TestResource {
+ @Authorize(targetType = TargetType.CLUSTER, action = "getCluster")
+ void getCluster() {
+ }
+ }
+
+ private Method getAnnotatedMethod() {
+ try {
+ return TestResource.class.getDeclaredMethod("getCluster");
+ } catch (NoSuchMethodException e) {
+ throw new RuntimeException(e);
+ }
+ }
+}
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
