Skip to content

Commit 2585b8e

Browse files
DarshitChanpuraDarshitChanpura
authored
Adds capability to automatically switch to old access-control if model-group is excluded from protected resources setting (opensearch-project#4244)
* Adds check to enable resource sharing for protected types supplied in cluster setting Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Refactored ResourceSharingClient imports to resolve NoClassDef found error Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds search tests for conditional test execution Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds rsc unit tests for remainder model group transport actions Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Fix static instantiation causing flakyness Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Boosts ModelAccessControlHelper coverage Signed-off-by: Darshit Chanpura <dchanp@amazon.com> --------- Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
1 parent 5964268 commit 2585b8e

14 files changed

+521
-33
lines changed

‎plugin/build.gradle‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -213,7 +213,7 @@ tasks.named("check").configure { dependsOn(integTest) }
213213

214214
// Enable Security if -Dsecurity=true or -Dhttps=true
215215
def securityEnabled = System.getProperty("security", "false") == "true" ||
216-
System.getProperty("https", "false") == "true"
216+
System.getProperty("https", "false") == "true"
217217

218218
integTest {
219219
dependsOn "bundlePlugin"
@@ -321,7 +321,7 @@ def configureClusterPlugins(cluster, jobSchedZip, securityZip, securityEnabled)
321321
plugin(project.tasks.bundlePlugin.archiveFile)
322322

323323
if (securityEnabled) {
324-
nodes.each { node ->
324+
cluster.nodes.each { node ->
325325
node.extraConfigFile("kirk.pem", file("build/resources/test/kirk.pem"))
326326
node.extraConfigFile("kirk-key.pem", file("build/resources/test/kirk-key.pem"))
327327
node.extraConfigFile("esnode.pem", file("build/resources/test/esnode.pem"))

‎plugin/src/main/java/org/opensearch/ml/action/handler/MLSearchHandler.java‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@
77

88
import static org.opensearch.core.rest.RestStatus.BAD_REQUEST;
99
import static org.opensearch.core.rest.RestStatus.INTERNAL_SERVER_ERROR;
10+
import static org.opensearch.ml.common.CommonValue.ML_MODEL_GROUP_RESOURCE_TYPE;
11+
import static org.opensearch.ml.helper.ModelAccessControlHelper.shouldUseResourceAuthz;
1012
import static org.opensearch.ml.utils.RestActionUtils.wrapListenerToHandleSearchIndexNotFound;
1113

1214
import java.util.ArrayList;
@@ -52,7 +54,6 @@
5254
import org.opensearch.search.SearchHits;
5355
import org.opensearch.search.builder.SearchSourceBuilder;
5456
import org.opensearch.search.fetch.subphase.FetchSourceContext;
55-
import org.opensearch.security.spi.resources.client.ResourceSharingClient;
5657
import org.opensearch.transport.client.Client;
5758

5859
import com.google.common.annotations.VisibleForTesting;
@@ -147,11 +148,13 @@ public void search(SdkClient sdkClient, SearchRequest request, String tenantId,
147148
mlFeatureEnabledSetting.isMultiTenancyEnabled(),
148149
CommonValue.ML_MODEL_GROUP_INDEX
149150
);
150-
boolean rsClientPresent = ResourceSharingClientAccessor.getInstance().getResourceSharingClient() != null;
151151

152-
if (rsClientPresent && user != null && modelAccessControlHelper.modelAccessControlEnabled() && hasModelGroupIndex) {
152+
if (shouldUseResourceAuthz(ML_MODEL_GROUP_RESOURCE_TYPE)
153+
&& user != null
154+
&& modelAccessControlHelper.modelAccessControlEnabled()
155+
&& hasModelGroupIndex) {
153156
// RSC fast-path: get accessible group IDs → gate models (IDs or missing)
154-
ResourceSharingClient rsc = ResourceSharingClientAccessor.getInstance().getResourceSharingClient();
157+
var rsc = ResourceSharingClientAccessor.getInstance().getResourceSharingClient();
155158
rsc.getAccessibleResourceIds(CommonValue.ML_MODEL_GROUP_INDEX, ActionListener.wrap(ids -> {
156159
SearchSourceBuilder gated = Optional.ofNullable(request.source()).orElseGet(SearchSourceBuilder::new);
157160
gated.query(rewriteQueryBuilderRSC(gated.query(), ids)); // ids may be empty → "missing only"

‎plugin/src/main/java/org/opensearch/ml/action/model_group/DeleteModelGroupTransportAction.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,9 @@
66
package org.opensearch.ml.action.model_group;
77

88
import static org.opensearch.ml.common.CommonValue.ML_MODEL_GROUP_INDEX;
9+
import static org.opensearch.ml.common.CommonValue.ML_MODEL_GROUP_RESOURCE_TYPE;
910
import static org.opensearch.ml.common.CommonValue.ML_MODEL_INDEX;
11+
import static org.opensearch.ml.helper.ModelAccessControlHelper.shouldUseResourceAuthz;
1012
import static org.opensearch.ml.utils.RestActionUtils.PARAMETER_MODEL_GROUP_ID;
1113

1214
import org.opensearch.ExceptionsHelper;
@@ -27,7 +29,6 @@
2729
import org.opensearch.index.IndexNotFoundException;
2830
import org.opensearch.index.query.BoolQueryBuilder;
2931
import org.opensearch.index.query.TermQueryBuilder;
30-
import org.opensearch.ml.common.ResourceSharingClientAccessor;
3132
import org.opensearch.ml.common.exception.MLValidationException;
3233
import org.opensearch.ml.common.settings.MLFeatureEnabledSetting;
3334
import org.opensearch.ml.common.transport.model_group.MLModelGroupDeleteAction;
@@ -96,7 +97,7 @@ protected void doExecute(Task task, ActionRequest request, ActionListener<Delete
9697
ActionListener<DeleteResponse> wrappedListener = ActionListener.runBefore(actionListener, context::restore);
9798

9899
// if resource sharing feature is enabled, access will be automatically checked by security plugin, so no need to check again
99-
if (ResourceSharingClientAccessor.getInstance().getResourceSharingClient() != null) {
100+
if (shouldUseResourceAuthz(ML_MODEL_GROUP_RESOURCE_TYPE)) {
100101
checkForAssociatedModels(modelGroupId, tenantId, wrappedListener);
101102
} else {
102103
validateAndDeleteModelGroup(modelGroupId, tenantId, wrappedListener);

‎plugin/src/main/java/org/opensearch/ml/action/model_group/GetModelGroupTransportAction.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@
88
import static org.opensearch.common.xcontent.json.JsonXContent.jsonXContent;
99
import static org.opensearch.core.xcontent.XContentParserUtils.ensureExpectedToken;
1010
import static org.opensearch.ml.common.CommonValue.ML_MODEL_GROUP_INDEX;
11+
import static org.opensearch.ml.common.CommonValue.ML_MODEL_GROUP_RESOURCE_TYPE;
12+
import static org.opensearch.ml.helper.ModelAccessControlHelper.shouldUseResourceAuthz;
1113

1214
import org.opensearch.ExceptionsHelper;
1315
import org.opensearch.OpenSearchStatusException;
@@ -27,7 +29,6 @@
2729
import org.opensearch.core.xcontent.XContentParser;
2830
import org.opensearch.index.IndexNotFoundException;
2931
import org.opensearch.ml.common.MLModelGroup;
30-
import org.opensearch.ml.common.ResourceSharingClientAccessor;
3132
import org.opensearch.ml.common.settings.MLFeatureEnabledSetting;
3233
import org.opensearch.ml.common.transport.model_group.MLModelGroupGetAction;
3334
import org.opensearch.ml.common.transport.model_group.MLModelGroupGetRequest;
@@ -186,7 +187,7 @@ private void validateModelGroupAccess(
186187
) {
187188
// if resource sharing feature is enabled, security plugin will have automatically evaluated access to this model group, hence no
188189
// need to validate again
189-
if (ResourceSharingClientAccessor.getInstance().getResourceSharingClient() != null) {
190+
if (shouldUseResourceAuthz(ML_MODEL_GROUP_RESOURCE_TYPE)) {
190191
wrappedListener.onResponse(MLModelGroupGetResponse.builder().mlModelGroup(mlModelGroup).build());
191192
return;
192193
}

‎plugin/src/main/java/org/opensearch/ml/action/model_group/SearchModelGroupTransportAction.java‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@
77

88
import static org.opensearch.ml.action.handler.MLSearchHandler.wrapRestActionListener;
99
import static org.opensearch.ml.common.CommonValue.ML_MODEL_GROUP_INDEX;
10+
import static org.opensearch.ml.common.CommonValue.ML_MODEL_GROUP_RESOURCE_TYPE;
11+
import static org.opensearch.ml.helper.ModelAccessControlHelper.shouldUseResourceAuthz;
1012
import static org.opensearch.ml.utils.RestActionUtils.wrapListenerToHandleSearchIndexNotFound;
1113

1214
import java.util.Collections;
@@ -31,7 +33,6 @@
3133
import org.opensearch.remote.metadata.client.SearchDataObjectRequest;
3234
import org.opensearch.remote.metadata.common.SdkClientUtils;
3335
import org.opensearch.search.builder.SearchSourceBuilder;
34-
import org.opensearch.security.spi.resources.client.ResourceSharingClient;
3536
import org.opensearch.tasks.Task;
3637
import org.opensearch.transport.TransportService;
3738
import org.opensearch.transport.client.Client;
@@ -89,7 +90,7 @@ private void preProcessRoleAndPerformSearch(
8990
.wrap(wrappedListener::onResponse, e -> wrapListenerToHandleSearchIndexNotFound(e, wrappedListener));
9091

9192
// If resource-sharing feature is enabled, we fetch accessible model-groups and restrict the search to those model-groups only.
92-
if (ResourceSharingClientAccessor.getInstance().getResourceSharingClient() != null) {
93+
if (shouldUseResourceAuthz(ML_MODEL_GROUP_RESOURCE_TYPE)) {
9394
// If a model-group is shared, then it will have been shared at-least at read access, hence the final result is guaranteed
9495
// to only contain model-groups that the user at-least has read access to.
9596
addAccessibleModelGroupsFilterAndSearch(tenantId, request, doubleWrappedListener);
@@ -113,7 +114,7 @@ private void addAccessibleModelGroupsFilterAndSearch(
113114
ActionListener<SearchResponse> wrappedListener
114115
) {
115116
SearchSourceBuilder sourceBuilder = request.source() != null ? request.source() : new SearchSourceBuilder();
116-
ResourceSharingClient rsc = ResourceSharingClientAccessor.getInstance().getResourceSharingClient();
117+
var rsc = ResourceSharingClientAccessor.getInstance().getResourceSharingClient();
117118
// filter by accessible model-groups
118119
rsc.getAccessibleResourceIds(ML_MODEL_GROUP_INDEX, ActionListener.wrap(ids -> {
119120
sourceBuilder.query(modelAccessControlHelper.mergeWithAccessFilter(sourceBuilder.query(), ids));

‎plugin/src/main/java/org/opensearch/ml/action/model_group/TransportUpdateModelGroupAction.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@
99
import static org.opensearch.core.xcontent.XContentParserUtils.ensureExpectedToken;
1010
import static org.opensearch.ml.common.CommonValue.BACKEND_ROLES_FIELD;
1111
import static org.opensearch.ml.common.CommonValue.ML_MODEL_GROUP_INDEX;
12+
import static org.opensearch.ml.common.CommonValue.ML_MODEL_GROUP_RESOURCE_TYPE;
13+
import static org.opensearch.ml.helper.ModelAccessControlHelper.shouldUseResourceAuthz;
1214
import static org.opensearch.ml.utils.MLExceptionUtils.logException;
1315

1416
import java.time.Instant;
@@ -36,7 +38,6 @@
3638
import org.opensearch.index.IndexNotFoundException;
3739
import org.opensearch.ml.common.AccessMode;
3840
import org.opensearch.ml.common.MLModelGroup;
39-
import org.opensearch.ml.common.ResourceSharingClientAccessor;
4041
import org.opensearch.ml.common.exception.MLValidationException;
4142
import org.opensearch.ml.common.settings.MLFeatureEnabledSetting;
4243
import org.opensearch.ml.common.transport.model_group.MLUpdateModelGroupAction;
@@ -150,7 +151,7 @@ protected void doExecute(Task task, ActionRequest request, ActionListener<MLUpda
150151
)) {
151152
// NOTE all sharing and revoking must happen through share API exposed by security plugin
152153
// client == null -> feature is disabled, follow old route
153-
if (ResourceSharingClientAccessor.getInstance().getResourceSharingClient() == null) {
154+
if (!shouldUseResourceAuthz(ML_MODEL_GROUP_RESOURCE_TYPE)) {
154155
// TODO: At some point, this call must be replaced by the one above, (i.e. no user info to
155156
// be stored in model-group index)
156157
if (modelAccessControlHelper.isSecurityEnabledAndModelAccessControlEnabled(user)) {

‎plugin/src/main/java/org/opensearch/ml/helper/ModelAccessControlHelper.java‎

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import static org.opensearch.core.xcontent.XContentParserUtils.ensureExpectedToken;
1010
import static org.opensearch.ml.common.CommonValue.BACKEND_ROLES_FIELD;
1111
import static org.opensearch.ml.common.CommonValue.ML_MODEL_GROUP_INDEX;
12+
import static org.opensearch.ml.common.CommonValue.ML_MODEL_GROUP_RESOURCE_TYPE;
1213
import static org.opensearch.ml.common.settings.MLCommonsSettings.ML_COMMONS_MODEL_ACCESS_CONTROL_ENABLED;
1314

1415
import java.util.Collections;
@@ -57,7 +58,6 @@
5758
import org.opensearch.remote.metadata.client.SdkClient;
5859
import org.opensearch.remote.metadata.common.SdkClientUtils;
5960
import org.opensearch.search.builder.SearchSourceBuilder;
60-
import org.opensearch.security.spi.resources.client.ResourceSharingClient;
6161
import org.opensearch.transport.client.Client;
6262

6363
import com.google.common.collect.ImmutableList;
@@ -98,8 +98,8 @@ public void validateModelGroupAccess(User user, String modelGroupId, String acti
9898
listener.onResponse(true);
9999
return;
100100
}
101-
if (ResourceSharingClientAccessor.getInstance().getResourceSharingClient() != null) {
102-
ResourceSharingClient resourceSharingClient = ResourceSharingClientAccessor.getInstance().getResourceSharingClient();
101+
if (shouldUseResourceAuthz(ML_MODEL_GROUP_RESOURCE_TYPE)) {
102+
var resourceSharingClient = ResourceSharingClientAccessor.getInstance().getResourceSharingClient();
103103
resourceSharingClient.verifyAccess(modelGroupId, ML_MODEL_GROUP_INDEX, action, ActionListener.wrap(isAuthorized -> {
104104
if (!isAuthorized) {
105105
listener
@@ -173,8 +173,8 @@ public void validateModelGroupAccess(
173173
listener.onResponse(true);
174174
return;
175175
}
176-
if (ResourceSharingClientAccessor.getInstance().getResourceSharingClient() != null) {
177-
ResourceSharingClient resourceSharingClient = ResourceSharingClientAccessor.getInstance().getResourceSharingClient();
176+
if (shouldUseResourceAuthz(ML_MODEL_GROUP_RESOURCE_TYPE)) {
177+
var resourceSharingClient = ResourceSharingClientAccessor.getInstance().getResourceSharingClient();
178178
resourceSharingClient.verifyAccess(modelGroupId, ML_MODEL_GROUP_INDEX, action, ActionListener.wrap(isAuthorized -> {
179179
if (!isAuthorized) {
180180
listener
@@ -288,6 +288,16 @@ public void checkModelGroupPermission(MLModelGroup mlModelGroup, User user, Acti
288288
}
289289
}
290290

291+
/**
292+
* Checks whether to utilize new ResourceAuthz
293+
* @param resourceType for which to decide whether to use resource authz
294+
* @return true if the resource-sharing feature is enabled, false otherwise.
295+
*/
296+
public static boolean shouldUseResourceAuthz(String resourceType) {
297+
var client = ResourceSharingClientAccessor.getInstance().getResourceSharingClient();
298+
return client != null && client.isFeatureEnabledForType(resourceType);
299+
}
300+
291301
public boolean skipModelAccessControl(User user) {
292302
// Case 1: user == null when 1. Security is disabled. 2. When user is super-admin
293303
// Case 2: If Security is enabled and filter is disabled, proceed with search as

0 commit comments

Comments
 (0)