This is an automated email from the ASF dual-hosted git repository. github-actions[bot] pushed a commit to branch cherry-pick-d7e8b22c-to-branch-1.3 in repository https://gitbox.apache.org/repos/asf/gravitino.git
commit 18244787a507f5e2a4f7c403eb6bd8ba831aec4a Author: MaSai <[email protected]> AuthorDate: Mon Oct 19 18:57:45 2026 +0800 [#13925] improvement(idp-basic): Require currentPassword for self password change (#13926) ### What changes were proposed in this pull request? Require `oldPassword` when a caller changes their own local IdP password, and verify it against the stored hash before applying `newPassword`. Service admins resetting another user's password continue to send only `newPassword` and must omit `oldPassword`. - Extend `UpdateUserRequest` / OpenAPI with `newPassword` and optional `oldPassword` (replacing the previous `password` field on update) - Enforce the self vs admin-reset rules in `IdpUserOperations` - Map `UnauthorizedException` from a failed old-password check to HTTP 401 in IdP REST - Update docs and unit/IT coverage ### Why are the changes needed? Self-service password change previously accepted only the new password. A caller with a valid Basic session could change their password without proving knowledge of the existing password again. Field names match the enterprise API (`oldPassword` / `newPassword`). Fix: #13925 ### Does this PR introduce _any_ user-facing change? Yes. `PUT /api/idp/users/{user}` for a self password change now requires: ```json {"newPassword":"{new_password}","oldPassword":"{old_password}"} ``` A service admin resetting another user uses: ```json {"newPassword":"{new_password}"} ``` Sending `oldPassword` on an admin reset of another user is rejected. Create-user still uses `password`. ### How was this patch tested? - `./gradlew :plugins:idp-basic:test --tests org.apache.gravitino.idp.TestIdpUserGroupManager --tests org.apache.gravitino.idp.dto.requests.TestUpdateUserRequest --tests org.apache.gravitino.idp.web.rest.TestIdpOperations -PskipITs` - `./gradlew :docs:build` --------- Co-authored-by: Cursor <[email protected]> # Conflicts: # docs/open-api/idp/idp.yaml # docs/security/local-users-and-groups.md # plugins/idp-basic/src/main/java/org/apache/gravitino/idp/IdpUserGroupManager.java # plugins/idp-basic/src/main/java/org/apache/gravitino/idp/dto/requests/ChangePasswordRequest.java # plugins/idp-basic/src/main/java/org/apache/gravitino/idp/web/IdpRESTUtils.java # plugins/idp-basic/src/main/java/org/apache/gravitino/idp/web/rest/IdpAuthorizationFilter.java # plugins/idp-basic/src/main/java/org/apache/gravitino/idp/web/rest/IdpUserOperations.java # plugins/idp-basic/src/test/java/org/apache/gravitino/idp/TestIdpUserGroupManager.java # plugins/idp-basic/src/test/java/org/apache/gravitino/idp/dto/requests/TestChangePasswordRequest.java # plugins/idp-basic/src/test/java/org/apache/gravitino/idp/integration/test/IdpRESTApiIT.java # plugins/idp-basic/src/test/java/org/apache/gravitino/idp/web/rest/TestIdpAuthorizationFilter.java # plugins/idp-basic/src/test/java/org/apache/gravitino/idp/web/rest/TestIdpOperations.java --- design-docs/gravitino-local-authentication.md | 32 +++-- docs/open-api/idp/idp.yaml | 140 +++++++++++++++++++-- docs/open-api/idp/openapi.yaml | 6 + docs/security/local-users-and-groups.md | 53 ++++++++ .../apache/gravitino/idp/IdpUserGroupManager.java | 36 ++++++ .../idp/basic/IdpCredentialValidator.java | 14 ++- .../idp/dto/requests/ChangePasswordRequest.java | 43 ++++++- ...dRequest.java => UpdateUserEnabledRequest.java} | 29 +++-- .../org/apache/gravitino/idp/web/IdpRESTUtils.java | 17 +++ .../idp/web/rest/IdpAuthorizationFilter.java | 37 ++++++ .../gravitino/idp/web/rest/IdpUserOperations.java | 87 ++++++++++++- .../gravitino/idp/TestIdpUserGroupManager.java | 77 ++++++++++++ .../dto/requests/TestChangePasswordRequest.java | 40 ++++++ .../dto/requests/TestUpdateUserEnabledRequest.java | 49 ++++++++ .../idp/integration/test/IdpRESTApiIT.java | 95 ++++++++++++++ .../idp/web/rest/TestIdpAuthorizationFilter.java | 45 +++++++ .../gravitino/idp/web/rest/TestIdpOperations.java | 60 ++++++++- 17 files changed, 823 insertions(+), 37 deletions(-) diff --git a/design-docs/gravitino-local-authentication.md b/design-docs/gravitino-local-authentication.md index 77e64c6010..20d2e55b6f 100644 --- a/design-docs/gravitino-local-authentication.md +++ b/design-docs/gravitino-local-authentication.md @@ -459,19 +459,33 @@ http://localhost:8090/api/idp/users/alice ``` -#### 9.1.4 Reset a local user password +#### 9.1.4 Change a local user password -`PUT /api/idp/users/{user}` — administrator-only reset (`password` only; no `oldPassword`). Usernames -must not contain `:`. Password length is 12–64 characters. +`PUT /api/idp/users/{user}/password` — service admins may reset any user's password with +`newPassword` only (must omit `oldPassword`). Authenticated non-admin users may change only their +own password and must also send `oldPassword`. Usernames must not contain `:`. Password length is +12–64 characters. ```shell curl -X PUT -H "Accept: application/vnd.gravitino.v1+json" \ -H "Content-Type: application/json" -d '{ - "password": "Passw0rd-For-Gravitino-Alice-V2" -}' http://localhost:8090/api/idp/users/alice + "newPassword": "Passw0rd-For-Gravitino-Alice-V2" +}' http://localhost:8090/api/idp/users/alice/password ``` -#### 9.1.5 Get a local group +#### 9.1.5 Update a local user enabled flag + +`PUT /api/idp/users/{user}/enabled` — service-admin only. Users listed in +`gravitino.authorization.serviceAdmins` cannot have their enabled flag changed. + +```shell +curl -X PUT -H "Accept: application/vnd.gravitino.v1+json" \ +-H "Content-Type: application/json" -d '{ + "enabled": false +}' http://localhost:8090/api/idp/users/alice/enabled +``` + +#### 9.1.6 Get a local group `GET /api/idp/groups/{group}` @@ -481,7 +495,7 @@ http://localhost:8090/api/idp/groups/engineering ``` -#### 9.1.6 Add a local group +#### 9.1.7 Add a local group `POST /api/idp/groups` @@ -493,7 +507,7 @@ curl -X POST -H "Accept: application/vnd.gravitino.v1+json" \ ``` -#### 9.1.7 Remove a local group +#### 9.1.8 Remove a local group `DELETE /api/idp/groups/{group}?force={true|false}` — soft-delete. Use `force=true` when the group still has members. @@ -504,7 +518,7 @@ curl -X DELETE -H "Accept: application/vnd.gravitino.v1+json" \ ``` -#### 9.1.8 Change local group membership +#### 9.1.9 Change local group membership `PUT /api/idp/groups/{group}/users` — at least one of `usersToAdd` or `usersToRemove` is required. diff --git a/docs/open-api/idp/idp.yaml b/docs/open-api/idp/idp.yaml index a3c793f414..24cfa32929 100644 --- a/docs/open-api/idp/idp.yaml +++ b/docs/open-api/idp/idp.yaml @@ -106,11 +106,38 @@ paths: "5xx": $ref: "../openapi.yaml#/components/responses/ServerErrorResponse" + delete: + tags: + - IDP + summary: Remove local user store user + description: Removes the specified local user store user. + operationId: removeIdpUser + responses: + "200": + $ref: "../openapi.yaml#/components/responses/RemoveResponse" + "403": + $ref: "#/components/responses/IdpForbiddenErrorResponse" + "5xx": + $ref: "../openapi.yaml#/components/responses/ServerErrorResponse" + + /idp/users/{user}/password: + parameters: + - $ref: "../openapi.yaml#/components/parameters/user" + put: tags: - IDP summary: Change local user store user password +<<<<<<< HEAD description: Updates the password of the specified local user store user and returns the user object with unchanged group membership. +======= + description: > + Changes the password of the specified local user store user and returns the user object. + Service admins may reset any user's password. Authenticated non-admin users may change only + their own password. Changing your own password requires `oldPassword` and verifies it + against the stored hash. A service admin resetting another user's password must omit + `oldPassword`. +>>>>>>> d7e8b22ca ([#13925] improvement(idp-basic): Require currentPassword for self password change (#13926)) operationId: changeIdpUserPassword requestBody: required: true @@ -119,8 +146,15 @@ paths: schema: $ref: "#/components/schemas/ChangePasswordRequest" examples: +<<<<<<< HEAD ChangePasswordRequest: $ref: "#/components/examples/ChangePasswordRequest" +======= + ChangeUserPasswordRequest: + $ref: "#/components/examples/ChangeUserPasswordRequest" + ChangeOwnUserPasswordRequest: + $ref: "#/components/examples/ChangeOwnUserPasswordRequest" +>>>>>>> d7e8b22ca ([#13925] improvement(idp-basic): Require currentPassword for self password change (#13926)) responses: "200": description: Returns the local user store user after the password change @@ -154,17 +188,58 @@ paths: "5xx": $ref: "../openapi.yaml#/components/responses/ServerErrorResponse" - delete: + /idp/users/{user}/enabled: + parameters: + - $ref: "../openapi.yaml#/components/parameters/user" + + put: tags: - IDP - summary: Remove local user store user - description: Removes the specified local user store user. - operationId: removeIdpUser + 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": - $ref: "../openapi.yaml#/components/responses/RemoveResponse" + 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" "403": $ref: "#/components/responses/IdpForbiddenErrorResponse" + "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" @@ -387,15 +462,38 @@ components: ChangePasswordRequest: type: object required: +<<<<<<< HEAD - password +======= + - newPassword +>>>>>>> d7e8b22ca ([#13925] improvement(idp-basic): Require currentPassword for self password change (#13926)) properties: - password: + newPassword: type: string format: password minLength: 12 maxLength: 64 description: The new password of the local user store user. writeOnly: true +<<<<<<< HEAD +======= + oldPassword: + type: string + format: password + description: > + The caller's current password. Required when changing your own password; must be + omitted when a service admin resets another user's password. + writeOnly: true + + UpdateUserEnabledRequest: + type: object + required: + - enabled + properties: + enabled: + type: boolean + description: Whether the local user store user should be enabled. +>>>>>>> d7e8b22ca ([#13925] improvement(idp-basic): Require currentPassword for self password change (#13926)) AddGroupRequest: type: object @@ -460,9 +558,16 @@ components: responses: IdpForbiddenErrorResponse: description: +<<<<<<< HEAD Forbidden - Only callers listed in `gravitino.authorization.serviceAdmins` may manage local user store users and groups. Unauthenticated requests and authenticated non-service-admin users receive this response. +======= + 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. +>>>>>>> d7e8b22ca ([#13925] improvement(idp-basic): Require currentPassword for self password change (#13926)) content: application/vnd.gravitino.v1+json: schema: @@ -478,9 +583,19 @@ components: "password": "Passw0rd-1234" } +<<<<<<< HEAD ChangePasswordRequest: +======= + ChangeUserPasswordRequest: +>>>>>>> d7e8b22ca ([#13925] improvement(idp-basic): Require currentPassword for self password change (#13926)) value: { - "password": "Passw0rd-5678" + "newPassword": "Passw0rd-5678" + } + + ChangeOwnUserPasswordRequest: + value: { + "newPassword": "Passw0rd-5678", + "oldPassword": "Passw0rd-1234" } AddGroupRequest: @@ -559,7 +674,18 @@ components: value: { "code": 1001, "type": "IllegalArgumentException", +<<<<<<< HEAD "message": "Failed to operate local user store user [alice] operation [UPDATE], reason [\"password\" field is required and cannot be empty]" +======= + "message": "Failed to operate local user store user [alice] operation [UPDATE], reason [\"newPassword\" field is required]" + } + + IdpUpdateEnabledIllegalArgumentException: + value: { + "code": 1001, + "type": "IllegalArgumentException", + "message": "Failed to operate local user store user [alice] operation [UPDATE], reason [\"enabled\" field is required]" +>>>>>>> d7e8b22ca ([#13925] improvement(idp-basic): Require currentPassword for self password change (#13926)) } IdpAddGroupIllegalArgumentException: diff --git a/docs/open-api/idp/openapi.yaml b/docs/open-api/idp/openapi.yaml index 263c50c7fa..12874a76d5 100644 --- a/docs/open-api/idp/openapi.yaml +++ b/docs/open-api/idp/openapi.yaml @@ -58,6 +58,12 @@ paths: /idp/users/{user}: $ref: "./idp.yaml#/paths/~1idp~1users~1%7Buser%7D" + /idp/users/{user}/password: + $ref: "./idp.yaml#/paths/~1idp~1users~1%7Buser%7D~1password" + + /idp/users/{user}/enabled: + $ref: "./idp.yaml#/paths/~1idp~1users~1%7Buser%7D~1enabled" + /idp/groups: $ref: "./idp.yaml#/paths/~1idp~1groups" diff --git a/docs/security/local-users-and-groups.md b/docs/security/local-users-and-groups.md index 069ab8d31d..020709366b 100644 --- a/docs/security/local-users-and-groups.md +++ b/docs/security/local-users-and-groups.md @@ -73,6 +73,7 @@ These rules apply to user creation, password changes, and `GRAVITINO_INITIAL_ADM | Username | Required, and must not contain a colon | | Password length | 12 to 64 characters inclusive | +<<<<<<< HEAD Passwords are reset by an administrator rather than changed by the user, so a password change request carries the new password only and no current password. ## Managing Users and Groups @@ -89,6 +90,58 @@ All management endpoints are under `http://{host}:{port}/api/idp` and require Ba | Remove a user | DELETE | `/api/idp/users/{user}` | None | The add-user body uses the field name `user` rather than `name`. +======= +Passwords may be reset by a service admin or changed by the user for their own account. +Callers authenticate with HTTP Basic. A self-service password change must also send +`oldPassword` in the request body so the server can verify it against the stored hash. +A service admin resetting another user's password sends only `newPassword` and must not +send `oldPassword`. + +## Managing Users and Groups + +All management endpoints are under `http://{host}:{port}/api/idp` and require Basic authentication. +Most operations require a service admin. Authenticated users may also `GET` +`/api/idp/users/{user}` or `PUT` `/api/idp/users/{user}/password` for their own username +(read profile / change password only). +Send `Accept: application/vnd.gravitino.v1+json` on every request, and +`Content-Type: application/json` on requests with a body. + +### User Operations + +| Operation | Method | Path | Body | +|------------------------|--------|----------------------------------|--------------------------------------------------------------------| +| Get a user | GET | `/api/idp/users/{user}` | None | +| Add a user | POST | `/api/idp/users` | `{"user":"alice","password":"{password}"}` | +| Change a user password | PUT | `/api/idp/users/{user}/password` | `{"newPassword":"{new_password}"}` (plus `oldPassword` for self) | +| Update a user enabled | PUT | `/api/idp/users/{user}/enabled` | `{"enabled":false}` | +| Remove a user | DELETE | `/api/idp/users/{user}` | None | + +The add-user body uses the field name `user` rather than `name`. `enabled` is optional on create and defaults to `true`. A disabled user cannot authenticate. Password changes use `PUT /api/idp/users/{user}/password` with required `newPassword`. Enabled-flag updates use `PUT /api/idp/users/{user}/enabled` with required `enabled` and are service-admin only; users listed in `gravitino.authorization.serviceAdmins` cannot have their enabled flag changed. Non-admin callers may get only their o [...] + +```shell +curl -s -X PUT -H "Accept: application/vnd.gravitino.v1+json" \ + -H "Content-Type: application/json" \ + -H "Authorization: Basic $(echo -n 'alice:{user_password}' | base64)" \ + -d '{"newPassword":"{new_password}","oldPassword":"{user_password}"}' \ + http://localhost:8090/api/idp/users/alice/password +``` + +```shell +curl -s -X PUT -H "Accept: application/vnd.gravitino.v1+json" \ + -H "Content-Type: application/json" \ + -H "Authorization: Basic $(echo -n 'admin:{admin_password}' | base64)" \ + -d '{"newPassword":"{new_password}"}' \ + http://localhost:8090/api/idp/users/alice/password +``` + +```shell +curl -s -X PUT -H "Accept: application/vnd.gravitino.v1+json" \ + -H "Content-Type: application/json" \ + -H "Authorization: Basic $(echo -n 'admin:{admin_password}' | base64)" \ + -d '{"enabled":false}' \ + http://localhost:8090/api/idp/users/alice/enabled +``` +>>>>>>> d7e8b22ca ([#13925] improvement(idp-basic): Require currentPassword for self password change (#13926)) ```shell curl -s -X POST -H "Accept: application/vnd.gravitino.v1+json" \ diff --git a/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/IdpUserGroupManager.java b/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/IdpUserGroupManager.java index 72e36d73f3..8204f3a7da 100644 --- a/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/IdpUserGroupManager.java +++ b/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/IdpUserGroupManager.java @@ -177,6 +177,42 @@ public class IdpUserGroupManager implements Closeable { } /** +<<<<<<< HEAD +======= + * Changes the password for a built-in IdP user after verifying the old password. + * + * @param username The username. + * @param newPassword The new plaintext password. + * @param oldPassword The caller's current plaintext password. + * @return {@code true} if the password was updated + * @throws NotFoundException if the user does not exist + * @throws IllegalArgumentException if {@code oldPassword} does not match the stored hash + */ + public boolean changePassword(String username, String newPassword, String oldPassword) { + IdpUser user = USER_SERVICE.getIdpUser(username); + if (user.passwordHash() == null || !passwordHasher.verify(oldPassword, user.passwordHash())) { + throw new IllegalArgumentException("\"oldPassword\" does not match the stored password"); + } + return changePassword(username, newPassword); + } + + /** + * Enables or disables a built-in IdP user. + * + * @param username The username. + * @param enabled Whether the user should be enabled. + * @return {@code true} if the enabled flag was updated + * @throws NotFoundException if the user does not exist + * @throws IllegalArgumentException if the username is a configured service admin + */ + public boolean updateEnabled(String username, boolean enabled) { + Preconditions.checkArgument( + !isServiceAdmin(username), "Cannot update the enabled flag of service admin %s", username); + return USER_SERVICE.updateIdpUserEnabled(username, enabled); + } + + /** +>>>>>>> d7e8b22ca ([#13925] improvement(idp-basic): Require currentPassword for self password change (#13926)) * Adds a built-in IdP group. * * @param groupName The group name. diff --git a/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/basic/IdpCredentialValidator.java b/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/basic/IdpCredentialValidator.java index b27855c577..66f0187d42 100644 --- a/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/basic/IdpCredentialValidator.java +++ b/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/basic/IdpCredentialValidator.java @@ -54,8 +54,20 @@ public final class IdpCredentialValidator { } public static void validatePassword(String password) { + validatePassword(password, "password"); + } + + /** + * Validates a password field. + * + * @param password the password value + * @param fieldName the JSON/request field name used in error messages + */ + public static void validatePassword(String password, String fieldName) { Preconditions.checkArgument( - StringUtils.isNotBlank(password), "\"password\" field is required and cannot be empty"); + StringUtils.isNotBlank(password), + "\"%s\" field is required and cannot be empty", + fieldName); Preconditions.checkArgument( password.length() >= MIN_PASSWORD_LENGTH && password.length() <= MAX_PASSWORD_LENGTH, "Password must be at least %s characters long and at most %s characters long", diff --git a/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/dto/requests/ChangePasswordRequest.java b/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/dto/requests/ChangePasswordRequest.java index f923585adf..56d0f21eb3 100644 --- a/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/dto/requests/ChangePasswordRequest.java +++ b/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/dto/requests/ChangePasswordRequest.java @@ -36,10 +36,11 @@ import org.apache.gravitino.rest.RESTRequest; @Jacksonized public class ChangePasswordRequest implements RESTRequest { - @JsonProperty("password") + @JsonProperty("newPassword") @ToString.Exclude - private final String password; + private final String newPassword; +<<<<<<< HEAD /** Default constructor for ChangePasswordRequest. (Used for Jackson deserialization.) */ public ChangePasswordRequest() { this(null); @@ -47,12 +48,44 @@ public class ChangePasswordRequest implements RESTRequest { /** * Creates a new ChangePasswordRequest. +======= + @JsonProperty("oldPassword") + @JsonInclude(JsonInclude.Include.NON_NULL) + @ToString.Exclude + private final String oldPassword; + + /** Default constructor for ChangePasswordRequest. (Used for Jackson deserialization.) */ + public ChangePasswordRequest() { + this(null, null); + } + + /** + * Creates a ChangePasswordRequest for an admin password reset (no old password). +>>>>>>> d7e8b22ca ([#13925] improvement(idp-basic): Require currentPassword for self password change (#13926)) * - * @param password The new password of the built-in IdP user. + * @param newPassword The new password. */ +<<<<<<< HEAD public ChangePasswordRequest(String password) { super(); this.password = password; +======= + public ChangePasswordRequest(String newPassword) { + this(newPassword, null); + } + + /** + * Creates a ChangePasswordRequest. + * + * @param newPassword The new password. + * @param oldPassword The caller's current password when changing their own password, or {@code + * null} when a service admin resets another user's password. + */ + public ChangePasswordRequest(String newPassword, String oldPassword) { + super(); + this.newPassword = newPassword; + this.oldPassword = oldPassword; +>>>>>>> d7e8b22ca ([#13925] improvement(idp-basic): Require currentPassword for self password change (#13926)) } /** @@ -62,6 +95,10 @@ public class ChangePasswordRequest implements RESTRequest { */ @Override public void validate() throws IllegalArgumentException { +<<<<<<< HEAD IdpCredentialValidator.validatePassword(password); +======= + IdpCredentialValidator.validatePassword(newPassword, "newPassword"); +>>>>>>> d7e8b22ca ([#13925] improvement(idp-basic): Require currentPassword for self password change (#13926)) } } diff --git a/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/dto/requests/ChangePasswordRequest.java b/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/dto/requests/UpdateUserEnabledRequest.java similarity index 64% copy from plugins/idp-basic/src/main/java/org/apache/gravitino/idp/dto/requests/ChangePasswordRequest.java copy to plugins/idp-basic/src/main/java/org/apache/gravitino/idp/dto/requests/UpdateUserEnabledRequest.java index f923585adf..11597c48c7 100644 --- a/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/dto/requests/ChangePasswordRequest.java +++ b/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/dto/requests/UpdateUserEnabledRequest.java @@ -7,7 +7,7 @@ * "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 + * 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 @@ -20,48 +20,47 @@ package org.apache.gravitino.idp.dto.requests; import com.fasterxml.jackson.annotation.JsonProperty; +import com.google.common.base.Preconditions; import lombok.Builder; import lombok.EqualsAndHashCode; import lombok.Getter; import lombok.ToString; import lombok.extern.jackson.Jacksonized; -import org.apache.gravitino.idp.basic.IdpCredentialValidator; import org.apache.gravitino.rest.RESTRequest; -/** Represents a request to change a built-in IdP user password. */ +/** Represents a request to update a built-in IdP user's enabled flag. */ @Getter @EqualsAndHashCode @ToString @Builder @Jacksonized -public class ChangePasswordRequest implements RESTRequest { +public class UpdateUserEnabledRequest implements RESTRequest { - @JsonProperty("password") - @ToString.Exclude - private final String password; + @JsonProperty("enabled") + private final Boolean enabled; - /** Default constructor for ChangePasswordRequest. (Used for Jackson deserialization.) */ - public ChangePasswordRequest() { + /** Default constructor for UpdateUserEnabledRequest. (Used for Jackson deserialization.) */ + public UpdateUserEnabledRequest() { this(null); } /** - * Creates a new ChangePasswordRequest. + * Creates an UpdateUserEnabledRequest. * - * @param password The new password of the built-in IdP user. + * @param enabled Whether the built-in IdP user should be enabled. */ - public ChangePasswordRequest(String password) { + public UpdateUserEnabledRequest(Boolean enabled) { super(); - this.password = password; + this.enabled = enabled; } /** - * Validates the {@link ChangePasswordRequest} request. + * Validates the {@link UpdateUserEnabledRequest} request. * * @throws IllegalArgumentException If the request is invalid, this exception is thrown. */ @Override public void validate() throws IllegalArgumentException { - IdpCredentialValidator.validatePassword(password); + Preconditions.checkArgument(enabled != null, "\"enabled\" field is required"); } } 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 089505654d..9d242b5699 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 @@ -29,6 +29,7 @@ import org.apache.gravitino.dto.responses.ErrorResponse; import org.apache.gravitino.exceptions.AlreadyExistsException; import org.apache.gravitino.exceptions.NonEmptyEntityException; import org.apache.gravitino.exceptions.NotFoundException; +import org.apache.gravitino.exceptions.UnauthorizedException; import org.apache.gravitino.server.web.ServerHealth; import org.apache.gravitino.utils.PrincipalUtils; import org.slf4j.Logger; @@ -104,6 +105,13 @@ public final class IdpRESTUtils { return json(Response.Status.FORBIDDEN, ErrorResponse.forbidden(message, throwable)); } + public static Response unauthorized(String message, Throwable throwable) { + return json( + Response.Status.UNAUTHORIZED, + ErrorResponse.unauthorized( + exceptionType(throwable, "UnauthorizedException"), message, throwable)); + } + public static Response unsupportedOperation(String message, Throwable throwable) { return json( Response.Status.NOT_IMPLEMENTED, ErrorResponse.unsupportedOperation(message, throwable)); @@ -119,6 +127,15 @@ public final class IdpRESTUtils { if (e instanceof IllegalArgumentException) { return illegalArguments(errorMsg, e); } +<<<<<<< HEAD +======= + if (e instanceof ForbiddenException) { + return forbidden(errorMsg, e); + } + if (e instanceof UnauthorizedException) { + return unauthorized(errorMsg, e); + } +>>>>>>> d7e8b22ca ([#13925] improvement(idp-basic): Require currentPassword for self password change (#13926)) if (e instanceof NotFoundException) { return notFound(errorMsg, e); } diff --git a/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/web/rest/IdpAuthorizationFilter.java b/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/web/rest/IdpAuthorizationFilter.java index 6bad34ea3f..8d0e8157a0 100644 --- a/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/web/rest/IdpAuthorizationFilter.java +++ b/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/web/rest/IdpAuthorizationFilter.java @@ -32,9 +32,19 @@ import org.apache.gravitino.utils.PrincipalUtils; /** * Enforces built-in IdP management API access rules without server interception. * +<<<<<<< HEAD * <p>Caller must be listed in {@link Configs#SERVICE_ADMINS}. This filter runs as a Jersey request * filter after the servlet {@code AuthenticationFilter} has authenticated the caller and populated * the current user principal. +======= + * <p>Callers listed in {@link Configs#SERVICE_ADMINS} may perform all IdP management operations. + * Authenticated non-admin users may {@code GET} {@code /idp/users/{user}} or {@code PUT} {@code + * /idp/users/{user}/password} only for their own username. Other IdP management APIs remain + * service-admin only. + * + * <p>This filter runs as a Jersey request filter after the servlet {@code AuthenticationFilter} has + * authenticated the caller and populated the current user principal. +>>>>>>> d7e8b22ca ([#13925] improvement(idp-basic): Require currentPassword for self password change (#13926)) * * <p>Registered only when {@code basic} is configured in {@link Configs#AUTHENTICATORS}. Scoped to * resources annotated with {@link IdpManagement} via Jersey name binding. @@ -72,4 +82,31 @@ public class IdpAuthorizationFilter implements ContainerRequestFilter { static boolean isServiceAdmin(List<String> serviceAdmins, String currentUser) { return currentUser != null && serviceAdmins != null && serviceAdmins.contains(currentUser); } +<<<<<<< HEAD +======= + + /** + * Returns whether the request is a self-service user access: {@code GET /idp/users/{currentUser}} + * or {@code PUT /idp/users/{currentUser}/password}. + */ + static boolean isSelfUserAccess(ContainerRequestContext requestContext, String currentUser) { + if (currentUser == null) { + return false; + } + List<PathSegment> segments = requestContext.getUriInfo().getPathSegments(); + if (segments.size() < 3 + || !"idp".equals(segments.get(0).getPath()) + || !"users".equals(segments.get(1).getPath()) + || !currentUser.equals(segments.get(2).getPath())) { + return false; + } + String method = requestContext.getMethod(); + if ("GET".equalsIgnoreCase(method) && segments.size() == 3) { + return true; + } + return "PUT".equalsIgnoreCase(method) + && segments.size() == 4 + && "password".equals(segments.get(3).getPath()); + } +>>>>>>> d7e8b22ca ([#13925] improvement(idp-basic): Require currentPassword for self password change (#13926)) } 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 9d73e30b3c..6460b014b4 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 @@ -20,6 +20,11 @@ package org.apache.gravitino.idp.web.rest; import com.codahale.metrics.annotation.ResponseMetered; import com.codahale.metrics.annotation.Timed; +<<<<<<< HEAD +======= +import com.google.common.base.Preconditions; +import java.util.List; +>>>>>>> d7e8b22ca ([#13925] improvement(idp-basic): Require currentPassword for self password change (#13926)) import javax.inject.Inject; import javax.servlet.http.HttpServletRequest; import javax.ws.rs.DELETE; @@ -31,10 +36,20 @@ import javax.ws.rs.PathParam; import javax.ws.rs.Produces; import javax.ws.rs.core.Context; import javax.ws.rs.core.Response; +<<<<<<< HEAD +======= +import org.apache.commons.lang3.StringUtils; +import org.apache.gravitino.Configs; +import org.apache.gravitino.GravitinoEnv; +>>>>>>> d7e8b22ca ([#13925] improvement(idp-basic): Require currentPassword for self password change (#13926)) import org.apache.gravitino.dto.responses.RemoveResponse; import org.apache.gravitino.idp.IdpUserGroupManager; import org.apache.gravitino.idp.dto.requests.AddUserRequest; import org.apache.gravitino.idp.dto.requests.ChangePasswordRequest; +<<<<<<< HEAD +======= +import org.apache.gravitino.idp.dto.requests.UpdateUserEnabledRequest; +>>>>>>> d7e8b22ca ([#13925] improvement(idp-basic): Require currentPassword for self password change (#13926)) import org.apache.gravitino.idp.dto.responses.IdpUserResponse; import org.apache.gravitino.idp.web.IdpManagement; import org.apache.gravitino.idp.web.IdpOperationType; @@ -87,8 +102,16 @@ public class IdpUserOperations { request.getUser()); } + /** + * Changes a user's password. Self-service changes require {@code oldPassword}. Service admins may + * reset another user without {@code oldPassword} and must not send it. + * + * @param user the username whose password is changed + * @param request the password change request + * @return the updated user response + */ @PUT - @Path("{user}") + @Path("{user}/password") @Produces("application/vnd.gravitino.v1+json") @Timed(name = "change-idp-user-password." + MetricNames.HTTP_PROCESS_DURATION, absolute = true) @ResponseMetered(name = "change-idp-user-password", absolute = true) @@ -97,7 +120,68 @@ public class IdpUserOperations { httpRequest, () -> { request.validate(); +<<<<<<< HEAD userGroupManager.changePassword(user, request.getPassword()); +======= + String currentUser = PrincipalUtils.getCurrentUserName(); + List<String> serviceAdmins = + GravitinoEnv.getInstance().config().get(Configs.SERVICE_ADMINS); + boolean isServiceAdmin = + IdpAuthorizationFilter.isServiceAdmin(serviceAdmins, currentUser); + if (!isServiceAdmin && !user.equals(currentUser)) { + throw new ForbiddenException("Only service admins can change another user's password"); + } + if (user.equals(currentUser)) { + Preconditions.checkArgument( + StringUtils.isNotBlank(request.getOldPassword()), + "\"oldPassword\" is required when changing your own password"); + userGroupManager.changePassword( + user, request.getNewPassword(), request.getOldPassword()); + } else { + Preconditions.checkArgument( + request.getOldPassword() == null, + "\"oldPassword\" is only allowed when changing your own password"); + userGroupManager.changePassword(user, request.getNewPassword()); + } +>>>>>>> d7e8b22ca ([#13925] improvement(idp-basic): Require currentPassword for self password change (#13926)) + return IdpRESTUtils.ok(new IdpUserResponse(userGroupManager.getUser(user).toDTO())); + }, + "user", + IdpOperationType.UPDATE, + 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) { + 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", @@ -105,6 +189,7 @@ public class IdpUserOperations { user); } +>>>>>>> d7e8b22ca ([#13925] improvement(idp-basic): Require currentPassword for self password change (#13926)) @DELETE @Path("{user}") @Produces("application/vnd.gravitino.v1+json") diff --git a/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/TestIdpUserGroupManager.java b/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/TestIdpUserGroupManager.java index 2ce53469d0..f812a0c302 100644 --- a/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/TestIdpUserGroupManager.java +++ b/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/TestIdpUserGroupManager.java @@ -135,6 +135,83 @@ public class TestIdpUserGroupManager { } @Test +<<<<<<< HEAD +======= + public void testChangePasswordWithOldPassword() throws IOException { + manager.addUser("testChangeWithOld", VALID_PASSWORD); + + Assertions.assertTrue( + manager.changePassword("testChangeWithOld", NEW_VALID_PASSWORD, VALID_PASSWORD)); + Assertions.assertEquals( + "testChangeWithOld", manager.authenticate("testChangeWithOld", NEW_VALID_PASSWORD).name()); + + Assertions.assertThrows( + IllegalArgumentException.class, + () -> manager.changePassword("testChangeWithOld", VALID_PASSWORD, "wrong-old-password")); + Assertions.assertEquals( + "testChangeWithOld", manager.authenticate("testChangeWithOld", NEW_VALID_PASSWORD).name()); + } + + @Test + public void testUpdateEnabled() throws IOException { + manager.addUser("testEnabled", VALID_PASSWORD); + Assertions.assertTrue(manager.getUser("testEnabled").enabled()); + Assertions.assertEquals( + "testEnabled", manager.authenticate("testEnabled", VALID_PASSWORD).name()); + + Assertions.assertTrue(manager.updateEnabled("testEnabled", false)); + IdpUser disabled = manager.getUser("testEnabled"); + Assertions.assertFalse(disabled.enabled()); + Assertions.assertEquals(AuthConstants.ANONYMOUS_USER, disabled.auditInfo().lastModifier()); + Assertions.assertNotNull(disabled.auditInfo().lastModifiedTime()); + Assertions.assertThrows( + UnauthorizedException.class, () -> manager.authenticate("testEnabled", VALID_PASSWORD)); + + Assertions.assertTrue(manager.updateEnabled("testEnabled", true)); + Assertions.assertTrue(manager.getUser("testEnabled").enabled()); + Assertions.assertEquals( + "testEnabled", manager.authenticate("testEnabled", VALID_PASSWORD).name()); + + Assertions.assertThrows( + NotFoundException.class, () -> manager.updateEnabled("not-exist", false)); + } + + @Test + public void testCannotUpdateEnabledOfServiceAdmin() throws IOException { + loadServiceAdminConfig(BASIC_AUTHENTICATOR, "svcAdminDisable"); + manager.addUser("svcAdminDisable", VALID_PASSWORD); + + IllegalArgumentException disableException = + Assertions.assertThrows( + IllegalArgumentException.class, () -> manager.updateEnabled("svcAdminDisable", false)); + Assertions.assertEquals( + "Cannot update the enabled flag of service admin svcAdminDisable", + disableException.getMessage()); + Assertions.assertTrue(manager.getUser("svcAdminDisable").enabled()); + + IllegalArgumentException enableException = + Assertions.assertThrows( + IllegalArgumentException.class, () -> manager.updateEnabled("svcAdminDisable", true)); + Assertions.assertEquals( + "Cannot update the enabled flag of service admin svcAdminDisable", + enableException.getMessage()); + Assertions.assertTrue(manager.getUser("svcAdminDisable").enabled()); + } + + @Test + public void testCannotAddDisabledServiceAdmin() { + loadServiceAdminConfig(BASIC_AUTHENTICATOR, "svcAdminCreateDisabled"); + + IllegalArgumentException exception = + Assertions.assertThrows( + IllegalArgumentException.class, + () -> manager.addUser("svcAdminCreateDisabled", VALID_PASSWORD, false)); + Assertions.assertEquals( + "Cannot disable service admin svcAdminCreateDisabled", exception.getMessage()); + } + + @Test +>>>>>>> d7e8b22ca ([#13925] improvement(idp-basic): Require currentPassword for self password change (#13926)) public void testAddGroup() throws IOException { IdpGroup group = manager.addGroup("testAddGroup"); Assertions.assertEquals("testAddGroup", group.name()); diff --git a/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/dto/requests/TestChangePasswordRequest.java b/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/dto/requests/TestChangePasswordRequest.java index 1a6b57a0fb..01eb62780d 100644 --- a/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/dto/requests/TestChangePasswordRequest.java +++ b/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/dto/requests/TestChangePasswordRequest.java @@ -7,7 +7,11 @@ * "License"); you may not use this file except in compliance * with the License. You may obtain a copy of the License at * +<<<<<<< HEAD * http://www.apache.org/licenses/LICENSE-2.0 +======= + * http://www.apache.org/licenses/LICENSE-2.0 +>>>>>>> d7e8b22ca ([#13925] improvement(idp-basic): Require currentPassword for self password change (#13926)) * * Unless required by applicable law or agreed to in writing, * software distributed under the License is distributed on an @@ -37,6 +41,7 @@ public class TestChangePasswordRequest { JsonUtils.objectMapper().readValue(serJson, ChangePasswordRequest.class); Assertions.assertEquals(request, deserRequest); +<<<<<<< HEAD Assertions.assertEquals(VALID_PASSWORD, deserRequest.getPassword()); // Test with null password @@ -48,25 +53,60 @@ public class TestChangePasswordRequest { Assertions.assertEquals(request1, deserRequest1); Assertions.assertNull(deserRequest1.getPassword()); +======= + Assertions.assertEquals(VALID_PASSWORD, deserRequest.getNewPassword()); + Assertions.assertNull(deserRequest.getOldPassword()); + Assertions.assertTrue(serJson.contains("\"newPassword\"")); + + ChangePasswordRequest withOld = new ChangePasswordRequest(VALID_PASSWORD, "old_password12"); + ChangePasswordRequest deserWithOld = + JsonUtils.objectMapper() + .readValue( + JsonUtils.objectMapper().writeValueAsString(withOld), ChangePasswordRequest.class); + Assertions.assertEquals(withOld, deserWithOld); + Assertions.assertEquals("old_password12", deserWithOld.getOldPassword()); +>>>>>>> d7e8b22ca ([#13925] improvement(idp-basic): Require currentPassword for self password change (#13926)) } @Test public void testChangePasswordRequestValidate() { Assertions.assertDoesNotThrow(() -> new ChangePasswordRequest(VALID_PASSWORD).validate()); +<<<<<<< HEAD Assertions.assertThrows( IllegalArgumentException.class, () -> new ChangePasswordRequest().validate()); Assertions.assertThrows( IllegalArgumentException.class, () -> new ChangePasswordRequest(" ").validate()); +======= + Assertions.assertDoesNotThrow( + () -> new ChangePasswordRequest(VALID_PASSWORD, "old_password12").validate()); + IllegalArgumentException missing = + Assertions.assertThrows( + IllegalArgumentException.class, () -> new ChangePasswordRequest().validate()); + Assertions.assertTrue(missing.getMessage().contains("\"newPassword\"")); + IllegalArgumentException blank = + Assertions.assertThrows( + IllegalArgumentException.class, () -> new ChangePasswordRequest(" ").validate()); + Assertions.assertTrue(blank.getMessage().contains("\"newPassword\"")); +>>>>>>> d7e8b22ca ([#13925] improvement(idp-basic): Require currentPassword for self password change (#13926)) Assertions.assertThrows( IllegalArgumentException.class, () -> new ChangePasswordRequest("short").validate()); } @Test public void testChangePasswordRequestToStringDoesNotExposePassword() { +<<<<<<< HEAD String requestString = new ChangePasswordRequest(VALID_PASSWORD).toString(); Assertions.assertFalse(requestString.contains(VALID_PASSWORD)); Assertions.assertFalse(requestString.contains("password=")); Assertions.assertFalse(requestString.contains("\"password\"")); +======= + String requestString = new ChangePasswordRequest(VALID_PASSWORD, "old_password12").toString(); + + Assertions.assertFalse(requestString.contains(VALID_PASSWORD)); + Assertions.assertFalse(requestString.contains("old_password12")); + Assertions.assertFalse(requestString.contains("newPassword=")); + Assertions.assertFalse(requestString.contains("oldPassword=")); +>>>>>>> d7e8b22ca ([#13925] improvement(idp-basic): Require currentPassword for self password change (#13926)) } } diff --git a/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/dto/requests/TestUpdateUserEnabledRequest.java b/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/dto/requests/TestUpdateUserEnabledRequest.java new file mode 100644 index 0000000000..2bb81d74a9 --- /dev/null +++ b/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/dto/requests/TestUpdateUserEnabledRequest.java @@ -0,0 +1,49 @@ +/* + * 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.dto.requests; + +import com.fasterxml.jackson.core.JsonProcessingException; +import org.apache.gravitino.json.JsonUtils; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +public class TestUpdateUserEnabledRequest { + + @Test + public void testUpdateUserEnabledRequestSerDe() throws JsonProcessingException { + UpdateUserEnabledRequest request = new UpdateUserEnabledRequest(false); + + String serJson = JsonUtils.objectMapper().writeValueAsString(request); + UpdateUserEnabledRequest deserRequest = + JsonUtils.objectMapper().readValue(serJson, UpdateUserEnabledRequest.class); + + Assertions.assertEquals(request, deserRequest); + Assertions.assertFalse(deserRequest.getEnabled()); + Assertions.assertTrue(serJson.contains("\"enabled\"")); + } + + @Test + public void testUpdateUserEnabledRequestValidate() { + Assertions.assertDoesNotThrow(() -> new UpdateUserEnabledRequest(true).validate()); + Assertions.assertDoesNotThrow(() -> new UpdateUserEnabledRequest(false).validate()); + Assertions.assertThrows( + IllegalArgumentException.class, () -> new UpdateUserEnabledRequest().validate()); + } +} diff --git a/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/integration/test/IdpRESTApiIT.java b/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/integration/test/IdpRESTApiIT.java index aaa3d91d5a..032da42e06 100644 --- a/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/integration/test/IdpRESTApiIT.java +++ b/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/integration/test/IdpRESTApiIT.java @@ -44,6 +44,10 @@ import org.apache.gravitino.idp.dto.requests.AddGroupRequest; import org.apache.gravitino.idp.dto.requests.AddUserRequest; import org.apache.gravitino.idp.dto.requests.ChangePasswordRequest; import org.apache.gravitino.idp.dto.requests.GroupMembershipChangeRequest; +<<<<<<< HEAD +======= +import org.apache.gravitino.idp.dto.requests.UpdateUserEnabledRequest; +>>>>>>> d7e8b22ca ([#13925] improvement(idp-basic): Require currentPassword for self password change (#13926)) import org.apache.gravitino.idp.dto.responses.IdpGroupResponse; import org.apache.gravitino.idp.dto.responses.IdpUserResponse; import org.apache.gravitino.idp.web.rest.feature.IdpRESTFeature; @@ -134,7 +138,71 @@ public class IdpRESTApiIT extends BaseIT { postUser(USER2, USER_PASSWORD); assertError( +<<<<<<< HEAD 403, get("/idp/users/" + USER2, USER2, USER_PASSWORD), ErrorConstants.FORBIDDEN_CODE); +======= + 403, get("/idp/users/" + USER1, USER2, USER_PASSWORD), ErrorConstants.FORBIDDEN_CODE); + Assertions.assertTrue(deleteUser(USER1)); + Assertions.assertTrue(deleteUser(USER2)); + } + + @Test + void testSelfUserAccess() throws Exception { + postUser(USER1, USER_PASSWORD); + postUser(USER2, USER_PASSWORD); + + HttpResponse<String> selfGet = get("/idp/users/" + USER1, USER1, USER_PASSWORD); + Assertions.assertEquals(200, selfGet.statusCode(), selfGet.body()); + IdpUserResponse self = + JsonUtils.objectMapper().readValue(selfGet.body(), IdpUserResponse.class); + self.validate(); + Assertions.assertEquals(USER1, self.getUser().name()); + + assertError( + 400, + put( + "/idp/users/" + USER1 + "/password", + new ChangePasswordRequest(UPDATED_PASSWORD), + USER1, + USER_PASSWORD), + ErrorConstants.ILLEGAL_ARGUMENTS_CODE); + assertError( + 400, + put( + "/idp/users/" + USER1 + "/password", + new ChangePasswordRequest(UPDATED_PASSWORD, "wrong-password-12"), + USER1, + USER_PASSWORD), + ErrorConstants.ILLEGAL_ARGUMENTS_CODE); + + HttpResponse<String> selfUpdate = + put( + "/idp/users/" + USER1 + "/password", + new ChangePasswordRequest(UPDATED_PASSWORD, USER_PASSWORD), + USER1, + USER_PASSWORD); + Assertions.assertEquals(200, selfUpdate.statusCode(), selfUpdate.body()); + Assertions.assertEquals(200, get("/version", USER1, UPDATED_PASSWORD).statusCode()); + + assertError( + 403, + put( + "/idp/users/" + USER2 + "/password", + new ChangePasswordRequest(UPDATED_PASSWORD), + USER1, + UPDATED_PASSWORD), + ErrorConstants.FORBIDDEN_CODE); + assertError( + 403, + put( + "/idp/users/" + USER1 + "/enabled", + new UpdateUserEnabledRequest(false), + USER1, + UPDATED_PASSWORD), + ErrorConstants.FORBIDDEN_CODE); + + Assertions.assertTrue(deleteUser(USER1)); +>>>>>>> d7e8b22ca ([#13925] improvement(idp-basic): Require currentPassword for self password change (#13926)) Assertions.assertTrue(deleteUser(USER2)); } @@ -165,17 +233,40 @@ public class IdpRESTApiIT extends BaseIT { changePassword(USER1, UPDATED_PASSWORD); Assertions.assertEquals(USER1, getUser(USER1).getUser().name()); +<<<<<<< HEAD +======= + put("/idp/users/" + USER1 + "/enabled", new UpdateUserEnabledRequest(false)); + Assertions.assertFalse(getUser(USER1).getUser().enabled()); + put("/idp/users/" + USER1 + "/enabled", new UpdateUserEnabledRequest(true)); + Assertions.assertTrue(getUser(USER1).getUser().enabled()); + assertError( + 400, + put("/idp/users/" + ADMIN + "/enabled", new UpdateUserEnabledRequest(false)), + ErrorConstants.ILLEGAL_ARGUMENTS_CODE); + Assertions.assertTrue(getUser(ADMIN).getUser().enabled()); + +>>>>>>> d7e8b22ca ([#13925] improvement(idp-basic): Require currentPassword for self password change (#13926)) assertError( 404, get("/idp/users/" + MISSING_USER, ADMIN, ADMIN_PASSWORD), ErrorConstants.NOT_FOUND_CODE); assertError( 404, +<<<<<<< HEAD put("/idp/users/" + MISSING_USER, new ChangePasswordRequest(UPDATED_PASSWORD)), ErrorConstants.NOT_FOUND_CODE); assertError( 400, put("/idp/users/" + USER1, new ChangePasswordRequest(" ")), +======= + put( + "/idp/users/" + MISSING_USER + "/password", + new ChangePasswordRequest(UPDATED_PASSWORD)), + ErrorConstants.NOT_FOUND_CODE); + assertError( + 400, + put("/idp/users/" + USER1 + "/password", new ChangePasswordRequest(" ")), +>>>>>>> d7e8b22ca ([#13925] improvement(idp-basic): Require currentPassword for self password change (#13926)) ErrorConstants.ILLEGAL_ARGUMENTS_CODE); Assertions.assertTrue(deleteUser(USER1)); @@ -284,7 +375,11 @@ public class IdpRESTApiIT extends BaseIT { private static void changePassword(String username, String password) throws Exception { HttpResponse<String> response = +<<<<<<< HEAD put("/idp/users/" + username, new ChangePasswordRequest(password)); +======= + put("/idp/users/" + username + "/password", new ChangePasswordRequest(password)); +>>>>>>> d7e8b22ca ([#13925] improvement(idp-basic): Require currentPassword for self password change (#13926)) Assertions.assertEquals(200, response.statusCode(), response.body()); } diff --git a/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/web/rest/TestIdpAuthorizationFilter.java b/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/web/rest/TestIdpAuthorizationFilter.java index 5fb8167357..5f7ea752e8 100644 --- a/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/web/rest/TestIdpAuthorizationFilter.java +++ b/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/web/rest/TestIdpAuthorizationFilter.java @@ -33,4 +33,49 @@ class TestIdpAuthorizationFilter { assertFalse(IdpAuthorizationFilter.isServiceAdmin(List.of("admin"), "other")); assertTrue(IdpAuthorizationFilter.isServiceAdmin(List.of("admin", "ops"), "admin")); } +<<<<<<< HEAD +======= + + @Test + void testIsSelfUserAccess() { + assertFalse( + IdpAuthorizationFilter.isSelfUserAccess(request("PUT", "idp", "users", "alice"), null)); + assertFalse( + IdpAuthorizationFilter.isSelfUserAccess( + request("DELETE", "idp", "users", "alice"), "alice")); + assertFalse( + IdpAuthorizationFilter.isSelfUserAccess(request("PUT", "idp", "users", "bob"), "alice")); + assertFalse( + IdpAuthorizationFilter.isSelfUserAccess(request("GET", "idp", "users", "bob"), "alice")); + assertFalse( + IdpAuthorizationFilter.isSelfUserAccess(request("PUT", "idp", "groups", "alice"), "alice")); + assertFalse( + IdpAuthorizationFilter.isSelfUserAccess( + request("PUT", "idp", "users", "alice", "enabled"), "alice")); + assertTrue( + IdpAuthorizationFilter.isSelfUserAccess(request("GET", "idp", "users", "alice"), "alice")); + assertTrue( + IdpAuthorizationFilter.isSelfUserAccess( + request("PUT", "idp", "users", "alice", "password"), "alice")); + assertFalse( + IdpAuthorizationFilter.isSelfUserAccess(request("PUT", "idp", "users", "alice"), "alice")); + } + + private static ContainerRequestContext request(String method, String... segments) { + List<PathSegment> pathSegments = + Arrays.stream(segments).map(TestIdpAuthorizationFilter::segment).toList(); + ContainerRequestContext context = mock(ContainerRequestContext.class); + UriInfo uriInfo = mock(UriInfo.class); + when(context.getMethod()).thenReturn(method); + when(context.getUriInfo()).thenReturn(uriInfo); + when(uriInfo.getPathSegments()).thenReturn(pathSegments); + return context; + } + + private static PathSegment segment(String path) { + PathSegment pathSegment = mock(PathSegment.class); + when(pathSegment.getPath()).thenReturn(path); + return pathSegment; + } +>>>>>>> d7e8b22ca ([#13925] improvement(idp-basic): Require currentPassword for self password change (#13926)) } 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 f8ae328fe1..9d9b65da3c 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 @@ -44,6 +44,10 @@ import org.apache.gravitino.idp.dto.requests.AddGroupRequest; import org.apache.gravitino.idp.dto.requests.AddUserRequest; import org.apache.gravitino.idp.dto.requests.ChangePasswordRequest; import org.apache.gravitino.idp.dto.requests.GroupMembershipChangeRequest; +<<<<<<< HEAD +======= +import org.apache.gravitino.idp.dto.requests.UpdateUserEnabledRequest; +>>>>>>> d7e8b22ca ([#13925] improvement(idp-basic): Require currentPassword for self password change (#13926)) import org.apache.gravitino.idp.dto.responses.IdpGroupResponse; import org.apache.gravitino.idp.dto.responses.IdpUserResponse; import org.apache.gravitino.idp.model.IdpGroup; @@ -133,11 +137,65 @@ class TestIdpOperations extends JerseyTest { when(MANAGER.removeUser("user1")).thenReturn(true); Assertions.assertEquals( - "user1", put("/idp/users/user1", req).readEntity(IdpUserResponse.class).getUser().name()); + "user1", + put("/idp/users/user1/password", req).readEntity(IdpUserResponse.class).getUser().name()); Assertions.assertTrue(delete("/idp/users/user1").readEntity(RemoveResponse.class).removed()); } @Test +<<<<<<< HEAD +======= + void testAdminSelfPasswordChangeRequiresOldPassword() { + ChangePasswordRequest missingOld = new ChangePasswordRequest(VALID_PASSWORD); + assertError( + Response.Status.BAD_REQUEST, + put("/idp/users/admin/password", missingOld), + ErrorConstants.ILLEGAL_ARGUMENTS_CODE); + + ChangePasswordRequest withOld = new ChangePasswordRequest(VALID_PASSWORD, "admin-old-pass12"); + when(MANAGER.changePassword("admin", VALID_PASSWORD, "admin-old-pass12")).thenReturn(true); + when(MANAGER.getUser("admin")).thenReturn(buildUser("admin")); + Assertions.assertEquals( + "admin", + put("/idp/users/admin/password", withOld) + .readEntity(IdpUserResponse.class) + .getUser() + .name()); + } + + @Test + void testAdminResetRejectsOldPassword() { + ChangePasswordRequest req = new ChangePasswordRequest(VALID_PASSWORD, "other-old-pass12"); + assertError( + Response.Status.BAD_REQUEST, + put("/idp/users/user1/password", req), + ErrorConstants.ILLEGAL_ARGUMENTS_CODE); + } + + @Test + void testUpdateEnabled() { + UpdateUserEnabledRequest req = new UpdateUserEnabledRequest(false); + when(MANAGER.updateEnabled("user1", false)).thenReturn(true); + when(MANAGER.getUser("user1")).thenReturn(new IdpUser("user1", Collections.emptyList(), false)); + + Assertions.assertFalse( + put("/idp/users/user1/enabled", req).readEntity(IdpUserResponse.class).getUser().enabled()); + } + + @Test + void testCannotUpdateEnabledOfServiceAdmin() { + assertError( + Response.Status.BAD_REQUEST, + put("/idp/users/admin/enabled", new UpdateUserEnabledRequest(false)), + ErrorConstants.ILLEGAL_ARGUMENTS_CODE); + assertError( + Response.Status.BAD_REQUEST, + put("/idp/users/admin/enabled", new UpdateUserEnabledRequest(true)), + ErrorConstants.ILLEGAL_ARGUMENTS_CODE); + } + + @Test +>>>>>>> d7e8b22ca ([#13925] improvement(idp-basic): Require currentPassword for self password change (#13926)) void testAddAndGetGroup() throws Exception { AddGroupRequest req = new AddGroupRequest("group1"); doReturn(buildGroup("group1")).when(MANAGER).addGroup("group1");
