Skip to content

Commit

Permalink
fix minor bugs and reformat error messsages (#953)
Browse files Browse the repository at this point in the history 
* fix minor bugs and reformat error messsages

Signed-off-by: Bhavana Ramaram <rbhavna@amazon.com>
  • Loading branch information
@rbhavna
rbhavna authored Jun 1, 2023
1 parent 0146dbd commit 3bddbfb
Show file tree
Hide file tree
Showing 12 changed files with 70 additions and 60 deletions.
19 changes: 11 additions & 8 deletions ...src/main/java/org/opensearch/ml/action/model_group/TransportRegisterModelGroupAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -150,41 +150,44 @@ private void validateRequestForAccessControl(MLRegisterModelGroupInput input, Us
Boolean isAddAllBackendRoles = input.getIsAddAllBackendRoles();
if (modelAccessMode == null) {
if (!Boolean.TRUE.equals(isAddAllBackendRoles) && CollectionUtils.isEmpty(input.getBackendRoles())) {
throw new IllegalArgumentException("User must specify at least one backend role or make the model public/private");
throw new IllegalArgumentException(
"You must specify at least one backend role or make the model group public/private for registering it."
);
} else {
input.setModelAccessMode(ModelAccessMode.RESTRICTED);
modelAccessMode = ModelAccessMode.RESTRICTED;
}
}
if ((ModelAccessMode.PUBLIC == modelAccessMode || ModelAccessMode.PRIVATE == modelAccessMode)
&& (!CollectionUtils.isEmpty(input.getBackendRoles()) || Boolean.TRUE.equals(isAddAllBackendRoles))) {
throw new IllegalArgumentException("User cannot specify backend roles to a public/private model group");
throw new IllegalArgumentException("You can specify backend roles only for a model group with the restricted access mode.");
} else if (ModelAccessMode.RESTRICTED == modelAccessMode) {
if (modelAccessControlHelper.isAdmin(user) && Boolean.TRUE.equals(isAddAllBackendRoles)) {
throw new IllegalArgumentException("Admin user cannot specify add all backend roles to a model group");
throw new IllegalArgumentException("Admin users cannot add all backend roles to a model group.");
}
if (CollectionUtils.isEmpty(user.getBackendRoles())) {
throw new IllegalArgumentException("Current user has no backend roles to specify the model group as restricted");
throw new IllegalArgumentException("You must have at least one backend role to register a restricted model group.");
}
if (CollectionUtils.isEmpty(input.getBackendRoles()) && !Boolean.TRUE.equals(isAddAllBackendRoles)) {
throw new IllegalArgumentException(
"User have to specify backend roles or set add all backend roles to true for a restricted model group"
"You must specify one or more backend roles or add all backend roles to register a restricted model group."
);
}
if (!CollectionUtils.isEmpty(input.getBackendRoles()) && Boolean.TRUE.equals(isAddAllBackendRoles)) {
throw new IllegalArgumentException("User cannot specify add all backed roles to true and backend roles not empty");
throw new IllegalArgumentException("You cannot specify backend roles and add all backend roles at the same time.");
}
if (!modelAccessControlHelper.isAdmin(user)
&& !Boolean.TRUE.equals(isAddAllBackendRoles)
&& !new HashSet<>(user.getBackendRoles()).containsAll(input.getBackendRoles())) {
throw new IllegalArgumentException("User cannot specify backend roles that doesn't belong to the current user");
throw new IllegalArgumentException("You don't have the backend roles specified.");
}
}
}

private void validateSecurityDisabledOrModelAccessControlDisabled(MLRegisterModelGroupInput input) {
if (input.getModelAccessMode() != null || input.getIsAddAllBackendRoles() != null || input.getBackendRoles() != null) {
throw new IllegalArgumentException(
"Cluster security plugin not enabled or model access control no enabled, can't pass access control data in request body"
"You cannot specify model access control parameters because the Security plugin or model access control is disabled on your cluster."
);
}
}
Expand Down
37 changes: 22 additions & 15 deletions ...n/src/main/java/org/opensearch/ml/action/model_group/TransportUpdateModelGroupAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,9 @@ private void updateModelGroup(
if (ModelAccessMode.RESTRICTED != updateModelGroupInput.getModelAccessMode()) {
source.put(MLModelGroup.BACKEND_ROLES_FIELD, ImmutableList.of());
}
} else if (updateModelGroupInput.getBackendRoles() != null
|| Boolean.TRUE.equals(updateModelGroupInput.getIsAddAllBackendRoles())) {
source.put(MLModelGroup.ACCESS, ModelAccessMode.RESTRICTED.getValue());
}
if (updateModelGroupInput.getBackendRoles() != null) {
source.put(MLModelGroup.BACKEND_ROLES_FIELD, updateModelGroupInput.getBackendRoles());
Expand Down Expand Up @@ -158,40 +161,48 @@ private void updateModelGroup(
private void validateRequestForAccessControl(MLUpdateModelGroupInput input, User user, MLModelGroup mlModelGroup) {
if (hasAccessControlChange(input)) {
if (!modelAccessControlHelper.isOwner(mlModelGroup.getOwner(), user) && !modelAccessControlHelper.isAdmin(user)) {
throw new IllegalArgumentException("Only owner/admin has valid privilege to perform update access control data");
throw new IllegalArgumentException("Only owner or admin can update access control data.");
} else if (modelAccessControlHelper.isOwner(mlModelGroup.getOwner(), user)
&& !modelAccessControlHelper.isAdmin(user)
&& !modelAccessControlHelper.isOwnerStillHasPermission(user, mlModelGroup)) {
throw new IllegalArgumentException(
"Owner doesn't have corresponding backend role to perform update access control data, please check with admin user"
"You don’t have the specified backend role to update access control data. For more information, contact your administrator."
);
}
}
if (!modelAccessControlHelper.isAdmin(user)
&& !modelAccessControlHelper.isOwner(mlModelGroup.getOwner(), user)
&& !modelAccessControlHelper.isUserHasBackendRole(user, mlModelGroup)) {
throw new IllegalArgumentException("User doesn't have corresponding backend role to perform update action");
throw new IllegalArgumentException("You don't have permissions to perform this operation on this model group.");
}
ModelAccessMode modelAccessMode = input.getModelAccessMode();
if ((ModelAccessMode.PUBLIC == modelAccessMode || ModelAccessMode.PRIVATE == modelAccessMode)
&& (!CollectionUtils.isEmpty(input.getBackendRoles()) || Boolean.TRUE.equals(input.getIsAddAllBackendRoles()))) {
throw new IllegalArgumentException("User cannot specify backend roles to a public/private model group");
throw new IllegalArgumentException("You can specify backend roles only for a model group with the restricted access mode.");
} else if (modelAccessMode == null || ModelAccessMode.RESTRICTED == modelAccessMode) {
if (modelAccessControlHelper.isAdmin(user) && Boolean.TRUE.equals(input.getIsAddAllBackendRoles())) {
throw new IllegalArgumentException("Admin user cannot specify add all backend roles to a model group");
throw new IllegalArgumentException("Admin users cannot add all backend roles to a model group.");
}
if (Boolean.TRUE.equals(input.getIsAddAllBackendRoles()) && CollectionUtils.isEmpty(user.getBackendRoles())) {
throw new IllegalArgumentException("Current user doesn't have any backend role");
throw new IllegalArgumentException("You don’t have any backend roles.");
}
if (CollectionUtils.isEmpty(input.getBackendRoles()) && Boolean.FALSE.equals(input.getIsAddAllBackendRoles())) {
throw new IllegalArgumentException("User have to specify backend roles when add all backend roles to false");
throw new IllegalArgumentException("User have to specify backend roles when add all backend roles is set to false.");
}
if (!CollectionUtils.isEmpty(input.getBackendRoles()) && Boolean.TRUE.equals(input.getIsAddAllBackendRoles())) {
throw new IllegalArgumentException("User cannot specify add all backed roles to true and backend roles not empty");
throw new IllegalArgumentException("You cannot specify backend roles and add all backend roles at the same time.");
}
if (ModelAccessMode.RESTRICTED == modelAccessMode
&& CollectionUtils.isEmpty(input.getBackendRoles())
&& !Boolean.TRUE.equals(input.getIsAddAllBackendRoles())) {
throw new IllegalArgumentException(
"You must specify one or more backend roles or add all backend roles to register a restricted model group."
);
}
if (!modelAccessControlHelper.isAdmin(user)
&& inputBackendRolesAndModelBackendRolesBothNotEmpty(input, mlModelGroup)
&& !CollectionUtils.isEmpty(input.getBackendRoles())
&& !new HashSet<>(user.getBackendRoles()).containsAll(input.getBackendRoles())) {
throw new IllegalArgumentException("User cannot specify backend roles that doesn't belong to the current user");
throw new IllegalArgumentException("You don't have the backend roles specified.");
}
}
}
Expand All @@ -200,14 +211,10 @@ private boolean hasAccessControlChange(MLUpdateModelGroupInput input) {
return input.getModelAccessMode() != null || input.getIsAddAllBackendRoles() != null || input.getBackendRoles() != null;
}

private boolean inputBackendRolesAndModelBackendRolesBothNotEmpty(MLUpdateModelGroupInput input, MLModelGroup mlModelGroup) {
return !CollectionUtils.isEmpty(input.getBackendRoles()) && !CollectionUtils.isEmpty(mlModelGroup.getBackendRoles());
}

private void validateSecurityDisabledOrModelAccessControlDisabled(MLUpdateModelGroupInput input) {
if (input.getModelAccessMode() != null || input.getIsAddAllBackendRoles() != null || input.getBackendRoles() != null) {
throw new IllegalArgumentException(
"Cluster security plugin not enabled or model access control not enabled, can't pass access control data in request body"
"You cannot specify model access control parameters because the Security plugin or model access control is disabled on your cluster."
);
}
}
Expand Down
7 changes: 2 additions & 5 deletions plugin/src/main/java/org/opensearch/ml/action/register/TransportRegisterModelAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,11 +121,8 @@ protected void doExecute(Task task, ActionRequest request, ActionListener<MLRegi
modelAccessControlHelper
.validateModelGroupAccess(user, registerModelInput.getModelGroupId(), client, ActionListener.wrap(access -> {
if (!access) {
log.error("User doesn't have valid privilege to perform this operation on this model");
listener
.onFailure(
new IllegalArgumentException("User doesn't have valid privilege to perform this operation on this model")
);
log.error("You don't have permissions to perform this operation on this model.");
listener.onFailure(new IllegalArgumentException("You don't have permissions to perform this operation on this model."));
} else {
if (url != null) {
boolean validUrl = pattern.matcher(url).find();
Expand Down
4 changes: 2 additions & 2 deletions plugin/src/main/java/org/opensearch/ml/action/upload_chunk/MLModelChunkUploader.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,11 +81,11 @@ public void uploadModelChunk(MLUploadModelChunkInput uploadModelChunkInput, Acti
modelAccessControlHelper
.validateModelGroupAccess(user, existingModel.getModelGroupId(), client, ActionListener.wrap(access -> {
if (!access) {
log.error("User doesn't have valid privilege to perform this operation on this model");
log.error("You don't have permissions to perform this operation on this model.");
listener
.onFailure(
new IllegalArgumentException(
"User doesn't have valid privilege to perform this operation on this model"
"You don't have permissions to perform this operation on this model."
)
);
} else {
Expand Down
5 changes: 2 additions & 3 deletions ...src/main/java/org/opensearch/ml/action/upload_chunk/TransportRegisterModelMetaAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,8 @@ protected void doExecute(Task task, ActionRequest request, ActionListener<MLRegi

modelAccessControlHelper.validateModelGroupAccess(user, mlUploadInput.getModelGroupId(), client, ActionListener.wrap(access -> {
if (!access) {
log.error("User doesn't have valid privilege to perform this operation on this model");
listener
.onFailure(new IllegalArgumentException("User doesn't have valid privilege to perform this operation on this model"));
log.error("You don't have permissions to perform this operation on this model.");
listener.onFailure(new IllegalArgumentException("You don't have permissions to perform this operation on this model."));
} else {
mlModelManager.registerModelMeta(mlUploadInput, ActionListener.wrap(modelId -> {
listener.onResponse(new MLRegisterModelMetaResponse(modelId, MLTaskState.CREATED.name()));
Expand Down
3 changes: 2 additions & 1 deletion plugin/src/main/java/org/opensearch/ml/helper/ModelAccessControlHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,8 @@ public boolean isOwnerStillHasPermission(User user, MLModelGroup mlModelGroup) {
if (CollectionUtils.isEmpty(mlModelGroup.getBackendRoles())) {
throw new IllegalStateException("Backend roles should not be null");
}
return user.getBackendRoles() != null && new HashSet<>(mlModelGroup.getBackendRoles()).containsAll(user.getBackendRoles());
return user.getBackendRoles() != null
&& new HashSet<>(mlModelGroup.getBackendRoles()).stream().anyMatch(x -> user.getBackendRoles().contains(x));
}
throw new IllegalStateException("Access shouldn't be null");
}
Expand Down
21 changes: 12 additions & 9 deletions ...est/java/org/opensearch/ml/action/model_group/TransportRegisterModelGroupActionTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ public void test_ExceptionAllAccessFieldsNull() {
ArgumentCaptor<Exception> argumentCaptor = ArgumentCaptor.forClass(Exception.class);
verify(actionListener).onFailure(argumentCaptor.capture());
assertEquals(
"User must specify at least one backend role or make the model public/private",
"You must specify at least one backend role or make the model group public/private for registering it.",
argumentCaptor.getValue().getMessage()
);
}
Expand All @@ -160,7 +160,7 @@ public void test_BackendRolesProvidedWithPublic() {
transportRegisterModelGroupAction.doExecute(task, actionRequest, actionListener);
ArgumentCaptor<Exception> argumentCaptor = ArgumentCaptor.forClass(Exception.class);
verify(actionListener).onFailure(argumentCaptor.capture());
assertEquals("User cannot specify backend roles to a public/private model group", argumentCaptor.getValue().getMessage());
assertEquals("You can specify backend roles only for a model group with the restricted access mode.", argumentCaptor.getValue().getMessage());
}

public void test_BackendRolesProvidedWithPrivate() {
Expand All @@ -170,7 +170,7 @@ public void test_BackendRolesProvidedWithPrivate() {
transportRegisterModelGroupAction.doExecute(task, actionRequest, actionListener);
ArgumentCaptor<Exception> argumentCaptor = ArgumentCaptor.forClass(Exception.class);
verify(actionListener).onFailure(argumentCaptor.capture());
assertEquals("User cannot specify backend roles to a public/private model group", argumentCaptor.getValue().getMessage());
assertEquals("You can specify backend roles only for a model group with the restricted access mode.", argumentCaptor.getValue().getMessage());
}

public void test_AdminSpecifiedAddAllBackendRolesForRestricted() {
Expand All @@ -182,7 +182,7 @@ public void test_AdminSpecifiedAddAllBackendRolesForRestricted() {
transportRegisterModelGroupAction.doExecute(task, actionRequest, actionListener);
ArgumentCaptor<Exception> argumentCaptor = ArgumentCaptor.forClass(Exception.class);
verify(actionListener).onFailure(argumentCaptor.capture());
assertEquals("Admin user cannot specify add all backend roles to a model group", argumentCaptor.getValue().getMessage());
assertEquals("Admin users cannot add all backend roles to a model group.", argumentCaptor.getValue().getMessage());
}

public void test_UserWithNoBackendRolesSpecifiedRestricted() {
Expand All @@ -193,7 +193,10 @@ public void test_UserWithNoBackendRolesSpecifiedRestricted() {
transportRegisterModelGroupAction.doExecute(task, actionRequest, actionListener);
ArgumentCaptor<Exception> argumentCaptor = ArgumentCaptor.forClass(Exception.class);
verify(actionListener).onFailure(argumentCaptor.capture());
assertEquals("Current user has no backend roles to specify the model group as restricted", argumentCaptor.getValue().getMessage());
assertEquals(
"You must have at least one backend role to register a restricted model group.",
argumentCaptor.getValue().getMessage()
);
}

public void test_UserSpecifiedRestrictedButNoBackendRolesFieldF() {
Expand All @@ -205,7 +208,7 @@ public void test_UserSpecifiedRestrictedButNoBackendRolesFieldF() {
ArgumentCaptor<Exception> argumentCaptor = ArgumentCaptor.forClass(Exception.class);
verify(actionListener).onFailure(argumentCaptor.capture());
assertEquals(
"User have to specify backend roles or set add all backend roles to true for a restricted model group",
"You must specify one or more backend roles or add all backend roles to register a restricted model group.",
argumentCaptor.getValue().getMessage()
);
}
Expand All @@ -219,7 +222,7 @@ public void test_RestrictedAndUserSpecifiedBothBackendRolesField() {
ArgumentCaptor<Exception> argumentCaptor = ArgumentCaptor.forClass(Exception.class);
verify(actionListener).onFailure(argumentCaptor.capture());
assertEquals(
"User cannot specify add all backed roles to true and backend roles not empty",
"You cannot specify backend roles and add all backend roles at the same time.",
argumentCaptor.getValue().getMessage()
);
}
Expand All @@ -234,7 +237,7 @@ public void test_RestrictedAndUserSpecifiedIncorrectBackendRoles() {
transportRegisterModelGroupAction.doExecute(task, actionRequest, actionListener);
ArgumentCaptor<Exception> argumentCaptor = ArgumentCaptor.forClass(Exception.class);
verify(actionListener).onFailure(argumentCaptor.capture());
assertEquals("User cannot specify backend roles that doesn't belong to the current user", argumentCaptor.getValue().getMessage());
assertEquals("You don't have the backend roles specified.", argumentCaptor.getValue().getMessage());
}

public void test_SuccessSecurityDisabledCluster() {
Expand All @@ -254,7 +257,7 @@ public void test_ExceptionSecurityDisabledCluster() {
ArgumentCaptor<Exception> argumentCaptor = ArgumentCaptor.forClass(Exception.class);
verify(actionListener).onFailure(argumentCaptor.capture());
assertEquals(
"Cluster security plugin not enabled or model access control no enabled, can't pass access control data in request body",
"You cannot specify model access control parameters because the Security plugin or model access control is disabled on your cluster.",
argumentCaptor.getValue().getMessage()
);
}
Expand Down
Loading

0 comments on commit 3bddbfb

Please sign in to comment.