Skip to content

Commit

Permalink
Change password security message (#3057)
Browse files Browse the repository at this point in the history
### Description
Returns the error message instead of the validation message because
validation message should already have been shown in above case
statements.

### Issues Resolved
Fix: #3055 

Is this a backport? If so, please add backport PR # and/or commits #

### Testing
[Please provide details of testing done: unit testing, integration
testing and manual testing]

### Check List
- [ ] New functionality includes testing
- [ ] New functionality has been documented
- [ ] Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and
signing off your commits, please check
[here](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin).

---------

Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
Co-authored-by: Craig Perkins <[email protected]>
  • Loading branch information
derek-ho and cwperks authored Aug 17, 2023
1 parent 0d915e2 commit 847f911
Show file tree
Hide file tree
Showing 5 changed files with 130 additions and 20 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*
*/
package org.opensearch.security.api;

import java.util.List;
import java.util.Map;

import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope;
import org.junit.ClassRule;
import org.junit.Test;
import org.junit.runner.RunWith;

import org.opensearch.security.dlic.rest.validation.RequestContentValidator;
import org.opensearch.security.support.ConfigConstants;
import org.opensearch.test.framework.TestSecurityConfig.User;
import org.opensearch.test.framework.cluster.ClusterManager;
import org.opensearch.test.framework.cluster.LocalCluster;
import org.opensearch.test.framework.cluster.TestRestClient;
import org.opensearch.test.framework.cluster.TestRestClient.HttpResponse;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.opensearch.security.SecurityConfigurationTests.ADDITIONAL_USER_1;
import static org.opensearch.security.SecurityConfigurationTests.CREATE_USER_BODY;
import static org.opensearch.security.SecurityConfigurationTests.INTERNAL_USERS_RESOURCE;
import static org.opensearch.security.support.ConfigConstants.SECURITY_BACKGROUND_INIT_IF_SECURITYINDEX_NOT_EXIST;
import static org.opensearch.security.support.ConfigConstants.SECURITY_RESTAPI_ROLES_ENABLED;
import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL;
import static org.opensearch.test.framework.TestSecurityConfig.Role.ALL_ACCESS;

@RunWith(com.carrotsearch.randomizedtesting.RandomizedRunner.class)
@ThreadLeakScope(ThreadLeakScope.Scope.NONE)
public class CreateResetPasswordTest {

private static final User USER_ADMIN = new User("admin").roles(ALL_ACCESS);

public static final String INVALID_PASSWORD_REGEX = "user 1 fair password";

public static final String VALID_WEAK_PASSWORD = "Asdfghjkl1!";

public static final String VALID_SIMILAR_PASSWORD = "456Additional00001_1234!";

private static final String CUSTOM_PASSWORD_MESSAGE =
"Password must be minimum 5 characters long and must contain at least one uppercase letter, one lowercase letter, one digit, and one special character.";

private static final String CUSTOM_PASSWORD_REGEX = "(?=.*[A-Z])(?=.*[^a-zA-Z\\d])(?=.*[0-9])(?=.*[a-z]).{5,}";

@ClassRule
public static LocalCluster cluster = new LocalCluster.Builder().clusterManager(ClusterManager.THREE_CLUSTER_MANAGERS)
.authc(AUTHC_HTTPBASIC_INTERNAL)
.users(USER_ADMIN)
.anonymousAuth(false)
.nodeSettings(
Map.of(
SECURITY_RESTAPI_ROLES_ENABLED,
List.of("user_" + USER_ADMIN.getName() + "__" + ALL_ACCESS.getName()),
SECURITY_BACKGROUND_INIT_IF_SECURITYINDEX_NOT_EXIST,
true,
ConfigConstants.SECURITY_RESTAPI_PASSWORD_VALIDATION_REGEX,
CUSTOM_PASSWORD_REGEX,
ConfigConstants.SECURITY_RESTAPI_PASSWORD_VALIDATION_ERROR_MESSAGE,
CUSTOM_PASSWORD_MESSAGE
)
)
.build();

@Test
public void shouldValidateCreateUserAPIErrorMessages() {
try (TestRestClient client = cluster.getRestClient(USER_ADMIN)) {
HttpResponse httpResponse = client.putJson(
INTERNAL_USERS_RESOURCE + ADDITIONAL_USER_1,
String.format(CREATE_USER_BODY, INVALID_PASSWORD_REGEX)
);

assertThat(httpResponse.getStatusCode(), equalTo(400));
assertThat(httpResponse.getBody(), containsString(CUSTOM_PASSWORD_MESSAGE));
}

try (TestRestClient client = cluster.getRestClient(USER_ADMIN)) {
HttpResponse httpResponse = client.putJson(
INTERNAL_USERS_RESOURCE + ADDITIONAL_USER_1,
String.format(CREATE_USER_BODY, VALID_WEAK_PASSWORD)
);

assertThat(httpResponse.getStatusCode(), equalTo(400));
assertThat(httpResponse.getBody(), containsString(RequestContentValidator.ValidationError.WEAK_PASSWORD.message()));
}

try (TestRestClient client = cluster.getRestClient(USER_ADMIN)) {
HttpResponse httpResponse = client.putJson(
INTERNAL_USERS_RESOURCE + ADDITIONAL_USER_1,
String.format(CREATE_USER_BODY, VALID_SIMILAR_PASSWORD)
);

assertThat(httpResponse.getStatusCode(), equalTo(400));
assertThat(httpResponse.getBody(), containsString(RequestContentValidator.ValidationError.SIMILAR_PASSWORD.message()));
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -84,19 +84,19 @@ public RequestContentValidator.ValidationError validate(final String username, f
minPasswordLength,
password.length()
);
return RequestContentValidator.ValidationError.INVALID_PASSWORD;
return RequestContentValidator.ValidationError.INVALID_PASSWORD_TOO_SHORT;
}
if (password.length() > MAX_LENGTH) {
logger.debug(
"Password is too long, the maximum required length is {}, but current length is {}",
MAX_LENGTH,
password.length()
);
return RequestContentValidator.ValidationError.INVALID_PASSWORD;
return RequestContentValidator.ValidationError.INVALID_PASSWORD_TOO_LONG;
}
if (Objects.nonNull(passwordRegexpPattern) && !passwordRegexpPattern.matcher(password).matches()) {
logger.debug("Regex does not match password");
return RequestContentValidator.ValidationError.INVALID_PASSWORD;
return RequestContentValidator.ValidationError.INVALID_PASSWORD_INVALID_REGEX;
}
final Strength strength = zxcvbn.measure(password, ImmutableList.of(username));
if (strength.getScore() < scoreStrength.score()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ private ValidationResult validatePassword(final RestRequest request, final JsonN
final PasswordValidator passwordValidator = PasswordValidator.of(validationContext.settings());
final String password = jsonContent.get("password").asText();
if (Strings.isNullOrEmpty(password)) {
this.validationError = ValidationError.INVALID_PASSWORD;
this.validationError = ValidationError.INVALID_PASSWORD_TOO_SHORT;
return ValidationResult.error(this);
}
final String username = Utils.coalesce(
Expand Down Expand Up @@ -278,22 +278,14 @@ public XContentBuilder toXContent(XContentBuilder builder, ToXContent.Params par
addErrorMessage(builder, MISSING_MANDATORY_KEYS_KEY, missingMandatoryKeys);
addErrorMessage(builder, MISSING_MANDATORY_OR_KEYS_KEY, missingMandatoryOrKeys);
break;
case INVALID_PASSWORD:
case INVALID_PASSWORD_INVALID_REGEX:
builder.field("status", "error");
builder.field(
"reason",
validationContext.settings()
.get(SECURITY_RESTAPI_PASSWORD_VALIDATION_ERROR_MESSAGE, "Password does not match minimum criteria")
);
break;
case WEAK_PASSWORD:
case SIMILAR_PASSWORD:
builder.field("status", "error");
builder.field(
"reason",
validationContext.settings().get(SECURITY_RESTAPI_PASSWORD_VALIDATION_ERROR_MESSAGE, validationError.message())
);
break;
case WRONG_DATATYPE:
builder.field("status", "error");
builder.field("reason", ValidationError.WRONG_DATATYPE.message());
Expand Down Expand Up @@ -357,7 +349,9 @@ public Map<String, DataType> allowedKeys() {
public enum ValidationError {
NONE("ok"),
INVALID_CONFIGURATION("Invalid configuration"),
INVALID_PASSWORD("Invalid password"),
INVALID_PASSWORD_TOO_SHORT("Password is too short"),
INVALID_PASSWORD_TOO_LONG("Password is too long"),
INVALID_PASSWORD_INVALID_REGEX("Password does not match validation regex"),
NO_USERNAME("No username is given"),
WEAK_PASSWORD("Weak password"),
SIMILAR_PASSWORD("Password is similar to user name"),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -665,7 +665,7 @@ public void testRegExpPasswordRules() throws Exception {

verifyCouldNotCreatePasswords(HttpStatus.SC_BAD_REQUEST);
verifyCanCreatePasswords();
verifySimilarity("xxx");
verifySimilarity(RequestContentValidator.ValidationError.SIMILAR_PASSWORD.message());

addUserWithPasswordAndHash("empty_password", "", "$%^123", HttpStatus.SC_BAD_REQUEST);
addUserWithPasswordAndHash("null_password", null, "$%^123", HttpStatus.SC_BAD_REQUEST);
Expand Down Expand Up @@ -808,7 +808,12 @@ public void testScoreBasedPasswordRules() throws Exception {
RequestContentValidator.ValidationError.WEAK_PASSWORD.message()
);

addUserWithPassword("admin", "pas", HttpStatus.SC_BAD_REQUEST, "Password does not match minimum criteria");
addUserWithPassword(
"admin",
"pas",
HttpStatus.SC_BAD_REQUEST,
RequestContentValidator.ValidationError.INVALID_PASSWORD_TOO_SHORT.message()
);

verifySimilarity(RequestContentValidator.ValidationError.SIMILAR_PASSWORD.message());

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,12 +126,12 @@ public void testRegExpBasedValidation() {
.put(SECURITY_RESTAPI_PASSWORD_VALIDATION_REGEX, "(?=.*[A-Z])(?=.*[^a-zA-Z\\\\d])(?=.*[0-9])(?=.*[a-z]).{8,}")
.build()
);
verifyWeakPasswords(passwordValidator, RequestContentValidator.ValidationError.INVALID_PASSWORD);
verifyFairPasswords(passwordValidator, RequestContentValidator.ValidationError.INVALID_PASSWORD);
verifyWeakPasswords(passwordValidator, RequestContentValidator.ValidationError.INVALID_PASSWORD_INVALID_REGEX);
verifyFairPasswords(passwordValidator, RequestContentValidator.ValidationError.INVALID_PASSWORD_INVALID_REGEX);
for (final String password : GOOD_PASSWORDS.subList(0, GOOD_PASSWORDS.size() - 2))
assertEquals(
password,
RequestContentValidator.ValidationError.INVALID_PASSWORD,
RequestContentValidator.ValidationError.INVALID_PASSWORD_INVALID_REGEX,
passwordValidator.validate("some_user_name", password)
);
for (final String password : GOOD_PASSWORDS.subList(GOOD_PASSWORDS.size() - 2, GOOD_PASSWORDS.size()))
Expand All @@ -151,7 +151,10 @@ public void testMinLength() {
Settings.builder().put(SECURITY_RESTAPI_PASSWORD_MIN_LENGTH, 15).build()
);
for (final String password : STRONG_PASSWORDS) {
assertEquals(RequestContentValidator.ValidationError.INVALID_PASSWORD, passwordValidator.validate(password, "some_user_name"));
assertEquals(
RequestContentValidator.ValidationError.INVALID_PASSWORD_TOO_SHORT,
passwordValidator.validate(password, "some_user_name")
);
}

}
Expand Down

0 comments on commit 847f911

Please sign in to comment.