This is an automated email from the ASF dual-hosted git repository.
roryqi pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new d7e8b22ca5 [#13925] improvement(idp-basic): Require currentPassword
for self password change (#13926)
d7e8b22ca5 is described below
commit d7e8b22ca56f4dab50c7ad728cf287ca735374b7
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]>
---
design-docs/gravitino-local-authentication.md | 32 +++--
docs/open-api/idp/idp.yaml | 148 ++++++++++++++++-----
docs/open-api/idp/openapi.yaml | 6 +
docs/security/local-users-and-groups.md | 46 +++++--
.../apache/gravitino/idp/IdpUserGroupManager.java | 24 +++-
.../idp/basic/IdpCredentialValidator.java | 14 +-
...UserRequest.java => ChangePasswordRequest.java} | 49 +++----
...rRequest.java => UpdateUserEnabledRequest.java} | 44 ++----
.../org/apache/gravitino/idp/web/IdpRESTUtils.java | 11 ++
.../idp/web/rest/IdpAuthorizationFilter.java | 28 ++--
.../gravitino/idp/web/rest/IdpUserOperations.java | 91 +++++++++----
.../gravitino/idp/TestIdpUserGroupManager.java | 33 ++++-
.../dto/requests/TestChangePasswordRequest.java | 79 +++++++++++
.../dto/requests/TestUpdateUserEnabledRequest.java | 49 +++++++
.../idp/dto/requests/TestUpdateUserRequest.java | 83 ------------
.../idp/integration/test/IdpRESTApiIT.java | 51 +++++--
.../idp/web/rest/TestIdpAuthorizationFilter.java | 6 +
.../gravitino/idp/web/rest/TestIdpOperations.java | 53 ++++++--
18 files changed, 579 insertions(+), 268 deletions(-)
diff --git a/design-docs/gravitino-local-authentication.md
b/design-docs/gravitino-local-authentication.md
index ba3cce4c65..a862573f83 100644
--- a/design-docs/gravitino-local-authentication.md
+++ b/design-docs/gravitino-local-authentication.md
@@ -460,19 +460,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}`
@@ -482,7 +496,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`
@@ -494,7 +508,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.
@@ -505,7 +519,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 a1b4c0d8fa..a3db6a9514 100644
--- a/docs/open-api/idp/idp.yaml
+++ b/docs/open-api/idp/idp.yaml
@@ -109,31 +109,49 @@ 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: Update local user store user
+ summary: Change local user store user password
description: >
- Updates the password and/or enabled flag of the specified local user
store user and returns
- the user object. At least one of `password` or `enabled` is required.
Disabled users cannot
- authenticate. Users listed in `gravitino.authorization.serviceAdmins`
cannot be disabled.
- Service admins may update any user. Authenticated non-admin users may
update only their own
- password and cannot change `enabled` or another user's account.
- operationId: updateIdpUser
+ 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`.
+ operationId: changeIdpUserPassword
requestBody:
required: true
content:
application/json:
schema:
- $ref: "#/components/schemas/UpdateUserRequest"
+ $ref: "#/components/schemas/ChangePasswordRequest"
examples:
- UpdateUserPasswordRequest:
- $ref: "#/components/examples/UpdateUserPasswordRequest"
- UpdateUserEnabledRequest:
- $ref: "#/components/examples/UpdateUserEnabledRequest"
+ ChangeUserPasswordRequest:
+ $ref: "#/components/examples/ChangeUserPasswordRequest"
+ ChangeOwnUserPasswordRequest:
+ $ref: "#/components/examples/ChangeOwnUserPasswordRequest"
responses:
"200":
- description: Returns the local user store user after the update
+ description: Returns the local user store user after the password
change
content:
application/vnd.gravitino.v1+json:
schema:
@@ -149,7 +167,7 @@ paths:
$ref: "../openapi.yaml#/components/schemas/ErrorModel"
examples:
IllegalArgumentException:
- $ref:
"#/components/examples/IdpUpdateUserIllegalArgumentException"
+ $ref:
"#/components/examples/IdpChangePasswordIllegalArgumentException"
"403":
$ref: "#/components/responses/IdpForbiddenErrorResponse"
"404":
@@ -164,17 +182,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"
@@ -410,21 +469,31 @@ components:
default: true
description: Whether the user is enabled. Disabled users cannot
authenticate.
- UpdateUserRequest:
+ ChangePasswordRequest:
type: object
- anyOf:
- - required:
- - password
- - required:
- - enabled
+ required:
+ - newPassword
properties:
- password:
+ newPassword:
type: string
format: password
minLength: 12
maxLength: 64
description: The new password of the local user store user.
writeOnly: true
+ 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.
@@ -497,9 +566,9 @@ components:
IdpForbiddenErrorResponse:
description:
Forbidden - Callers listed in `gravitino.authorization.serviceAdmins`
may manage local
- user store users and groups. Authenticated non-admin users may only
`GET` or `PUT`
- `/idp/users/{user}` for their own username (password-only on update).
Unauthenticated
- requests and other non-admin IdP management calls receive this
response.
+ 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.
content:
application/vnd.gravitino.v1+json:
schema:
@@ -515,9 +584,15 @@ components:
"password": "Passw0rd-1234"
}
- UpdateUserPasswordRequest:
+ ChangeUserPasswordRequest:
+ value: {
+ "newPassword": "Passw0rd-5678"
+ }
+
+ ChangeOwnUserPasswordRequest:
value: {
- "password": "Passw0rd-5678"
+ "newPassword": "Passw0rd-5678",
+ "oldPassword": "Passw0rd-1234"
}
UpdateUserEnabledRequest:
@@ -630,11 +705,18 @@ components:
"message": "Failed to operate local user store user [] operation
[ADD], reason [\"user\" field is required and cannot be empty]"
}
- IdpUpdateUserIllegalArgumentException:
+ IdpChangePasswordIllegalArgumentException:
+ value: {
+ "code": 1001,
+ "type": "IllegalArgumentException",
+ "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 [\"password\" or \"enabled\" field is required]"
+ "message": "Failed to operate local user store user [alice] operation
[UPDATE], reason [\"enabled\" field is required]"
}
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 70b18c9e23..86048556a7 100644
--- a/docs/security/local-users-and-groups.md
+++ b/docs/security/local-users-and-groups.md
@@ -74,34 +74,54 @@ These rules apply to user creation, password changes, and
`GRAVITINO_INITIAL_ADM
| Password length | 12 to 64 characters inclusive |
Passwords may be reset by a service admin or changed by the user for their own
account.
-The request carries the new password only and no current password; callers
authenticate with
-HTTP Basic using their current credentials.
+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` or
`PUT`
-`/api/idp/users/{user}` for their own username (read profile / change password
only).
+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}"}` |
-| Update a user | PUT | `/api/idp/users/{user}` |
`{"password":"{new_password}"}` and/or `{"enabled":false}` |
-| Remove a user | DELETE | `/api/idp/users/{user}` | None
|
+| 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.
`PUT /api/idp/users/{user}` accepts `password` and/or `enabled`; at least one
is required. Users listed in `gravitino.authorization.serviceAdmins` cannot be
disabled. Non-admin callers may get or update only their own user, and on
update may change only their password (not `enabled` or another user).
+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 '{"password":"{new_password}"}' \
- http://localhost:8090/api/idp/users/alice
+ -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
```
```shell
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 63300314fb..0e5fb53fbb 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
@@ -196,6 +196,24 @@ public class IdpUserGroupManager implements Closeable {
return USER_SERVICE.updateIdpUserPassword(username,
passwordHasher.hash(password));
}
+ /**
+ * 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.
*
@@ -203,11 +221,11 @@ public class IdpUserGroupManager implements Closeable {
* @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 {@code enabled} is {@code false} and
the username is a
- * configured service admin
+ * @throws IllegalArgumentException if the username is a configured service
admin
*/
public boolean updateEnabled(String username, boolean enabled) {
- checkCanDisableUser(username, enabled);
+ Preconditions.checkArgument(
+ !isServiceAdmin(username), "Cannot update the enabled flag of service
admin %s", username);
return USER_SERVICE.updateIdpUserEnabled(username, enabled);
}
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 a565f0ab3b..8528a07b8c 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
@@ -67,8 +67,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/UpdateUserRequest.java
b/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/dto/requests/ChangePasswordRequest.java
similarity index 55%
copy from
plugins/idp-basic/src/main/java/org/apache/gravitino/idp/dto/requests/UpdateUserRequest.java
copy to
plugins/idp-basic/src/main/java/org/apache/gravitino/idp/dto/requests/ChangePasswordRequest.java
index 6414d0c603..727dc7127d 100644
---
a/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/dto/requests/UpdateUserRequest.java
+++
b/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/dto/requests/ChangePasswordRequest.java
@@ -21,7 +21,6 @@ package org.apache.gravitino.idp.dto.requests;
import com.fasterxml.jackson.annotation.JsonInclude;
import com.fasterxml.jackson.annotation.JsonProperty;
-import com.google.common.base.Preconditions;
import lombok.Builder;
import lombok.EqualsAndHashCode;
import lombok.Getter;
@@ -30,61 +29,57 @@ import lombok.extern.jackson.Jacksonized;
import org.apache.gravitino.idp.basic.IdpCredentialValidator;
import org.apache.gravitino.rest.RESTRequest;
-/** Represents a request to update a built-in IdP user password and/or enabled
flag. */
+/** Represents a request to change a built-in IdP user password. */
@Getter
@EqualsAndHashCode
@ToString
@Builder
@Jacksonized
-public class UpdateUserRequest implements RESTRequest {
+public class ChangePasswordRequest implements RESTRequest {
- @JsonProperty("password")
+ @JsonProperty("newPassword")
@ToString.Exclude
- private final String password;
+ private final String newPassword;
- @JsonProperty("enabled")
+ @JsonProperty("oldPassword")
@JsonInclude(JsonInclude.Include.NON_NULL)
- private final Boolean enabled;
+ @ToString.Exclude
+ private final String oldPassword;
- /** Default constructor for UpdateUserRequest. (Used for Jackson
deserialization.) */
- public UpdateUserRequest() {
+ /** Default constructor for ChangePasswordRequest. (Used for Jackson
deserialization.) */
+ public ChangePasswordRequest() {
this(null, null);
}
/**
- * Creates a new UpdateUserRequest that updates only the password.
+ * Creates a ChangePasswordRequest for an admin password reset (no old
password).
*
- * @param password The new password of the built-in IdP user.
+ * @param newPassword The new password.
*/
- public UpdateUserRequest(String password) {
- this(password, null);
+ public ChangePasswordRequest(String newPassword) {
+ this(newPassword, null);
}
/**
- * Creates a new UpdateUserRequest.
+ * Creates a ChangePasswordRequest.
*
- * @param password The new password of the built-in IdP user, or {@code
null} to leave it
- * unchanged.
- * @param enabled Whether the built-in IdP user should be enabled, or {@code
null} to leave it
- * unchanged.
+ * @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 UpdateUserRequest(String password, Boolean enabled) {
+ public ChangePasswordRequest(String newPassword, String oldPassword) {
super();
- this.password = password;
- this.enabled = enabled;
+ this.newPassword = newPassword;
+ this.oldPassword = oldPassword;
}
/**
- * Validates the {@link UpdateUserRequest} request.
+ * Validates the {@link ChangePasswordRequest} request.
*
* @throws IllegalArgumentException If the request is invalid, this
exception is thrown.
*/
@Override
public void validate() throws IllegalArgumentException {
- Preconditions.checkArgument(
- password != null || enabled != null, "\"password\" or \"enabled\"
field is required");
- if (password != null) {
- IdpCredentialValidator.validatePassword(password);
- }
+ IdpCredentialValidator.validatePassword(newPassword, "newPassword");
}
}
diff --git
a/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/dto/requests/UpdateUserRequest.java
b/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/dto/requests/UpdateUserEnabledRequest.java
similarity index 54%
rename from
plugins/idp-basic/src/main/java/org/apache/gravitino/idp/dto/requests/UpdateUserRequest.java
rename to
plugins/idp-basic/src/main/java/org/apache/gravitino/idp/dto/requests/UpdateUserEnabledRequest.java
index 6414d0c603..11597c48c7 100644
---
a/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/dto/requests/UpdateUserRequest.java
+++
b/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/dto/requests/UpdateUserEnabledRequest.java
@@ -19,7 +19,6 @@
package org.apache.gravitino.idp.dto.requests;
-import com.fasterxml.jackson.annotation.JsonInclude;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.google.common.base.Preconditions;
import lombok.Builder;
@@ -27,64 +26,41 @@ 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 update a built-in IdP user password and/or enabled
flag. */
+/** Represents a request to update a built-in IdP user's enabled flag. */
@Getter
@EqualsAndHashCode
@ToString
@Builder
@Jacksonized
-public class UpdateUserRequest implements RESTRequest {
-
- @JsonProperty("password")
- @ToString.Exclude
- private final String password;
+public class UpdateUserEnabledRequest implements RESTRequest {
@JsonProperty("enabled")
- @JsonInclude(JsonInclude.Include.NON_NULL)
private final Boolean enabled;
- /** Default constructor for UpdateUserRequest. (Used for Jackson
deserialization.) */
- public UpdateUserRequest() {
- this(null, null);
- }
-
- /**
- * Creates a new UpdateUserRequest that updates only the password.
- *
- * @param password The new password of the built-in IdP user.
- */
- public UpdateUserRequest(String password) {
- this(password, null);
+ /** Default constructor for UpdateUserEnabledRequest. (Used for Jackson
deserialization.) */
+ public UpdateUserEnabledRequest() {
+ this(null);
}
/**
- * Creates a new UpdateUserRequest.
+ * Creates an UpdateUserEnabledRequest.
*
- * @param password The new password of the built-in IdP user, or {@code
null} to leave it
- * unchanged.
- * @param enabled Whether the built-in IdP user should be enabled, or {@code
null} to leave it
- * unchanged.
+ * @param enabled Whether the built-in IdP user should be enabled.
*/
- public UpdateUserRequest(String password, Boolean enabled) {
+ public UpdateUserEnabledRequest(Boolean enabled) {
super();
- this.password = password;
this.enabled = enabled;
}
/**
- * Validates the {@link UpdateUserRequest} request.
+ * Validates the {@link UpdateUserEnabledRequest} request.
*
* @throws IllegalArgumentException If the request is invalid, this
exception is thrown.
*/
@Override
public void validate() throws IllegalArgumentException {
- Preconditions.checkArgument(
- password != null || enabled != null, "\"password\" or \"enabled\"
field is required");
- if (password != null) {
- 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 35a2831b5a..324bbf2f46 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
@@ -30,6 +30,7 @@ import org.apache.gravitino.exceptions.AlreadyExistsException;
import org.apache.gravitino.exceptions.ForbiddenException;
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;
@@ -105,6 +106,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));
@@ -123,6 +131,9 @@ public final class IdpRESTUtils {
if (e instanceof ForbiddenException) {
return forbidden(errorMsg, e);
}
+ if (e instanceof UnauthorizedException) {
+ return unauthorized(errorMsg, e);
+ }
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 24f8ff6f05..f445778881 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
@@ -34,8 +34,8 @@ import org.apache.gravitino.utils.PrincipalUtils;
* Enforces built-in IdP management API access rules without server
interception.
*
* <p>Callers listed in {@link Configs#SERVICE_ADMINS} may perform all IdP
management operations.
- * Authenticated non-admin users may {@code GET} or {@code PUT} {@code
/idp/users/{user}} only for
- * their own username (read profile / change password). Other IdP management
APIs remain
+ * 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
@@ -82,22 +82,26 @@ public class IdpAuthorizationFilter implements
ContainerRequestFilter {
}
/**
- * Returns whether the request is a GET or PUT to {@code
/idp/users/{currentUser}}.
- *
- * <p>The resource method still rejects non-admin updates that change {@code
enabled}.
+ * 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;
}
- String method = requestContext.getMethod();
- if (!"GET".equalsIgnoreCase(method) && !"PUT".equalsIgnoreCase(method)) {
+ 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;
}
- List<PathSegment> segments = requestContext.getUriInfo().getPathSegments();
- return segments.size() == 3
- && "idp".equals(segments.get(0).getPath())
- && "users".equals(segments.get(1).getPath())
- && currentUser.equals(segments.get(2).getPath());
+ 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());
}
}
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 5db95b4f20..35d994d896 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,7 @@ package org.apache.gravitino.idp.web.rest;
import com.codahale.metrics.annotation.ResponseMetered;
import com.codahale.metrics.annotation.Timed;
+import com.google.common.base.Preconditions;
import java.util.List;
import javax.inject.Inject;
import javax.servlet.http.HttpServletRequest;
@@ -32,13 +33,15 @@ import javax.ws.rs.PathParam;
import javax.ws.rs.Produces;
import javax.ws.rs.core.Context;
import javax.ws.rs.core.Response;
+import org.apache.commons.lang3.StringUtils;
import org.apache.gravitino.Configs;
import org.apache.gravitino.GravitinoEnv;
import org.apache.gravitino.dto.responses.RemoveResponse;
import org.apache.gravitino.exceptions.ForbiddenException;
import org.apache.gravitino.idp.IdpUserGroupManager;
import org.apache.gravitino.idp.dto.requests.AddUserRequest;
-import org.apache.gravitino.idp.dto.requests.UpdateUserRequest;
+import org.apache.gravitino.idp.dto.requests.ChangePasswordRequest;
+import org.apache.gravitino.idp.dto.requests.UpdateUserEnabledRequest;
import org.apache.gravitino.idp.dto.responses.IdpUserResponse;
import org.apache.gravitino.idp.web.IdpManagement;
import org.apache.gravitino.idp.web.IdpOperationType;
@@ -94,22 +97,43 @@ 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 = "update-idp-user." + MetricNames.HTTP_PROCESS_DURATION,
absolute = true)
- @ResponseMetered(name = "update-idp-user", absolute = true)
- public Response updateUser(@PathParam("user") String user, UpdateUserRequest
request) {
+ @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) {
return IdpRESTUtils.doAs(
httpRequest,
() -> {
request.validate();
- enforceSelfPasswordUpdateRules(user, request);
- if (request.getPassword() != null) {
- 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 (request.getEnabled() != null) {
- userGroupManager.updateEnabled(user, request.getEnabled());
+ 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());
}
return IdpRESTUtils.ok(new
IdpUserResponse(userGroupManager.getUser(user).toDTO()));
},
@@ -119,24 +143,39 @@ public class IdpUserOperations {
}
/**
- * Non-service-admins may only change their own password and cannot update
{@code enabled}.
+ * 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 path username being updated
- * @param request the update request
+ * @param user the username whose enabled flag is updated
+ * @param request the enabled-flag update request
+ * @return the updated user response
*/
- private static void enforceSelfPasswordUpdateRules(String user,
UpdateUserRequest request) {
- String currentUser = PrincipalUtils.getCurrentUserName();
- List<String> serviceAdmins =
GravitinoEnv.getInstance().config().get(Configs.SERVICE_ADMINS);
- if (IdpAuthorizationFilter.isServiceAdmin(serviceAdmins, currentUser)) {
- return;
- }
- if (!user.equals(currentUser)) {
- throw new ForbiddenException(
- "Only service admins can update another user's password or enabled
flag");
- }
- if (request.getEnabled() != null) {
- throw new ForbiddenException("Only service admins can update the enabled
flag");
- }
+ @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",
+ IdpOperationType.UPDATE,
+ user);
}
@DELETE
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 d3d017bc96..1afa1b5e14 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
@@ -151,6 +151,22 @@ public class TestIdpUserGroupManager {
NotFoundException.class, () -> manager.changePassword("not-exist",
VALID_PASSWORD));
}
+ @Test
+ 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);
@@ -176,16 +192,25 @@ public class TestIdpUserGroupManager {
}
@Test
- public void testCannotDisableServiceAdmin() throws IOException {
+ public void testCannotUpdateEnabledOfServiceAdmin() throws IOException {
loadServiceAdminConfig(BASIC_AUTHENTICATOR, "svcAdminDisable");
manager.addUser("svcAdminDisable", VALID_PASSWORD);
- IllegalArgumentException exception =
+ IllegalArgumentException disableException =
Assertions.assertThrows(
IllegalArgumentException.class, () ->
manager.updateEnabled("svcAdminDisable", false));
- Assertions.assertEquals("Cannot disable service admin svcAdminDisable",
exception.getMessage());
+ 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());
- Assertions.assertTrue(manager.updateEnabled("svcAdminDisable", true));
}
@Test
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
new file mode 100644
index 0000000000..2da1c6a6da
--- /dev/null
+++
b/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/dto/requests/TestChangePasswordRequest.java
@@ -0,0 +1,79 @@
+/*
+ * 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 TestChangePasswordRequest {
+
+ private static final String VALID_PASSWORD = "new_password12";
+
+ @Test
+ public void testChangePasswordRequestSerDe() throws JsonProcessingException {
+ ChangePasswordRequest request = new ChangePasswordRequest(VALID_PASSWORD);
+
+ String serJson = JsonUtils.objectMapper().writeValueAsString(request);
+ ChangePasswordRequest deserRequest =
+ JsonUtils.objectMapper().readValue(serJson,
ChangePasswordRequest.class);
+
+ Assertions.assertEquals(request, deserRequest);
+ 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());
+ }
+
+ @Test
+ public void testChangePasswordRequestValidate() {
+ Assertions.assertDoesNotThrow(() -> new
ChangePasswordRequest(VALID_PASSWORD).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\""));
+ Assertions.assertThrows(
+ IllegalArgumentException.class, () -> new
ChangePasswordRequest("short").validate());
+ }
+
+ @Test
+ public void testChangePasswordRequestToStringDoesNotExposePassword() {
+ 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="));
+ }
+}
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/dto/requests/TestUpdateUserRequest.java
b/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/dto/requests/TestUpdateUserRequest.java
deleted file mode 100644
index 9fa7c88a0c..0000000000
---
a/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/dto/requests/TestUpdateUserRequest.java
+++ /dev/null
@@ -1,83 +0,0 @@
-/*
- * 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 TestUpdateUserRequest {
-
- private static final String VALID_PASSWORD = "new_password12";
-
- @Test
- public void testUpdateUserRequestSerDe() throws JsonProcessingException {
- UpdateUserRequest request = new UpdateUserRequest(VALID_PASSWORD);
-
- String serJson = JsonUtils.objectMapper().writeValueAsString(request);
- UpdateUserRequest deserRequest =
- JsonUtils.objectMapper().readValue(serJson, UpdateUserRequest.class);
-
- Assertions.assertEquals(request, deserRequest);
- Assertions.assertEquals(VALID_PASSWORD, deserRequest.getPassword());
-
- // Test with null password
- UpdateUserRequest request1 = new UpdateUserRequest();
-
- String serJson1 = JsonUtils.objectMapper().writeValueAsString(request1);
- UpdateUserRequest deserRequest1 =
- JsonUtils.objectMapper().readValue(serJson1, UpdateUserRequest.class);
-
- Assertions.assertEquals(request1, deserRequest1);
- Assertions.assertNull(deserRequest1.getPassword());
-
- UpdateUserRequest enabledOnly = new UpdateUserRequest(null, false);
- UpdateUserRequest deserEnabled =
- JsonUtils.objectMapper()
- .readValue(
- JsonUtils.objectMapper().writeValueAsString(enabledOnly),
UpdateUserRequest.class);
- Assertions.assertEquals(enabledOnly, deserEnabled);
- Assertions.assertNull(deserEnabled.getPassword());
- Assertions.assertFalse(deserEnabled.getEnabled());
- }
-
- @Test
- public void testUpdateUserRequestValidate() {
- Assertions.assertDoesNotThrow(() -> new
UpdateUserRequest(VALID_PASSWORD).validate());
- Assertions.assertDoesNotThrow(() -> new UpdateUserRequest(null,
false).validate());
- Assertions.assertDoesNotThrow(() -> new UpdateUserRequest(VALID_PASSWORD,
true).validate());
- Assertions.assertThrows(
- IllegalArgumentException.class, () -> new
UpdateUserRequest().validate());
- Assertions.assertThrows(
- IllegalArgumentException.class, () -> new UpdateUserRequest("
").validate());
- Assertions.assertThrows(
- IllegalArgumentException.class, () -> new
UpdateUserRequest("short").validate());
- }
-
- @Test
- public void testUpdateUserRequestToStringDoesNotExposePassword() {
- String requestString = new UpdateUserRequest(VALID_PASSWORD).toString();
-
- Assertions.assertFalse(requestString.contains(VALID_PASSWORD));
- Assertions.assertFalse(requestString.contains("password="));
- Assertions.assertFalse(requestString.contains("\"password\""));
- }
-}
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 528d34da16..463b44282b 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
@@ -42,8 +42,9 @@ import org.apache.gravitino.auth.AuthenticatorType;
import org.apache.gravitino.dto.responses.ErrorConstants;
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;
-import org.apache.gravitino.idp.dto.requests.UpdateUserRequest;
+import org.apache.gravitino.idp.dto.requests.UpdateUserEnabledRequest;
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;
@@ -153,22 +154,47 @@ public class IdpRESTApiIT extends BaseIT {
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, new UpdateUserRequest(UPDATED_PASSWORD),
USER1, USER_PASSWORD);
+ 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,
- new UpdateUserRequest(UPDATED_PASSWORD),
+ "/idp/users/" + USER2 + "/password",
+ new ChangePasswordRequest(UPDATED_PASSWORD),
USER1,
UPDATED_PASSWORD),
ErrorConstants.FORBIDDEN_CODE);
assertError(
403,
- put("/idp/users/" + USER1, new UpdateUserRequest(null, false), USER1,
UPDATED_PASSWORD),
+ put(
+ "/idp/users/" + USER1 + "/enabled",
+ new UpdateUserEnabledRequest(false),
+ USER1,
+ UPDATED_PASSWORD),
ErrorConstants.FORBIDDEN_CODE);
Assertions.assertTrue(deleteUser(USER1));
@@ -202,13 +228,13 @@ public class IdpRESTApiIT extends BaseIT {
changePassword(USER1, UPDATED_PASSWORD);
Assertions.assertEquals(USER1, getUser(USER1).getUser().name());
- put("/idp/users/" + USER1, new UpdateUserRequest(null, false));
+ put("/idp/users/" + USER1 + "/enabled", new
UpdateUserEnabledRequest(false));
Assertions.assertFalse(getUser(USER1).getUser().enabled());
- put("/idp/users/" + USER1, new UpdateUserRequest(null, true));
+ put("/idp/users/" + USER1 + "/enabled", new
UpdateUserEnabledRequest(true));
Assertions.assertTrue(getUser(USER1).getUser().enabled());
assertError(
400,
- put("/idp/users/" + ADMIN, new UpdateUserRequest(null, false)),
+ put("/idp/users/" + ADMIN + "/enabled", new
UpdateUserEnabledRequest(false)),
ErrorConstants.ILLEGAL_ARGUMENTS_CODE);
Assertions.assertTrue(getUser(ADMIN).getUser().enabled());
@@ -218,11 +244,13 @@ public class IdpRESTApiIT extends BaseIT {
ErrorConstants.NOT_FOUND_CODE);
assertError(
404,
- put("/idp/users/" + MISSING_USER, new
UpdateUserRequest(UPDATED_PASSWORD)),
+ put(
+ "/idp/users/" + MISSING_USER + "/password",
+ new ChangePasswordRequest(UPDATED_PASSWORD)),
ErrorConstants.NOT_FOUND_CODE);
assertError(
400,
- put("/idp/users/" + USER1, new UpdateUserRequest(" ")),
+ put("/idp/users/" + USER1 + "/password", new ChangePasswordRequest("
")),
ErrorConstants.ILLEGAL_ARGUMENTS_CODE);
Assertions.assertTrue(deleteUser(USER1));
@@ -340,7 +368,8 @@ public class IdpRESTApiIT extends BaseIT {
}
private static void changePassword(String username, String password) throws
Exception {
- HttpResponse<String> response = put("/idp/users/" + username, new
UpdateUserRequest(password));
+ HttpResponse<String> response =
+ put("/idp/users/" + username + "/password", new
ChangePasswordRequest(password));
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 2c4a017f83..75b656d201 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
@@ -53,9 +53,15 @@ class TestIdpAuthorizationFilter {
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"));
}
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 153d5908ac..68c56b5c47 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
@@ -51,8 +51,9 @@ import org.apache.gravitino.idp.IdpUserGroupManager;
import org.apache.gravitino.idp.dto.IdpGroupDTO;
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;
-import org.apache.gravitino.idp.dto.requests.UpdateUserRequest;
+import org.apache.gravitino.idp.dto.requests.UpdateUserEnabledRequest;
import org.apache.gravitino.idp.dto.responses.IdpGroupResponse;
import org.apache.gravitino.idp.dto.responses.IdpUserResponse;
import org.apache.gravitino.idp.model.IdpGroup;
@@ -155,36 +156,64 @@ class TestIdpOperations extends JerseyTest {
@Test
void testChangePasswordAndRemoveUser() {
- UpdateUserRequest req = new UpdateUserRequest(VALID_PASSWORD);
+ ChangePasswordRequest req = new ChangePasswordRequest(VALID_PASSWORD);
when(MANAGER.changePassword("user1", VALID_PASSWORD)).thenReturn(true);
when(MANAGER.getUser("user1")).thenReturn(buildUser("user1"));
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
+ 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() {
- UpdateUserRequest req = new UpdateUserRequest(null, false);
+ 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",
req).readEntity(IdpUserResponse.class).getUser().enabled());
+ put("/idp/users/user1/enabled",
req).readEntity(IdpUserResponse.class).getUser().enabled());
}
@Test
- void testCannotDisableServiceAdmin() {
- UpdateUserRequest req = new UpdateUserRequest(null, false);
- doThrow(new IllegalArgumentException("Cannot disable service admin admin"))
- .when(MANAGER)
- .updateEnabled("admin", false);
-
+ 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", req),
+ put("/idp/users/admin/enabled", new UpdateUserEnabledRequest(true)),
ErrorConstants.ILLEGAL_ARGUMENTS_CODE);
}
