dimas-b commented on code in PR #848:
URL: https://github.com/apache/polaris/pull/848#discussion_r1929243872
##########
integration-tests/src/main/java/org/apache/polaris/service/it/env/PolarisApiEndpoints.java:
##########
@@ -28,7 +28,11 @@
*/
public final class PolarisApiEndpoints implements Serializable {
- public static String REALM_HEADER = "realm";
+ /**
+ * The header name for the realm ID. Tests must make sure that Polaris is
configured with this
+ * header name.
+ */
+ public static String REALM_HEADER = "Polaris-Realm";
Review Comment:
Would you mind moving this to a (new) method on `PolarisServerManager` or
`Server` so that it is possible to customize?
##########
service/common/src/main/java/org/apache/polaris/service/context/DefaultRealmIdResolver.java:
##########
@@ -36,15 +37,15 @@ public DefaultRealmIdResolver(RealmContextConfiguration
configuration) {
}
@Override
- public RealmId resolveRealmContext(
+ public RealmId resolveRealmId(
String requestURL, String method, String path, Map<String, String>
headers) {
String realm;
if (headers.containsKey(configuration.headerName())) {
realm = headers.get(configuration.headerName());
if (!configuration.realms().contains(realm)) {
- throw new IllegalArgumentException("Unknown realm: " + realm);
+ throw new NotAuthorizedException("Unknown realm: " + realm);
}
} else {
realm = configuration.defaultRealm();
Review Comment:
I think it could be valuable to cover this with an explicit config flag. If
realms are used in a particular deployment, it is probably not wise to allow
potentially malicious clients to try sending requests at the default realm
without knowing its ID.
##########
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/QuarkusProducers.java:
##########
@@ -100,13 +100,8 @@ public PolarisDiagnostics polarisDiagnostics() {
@Produces
@RequestScoped
- public RealmId realmId(@Context HttpServerRequest request, RealmIdResolver
realmIdResolver) {
- return realmIdResolver.resolveRealmContext(
- request.absoluteURI(),
- request.method().name(),
- request.path(),
- request.headers().entries().stream()
- .collect(HashMap::new, (m, e) -> m.put(e.getKey(), e.getValue()),
HashMap::putAll));
+ public RealmId realmId(@Context ContainerRequestContext request) {
+ return (RealmId) request.getProperty(RealmIdFilter.REALM_ID_KEY);
Review Comment:
Should we check that the property is present (as it should at this
stage)?... just for extra safety.
##########
service/common/src/main/java/org/apache/polaris/service/context/RealmIdFilter.java:
##########
@@ -0,0 +1,68 @@
+/*
+ * 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.polaris.service.context;
+
+import jakarta.annotation.Priority;
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.inject.Inject;
+import jakarta.ws.rs.NotAuthorizedException;
+import jakarta.ws.rs.container.ContainerRequestContext;
+import jakarta.ws.rs.container.ContainerRequestFilter;
+import jakarta.ws.rs.container.PreMatching;
+import jakarta.ws.rs.core.Response;
+import jakarta.ws.rs.core.Response.Status;
+import jakarta.ws.rs.ext.Provider;
+import java.util.HashMap;
+import java.util.Map;
+import org.apache.polaris.core.context.RealmId;
+import org.apache.polaris.service.config.PolarisFilterPriorities;
+
+@PreMatching
+@ApplicationScoped
+@Priority(PolarisFilterPriorities.REALM_ID_FILTER)
+@Provider
+public class RealmIdFilter implements ContainerRequestFilter {
+
+ public static final String REALM_ID_KEY = "realmId";
+
+ @Inject RealmIdResolver realmIdResolver;
+
+ @Override
+ public void filter(ContainerRequestContext rc) {
+ RealmId realmId = null;
+ try {
+ realmId = resolveRealmContext(rc);
+ } catch (NotAuthorizedException e) {
+ rc.abortWith(Response.status(Status.UNAUTHORIZED).build());
Review Comment:
I'm not sure about 401 in this context. I believe it generally means your
credentials are not valid, which is not applicable to failures in realm
resolution.
I'm thinking that a 404 may fit better. For example, GH gives 404 on
accessing repositories (i.e. realms) to which the user has no access.
##########
integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisApplicationIntegrationTest.java:
##########
@@ -657,4 +663,52 @@ public void testRequestBodyTooLarge() throws Exception {
});
}
}
+
+ @Test
+ public void testNoRealmHeader() {
+ try (Response response =
+ managementApi
+ .request(
+ "v1/catalogs", Map.of(), Map.of(), Map.of("Authorization",
"Bearer " + authToken))
+ .get()) {
+ assertThat(response.getStatus()).isEqualTo(Status.OK.getStatusCode());
+ Catalogs roles = response.readEntity(Catalogs.class);
+
assertThat(roles.getCatalogs()).extracting(Catalog::getName).contains(internalCatalogName);
+ }
+ }
+
+ @ParameterizedTest
+ @ValueSource(strings = {"POLARIS", "OTHER"})
+ public void testRealmHeaderValid(String realmId) {
+ try (Response response =
+ managementApi
+ .request(
+ "v1/catalogs",
+ Map.of(),
+ Map.of(),
+ Map.of("Authorization", "Bearer " + authToken, REALM_HEADER,
realmId))
+ .get()) {
+ assertThat(response.getStatus()).isEqualTo(Status.OK.getStatusCode());
+ Catalogs roles = response.readEntity(Catalogs.class);
+ if ("POLARIS".equals(realmId)) {
+
assertThat(roles.getCatalogs()).extracting(Catalog::getName).contains(internalCatalogName);
Review Comment:
nit: maybe make `internalCatalogName` a parameter to this test?
--
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]
