Skip to content

Commit

Permalink
[BACKPORT 2.20][PLAT-10472] Insecure Account Profile Management
Browse files Browse the repository at this point in the history
Summary:
  - Deprecate the API

```
PUT    /customers/:cUUID/users/:uUUID/change_password                          com.yugabyte.yw.controllers.UsersController.changePassword(cUUID: java.util.UUID, uUUID: java.util.UUID, request: Request)
```

  - The deprecated change_password  API would throw Forbidden response always. Also, it would suggest the user to use the new API for changing the password.

  - The reason for deprecating this API is because the request body does not support a way to provide the current password. Also, we do not require the userUUID as part of the request as we only allow password change from the same user.

  - The API
```
PUT    /customers/:cUUID/users/:uUUID/update_profile                           com.yugabyte.yw.controllers.UsersController.updateProfile(cUUID: java.util.UUID, uUUID: java.util.UUID, request: Request)
```
 I would throw Forbidden response in case the request attempts to change the password. I would also make a change to disallow a user to change their own Role in the old RBAC way using the update_profile API.

  - The new API PUT    /customers/:cUUID/reset_password is used to reset the password of the loggedin user only. The user is identified using the Auth/API token.

```
{
    "currentPassword": "oldPassword",
    "newPassword": "newPassword"
}
```

Original commit: db597f2
Original diff: https://phorge.dev.yugabyte.com/D34049

Test Plan:
Manual testing with old RBAC and new RBAC flows
UTs

Reviewers: #yba-api-review, sneelakantan

Reviewed By: #yba-api-review, sneelakantan

Subscribers: yugaware

Tags: #jenkins-ready

Differential Revision: https://phorge.dev.yugabyte.com/D34429
  • Loading branch information
vpatibandla-yb committed Apr 25, 2024
1 parent 98c737a commit 9687371
Show file tree
Hide file tree
Showing 9 changed files with 441 additions and 169 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ public class RedactingService {
.addAll(SECRET_PATHS_FOR_APIS)
.add("$..password")
.add("$..confirmPassword")
.add("$..newPassword")
.add("$..currentPassword")
.add("$..['config.AWS_ACCESS_KEY_ID']")
.add("$..['config.AWS_SECRET_ACCESS_KEY']")
// GCP private key
Expand Down
110 changes: 68 additions & 42 deletions managed/src/main/java/com/yugabyte/yw/controllers/UsersController.java
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,14 @@
import com.yugabyte.yw.common.user.UserService;
import com.yugabyte.yw.forms.PlatformResults;
import com.yugabyte.yw.forms.PlatformResults.YBPSuccess;
import com.yugabyte.yw.forms.UserPasswordChangeFormData;
import com.yugabyte.yw.forms.UserProfileFormData;
import com.yugabyte.yw.forms.UserRegisterFormData;
import com.yugabyte.yw.models.Audit;
import com.yugabyte.yw.models.Customer;
import com.yugabyte.yw.models.Users;
import com.yugabyte.yw.models.Users.UserType;
import com.yugabyte.yw.models.common.YbaApi;
import com.yugabyte.yw.models.extended.UserWithFeatures;
import com.yugabyte.yw.models.rbac.ResourceGroup;
import com.yugabyte.yw.models.rbac.Role;
Expand All @@ -46,6 +48,7 @@
import java.util.*;
import java.util.stream.Collectors;
import javax.inject.Inject;
import lombok.extern.slf4j.Slf4j;
import org.apache.commons.lang3.StringUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand All @@ -57,6 +60,7 @@
@Api(
value = "User management",
authorizations = @Authorization(AbstractPlatformController.API_KEY_AUTH))
@Slf4j
public class UsersController extends AuthenticatedController {

public static final Logger LOG = LoggerFactory.getLogger(UsersController.class);
Expand Down Expand Up @@ -441,9 +445,8 @@ public Result changeRole(UUID customerUUID, UUID userUUID, String role, Http.Req
* @return JSON response on whether role change was successful or not.
*/
@ApiOperation(
value = "Change a user's password",
nickname = "updateUserPassword",
response = YBPSuccess.class)
notes = "<b style=\"color:#ff0000\">Deprecated since YBA version 2024.1.0.0.</b></p>",
value = "Change password - deprecated")
@ApiImplicitParams({
@ApiImplicitParam(
name = "Users",
Expand All @@ -459,36 +462,64 @@ public Result changeRole(UUID customerUUID, UUID userUUID, String role, Http.Req
resourceType = ResourceType.USER,
action = Action.UPDATE_PROFILE),
resourceLocation = @Resource(path = Util.USERS, sourceType = SourceType.ENDPOINT)))
@YbaApi(visibility = YbaApi.YbaApiVisibility.DEPRECATED, sinceYBAVersion = "2.20.4.0")
@Deprecated
public Result changePassword(UUID customerUUID, UUID userUUID, Http.Request request) {
Users user = Users.getOrBadRequest(customerUUID, userUUID);
if (UserType.ldap == user.getUserType()) {
throw new PlatformServiceException(BAD_REQUEST, "Can't change password for LDAP user.");
}
throw new PlatformServiceException(
MOVED_PERMANENTLY, String.format("Moved to /customers/%s/reset_password", customerUUID));
}

/**
* PUT endpoint for changing the password of an existing user.
*
* @return JSON response on whether role change was successful or not.
*/
@ApiOperation(
value = "Reset the user's password",
nickname = "resetUserPassword",
response = YBPSuccess.class)
@ApiImplicitParams({
@ApiImplicitParam(
name = "Users",
value = "User data containing the current, new password",
required = true,
dataType = "com.yugabyte.yw.forms.UserPasswordChangeFormData",
paramType = "body")
})
@AuthzPath(
@RequiredPermissionOnResource(
requiredPermission =
@PermissionAttribute(
resourceType = ResourceType.USER,
action = Action.UPDATE_PROFILE),
resourceLocation = @Resource(path = Util.USERS, sourceType = SourceType.REQUEST_CONTEXT)))
public Result resetPassword(UUID customerUUID, Http.Request request) {
Users user = getLoggedInUser(request);
Form<UserPasswordChangeFormData> form =
formFactory.getFormDataOrBadRequest(request, UserPasswordChangeFormData.class);
UserPasswordChangeFormData formData = form.get();

if (!checkUpdateProfileAccessForPasswordChange(userUUID, request)) {
if (user.getUserType() == UserType.ldap) {
throw new PlatformServiceException(
BAD_REQUEST, "Only the User can change his/her own password.");
BAD_REQUEST, "Reset password not supported for LDAP users");
}

Form<UserRegisterFormData> form =
formFactory.getFormDataOrBadRequest(request, UserRegisterFormData.class);

UserRegisterFormData formData = form.get();
passwordPolicyService.checkPasswordPolicy(customerUUID, formData.getPassword());
if (formData.getEmail().equals(user.getEmail())) {
if (formData.getPassword().equals(formData.getConfirmPassword())) {
user.setPassword(formData.getPassword());
user.save();
auditService()
.createAuditEntry(
request,
Audit.TargetType.User,
userUUID.toString(),
Audit.ActionType.ChangeUserPassword);
return YBPSuccess.empty();
}
user = Users.authWithPassword(user.getEmail(), formData.getCurrentPassword());
if (user == null) {
throw new PlatformServiceException(UNAUTHORIZED, "Incorrect current password provided");
}
throw new PlatformServiceException(BAD_REQUEST, "Invalid user credentials.");

passwordPolicyService.checkPasswordPolicy(customerUUID, formData.getNewPassword());
user.setPassword(formData.getNewPassword());
user.save();
auditService()
.createAuditEntryWithReqBody(
request,
Audit.TargetType.User,
user.getUuid().toString(),
Audit.ActionType.ChangeUserPassword,
Json.toJson(formData));
return YBPSuccess.empty();
}

private Users getLoggedInUser(Http.Request request) {
Expand Down Expand Up @@ -542,21 +573,11 @@ public Result updateProfile(UUID customerUUID, UUID userUUID, Http.Request reque

// Password validation for both old RBAC and new RBAC is same.
if (StringUtils.isNotEmpty(formData.getPassword())) {
if (UserType.ldap == user.getUserType()) {
throw new PlatformServiceException(BAD_REQUEST, "Can't change password for LDAP user.");
}

if (!checkUpdateProfileAccessForPasswordChange(userUUID, request)) {
throw new PlatformServiceException(
BAD_REQUEST, "Only the User can change his/her own password.");
}

passwordPolicyService.checkPasswordPolicy(customerUUID, formData.getPassword());
if (!formData.getPassword().equals(formData.getConfirmPassword())) {
throw new PlatformServiceException(
BAD_REQUEST, "Password and confirm password do not match.");
}
user.setPassword(formData.getPassword());
throw new PlatformServiceException(
FORBIDDEN,
String.format(
"API does not support password change. Use /customers/%s/reset_password",
customerUUID));
}

if (useNewAuthz) {
Expand Down Expand Up @@ -606,6 +627,11 @@ public Result updateProfile(UUID customerUUID, UUID userUUID, Http.Request reque
BAD_REQUEST, "Can't Assign the role of SuperAdmin to another user.");
}

if (loggedInUser.getUuid().equals(user.getUuid())) {
throw new PlatformServiceException(
FORBIDDEN, "User cannot modify their own role privileges");
}

if (user.getUserType() == UserType.ldap && user.isLdapSpecifiedRole() == true) {
throw new PlatformServiceException(BAD_REQUEST, "Cannot change role for LDAP user.");
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package com.yugabyte.yw.forms;

import io.swagger.annotations.ApiModel;
import io.swagger.annotations.ApiModelProperty;
import lombok.Getter;
import lombok.Setter;

@ApiModel(
value = "UserPasswordChangeFormData",
description = "User registration data. The API and UI use this to validate form data.")
@Getter
@Setter
public class UserPasswordChangeFormData {

@ApiModelProperty(value = "Current Password", example = "Test@1234")
private String currentPassword;

@ApiModelProperty(value = "New Password", example = "Test@1234")
private String newPassword;
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@
public enum SourceType {
ENDPOINT("endpoint"),
REQUEST_BODY("requestBody"),
DB("db");
DB("db"),
REQUEST_CONTEXT("requestContext");

private final String type;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import com.fasterxml.jackson.databind.ObjectMapper;
import com.google.inject.Inject;
import com.typesafe.config.Config;
import com.yugabyte.yw.common.Util;
import com.yugabyte.yw.common.config.GlobalConfKeys;
import com.yugabyte.yw.common.config.RuntimeConfigCache;
import com.yugabyte.yw.controllers.JWTVerifier;
Expand Down Expand Up @@ -44,7 +45,6 @@ public class AuthorizationHandler extends Action<AuthzPath> {
public static final String API_TOKEN_HEADER = "X-AUTH-YW-API-TOKEN";
public static final String API_JWT_HEADER = "X-AUTH-YW-API-JWT";
public static final String COOKIE_PLAY_SESSION = "PLAY_SESSION";
private static final String CUSTOMERS = "customers";

private final Config config;
private final RuntimeConfigCache runtimeConfigCache;
Expand Down Expand Up @@ -81,12 +81,13 @@ public CompletionStage<Result> call(Http.Request request) {
return CompletableFuture.completedFuture(Results.unauthorized("Unable To authenticate User"));
}
UserWithFeatures userWithFeatures = new UserWithFeatures().setUser(user);
RequestContext.put(TokenAuthenticator.CUSTOMER, Customer.get(user.getCustomerUUID()));
Customer customer = Customer.get(user.getCustomerUUID());
RequestContext.put(TokenAuthenticator.CUSTOMER, customer);
RequestContext.put(TokenAuthenticator.USER, userWithFeatures);

String endpoint = request.uri();
UUID customerUUID = null;
Pattern custPattern = Pattern.compile(String.format(".*/%s/" + UUID_PATTERN, CUSTOMERS));
Pattern custPattern = Pattern.compile(String.format(".*/%s/" + UUID_PATTERN, Util.CUSTOMERS));
Matcher custMatcher = custPattern.matcher(endpoint);
if (custMatcher.find()) {
customerUUID = UUID.fromString(custMatcher.group(1));
Expand Down Expand Up @@ -149,7 +150,7 @@ public CompletionStage<Result> call(Http.Request request) {
Matcher matcher = pattern.matcher(endpoint);
if (matcher.find()) {
resourceUUID = UUID.fromString(matcher.group(3));
} else if (resource.path().equals(CUSTOMERS)) {
} else if (resource.path().equals(Util.CUSTOMERS)) {
resourceUUID = user.getCustomerUUID();
}
isPermissionPresentOnResource =
Expand Down Expand Up @@ -233,6 +234,46 @@ public CompletionStage<Result> call(Http.Request request) {
}
break;
}
case REQUEST_CONTEXT:
{
switch (resource.path()) {
case Util.USERS:
{
isPermissionPresentOnResource =
checkResourcePermission(applicableRoleBindings, attribute, user.getUuid());
if (!isPermissionPresentOnResource) {
log.debug(
"User {} does not have role bindings for the permission {}",
user.getUuid(),
attribute);
return CompletableFuture.completedFuture(
Results.unauthorized("Unable to authorize user"));
}
break;
}
case Util.CUSTOMERS:
{
isPermissionPresentOnResource =
checkResourcePermission(
applicableRoleBindings, attribute, customer.getUuid());
if (!isPermissionPresentOnResource) {
log.debug(
"User {} does not have role bindings for the permission {}",
user.getUuid(),
attribute);
return CompletableFuture.completedFuture(
Results.unauthorized("Unable to authorize user"));
}
break;
}
default:
{
return CompletableFuture.completedFuture(
Results.unauthorized("Unable to authorize user"));
}
}
break;
}
default:
{
log.debug("Authorization logic {} not supported", resource.sourceType());
Expand Down
Loading

0 comments on commit 9687371

Please sign in to comment.