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 a8411c02d9 Change the Forbidden error to Unauthorized (#11501)
a8411c02d9 is described below
commit a8411c02d92b10af462592f49e7b10c0c2cb8f76
Author: Abhishek Sharma <[email protected]>
AuthorDate: Sun Sep 17 16:13:01 2023 -0400
Change the Forbidden error to Unauthorized (#11501)
---
.../apache/pinot/broker/broker/BasicAuthAccessControlFactory.java | 7 +++----
.../pinot/broker/broker/ZkBasicAuthAccessControlFactory.java | 4 ++--
.../org/apache/pinot/broker/broker/BasicAuthAccessControlTest.java | 7 ++++++-
.../org/apache/pinot/controller/api/access/AccessControlUtils.java | 4 ++++
.../pinot/controller/api/access/BasicAuthAccessControlFactory.java | 6 +++++-
.../pinot/integration/tests/BasicAuthBatchIntegrationTest.java | 4 ++--
6 files changed, 22 insertions(+), 10 deletions(-)
diff --git
a/pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java
b/pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java
index 1eb134cfa7..e99006f592 100644
---
a/pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java
+++
b/pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java
@@ -25,6 +25,7 @@ import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;
+import javax.ws.rs.NotAuthorizedException;
import org.apache.pinot.broker.api.AccessControl;
import org.apache.pinot.broker.api.HttpRequesterIdentity;
import org.apache.pinot.broker.api.RequesterIdentity;
@@ -86,8 +87,7 @@ public class BasicAuthAccessControlFactory extends
AccessControlFactory {
Optional<BasicAuthPrincipal> principalOpt =
getPrincipalOpt(requesterIdentity);
if (!principalOpt.isPresent()) {
- // no matching token? reject
- return false;
+ throw new NotAuthorizedException("Basic");
}
BasicAuthPrincipal principal = principalOpt.get();
@@ -105,8 +105,7 @@ public class BasicAuthAccessControlFactory extends
AccessControlFactory {
Optional<BasicAuthPrincipal> principalOpt =
getPrincipalOpt(requesterIdentity);
if (!principalOpt.isPresent()) {
- // no matching token? reject
- return false;
+ throw new NotAuthorizedException("Basic");
}
if (tables == null || tables.isEmpty()) {
diff --git
a/pinot-broker/src/main/java/org/apache/pinot/broker/broker/ZkBasicAuthAccessControlFactory.java
b/pinot-broker/src/main/java/org/apache/pinot/broker/broker/ZkBasicAuthAccessControlFactory.java
index 9541492818..168d1a3ddf 100644
---
a/pinot-broker/src/main/java/org/apache/pinot/broker/broker/ZkBasicAuthAccessControlFactory.java
+++
b/pinot-broker/src/main/java/org/apache/pinot/broker/broker/ZkBasicAuthAccessControlFactory.java
@@ -26,6 +26,7 @@ import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;
+import javax.ws.rs.NotAuthorizedException;
import org.apache.helix.store.zk.ZkHelixPropertyStore;
import org.apache.helix.zookeeper.datamodel.ZNRecord;
import org.apache.pinot.broker.api.AccessControl;
@@ -99,8 +100,7 @@ public class ZkBasicAuthAccessControlFactory extends
AccessControlFactory {
public boolean hasAccess(RequesterIdentity requesterIdentity,
Set<String> tables) {
Optional<ZkBasicAuthPrincipal> principalOpt =
getPrincipalAuth(requesterIdentity);
if (!principalOpt.isPresent()) {
- // no matching token? reject
- return false;
+ throw new NotAuthorizedException("Basic");
}
if (tables == null || tables.isEmpty()) {
return true;
diff --git
a/pinot-broker/src/test/java/org/apache/pinot/broker/broker/BasicAuthAccessControlTest.java
b/pinot-broker/src/test/java/org/apache/pinot/broker/broker/BasicAuthAccessControlTest.java
index a491e4f402..42f121ecef 100644
---
a/pinot-broker/src/test/java/org/apache/pinot/broker/broker/BasicAuthAccessControlTest.java
+++
b/pinot-broker/src/test/java/org/apache/pinot/broker/broker/BasicAuthAccessControlTest.java
@@ -24,6 +24,7 @@ import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;
+import javax.ws.rs.WebApplicationException;
import org.apache.pinot.broker.api.AccessControl;
import org.apache.pinot.broker.api.HttpRequesterIdentity;
import org.apache.pinot.common.request.BrokerRequest;
@@ -75,7 +76,11 @@ public class BasicAuthAccessControlTest {
HttpRequesterIdentity identity = new HttpRequesterIdentity();
identity.setHttpHeaders(headers);
- Assert.assertFalse(_accessControl.hasAccess(identity, (BrokerRequest)
null));
+ try {
+ _accessControl.hasAccess(identity, (BrokerRequest) null);
+ } catch (WebApplicationException e) {
+ Assert.assertEquals(e.getResponse().getStatus(), 401, "must return 401");
+ }
}
@Test
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 59c0b3a9d4..614d782016 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
@@ -20,6 +20,7 @@
package org.apache.pinot.controller.api.access;
import javax.annotation.Nullable;
+import javax.ws.rs.WebApplicationException;
import javax.ws.rs.core.HttpHeaders;
import javax.ws.rs.core.Response;
import org.apache.commons.lang3.StringUtils;
@@ -63,6 +64,9 @@ public final class AccessControlUtils {
return;
}
}
+ } 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);
diff --git
a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/BasicAuthAccessControlFactory.java
b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/BasicAuthAccessControlFactory.java
index 9f53659c13..3283064934 100644
---
a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/BasicAuthAccessControlFactory.java
+++
b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/BasicAuthAccessControlFactory.java
@@ -24,6 +24,7 @@ import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.stream.Collectors;
+import javax.ws.rs.NotAuthorizedException;
import javax.ws.rs.core.HttpHeaders;
import org.apache.pinot.core.auth.BasicAuthPrincipal;
import org.apache.pinot.core.auth.BasicAuthUtils;
@@ -88,7 +89,10 @@ public class BasicAuthAccessControlFactory implements
AccessControlFactory {
@Override
public boolean hasAccess(AccessType accessType, HttpHeaders httpHeaders,
String endpointUrl) {
- return getPrincipal(httpHeaders).isPresent();
+ if (getPrincipal(httpHeaders).isEmpty()) {
+ throw new NotAuthorizedException("Basic");
+ }
+ return true;
}
private Optional<BasicAuthPrincipal> getPrincipal(HttpHeaders headers) {
diff --git
a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/BasicAuthBatchIntegrationTest.java
b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/BasicAuthBatchIntegrationTest.java
index fb209db1f4..4421852434 100644
---
a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/BasicAuthBatchIntegrationTest.java
+++
b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/BasicAuthBatchIntegrationTest.java
@@ -104,7 +104,7 @@ public class BasicAuthBatchIntegrationTest extends
ClusterTest {
sendPostRequest("http://localhost:"; + getRandomBrokerPort() +
"/query/sql", "{\"sql\":\"SELECT now()\"}");
} catch (IOException e) {
HttpErrorStatusException httpErrorStatusException =
(HttpErrorStatusException) e.getCause();
- Assert.assertEquals(httpErrorStatusException.getStatusCode(), 403, "must
return 403");
+ Assert.assertEquals(httpErrorStatusException.getStatusCode(), 401, "must
return 401");
}
}
@@ -134,7 +134,7 @@ public class BasicAuthBatchIntegrationTest extends
ClusterTest {
// NOTE: the endpoint is protected implicitly (without annotation) by
BasicAuthAccessControlFactory
sendGetRequest("http://localhost:"; + getControllerPort() + "/tables");
} catch (IOException e) {
- Assert.assertTrue(e.getMessage().contains("403"));
+ Assert.assertTrue(e.getMessage().contains("401"));
}
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
