lasdf1234 commented on code in PR #10971: URL: https://github.com/apache/gravitino/pull/10971#discussion_r3213111009
########## server/src/main/java/org/apache/gravitino/server/web/rest/IdpGroupOperations.java: ########## @@ -0,0 +1,146 @@ +/* + * 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.gravitino.server.web.rest; + +import com.codahale.metrics.annotation.ResponseMetered; +import com.codahale.metrics.annotation.Timed; +import javax.servlet.http.HttpServletRequest; +import javax.ws.rs.DELETE; +import javax.ws.rs.DefaultValue; +import javax.ws.rs.GET; +import javax.ws.rs.POST; +import javax.ws.rs.PUT; +import javax.ws.rs.Path; +import javax.ws.rs.PathParam; +import javax.ws.rs.Produces; +import javax.ws.rs.QueryParam; +import javax.ws.rs.core.Context; +import javax.ws.rs.core.Response; +import org.apache.gravitino.authorization.IdpGroupManager; +import org.apache.gravitino.dto.requests.CreateGroupRequest; +import org.apache.gravitino.dto.requests.UpdateGroupUsersRequest; +import org.apache.gravitino.dto.responses.IdpGroupResponse; +import org.apache.gravitino.dto.responses.RemoveResponse; +import org.apache.gravitino.metrics.MetricNames; +import org.apache.gravitino.server.web.Utils; + +@Path("/idp/groups") +public class IdpGroupOperations { + + private final IdpGroupManager groupManager; + + @Context private HttpServletRequest httpRequest; + + public IdpGroupOperations() { + this(IdpGroupManager.fromEnvironment()); + } + + IdpGroupOperations(IdpGroupManager groupManager) { + this.groupManager = groupManager; + } + + @GET + @Path("{group}") + @Produces("application/vnd.gravitino.v1+json") + @Timed(name = "get-idp-group." + MetricNames.HTTP_PROCESS_DURATION, absolute = true) + @ResponseMetered(name = "get-idp-group", absolute = true) + public Response getGroup(@PathParam("group") String group) { + try { + return Utils.doAs( + httpRequest, () -> Utils.ok(new IdpGroupResponse(groupManager.getGroup(group)))); + } catch (Exception e) { + return ExceptionHandlers.handleGroupException(OperationType.GET, group, "", e); + } + } + + @POST + @Produces("application/vnd.gravitino.v1+json") + @Timed(name = "add-idp-group." + MetricNames.HTTP_PROCESS_DURATION, absolute = true) + @ResponseMetered(name = "add-idp-group", absolute = true) + public Response addGroup(CreateGroupRequest request) { + try { + return Utils.doAs( + httpRequest, + () -> { + request.validate(); + return Utils.ok(new IdpGroupResponse(groupManager.createGroup(request.getGroup()))); + }); + } catch (Exception e) { + return ExceptionHandlers.handleGroupException(OperationType.ADD, request.getGroup(), "", e); + } + } + + @DELETE + @Path("{group}") + @Produces("application/vnd.gravitino.v1+json") + @Timed(name = "remove-idp-group." + MetricNames.HTTP_PROCESS_DURATION, absolute = true) + @ResponseMetered(name = "remove-idp-group", absolute = true) + public Response removeGroup( + @PathParam("group") String group, @DefaultValue("false") @QueryParam("force") boolean force) { + try { + return Utils.doAs( + httpRequest, + () -> { + boolean removed = groupManager.deleteGroup(group, force); + return Utils.ok(new RemoveResponse(removed)); + }); + } catch (Exception e) { + return ExceptionHandlers.handleGroupException(OperationType.REMOVE, group, "", e); + } + } + + @PUT + @Path("{group}/add") + @Produces("application/vnd.gravitino.v1+json") + @Timed(name = "add-idp-group-user." + MetricNames.HTTP_PROCESS_DURATION, absolute = true) + @ResponseMetered(name = "add-idp-group-user", absolute = true) + public Response addUsers(@PathParam("group") String group, UpdateGroupUsersRequest request) { + try { + return Utils.doAs( + httpRequest, + () -> { + request.validate(); + return Utils.ok( + new IdpGroupResponse(groupManager.addUsersToGroup(group, request.getUsers()))); + }); + } catch (Exception e) { Review Comment: Fixed. `IdpGroupOperations#addUsers` now performs an explicit null-body check and returns a 400-style illegal-arguments response before validation. ########## core/src/main/java/org/apache/gravitino/GravitinoEnv.java: ########## @@ -685,5 +702,7 @@ private void initGravitinoServerComponents() { BuiltInJobTemplateEventListener builtInJobTemplateListener = new BuiltInJobTemplateEventListener(jobManager, entityStore, idGenerator); eventListenerManager.addEventListener("builtin-job-template", builtInJobTemplateListener); + + this.idpManager = IdpManagerFactory.create(); Review Comment: Fixed. `GravitinoEnv` now uses `IdpManagerFactory.createOrDefault()`, so startup no longer fails when no IdP provider is present on the runtime classpath. In that case, the IdP endpoints surface a clear unsupported-operation error instead of breaking server initialization. ########## core/src/main/java/org/apache/gravitino/authorization/IdpManagerFactory.java: ########## @@ -0,0 +1,41 @@ +/* + * 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.gravitino.authorization; + +import java.util.ServiceLoader; + +/** Factory for loading built-in IdP manager implementations from the runtime classpath. */ +public final class IdpManagerFactory { + + private IdpManagerFactory() {} + + /** Create the built-in IdP manager implementation. */ + public static IdpManager create() { + return loadService(IdpManager.class); + } + + private static <T> T loadService(Class<T> serviceClass) { + for (T service : ServiceLoader.load(serviceClass)) { + return service; + } + + throw new IllegalStateException( + String.format("No %s implementation found", serviceClass.getSimpleName())); Review Comment: Fixed. `IdpManagerFactory` now enforces a single provider and throws if multiple `IdpManager` implementations are present, removing the previous order-dependent behavior. ########## plugins/idp-basic/src/main/java/org/apache/gravitino/idp/basic/authorization/IdpManager.java: ########## @@ -0,0 +1,331 @@ +/* + * 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.gravitino.idp.basic.authorization; + +import com.google.common.base.Preconditions; +import java.util.ArrayList; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Optional; +import java.util.Set; +import java.util.stream.Collectors; +import org.apache.commons.lang3.StringUtils; +import org.apache.gravitino.Config; +import org.apache.gravitino.Configs; +import org.apache.gravitino.GravitinoEnv; +import org.apache.gravitino.dto.IdpGroupDTO; +import org.apache.gravitino.dto.IdpUserDTO; +import org.apache.gravitino.exceptions.ForbiddenException; +import org.apache.gravitino.exceptions.GroupAlreadyExistsException; +import org.apache.gravitino.exceptions.NoSuchGroupException; +import org.apache.gravitino.exceptions.NoSuchUserException; +import org.apache.gravitino.exceptions.UserAlreadyExistsException; +import org.apache.gravitino.idp.basic.password.PasswordHasher; +import org.apache.gravitino.idp.basic.password.PasswordHasherFactory; +import org.apache.gravitino.storage.IdGenerator; +import org.apache.gravitino.storage.relational.po.IdpGroupPO; +import org.apache.gravitino.storage.relational.po.IdpGroupUserRelPO; +import org.apache.gravitino.storage.relational.po.IdpUserPO; +import org.apache.gravitino.storage.relational.service.IdpGroupMetaService; +import org.apache.gravitino.storage.relational.service.IdpUserMetaService; +import org.apache.gravitino.utils.PrincipalUtils; + +/** + * Built-in IdP manager implementation loaded from the {@code idp-basic} plugin. + * + * <p>This implementation manages both built-in IdP users and groups and restricts mutation + * operations to Gravitino service admins. + */ +public class IdpManager implements org.apache.gravitino.authorization.IdpManager { + private static final long INITIAL_VERSION = 1L; + + private final Config config; + private final IdGenerator idGenerator; + private final IdpUserMetaService userMetaService; + private final IdpGroupMetaService groupMetaService; + private final PasswordHasher passwordHasher; + + public IdpManager() { + this( + GravitinoEnv.getInstance().config(), + GravitinoEnv.getInstance().idGenerator(), + IdpUserMetaService.getInstance(), + IdpGroupMetaService.getInstance(), + PasswordHasherFactory.create()); + } + + IdpManager( + Config config, + IdGenerator idGenerator, + IdpUserMetaService userMetaService, + IdpGroupMetaService groupMetaService, + PasswordHasher passwordHasher) { + this.config = config; + this.idGenerator = idGenerator; + this.userMetaService = userMetaService; + this.groupMetaService = groupMetaService; + this.passwordHasher = passwordHasher; + } + + @Override + public IdpUserDTO createUser(String userName, String password) { + ensureServiceAdmin(); + validateUserName(userName); + validatePassword(password); + if (userMetaService().findUser(userName).isPresent()) { + throw new UserAlreadyExistsException("Built-in IdP user %s already exists", userName); + } + + userMetaService() + .createUser( + IdpUserPO.builder() + .withUserId(nextId()) + .withUserName(userName) + .withPasswordHash(passwordHasher.hash(password)) + .withCurrentVersion(INITIAL_VERSION) + .withLastVersion(INITIAL_VERSION) + .withDeletedAt(0L) + .build()); + return getUser(userName); + } + + @Override + public IdpUserDTO getUser(String userName) { + ensureServiceAdmin(); + validateUserName(userName); + IdpUserPO userPO = + userMetaService() + .findUser(userName) + .orElseThrow( + () -> new NoSuchUserException("Built-in IdP user %s does not exist", userName)); Review Comment: Addressed via the operations layer, which is the pattern we aligned with in this PR. `IdpUserOperations#getUser` now authorizes with `SERVICE_ADMIN || USER::SELF`, while the manager remains authorization-free. ########## plugins/idp-basic/src/main/java/org/apache/gravitino/idp/basic/authorization/IdpManager.java: ########## @@ -0,0 +1,331 @@ +/* + * 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.gravitino.idp.basic.authorization; + +import com.google.common.base.Preconditions; +import java.util.ArrayList; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Optional; +import java.util.Set; +import java.util.stream.Collectors; +import org.apache.commons.lang3.StringUtils; +import org.apache.gravitino.Config; +import org.apache.gravitino.Configs; +import org.apache.gravitino.GravitinoEnv; +import org.apache.gravitino.dto.IdpGroupDTO; +import org.apache.gravitino.dto.IdpUserDTO; +import org.apache.gravitino.exceptions.ForbiddenException; +import org.apache.gravitino.exceptions.GroupAlreadyExistsException; +import org.apache.gravitino.exceptions.NoSuchGroupException; +import org.apache.gravitino.exceptions.NoSuchUserException; +import org.apache.gravitino.exceptions.UserAlreadyExistsException; +import org.apache.gravitino.idp.basic.password.PasswordHasher; +import org.apache.gravitino.idp.basic.password.PasswordHasherFactory; +import org.apache.gravitino.storage.IdGenerator; +import org.apache.gravitino.storage.relational.po.IdpGroupPO; +import org.apache.gravitino.storage.relational.po.IdpGroupUserRelPO; +import org.apache.gravitino.storage.relational.po.IdpUserPO; +import org.apache.gravitino.storage.relational.service.IdpGroupMetaService; +import org.apache.gravitino.storage.relational.service.IdpUserMetaService; +import org.apache.gravitino.utils.PrincipalUtils; + +/** + * Built-in IdP manager implementation loaded from the {@code idp-basic} plugin. + * + * <p>This implementation manages both built-in IdP users and groups and restricts mutation + * operations to Gravitino service admins. + */ +public class IdpManager implements org.apache.gravitino.authorization.IdpManager { + private static final long INITIAL_VERSION = 1L; + + private final Config config; + private final IdGenerator idGenerator; + private final IdpUserMetaService userMetaService; + private final IdpGroupMetaService groupMetaService; + private final PasswordHasher passwordHasher; + + public IdpManager() { + this( + GravitinoEnv.getInstance().config(), + GravitinoEnv.getInstance().idGenerator(), + IdpUserMetaService.getInstance(), + IdpGroupMetaService.getInstance(), + PasswordHasherFactory.create()); + } + + IdpManager( + Config config, + IdGenerator idGenerator, + IdpUserMetaService userMetaService, + IdpGroupMetaService groupMetaService, + PasswordHasher passwordHasher) { + this.config = config; + this.idGenerator = idGenerator; + this.userMetaService = userMetaService; + this.groupMetaService = groupMetaService; + this.passwordHasher = passwordHasher; + } + + @Override + public IdpUserDTO createUser(String userName, String password) { + ensureServiceAdmin(); + validateUserName(userName); + validatePassword(password); + if (userMetaService().findUser(userName).isPresent()) { + throw new UserAlreadyExistsException("Built-in IdP user %s already exists", userName); + } + + userMetaService() + .createUser( + IdpUserPO.builder() + .withUserId(nextId()) + .withUserName(userName) + .withPasswordHash(passwordHasher.hash(password)) + .withCurrentVersion(INITIAL_VERSION) + .withLastVersion(INITIAL_VERSION) + .withDeletedAt(0L) + .build()); + return getUser(userName); + } + + @Override + public IdpUserDTO getUser(String userName) { + ensureServiceAdmin(); + validateUserName(userName); + IdpUserPO userPO = + userMetaService() + .findUser(userName) + .orElseThrow( + () -> new NoSuchUserException("Built-in IdP user %s does not exist", userName)); + return toUserDTO(userPO); + } + + @Override + public boolean deleteUser(String userName) { + ensureServiceAdmin(); + validateUserName(userName); + Optional<IdpUserPO> user = userMetaService().findUser(userName); + if (!user.isPresent()) { + return false; + } + + return userMetaService().deleteUser(user.get(), System.currentTimeMillis()); + } + + @Override + public IdpUserDTO resetPassword(String userName, String password) { + ensureServiceAdmin(); + validateUserName(userName); + validatePassword(password); + IdpUserPO userPO = + userMetaService() + .findUser(userName) + .orElseThrow( + () -> new NoSuchUserException("Built-in IdP user %s does not exist", userName)); + if (passwordHasher.verify(password, userPO.getPasswordHash())) { + throw new IllegalArgumentException( + "The new password must be different from the old password"); + } + + userMetaService() + .updatePassword(userPO, passwordHasher.hash(password), userPO.getCurrentVersion() + 1); + return getUser(userName); + } + + @Override + public IdpGroupDTO createGroup(String groupName) { + ensureServiceAdmin(); + validateGroupName(groupName); + if (groupMetaService().findGroup(groupName).isPresent()) { + throw new GroupAlreadyExistsException("Built-in IdP group %s already exists", groupName); + } + + groupMetaService() + .createGroup( + IdpGroupPO.builder() + .withGroupId(nextId()) + .withGroupName(groupName) + .withCurrentVersion(INITIAL_VERSION) + .withLastVersion(INITIAL_VERSION) + .withDeletedAt(0L) + .build()); + return getGroup(groupName); + } + + @Override + public IdpGroupDTO getGroup(String groupName) { + ensureServiceAdmin(); + validateGroupName(groupName); + IdpGroupPO groupPO = + groupMetaService() + .findGroup(groupName) + .orElseThrow( Review Comment: Addressed via the operations layer, which is the pattern we aligned with in this PR. `IdpGroupOperations#getGroup` now requires `SERVICE_ADMIN`, while the manager remains authorization-free. ########## docs/open-api/idp.yaml: ########## @@ -0,0 +1,514 @@ +# 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. + +--- + +paths: + + /idp/users: + post: + tags: + - authentication + summary: Add built-in IdP user + operationId: addIdpUser + requestBody: + required: true + content: + application/json: + schema: + $ref: "#/components/schemas/CreateIdpUserRequest" + examples: + CreateIdpUserRequest: + $ref: "#/components/examples/CreateIdpUserRequest" + responses: + "200": + description: Returns the added built-in IdP user + content: + application/vnd.gravitino.v1+json: + schema: + $ref: "#/components/schemas/IdpUserResponse" + examples: + IdpUserResponse: + $ref: "#/components/examples/IdpUserResponse" + "400": + $ref: "./openapi.yaml#/components/responses/BadRequestErrorResponse" + "409": + description: Conflict - The target built-in IdP user already exists + content: + application/vnd.gravitino.v1+json: + schema: + $ref: "./openapi.yaml#/components/schemas/ErrorModel" + examples: + UserAlreadyExistsException: + $ref: "#/components/examples/UserAlreadyExistsException" + "5xx": + $ref: "./openapi.yaml#/components/responses/ServerErrorResponse" Review Comment: Fixed. The OpenAPI spec now documents the `403` response for `POST /idp/users`. ########## docs/open-api/idp.yaml: ########## @@ -0,0 +1,514 @@ +# 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. + +--- + +paths: + + /idp/users: + post: + tags: + - authentication + summary: Add built-in IdP user + operationId: addIdpUser + requestBody: + required: true + content: + application/json: + schema: + $ref: "#/components/schemas/CreateIdpUserRequest" + examples: + CreateIdpUserRequest: + $ref: "#/components/examples/CreateIdpUserRequest" + responses: + "200": + description: Returns the added built-in IdP user + content: + application/vnd.gravitino.v1+json: + schema: + $ref: "#/components/schemas/IdpUserResponse" + examples: + IdpUserResponse: + $ref: "#/components/examples/IdpUserResponse" + "400": + $ref: "./openapi.yaml#/components/responses/BadRequestErrorResponse" + "409": + description: Conflict - The target built-in IdP user already exists + content: + application/vnd.gravitino.v1+json: + schema: + $ref: "./openapi.yaml#/components/schemas/ErrorModel" + examples: + UserAlreadyExistsException: + $ref: "#/components/examples/UserAlreadyExistsException" + "5xx": + $ref: "./openapi.yaml#/components/responses/ServerErrorResponse" + + /idp/users/{user}: + parameters: + - $ref: "./openapi.yaml#/components/parameters/user" + + get: + tags: + - authentication + summary: Get built-in IdP user + operationId: getIdpUser + responses: + "200": + description: Returns the built-in IdP user + content: + application/vnd.gravitino.v1+json: + schema: + $ref: "#/components/schemas/IdpUserResponse" + examples: + IdpUserResponse: + $ref: "#/components/examples/IdpUserResponse" + "404": + description: Not Found - The specified built-in IdP user does not exist + content: + application/vnd.gravitino.v1+json: + schema: + $ref: "./openapi.yaml#/components/schemas/ErrorModel" + examples: + NoSuchUserException: + $ref: "#/components/examples/NoSuchUserException" + "5xx": + $ref: "./openapi.yaml#/components/responses/ServerErrorResponse" + + put: + tags: + - authentication + summary: Reset built-in IdP user password + operationId: resetIdpUserPassword + requestBody: + required: true + content: + application/json: + schema: + $ref: "#/components/schemas/ResetIdpUserPasswordRequest" + examples: + ResetIdpUserPasswordRequest: + $ref: "#/components/examples/ResetIdpUserPasswordRequest" + responses: + "200": + description: Returns the updated built-in IdP user + content: + application/vnd.gravitino.v1+json: + schema: + $ref: "#/components/schemas/IdpUserResponse" + examples: + IdpUserResponse: + $ref: "#/components/examples/IdpUserResponse" + "400": + $ref: "./openapi.yaml#/components/responses/BadRequestErrorResponse" + "404": + description: Not Found - The specified built-in IdP user does not exist + content: + application/vnd.gravitino.v1+json: + schema: + $ref: "./openapi.yaml#/components/schemas/ErrorModel" + examples: + NoSuchUserException: + $ref: "#/components/examples/NoSuchUserException" + "5xx": + $ref: "./openapi.yaml#/components/responses/ServerErrorResponse" + + delete: + tags: + - authentication + summary: Remove built-in IdP user + operationId: removeIdpUser + responses: + "200": + $ref: "./openapi.yaml#/components/responses/RemoveResponse" + "404": + description: Not Found - The specified built-in IdP user does not exist + content: + application/vnd.gravitino.v1+json: + schema: + $ref: "./openapi.yaml#/components/schemas/ErrorModel" + examples: + NoSuchUserException: + $ref: "#/components/examples/NoSuchUserException" + "5xx": Review Comment: Fixed. The OpenAPI spec now documents the actual `200`/`removed=false` behavior for deleting a missing user and also includes the `403` response. -- 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]
