smolnar82 commented on code in PR #1321: URL: https://github.com/apache/knox/pull/1321#discussion_r3639088265
########## gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/deploy/KnoxIDFAdminServiceDeploymentContributor.java: ########## @@ -0,0 +1,48 @@ +/* + * 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 + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * 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.knox.gateway.service.knoxidf.deploy; + +import org.apache.knox.gateway.jersey.JerseyServiceDeploymentContributorBase; + +/** + * Deployment contributor for the KNOXIDF_ADMIN service role, which hosts the + * Knox IDF admin REST API. This contributor initially registers + * {@link org.apache.knox.gateway.service.knoxidf.TrustedOidcIssuersResource}; + * it will be extended in a later task to also register DelegationAdminResource. + */ +public class KnoxIDFAdminServiceDeploymentContributor extends JerseyServiceDeploymentContributorBase { + + @Override + public String getRole() { + return "KNOXIDF_ADMIN"; + } + + @Override + public String getName() { + return "KNOXIDF_ADMIN"; + } + + @Override + protected String[] getPackages() { + return new String[] { "org.apache.knox.gateway.service.knoxidf" }; + } + + @Override + protected String[] getPatterns() { + return new String[] { "knoxidf/api/**?**" }; + } Review Comment: `KNOXIDF_ADMIN` and `KNOXIDF` share the same package AND URL pattern. Both contributors return: - `getPackages()` → `{"org.apache.knox.gateway.service.knoxidf"}` - `getPatterns()` → `{"knoxidf/api/**?**"}` Because Jersey scans the whole package, the `TrustedOidcIssuersResource` admin endpoints are served by the `KNOXIDF` role too, not just `KNOXIDF_ADMIN`. If the intent is for admin operations to sit behind stricter authorization (a separate role usually implies a separate authz/ACL binding in the topology), that intent is defeated: the same resource is reachable through the non-admin role. Conversely, deploying both roles in one topology with the identical knoxidf/api/**?** pattern invites a routing conflict. The test even notes this (`"Same prefix … so both roles can coexist … Jersey disambiguates by @Path"`), but path-based disambiguation doesn't give you role-based access control. Is `KNOXIDF_ADMIN` meant to be independently protected, and if so, shouldn't its package/pattern be scoped to the admin resources only? ########## gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/TrustedOidcIssuersResource.java: ########## @@ -0,0 +1,227 @@ +/* + * 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 + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * 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.knox.gateway.service.knoxidf; + +import com.fasterxml.jackson.core.type.TypeReference; +import com.fasterxml.jackson.databind.ObjectMapper; +import org.apache.knox.gateway.audit.api.Action; +import org.apache.knox.gateway.audit.api.ActionOutcome; +import org.apache.knox.gateway.audit.api.AuditServiceFactory; +import org.apache.knox.gateway.audit.api.Auditor; +import org.apache.knox.gateway.audit.api.ResourceType; +import org.apache.knox.gateway.audit.log4j.audit.AuditConstants; +import org.apache.knox.gateway.services.GatewayServices; +import org.apache.knox.gateway.services.ServiceType; +import org.apache.commons.lang3.StringUtils; +import org.apache.knox.gateway.services.knoxidf.trustedoidcissuer.TrustedOidcIssuer; +import org.apache.knox.gateway.services.knoxidf.trustedoidcissuer.TrustedOidcIssuerService; +import org.apache.knox.gateway.util.JsonUtils; + +import javax.annotation.PostConstruct; +import javax.servlet.ServletContext; +import javax.servlet.http.HttpServletRequest; +import javax.ws.rs.Consumes; +import javax.ws.rs.DELETE; +import javax.ws.rs.GET; +import javax.ws.rs.POST; +import javax.ws.rs.Path; +import javax.ws.rs.Produces; +import javax.ws.rs.QueryParam; +import javax.ws.rs.core.Context; +import javax.ws.rs.core.MediaType; +import javax.ws.rs.core.Response; +import java.io.IOException; +import java.net.URI; +import java.net.URISyntaxException; +import java.security.Principal; +import java.time.Instant; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.stream.Collectors; + +import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.BASE_RESORCE_PATH; + +@Path(TrustedOidcIssuersResource.RESOURCE_PATH) +@Produces(MediaType.APPLICATION_JSON) +public class TrustedOidcIssuersResource { + + static final String RESOURCE_PATH = BASE_RESORCE_PATH + "/admin/trusted-oidc-issuers"; + + // Non-final and package-private to allow test injection of a mock Auditor. + static Auditor auditor = AuditServiceFactory.getAuditService() + .getAuditor(AuditConstants.DEFAULT_AUDITOR_NAME, + AuditConstants.KNOX_SERVICE_NAME, AuditConstants.KNOX_COMPONENT_NAME); + + @Context + private ServletContext servletContext; + + @Context + private HttpServletRequest request; + + private TrustedOidcIssuerService trustedIssuers; + + @PostConstruct + public void init() { + final GatewayServices services = (GatewayServices) + servletContext.getAttribute(GatewayServices.GATEWAY_SERVICES_ATTRIBUTE); + trustedIssuers = services.getService(ServiceType.TRUSTED_OIDC_ISSUER_SERVICE); + } + + @POST + @Consumes(MediaType.APPLICATION_JSON) + public Response registerIssuer(String body) { + String issuerUrl = "INVALID_REQUEST"; + final String operatorId = getOperatorId(); + String outcome = ActionOutcome.FAILURE; + + try { + final Map<String, Object> parsed; + try { + parsed = new ObjectMapper().readValue(body, new TypeReference<Map<String, Object>>() {}); Review Comment: `ObjectMapper` is expensive to construct and thread-safe to reuse: make it a `private static final`, or parse via `JsonUtils` for consistency with the rest of the class. ########## pom.xml: ########## @@ -259,6 +259,7 @@ <mina.version>2.2.8</mina.version> <netty.version>4.1.135.Final</netty.version> <nimbus-jose-jwt.version>10.9.1</nimbus-jose-jwt.version> + <oauth2-oidc-sdk.version>11.37.2</oauth2-oidc-sdk.version> Review Comment: This must be left here because of the recent changes in #1320 -- 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]
