This is an automated email from the ASF dual-hosted git repository. github-actions[bot] pushed a commit to branch cherry-pick-58ef9fd6-to-branch-1.3 in repository https://gitbox.apache.org/repos/asf/gravitino.git
commit 4a9a26a732dc3b350f6b90b2a3a181e3bf84e8f5 Author: MaSai <[email protected]> AuthorDate: Tue Nov 3 22:25:59 2026 +0800 [#14571][#14579] docs/fix(idp): Document 401/406; reject null IdP bodies (#14572) ### What changes were proposed in this pull request? - Document **401** and **406** on all nine built-in IdP operations in `docs/open-api/idp/idp.yaml`. - Narrow `IdpForbiddenErrorResponse` so it no longer claims unauthenticated callers receive 403. - Add Unauthorized / unsupported-API-version (406) response examples aligned with `AuthenticationFilter` and `VersioningFilter`. - Align the Forbidden example message with `IdpAuthorizationFilter.SERVICE_ADMIN_REQUIRED_MESSAGE`. - Return **400** (`Request body cannot be null`) for null JSON bodies on IdP user/group write APIs instead of NPE **500**. ### Why are the changes needed? Missing or invalid Basic credentials return 401. An **unsupported** Gravitino API version in `Accept` (for example `application/vnd.gravitino.v99+json`) returns 406 with `code` 1001 / `IllegalArgumentException`. Missing Accept, `*/*`, or `application/json` alone are rewritten to the default v1 media type and do not return 406. The IdP OpenAPI only listed 403 and said unauthenticated requests receive Forbidden, which does not match the server. Null / empty JSON bodies on IdP write endpoints previously caused `NullPointerException` (HTTP 500); metalake user APIs already return 400 for the same case. Fixes #14571 Fixes #14579 ### Does this PR introduce _any_ user-facing change? - Docs / OpenAPI contract updates for 401/406/Forbidden wording. - IdP write APIs now return 400 instead of 500 when the request body is null. ### How was this patch tested? - `./gradlew :docs:build` - `./gradlew :plugins:idp-basic:test --tests org.apache.gravitino.idp.web.rest.TestIdpOperations` (12 tests, including `testNullRequestBodyReturnsBadRequest`) - Checked `docs/open-api/idp/idp.yaml` against `VersioningFilter` / `JsonErrorHandlerIT` and `IdpAuthorizationFilter` Made with [Cursor](https://cursor.com) --------- Co-authored-by: Cursor <[email protected]> # Conflicts: # docs/open-api/idp/idp.yaml # plugins/idp-basic/src/main/java/org/apache/gravitino/idp/web/rest/IdpUserOperations.java --- docs/open-api/idp/idp.yaml | 137 ++++++++++++++++++++- .../org/apache/gravitino/idp/web/IdpRESTUtils.java | 17 +++ .../gravitino/idp/web/rest/IdpGroupOperations.java | 8 ++ .../gravitino/idp/web/rest/IdpUserOperations.java | 51 ++++++++ .../gravitino/idp/web/rest/TestIdpOperations.java | 24 ++++ 5 files changed, 235 insertions(+), 2 deletions(-) diff --git a/docs/open-api/idp/idp.yaml b/docs/open-api/idp/idp.yaml index 74e64a395b..bd28a6fa1a 100644 --- a/docs/open-api/idp/idp.yaml +++ b/docs/open-api/idp/idp.yaml @@ -67,8 +67,12 @@ paths: examples: AlreadyExistsException: $ref: "#/components/examples/IdpAlreadyExistsException" + "401": + $ref: "#/components/responses/IdpUnauthorizedErrorResponse" "403": $ref: "#/components/responses/IdpForbiddenErrorResponse" + "406": + $ref: "#/components/responses/IdpNotAcceptableErrorResponse" "5xx": $ref: "../openapi.yaml#/components/responses/ServerErrorResponse" @@ -92,8 +96,12 @@ paths: examples: IdpUserGetResponse: $ref: "#/components/examples/IdpUserGetResponse" + "401": + $ref: "#/components/responses/IdpUnauthorizedErrorResponse" "403": $ref: "#/components/responses/IdpForbiddenErrorResponse" + "406": + $ref: "#/components/responses/IdpNotAcceptableErrorResponse" "404": description: Not Found - The specified local user store user does not exist content: @@ -115,8 +123,12 @@ paths: responses: "200": $ref: "../openapi.yaml#/components/responses/RemoveResponse" + "401": + $ref: "#/components/responses/IdpUnauthorizedErrorResponse" "403": $ref: "#/components/responses/IdpForbiddenErrorResponse" + "406": + $ref: "#/components/responses/IdpNotAcceptableErrorResponse" "5xx": $ref: "../openapi.yaml#/components/responses/ServerErrorResponse" @@ -165,8 +177,12 @@ paths: examples: IllegalArgumentException: $ref: "#/components/examples/IdpChangePasswordIllegalArgumentException" + "401": + $ref: "#/components/responses/IdpUnauthorizedErrorResponse" "403": $ref: "#/components/responses/IdpForbiddenErrorResponse" + "406": + $ref: "#/components/responses/IdpNotAcceptableErrorResponse" "404": description: Not Found - The specified local user store user does not exist content: @@ -179,6 +195,68 @@ paths: "5xx": $ref: "../openapi.yaml#/components/responses/ServerErrorResponse" +<<<<<<< HEAD +======= + /idp/users/{user}/enabled: + parameters: + - $ref: "../openapi.yaml#/components/parameters/user" + + put: + tags: + - IDP + summary: Update local user store user enabled flag + description: > + Updates the enabled flag of the specified local user store user and returns the user object. + Disabled users cannot authenticate. Users listed in `gravitino.authorization.serviceAdmins` + cannot have their enabled flag changed. Only service admins may call this API. + operationId: updateIdpUserEnabled + requestBody: + required: true + content: + application/json: + schema: + $ref: "#/components/schemas/UpdateUserEnabledRequest" + examples: + UpdateUserEnabledRequest: + $ref: "#/components/examples/UpdateUserEnabledRequest" + responses: + "200": + description: Returns the local user store user after the enabled-flag update + content: + application/vnd.gravitino.v1+json: + schema: + $ref: "#/components/schemas/IdpUserResponse" + examples: + IdpUserUpdateResponse: + $ref: "#/components/examples/IdpUserUpdateResponse" + "400": + description: Bad Request - The request body is invalid + content: + application/vnd.gravitino.v1+json: + schema: + $ref: "../openapi.yaml#/components/schemas/ErrorModel" + examples: + IllegalArgumentException: + $ref: "#/components/examples/IdpUpdateEnabledIllegalArgumentException" + "401": + $ref: "#/components/responses/IdpUnauthorizedErrorResponse" + "403": + $ref: "#/components/responses/IdpForbiddenErrorResponse" + "406": + $ref: "#/components/responses/IdpNotAcceptableErrorResponse" + "404": + description: Not Found - The specified local user store user does not exist + content: + application/vnd.gravitino.v1+json: + schema: + $ref: "../openapi.yaml#/components/schemas/ErrorModel" + examples: + NotFoundException: + $ref: "#/components/examples/IdpUserUpdateNotFoundException" + "5xx": + $ref: "../openapi.yaml#/components/responses/ServerErrorResponse" + +>>>>>>> 58ef9fd6f ([#14571][#14579] docs/fix(idp): Document 401/406; reject null IdP bodies (#14572)) /idp/groups: post: tags: @@ -223,8 +301,12 @@ paths: examples: AlreadyExistsException: $ref: "#/components/examples/IdpGroupAlreadyExistsException" + "401": + $ref: "#/components/responses/IdpUnauthorizedErrorResponse" "403": $ref: "#/components/responses/IdpForbiddenErrorResponse" + "406": + $ref: "#/components/responses/IdpNotAcceptableErrorResponse" "5xx": $ref: "../openapi.yaml#/components/responses/ServerErrorResponse" @@ -248,8 +330,12 @@ paths: examples: IdpGroupGetResponse: $ref: "#/components/examples/IdpGroupGetResponse" + "401": + $ref: "#/components/responses/IdpUnauthorizedErrorResponse" "403": $ref: "#/components/responses/IdpForbiddenErrorResponse" + "406": + $ref: "#/components/responses/IdpNotAcceptableErrorResponse" "404": description: Not Found - The specified local user store group does not exist content: @@ -273,8 +359,12 @@ paths: responses: "200": $ref: "../openapi.yaml#/components/responses/RemoveResponse" + "401": + $ref: "#/components/responses/IdpUnauthorizedErrorResponse" "403": $ref: "#/components/responses/IdpForbiddenErrorResponse" + "406": + $ref: "#/components/responses/IdpNotAcceptableErrorResponse" "409": description: Conflict - The group is not empty and force is false content: @@ -325,8 +415,12 @@ paths: examples: IllegalArgumentException: $ref: "#/components/examples/IdpGroupMembershipIllegalArgumentException" + "401": + $ref: "#/components/responses/IdpUnauthorizedErrorResponse" "403": $ref: "#/components/responses/IdpForbiddenErrorResponse" + "406": + $ref: "#/components/responses/IdpNotAcceptableErrorResponse" "404": description: Not Found - The specified local user store group or user does not exist content: @@ -476,12 +570,23 @@ components: $ref: "#/components/schemas/IdpGroup" responses: + IdpUnauthorizedErrorResponse: + description: Unauthorized - Authentication credentials are missing or invalid + content: + application/vnd.gravitino.v1+json: + schema: + $ref: "../openapi.yaml#/components/schemas/ErrorModel" + examples: + UnauthorizedException: + $ref: "#/components/examples/IdpUnauthorizedException" + IdpForbiddenErrorResponse: description: Forbidden - Callers listed in `gravitino.authorization.serviceAdmins` may manage local user store users and groups. Authenticated non-admin users may only `GET` `/idp/users/{user}` or `PUT` `/idp/users/{user}/password` for their own username. - Unauthenticated requests and other non-admin IdP management calls receive this response. + Other authenticated non-admin IdP management calls receive this response. Missing or + invalid credentials return 401 instead. content: application/vnd.gravitino.v1+json: schema: @@ -490,6 +595,20 @@ components: ForbiddenException: $ref: "#/components/examples/IdpForbiddenException" + IdpNotAcceptableErrorResponse: + description: > + Not Acceptable - Returned when `Accept` (or a versioned `Content-Type`) declares an + unsupported Gravitino API version, for example `application/vnd.gravitino.v99+json`. + Missing Accept, `*/*`, or `application/json` alone are rewritten to the default + `application/vnd.gravitino.v1+json` and do not produce this response. + content: + application/vnd.gravitino.v1+json: + schema: + $ref: "../openapi.yaml#/components/schemas/ErrorModel" + examples: + UnsupportedApiVersion: + $ref: "#/components/examples/IdpUnsupportedApiVersionException" + examples: AddUserRequest: value: { @@ -657,9 +776,23 @@ components: "message": "Failed to operate local user store group [engineers] operation [REMOVE], reason [IdP group engineers is not empty, use force=true to delete it]" } + IdpUnauthorizedException: + value: { + "code": 1011, + "type": "UnauthorizedException", + "message": "The provided credentials did not support" + } + IdpForbiddenException: value: { "code": 1008, "type": "ForbiddenException", - "message": "Only service admins can manage local user store users and groups." + "message": "Only service admins can manage built-in IdP users and groups." + } + + IdpUnsupportedApiVersionException: + value: { + "code": 1001, + "type": "IllegalArgumentException", + "message": "Unsupported version v99 in request header" } diff --git a/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/web/IdpRESTUtils.java b/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/web/IdpRESTUtils.java index 35a2831b5a..8e1be73c67 100644 --- a/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/web/IdpRESTUtils.java +++ b/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/web/IdpRESTUtils.java @@ -19,6 +19,7 @@ package org.apache.gravitino.idp.web; import java.security.PrivilegedExceptionAction; +import javax.annotation.Nullable; import javax.servlet.http.HttpServletRequest; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; @@ -81,6 +82,22 @@ public final class IdpRESTUtils { return toErrorResponse(errorMsg, e); } + /** + * Returns a 400 response when {@code request} is null; otherwise {@code null}. + * + * @param request the deserialized request body, which may be null + * @return a bad-request response if {@code request} is null, otherwise {@code null} + */ + @Nullable + public static Response rejectNullRequestBody(Object request) { + if (request != null) { + return null; + } + LOG.warn("Received IdP request with null request body"); + return illegalArguments( + "Request body cannot be null", new IllegalArgumentException("Request body cannot be null")); + } + public static Response illegalArguments(String message, Throwable throwable) { return json( Response.Status.BAD_REQUEST, diff --git a/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/web/rest/IdpGroupOperations.java b/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/web/rest/IdpGroupOperations.java index f2ad5233d4..e98ffbd656 100644 --- a/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/web/rest/IdpGroupOperations.java +++ b/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/web/rest/IdpGroupOperations.java @@ -77,6 +77,10 @@ public class IdpGroupOperations { @Timed(name = "add-idp-group." + MetricNames.HTTP_PROCESS_DURATION, absolute = true) @ResponseMetered(name = "add-idp-group", absolute = true) public Response addGroup(AddGroupRequest request) { + Response rejection = IdpRESTUtils.rejectNullRequestBody(request); + if (rejection != null) { + return rejection; + } return IdpRESTUtils.doAs( httpRequest, () -> { @@ -111,6 +115,10 @@ public class IdpGroupOperations { @ResponseMetered(name = "change-idp-group-users", absolute = true) public Response changeGroupMembership( @PathParam("group") String group, GroupMembershipChangeRequest request) { + Response rejection = IdpRESTUtils.rejectNullRequestBody(request); + if (rejection != null) { + return rejection; + } return IdpRESTUtils.doAs( httpRequest, () -> { diff --git a/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/web/rest/IdpUserOperations.java b/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/web/rest/IdpUserOperations.java index 0335b41cf5..f75d7859eb 100644 --- a/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/web/rest/IdpUserOperations.java +++ b/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/web/rest/IdpUserOperations.java @@ -81,6 +81,10 @@ public class IdpUserOperations { @Timed(name = "add-idp-user." + MetricNames.HTTP_PROCESS_DURATION, absolute = true) @ResponseMetered(name = "add-idp-user", absolute = true) public Response addUser(AddUserRequest request) { + Response rejection = IdpRESTUtils.rejectNullRequestBody(request); + if (rejection != null) { + return rejection; + } return IdpRESTUtils.doAs( httpRequest, () -> { @@ -108,6 +112,10 @@ public class IdpUserOperations { @Timed(name = "change-idp-user-password." + MetricNames.HTTP_PROCESS_DURATION, absolute = true) @ResponseMetered(name = "change-idp-user-password", absolute = true) public Response changePassword(@PathParam("user") String user, ChangePasswordRequest request) { + Response rejection = IdpRESTUtils.rejectNullRequestBody(request); + if (rejection != null) { + return rejection; + } return IdpRESTUtils.doAs( httpRequest, () -> { @@ -139,6 +147,49 @@ public class IdpUserOperations { user); } +<<<<<<< HEAD +======= + /** + * Updates a user's enabled flag. Only service admins may call this API, and the target must not + * be a configured service admin. + * + * @param user the username whose enabled flag is updated + * @param request the enabled-flag update request + * @return the updated user response + */ + @PUT + @Path("{user}/enabled") + @Produces("application/vnd.gravitino.v1+json") + @Timed(name = "update-idp-user-enabled." + MetricNames.HTTP_PROCESS_DURATION, absolute = true) + @ResponseMetered(name = "update-idp-user-enabled", absolute = true) + public Response updateEnabled(@PathParam("user") String user, UpdateUserEnabledRequest request) { + Response rejection = IdpRESTUtils.rejectNullRequestBody(request); + if (rejection != null) { + return rejection; + } + return IdpRESTUtils.doAs( + httpRequest, + () -> { + request.validate(); + String currentUser = PrincipalUtils.getCurrentUserName(); + List<String> serviceAdmins = + GravitinoEnv.getInstance().config().get(Configs.SERVICE_ADMINS); + if (!IdpAuthorizationFilter.isServiceAdmin(serviceAdmins, currentUser)) { + throw new ForbiddenException("Only service admins can update the enabled flag"); + } + Preconditions.checkArgument( + !IdpAuthorizationFilter.isServiceAdmin(serviceAdmins, user), + "Cannot update the enabled flag of service admin %s", + user); + userGroupManager.updateEnabled(user, request.getEnabled()); + return IdpRESTUtils.ok(new IdpUserResponse(userGroupManager.getUser(user).toDTO())); + }, + "user", + IdpOperationType.UPDATE, + user); + } + +>>>>>>> 58ef9fd6f ([#14571][#14579] docs/fix(idp): Document 401/406; reject null IdP bodies (#14572)) @DELETE @Path("{user}") @Produces("application/vnd.gravitino.v1+json") diff --git a/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/web/rest/TestIdpOperations.java b/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/web/rest/TestIdpOperations.java index 42580f0861..277bd0993c 100644 --- a/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/web/rest/TestIdpOperations.java +++ b/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/web/rest/TestIdpOperations.java @@ -120,6 +120,15 @@ class TestIdpOperations extends JerseyTest { return resourceConfig; } + @Test + void testNullRequestBodyReturnsBadRequest() { + assertNullBody(postNull("/idp/users")); + assertNullBody(putNull("/idp/users/user1/password")); + assertNullBody(putNull("/idp/users/user1/enabled")); + assertNullBody(postNull("/idp/groups")); + assertNullBody(putNull("/idp/groups/group1/users")); + } + @Test void testAddUser() throws Exception { AddUserRequest req = new AddUserRequest("user1", VALID_PASSWORD); @@ -252,10 +261,18 @@ class TestIdpOperations extends JerseyTest { return request(path).post(Entity.entity(body, MediaType.APPLICATION_JSON_TYPE)); } + private Response postNull(String path) { + return request(path).post(Entity.entity("null", MediaType.APPLICATION_JSON_TYPE)); + } + private Response put(String path, Object body) { return request(path).put(Entity.entity(body, MediaType.APPLICATION_JSON_TYPE)); } + private Response putNull(String path) { + return request(path).put(Entity.entity("null", MediaType.APPLICATION_JSON_TYPE)); + } + private Response delete(String path) { return request(path).delete(); } @@ -269,6 +286,13 @@ class TestIdpOperations extends JerseyTest { Assertions.assertEquals(code, response.readEntity(ErrorResponse.class).getCode()); } + private void assertNullBody(Response response) { + assertStatus(Response.Status.BAD_REQUEST, response); + ErrorResponse error = response.readEntity(ErrorResponse.class); + Assertions.assertEquals(ErrorConstants.ILLEGAL_ARGUMENTS_CODE, error.getCode()); + Assertions.assertTrue(error.getMessage().contains("Request body cannot be null")); + } + private IdpUser buildUser(String user) { return new IdpUser(user, Collections.emptyList()); }
