This is an automated email from the ASF dual-hosted git repository.
roryqi pushed a commit to branch branch-1.3
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/branch-1.3 by this push:
new 758ace28cd [Cherry-pick to branch-1.3] [#13925]
improvement(idp-basic): Require currentPassword for self password change
(#13926) (#13963)
758ace28cd is described below
commit 758ace28cd252497f9cf70d0326138ef1cb59d39
Author: MaSai <[email protected]>
AuthorDate: Tue Oct 20 15:03:27 2026 +0800
[Cherry-pick to branch-1.3] [#13925] improvement(idp-basic): Require
currentPassword for self password change (#13926) (#13963)
**Cherry-pick Information:**
- Original commit: d7e8b22ca56f4dab50c7ad728cf287ca735374b7
- Target branch: `branch-1.3`
- Status: Clean cherry-pick adapted for branch-1.3 (no conflict
markers).
### What changes were proposed in this pull request?
Backport [#13926](https://github.com/apache/gravitino/pull/13926) /
[#13925](https://github.com/apache/gravitino/issues/13925) to
`branch-1.3`:
- Require `oldPassword` for self password changes and verify it against
the stored hash
- Move password changes to `PUT /api/idp/users/{user}/password` with
`newPassword` / optional `oldPassword`
- Allow authenticated non-admin users to `GET` their own user and `PUT`
their own password
- Service admins resetting another user send only `newPassword` and must
omit `oldPassword`
### Backport notes
- `branch-1.3` does not include the local-user `enabled` flag / `PUT
.../enabled` API from main, so that part of #13926 is omitted.
- Replaces auto cherry-pick
[#13956](https://github.com/apache/gravitino/pull/13956), which landed
with unresolved conflict markers.
### Why are the changes needed?
Self-service (and admin self) password change previously accepted only
the new password. Callers with a valid Basic session could change a
password without proving knowledge of the existing password again.
Fix: #13925
### Does this PR introduce _any_ user-facing change?
Yes.
- Password change path: `PUT /api/idp/users/{user}/password`
- Self-change body: `{"newPassword":"...","oldPassword":"..."}`
- Admin reset of another user: `{"newPassword":"..."}` (must omit
`oldPassword`)
- 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.TestChangePasswordRequest --tests
org.apache.gravitino.idp.web.rest.TestIdpOperations --tests
org.apache.gravitino.idp.web.rest.TestIdpAuthorizationFilter -PskipITs`
- `./gradlew :docs:build`
Made with [Cursor](https://cursor.com)
Co-authored-by: Cursor <[email protected]>
---
design-docs/gravitino-local-authentication.md | 12 ++--
docs/open-api/idp/idp.yaml | 75 ++++++++++++++--------
docs/open-api/idp/openapi.yaml | 3 +
docs/security/local-users-and-groups.md | 47 +++++++++++---
.../apache/gravitino/idp/IdpUserGroupManager.java | 18 ++++++
.../idp/basic/IdpCredentialValidator.java | 14 +++-
.../idp/dto/requests/ChangePasswordRequest.java | 36 ++++++++---
.../org/apache/gravitino/idp/web/IdpRESTUtils.java | 4 ++
.../idp/web/rest/IdpAuthorizationFilter.java | 44 +++++++++++--
.../gravitino/idp/web/rest/IdpUserOperations.java | 38 ++++++++++-
.../gravitino/idp/TestIdpUserGroupManager.java | 16 +++++
.../dto/requests/TestChangePasswordRequest.java | 43 +++++++------
.../idp/integration/test/IdpRESTApiIT.java | 59 +++++++++++++++--
.../idp/web/rest/TestIdpAuthorizationFilter.java | 45 +++++++++++++
.../gravitino/idp/web/rest/TestIdpOperations.java | 56 +++++++++++++++-
15 files changed, 429 insertions(+), 81 deletions(-)
diff --git a/design-docs/gravitino-local-authentication.md
b/design-docs/gravitino-local-authentication.md
index 77e64c6010..4cae6f30f2 100644
--- a/design-docs/gravitino-local-authentication.md
+++ b/design-docs/gravitino-local-authentication.md
@@ -459,16 +459,18 @@ 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
diff --git a/docs/open-api/idp/idp.yaml b/docs/open-api/idp/idp.yaml
index a3c793f414..74e64a395b 100644
--- a/docs/open-api/idp/idp.yaml
+++ b/docs/open-api/idp/idp.yaml
@@ -106,11 +106,34 @@ 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
- 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`.
operationId: changeIdpUserPassword
requestBody:
required: true
@@ -119,8 +142,10 @@ paths:
schema:
$ref: "#/components/schemas/ChangePasswordRequest"
examples:
- ChangePasswordRequest:
- $ref: "#/components/examples/ChangePasswordRequest"
+ ChangeUserPasswordRequest:
+ $ref: "#/components/examples/ChangeUserPasswordRequest"
+ ChangeOwnUserPasswordRequest:
+ $ref: "#/components/examples/ChangeOwnUserPasswordRequest"
responses:
"200":
description: Returns the local user store user after the password
change
@@ -154,20 +179,6 @@ 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/groups:
post:
tags:
@@ -387,15 +398,22 @@ components:
ChangePasswordRequest:
type: object
required:
- - password
+ - 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
AddGroupRequest:
type: object
@@ -460,9 +478,10 @@ components:
responses:
IdpForbiddenErrorResponse:
description:
- 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.
content:
application/vnd.gravitino.v1+json:
schema:
@@ -478,9 +497,15 @@ components:
"password": "Passw0rd-1234"
}
- ChangePasswordRequest:
+ ChangeUserPasswordRequest:
+ value: {
+ "newPassword": "Passw0rd-5678"
+ }
+
+ ChangeOwnUserPasswordRequest:
value: {
- "password": "Passw0rd-5678"
+ "newPassword": "Passw0rd-5678",
+ "oldPassword": "Passw0rd-1234"
}
AddGroupRequest:
@@ -559,7 +584,7 @@ components:
value: {
"code": 1001,
"type": "IllegalArgumentException",
- "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 and cannot be empty]"
}
IdpAddGroupIllegalArgumentException:
diff --git a/docs/open-api/idp/openapi.yaml b/docs/open-api/idp/openapi.yaml
index 263c50c7fa..782edd3578 100644
--- a/docs/open-api/idp/openapi.yaml
+++ b/docs/open-api/idp/openapi.yaml
@@ -58,6 +58,9 @@ 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/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..1114b2fd96 100644
--- a/docs/security/local-users-and-groups.md
+++ b/docs/security/local-users-and-groups.md
@@ -73,22 +73,35 @@ 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 |
-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.
+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 as a service admin. Send `Accept:
application/vnd.gravitino.v1+json` on every request, and `Content-Type:
application/json` on requests with a body.
+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}"}` |
-| Reset a password| PUT | `/api/idp/users/{user}` |
`{"password":"{new_password}"}` |
-| 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) |
+| Remove a user | DELETE | `/api/idp/users/{user}` | None
|
-The add-user body uses the field name `user` rather than `name`.
+The add-user body uses the field name `user` rather than `name`. Password
changes use
+`PUT /api/idp/users/{user}/password` with required `newPassword`. Non-admin
callers may get
+only their own user and change only their own password. When changing your own
password,
+`oldPassword` is required and must match the stored password. When a service
admin resets
+another user's password, `oldPassword` must be omitted.
```shell
curl -s -X POST -H "Accept: application/vnd.gravitino.v1+json" \
@@ -98,6 +111,22 @@ curl -s -X POST -H "Accept:
application/vnd.gravitino.v1+json" \
http://localhost:8090/api/idp/users
```
+```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
+```
+
### Group Operations
| Operation | Method | Path
| Body |
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..e866708892 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
@@ -176,6 +176,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);
+ }
+
/**
* Adds a built-in IdP group.
*
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..727dc7127d 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
@@ -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
@@ -19,6 +19,7 @@
package org.apache.gravitino.idp.dto.requests;
+import com.fasterxml.jackson.annotation.JsonInclude;
import com.fasterxml.jackson.annotation.JsonProperty;
import lombok.Builder;
import lombok.EqualsAndHashCode;
@@ -36,23 +37,40 @@ 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;
+
+ @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);
+ this(null, null);
+ }
+
+ /**
+ * Creates a ChangePasswordRequest for an admin password reset (no old
password).
+ *
+ * @param newPassword The new password.
+ */
+ public ChangePasswordRequest(String newPassword) {
+ this(newPassword, null);
}
/**
- * Creates a new ChangePasswordRequest.
+ * Creates a ChangePasswordRequest.
*
- * @param password The new password of the built-in IdP user.
+ * @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 password) {
+ public ChangePasswordRequest(String newPassword, String oldPassword) {
super();
- this.password = password;
+ this.newPassword = newPassword;
+ this.oldPassword = oldPassword;
}
/**
@@ -62,6 +80,6 @@ public class ChangePasswordRequest implements RESTRequest {
*/
@Override
public void validate() throws IllegalArgumentException {
- IdpCredentialValidator.validatePassword(password);
+ IdpCredentialValidator.validatePassword(newPassword, "newPassword");
}
}
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..35a2831b5a 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
@@ -27,6 +27,7 @@ import org.apache.gravitino.UserPrincipal;
import org.apache.gravitino.auth.AuthConstants;
import org.apache.gravitino.dto.responses.ErrorResponse;
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.server.web.ServerHealth;
@@ -119,6 +120,9 @@ public final class IdpRESTUtils {
if (e instanceof IllegalArgumentException) {
return illegalArguments(errorMsg, e);
}
+ if (e instanceof ForbiddenException) {
+ return forbidden(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 6bad34ea3f..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
@@ -23,6 +23,7 @@ import java.util.List;
import java.util.function.Supplier;
import javax.ws.rs.container.ContainerRequestContext;
import javax.ws.rs.container.ContainerRequestFilter;
+import javax.ws.rs.core.PathSegment;
import org.apache.gravitino.Configs;
import org.apache.gravitino.GravitinoEnv;
import org.apache.gravitino.idp.web.IdpManagement;
@@ -32,9 +33,13 @@ import org.apache.gravitino.utils.PrincipalUtils;
/**
* Enforces built-in IdP management API access rules without server
interception.
*
- * <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.
*
* <p>Registered only when {@code basic} is configured in {@link
Configs#AUTHENTICATORS}. Scoped to
* resources annotated with {@link IdpManagement} via Jersey name binding.
@@ -42,7 +47,7 @@ import org.apache.gravitino.utils.PrincipalUtils;
@IdpManagement
public class IdpAuthorizationFilter implements ContainerRequestFilter {
- /** Error message when the caller is not a service admin. */
+ /** Error message when the caller is not allowed to perform the IdP
management operation. */
public static final String SERVICE_ADMIN_REQUIRED_MESSAGE =
"Only service admins can manage built-in IdP users and groups.";
@@ -64,12 +69,39 @@ public class IdpAuthorizationFilter implements
ContainerRequestFilter {
@Override
public void filter(ContainerRequestContext requestContext) throws
IOException {
- if (!isServiceAdmin(serviceAdminsSupplier.get(),
currentUserSupplier.get())) {
-
requestContext.abortWith(IdpRESTUtils.forbidden(SERVICE_ADMIN_REQUIRED_MESSAGE,
null));
+ String currentUser = currentUserSupplier.get();
+ if (isServiceAdmin(serviceAdminsSupplier.get(), currentUser)
+ || isSelfUserAccess(requestContext, currentUser)) {
+ return;
}
+
requestContext.abortWith(IdpRESTUtils.forbidden(SERVICE_ADMIN_REQUIRED_MESSAGE,
null));
}
static boolean isServiceAdmin(List<String> serviceAdmins, String
currentUser) {
return currentUser != null && serviceAdmins != null &&
serviceAdmins.contains(currentUser);
}
+
+ /**
+ * 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());
+ }
}
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..0335b41cf5 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,8 @@ 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;
import javax.ws.rs.DELETE;
@@ -31,7 +33,11 @@ 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.ChangePasswordRequest;
@@ -40,6 +46,7 @@ import org.apache.gravitino.idp.web.IdpManagement;
import org.apache.gravitino.idp.web.IdpOperationType;
import org.apache.gravitino.idp.web.IdpRESTUtils;
import org.apache.gravitino.metrics.MetricNames;
+import org.apache.gravitino.utils.PrincipalUtils;
/** REST resource for built-in IdP user management exposed by the {@code
idp-basic} plugin. */
@IdpManagement
@@ -87,8 +94,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 +112,26 @@ public class IdpUserOperations {
httpRequest,
() -> {
request.validate();
- 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());
+ }
return IdpRESTUtils.ok(new
IdpUserResponse(userGroupManager.getUser(user).toDTO()));
},
"user",
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..089db3082e 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
@@ -134,6 +134,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 testAddGroup() throws IOException {
IdpGroup group = manager.addGroup("testAddGroup");
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..2da1c6a6da 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,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
@@ -37,36 +37,43 @@ public class TestChangePasswordRequest {
JsonUtils.objectMapper().readValue(serJson,
ChangePasswordRequest.class);
Assertions.assertEquals(request, deserRequest);
- Assertions.assertEquals(VALID_PASSWORD, deserRequest.getPassword());
+ Assertions.assertEquals(VALID_PASSWORD, deserRequest.getNewPassword());
+ Assertions.assertNull(deserRequest.getOldPassword());
+ Assertions.assertTrue(serJson.contains("\"newPassword\""));
- // Test with null password
- ChangePasswordRequest request1 = new ChangePasswordRequest();
-
- String serJson1 = JsonUtils.objectMapper().writeValueAsString(request1);
- ChangePasswordRequest deserRequest1 =
- JsonUtils.objectMapper().readValue(serJson1,
ChangePasswordRequest.class);
-
- Assertions.assertEquals(request1, deserRequest1);
- Assertions.assertNull(deserRequest1.getPassword());
+ 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.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\""));
Assertions.assertThrows(
IllegalArgumentException.class, () -> new
ChangePasswordRequest("short").validate());
}
@Test
public void testChangePasswordRequestToStringDoesNotExposePassword() {
- String requestString = new
ChangePasswordRequest(VALID_PASSWORD).toString();
+ String requestString = new ChangePasswordRequest(VALID_PASSWORD,
"old_password12").toString();
Assertions.assertFalse(requestString.contains(VALID_PASSWORD));
- Assertions.assertFalse(requestString.contains("password="));
- Assertions.assertFalse(requestString.contains("\"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/integration/test/IdpRESTApiIT.java
b/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/integration/test/IdpRESTApiIT.java
index aaa3d91d5a..6dd886ea35 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
@@ -134,7 +134,49 @@ public class IdpRESTApiIT extends BaseIT {
postUser(USER2, USER_PASSWORD);
assertError(
- 403, get("/idp/users/" + USER2, USER2, USER_PASSWORD),
ErrorConstants.FORBIDDEN_CODE);
+ 403, get("/idp/users/" + USER1, USER2, USER_PASSWORD),
ErrorConstants.FORBIDDEN_CODE);
+ HttpResponse<String> selfGet = get("/idp/users/" + USER2, USER2,
USER_PASSWORD);
+ Assertions.assertEquals(200, selfGet.statusCode(), selfGet.body());
+ IdpUserResponse self =
+ JsonUtils.objectMapper().readValue(selfGet.body(),
IdpUserResponse.class);
+ self.validate();
+ Assertions.assertEquals(USER2, self.getUser().name());
+
+ assertError(
+ 400,
+ put(
+ "/idp/users/" + USER2 + "/password",
+ new ChangePasswordRequest(UPDATED_PASSWORD),
+ USER2,
+ USER_PASSWORD),
+ ErrorConstants.ILLEGAL_ARGUMENTS_CODE);
+ assertError(
+ 400,
+ put(
+ "/idp/users/" + USER2 + "/password",
+ new ChangePasswordRequest(UPDATED_PASSWORD, "wrong-password-12"),
+ USER2,
+ USER_PASSWORD),
+ ErrorConstants.ILLEGAL_ARGUMENTS_CODE);
+
+ HttpResponse<String> selfUpdate =
+ put(
+ "/idp/users/" + USER2 + "/password",
+ new ChangePasswordRequest(UPDATED_PASSWORD, USER_PASSWORD),
+ USER2,
+ USER_PASSWORD);
+ Assertions.assertEquals(200, selfUpdate.statusCode(), selfUpdate.body());
+ Assertions.assertEquals(200, get("/version", USER2,
UPDATED_PASSWORD).statusCode());
+
+ assertError(
+ 403,
+ put(
+ "/idp/users/" + USER1 + "/password",
+ new ChangePasswordRequest(UPDATED_PASSWORD),
+ USER2,
+ UPDATED_PASSWORD),
+ ErrorConstants.FORBIDDEN_CODE);
+
Assertions.assertTrue(deleteUser(USER2));
}
@@ -171,11 +213,13 @@ public class IdpRESTApiIT extends BaseIT {
ErrorConstants.NOT_FOUND_CODE);
assertError(
404,
- put("/idp/users/" + MISSING_USER, new
ChangePasswordRequest(UPDATED_PASSWORD)),
+ put(
+ "/idp/users/" + MISSING_USER + "/password",
+ new ChangePasswordRequest(UPDATED_PASSWORD)),
ErrorConstants.NOT_FOUND_CODE);
assertError(
400,
- put("/idp/users/" + USER1, new ChangePasswordRequest(" ")),
+ put("/idp/users/" + USER1 + "/password", new ChangePasswordRequest("
")),
ErrorConstants.ILLEGAL_ARGUMENTS_CODE);
Assertions.assertTrue(deleteUser(USER1));
@@ -284,7 +328,7 @@ public class IdpRESTApiIT extends BaseIT {
private static void changePassword(String username, String password) throws
Exception {
HttpResponse<String> response =
- put("/idp/users/" + username, new ChangePasswordRequest(password));
+ put("/idp/users/" + username + "/password", new
ChangePasswordRequest(password));
Assertions.assertEquals(200, response.statusCode(), response.body());
}
@@ -349,8 +393,13 @@ public class IdpRESTApiIT extends BaseIT {
}
private static HttpResponse<String> put(String path, Object body) throws
Exception {
+ return put(path, body, ADMIN, ADMIN_PASSWORD);
+ }
+
+ private static HttpResponse<String> put(
+ String path, Object body, String username, String password) throws
Exception {
return HTTP.send(
- authorized(ADMIN, ADMIN_PASSWORD)
+ authorized(username, password)
.uri(URI.create(apiBase + path))
.header("Content-Type", ACCEPT)
.PUT(jsonBody(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..de0b94a736 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
@@ -20,8 +20,14 @@ package org.apache.gravitino.idp.web.rest;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+import java.util.Arrays;
import java.util.List;
+import javax.ws.rs.container.ContainerRequestContext;
+import javax.ws.rs.core.PathSegment;
+import javax.ws.rs.core.UriInfo;
import org.junit.jupiter.api.Test;
class TestIdpAuthorizationFilter {
@@ -33,4 +39,43 @@ class TestIdpAuthorizationFilter {
assertFalse(IdpAuthorizationFilter.isServiceAdmin(List.of("admin"),
"other"));
assertTrue(IdpAuthorizationFilter.isServiceAdmin(List.of("admin", "ops"),
"admin"));
}
+
+ @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"));
+ 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;
+ }
}
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..42580f0861 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
@@ -34,6 +34,12 @@ import javax.ws.rs.client.Invocation;
import javax.ws.rs.core.Application;
import javax.ws.rs.core.MediaType;
import javax.ws.rs.core.Response;
+import org.apache.commons.lang3.reflect.FieldUtils;
+import org.apache.gravitino.Config;
+import org.apache.gravitino.Configs;
+import org.apache.gravitino.GravitinoEnv;
+import org.apache.gravitino.UserPrincipal;
+import org.apache.gravitino.auth.AuthConstants;
import org.apache.gravitino.dto.responses.ErrorConstants;
import org.apache.gravitino.dto.responses.ErrorResponse;
import org.apache.gravitino.dto.responses.RemoveResponse;
@@ -53,7 +59,9 @@ import org.glassfish.hk2.utilities.binding.AbstractBinder;
import org.glassfish.jersey.server.ResourceConfig;
import org.glassfish.jersey.test.JerseyTest;
import org.glassfish.jersey.test.TestProperties;
+import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
@@ -63,6 +71,21 @@ class TestIdpOperations extends JerseyTest {
private static final String VALID_PASSWORD = "Passw0rd-For-User";
private static final IdpUserGroupManager MANAGER =
mock(IdpUserGroupManager.class);
+ private static Config previousConfig;
+
+ @BeforeAll
+ static void installServiceAdminConfig() throws Exception {
+ previousConfig = GravitinoEnv.getInstance().config();
+ Config config = mock(Config.class);
+ when(config.get(Configs.SERVICE_ADMINS)).thenReturn(List.of("admin"));
+ FieldUtils.writeField(GravitinoEnv.getInstance(), "config", config, true);
+ }
+
+ @AfterAll
+ static void restoreConfig() throws Exception {
+ FieldUtils.writeField(GravitinoEnv.getInstance(), "config",
previousConfig, true);
+ }
+
@BeforeEach
void resetManager() {
reset(MANAGER);
@@ -79,6 +102,8 @@ class TestIdpOperations extends JerseyTest {
HttpServletRequest request = mock(HttpServletRequest.class);
when(request.getRemoteUser()).thenReturn(null);
+
when(request.getAttribute(AuthConstants.AUTHENTICATED_PRINCIPAL_ATTRIBUTE_NAME))
+ .thenReturn(new UserPrincipal("admin"));
ResourceConfig resourceConfig = new ResourceConfig();
resourceConfig.register(IdpUserOperations.class);
@@ -133,10 +158,39 @@ 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
+ 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 testAddAndGetGroup() throws Exception {
AddGroupRequest req = new AddGroupRequest("group1");
