Skip to content

Commit 4f8053c

Browse files
Make migrate api require default access level to be supplied and updates documentations + tests
Signed-off-by: Darshit Chanpura <[email protected]>
1 parent 7d5b506 commit 4f8053c

File tree

5 files changed

+109
-63
lines changed

5 files changed

+109
-63
lines changed

RESOURCE_SHARING_AND_ACCESS_CONTROL.md

Lines changed: 43 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,7 @@ opensearchplugin {
7575
```
7676
- **Implement** the `ResourceSharingExtension` interface. For guidance, refer [SPI README.md](./spi/README.md#4-implement-the-resourcesharingextension-interface).
7777
- **Implement** the `ResourceSharingClientAccessor` wrapper class to access ResourceSharingClient. Refer [SPI README.md](./spi/README.md#5-implement-the-resourcesharingclientaccessor-class).
78+
- If plugin implements search, add a **plugin client** if not already present. Can be copied from sample-plugin's [PluginClient.java](./sample-resource-plugin/src/main/java/org/opensearch/sample/utils/PluginClient.java).
7879
- **Ensure** that each resource index only contains 1 type of resource.
7980
- **Register itself** in `META-INF/services` by creating the following file:
8081
```
@@ -129,10 +130,28 @@ resource_types:
129130
- "cluster:admin/sample-resource-plugin/*"
130131
- "cluster:admin/security/resource/share"
131132
```
133+
- If your plugin enabled testing with security, add the following to you node-setup for `integTest` task:
134+
```build.gradle
135+
integTest {
136+
...
137+
systemProperty "resource_sharing.enabled", System.getProperty("resource_sharing.enabled")
138+
...
139+
}
140+
...
141+
<test-cluster-setup-task> {
142+
...
143+
node.setting("plugins.security.system_indices.enabled", "true")
144+
if (System.getProperty("resource_sharing.enabled") == "true") {
145+
node.setting("plugins.security.experimental.resource_sharing.enabled", "true")
146+
node.setting("plugins.security.experimental.resource_sharing.protected_types", "[\"anomaly-detector\", \"forecaster\"]")
147+
}
148+
...
149+
}
150+
```
132151

133152
## **3. Resource Sharing API Design**
134153

135-
### **Resource Sharing Index **
154+
### **Resource Sharing Index**
136155

137156
Each plugin receives its own sharing index, centrally managed by security plugin, which stores **resource access metadata**, mapping **resources to their access control policies**.
138157

@@ -323,44 +342,21 @@ void revoke(String resourceId, String resourceIndex, ShareWith target, ActionLis
323342
void getAccessibleResourceIds(String resourceIndex, ActionListener<Set<String>> listener);
324343
```
325344

326-
Example usage:
327-
```java
328-
@Inject
329-
public ShareResourceTransportAction(
330-
TransportService transportService,
331-
ActionFilters actionFilters,
332-
SampleResourceExtension sampleResourceExtension
333-
) {
334-
super(ShareResourceAction.NAME, transportService, actionFilters, ShareResourceRequest::new);
335-
this.resourceSharingClient = sampleResourceExtension == null ? null : sampleResourceExtension.getResourceSharingClient();
336-
}
345+
### **5. `isFeatureEnabledForType`**
337346

338-
@Override
339-
protected void doExecute(Task task, ShareResourceRequest request, ActionListener<ShareResourceResponse> listener) {
340-
if (request.getResourceId() == null || request.getResourceId().isEmpty()) {
341-
listener.onFailure(new IllegalArgumentException("Resource ID cannot be null or empty"));
342-
return;
343-
}
347+
**Add as code-control to execute resource-sharing code only if the feature is enabled for the given type.**
344348

345-
if (resourceSharingClient == null) {
346-
listener.onFailure(
347-
new OpenSearchStatusException(
348-
"Resource sharing is not enabled. Cannot share resource " + request.getResourceId(),
349-
RestStatus.NOT_IMPLEMENTED
350-
)
351-
);
352-
return;
353-
}
354-
ShareWith shareWith = request.getShareWith();
355-
resourceSharingClient.share(request.getResourceId(), RESOURCE_INDEX_NAME, shareWith, ActionListener.wrap(sharing -> {
356-
ShareWith finalShareWith = sharing == null ? null : sharing.getShareWith();
357-
ShareResourceResponse response = new ShareResourceResponse(finalShareWith);
358-
log.debug("Shared resource: {}", response.toString());
359-
listener.onResponse(response);
360-
}, listener::onFailure));
361-
}
349+
```
350+
boolean isFeatureEnabledForType(String resourceType);
362351
```
363352

353+
Example usage `isFeatureEnabledForType()`:
354+
```java
355+
public static boolean shouldUseResourceAuthz(String resourceType) {
356+
var client = ResourceSharingClientAccessor.getInstance().getResourceSharingClient();
357+
return client != null && client.isFeatureEnabledForType(resourceType);
358+
}
359+
```
364360

365361
> For more details, refer [spi/README.md](./spi/README.md#available-java-apis)
366362
@@ -396,15 +392,18 @@ sequenceDiagram
396392
Plugin ->> SPI: getAccessibleResourceIds (noop)
397393
SPI -->> Plugin: Error 501 Not Implemented
398394
395+
Plugin ->> SPI: isFeatureEnabledForType (noop)
396+
SPI -->> Plugin: Disabled
397+
399398
else Security Plugin Enabled
400399
%% Step 3: Plugin calls Java APIs declared by ResourceSharingClient
401-
Plugin ->> SPI: Calls Java API (`verifyAccess`, `share`, `revoke`, `getAccessibleResourceIds`)
400+
Plugin ->> SPI: Calls Java API (`verifyAccess`, `share`, `revoke`, `getAccessibleResourceIds`, `isFeatureEnabledForType`)
402401
403402
%% Step 4: Request is sent to Security Plugin
404403
SPI ->> Security: Sends request to Security Plugin for processing
405404
406405
%% Step 5: Security Plugin handles request and returns response
407-
Security -->> SPI: Response (Access Granted or Denied / Resource Shared or Revoked / List Resource IDs )
406+
Security -->> SPI: Response (Access Granted or Denied / Resource Shared or Revoked / List Resource IDs / Feature Enabled or Disabled for Resource Type)
408407
409408
%% Step 6: Security SPI sends response back to Plugin
410409
SPI -->> Plugin: Passes processed response back to Plugin
@@ -628,19 +627,19 @@ Read documents from a plugin’s index and migrate ownership and backend role-ba
628627

629628
**Request Body**
630629

631-
| Parameter | Type | Required | Description |
632-
|------------------------|---------|----|-----------------------------------------------------------------------------|
633-
| `source_index` | string | yes | Name of the plugin index containing the existing resource documents |
634-
| `username_path` | string | yes | JSON Pointer to the username field inside each document (e.g., `/owner`) |
635-
| `backend_roles_path` | string | yes | JSON Pointer to the backend_roles field (must point to a JSON array) |
636-
| `default_access_level` | string | no | Default access level to assign migrated backend_roles (default: `"default"`) |
630+
| Parameter | Type | Required | Description |
631+
|------------------------|---------|----------|-------------------------------------------------------------------------|
632+
| `source_index` | string | yes | Name of the plugin index containing the existing resource documents |
633+
| `username_path` | string | yes | JSON Pointer to the username field inside each document |
634+
| `backend_roles_path` | string | yes | JSON Pointer to the backend_roles field (must point to a JSON array) |
635+
| `default_access_level` | string | yes | Default access level to assign migrated backend_roles |
637636

638637
**Example Request**
639638
`POST /_plugins/_security/api/resources/migrate`
640639
**Request Body:**
641640
```json
642641
{
643-
"source_index": "sample_plugin_index",
642+
"source_index": ".sample_resource",
644643
"username_path": "/owner",
645644
"backend_roles_path": "/access/backend_roles",
646645
"default_access_level": "read_only"

sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/TestUtils.java

Lines changed: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -148,46 +148,60 @@ public static String migrationPayload_valid() {
148148
"source_index": "%s",
149149
"username_path": "%s",
150150
"backend_roles_path": "%s"
151+
"default_access_level": "%s"
151152
}
152-
""".formatted(RESOURCE_INDEX_NAME, "user/name", "user/backend_roles");
153+
""".formatted(RESOURCE_INDEX_NAME, "user/name", "user/backend_roles", "sample_read_only");
153154
}
154155

155-
public static String migrationPayload_valid_withSpecifiedAccessLevel() {
156+
public static String migrationPayload_valid_withSpecifiedAccessLevel(String accessLevel) {
156157
return """
157158
{
158159
"source_index": "%s",
159160
"username_path": "%s",
160161
"backend_roles_path": "%s",
161162
"default_access_level": "%s"
162163
}
163-
""".formatted(RESOURCE_INDEX_NAME, "user/name", "user/backend_roles", "read_only");
164+
""".formatted(RESOURCE_INDEX_NAME, "user/name", "user/backend_roles", accessLevel);
164165
}
165166

166167
public static String migrationPayload_missingSourceIndex() {
167168
return """
168169
{
169170
"username_path": "%s",
170-
"backend_roles_path": "%s"
171+
"backend_roles_path": "%s",
172+
"default_access_level": "%s"
171173
}
172-
""".formatted("user/name", "user/backend_roles");
174+
""".formatted("user/name", "user/backend_roles", "sample_read_only");
173175
}
174176

175177
public static String migrationPayload_missingUserName() {
176178
return """
177179
{
178180
"source_index": "%s",
179-
"backend_roles_path": "%s"
181+
"backend_roles_path": "%s",
182+
"default_access_level": "%s"
180183
}
181-
""".formatted(RESOURCE_INDEX_NAME, "user/backend_roles");
184+
""".formatted(RESOURCE_INDEX_NAME, "user/backend_roles", "sample_read_only");
182185
}
183186

184187
public static String migrationPayload_missingBackendRoles() {
185188
return """
186189
{
187190
"source_index": "%s",
188-
"username_path": "%s"
191+
"username_path": "%s",
192+
"default_access_level": "%s"
189193
}
190-
""".formatted(RESOURCE_INDEX_NAME, "user/name");
194+
""".formatted(RESOURCE_INDEX_NAME, "user/name", "sample_read_only");
195+
}
196+
197+
public static String migrationPayload_missingDefaultAccessLevel() {
198+
return """
199+
{
200+
"source_index": "%s",
201+
"username_path": "%s",
202+
"backend_roles_path": "%s"
203+
}
204+
""".formatted(RESOURCE_INDEX_NAME, "user/name", "user/backend_roles");
191205
}
192206

193207
public static String putSharingInfoPayload(

sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/MigrateApiTests.java

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@
4141
import static org.opensearch.sample.resource.TestUtils.SAMPLE_RESOURCE_CREATE_ENDPOINT;
4242
import static org.opensearch.sample.resource.TestUtils.SAMPLE_RESOURCE_GET_ENDPOINT;
4343
import static org.opensearch.sample.resource.TestUtils.migrationPayload_missingBackendRoles;
44+
import static org.opensearch.sample.resource.TestUtils.migrationPayload_missingDefaultAccessLevel;
4445
import static org.opensearch.sample.resource.TestUtils.migrationPayload_missingSourceIndex;
4546
import static org.opensearch.sample.resource.TestUtils.migrationPayload_missingUserName;
4647
import static org.opensearch.sample.resource.TestUtils.migrationPayload_valid;
@@ -125,9 +126,9 @@ public void testMigrateAPIWithRestAdmin_valid() {
125126
TestRestClient.HttpResponse sharingResponse = client.get(RESOURCE_SHARING_INDEX + "/_search");
126127
sharingResponse.assertStatusCode(HttpStatus.SC_OK);
127128
assertThat(sharingResponse.bodyAsJsonNode().get("hits").get("hits").size(), equalTo(1)); // 1 of 2 entries was skipped
128-
assertThat(sharingResponse.bodyAsJsonNode().get("hits").get("hits"), equalTo(expectedHits(resourceId, "default"))); // with
129-
// default
130-
// access-level
129+
assertThat(sharingResponse.bodyAsJsonNode().get("hits").get("hits"), equalTo(expectedHits(resourceId, "sample_read_only"))); // with
130+
// default
131+
// access-level
131132
}
132133
}
133134

@@ -140,7 +141,7 @@ public void testMigrateAPIWithRestAdmin_valid_withSpecifiedAccessLevel() {
140141
try (TestRestClient client = cluster.getRestClient(cluster.getAdminCertificate())) {
141142
TestRestClient.HttpResponse migrateResponse = client.postJson(
142143
RESOURCE_SHARING_MIGRATION_ENDPOINT,
143-
migrationPayload_valid_withSpecifiedAccessLevel()
144+
migrationPayload_valid_withSpecifiedAccessLevel("sample_read_write")
144145
);
145146
migrateResponse.assertStatusCode(HttpStatus.SC_OK);
146147
assertThat(migrateResponse.bodyAsMap().get("summary"), equalTo("Migration complete. migrated 1; skippedNoUser 1; failed 0"));
@@ -149,9 +150,9 @@ public void testMigrateAPIWithRestAdmin_valid_withSpecifiedAccessLevel() {
149150
TestRestClient.HttpResponse sharingResponse = client.get(RESOURCE_SHARING_INDEX + "/_search");
150151
sharingResponse.assertStatusCode(HttpStatus.SC_OK);
151152
assertThat(sharingResponse.bodyAsJsonNode().get("hits").get("hits").size(), equalTo(1)); // 1 of 2 entries was skipped
152-
assertThat(sharingResponse.bodyAsJsonNode().get("hits").get("hits"), equalTo(expectedHits(resourceId, "read_only"))); // with
153-
// custom
154-
// access-level
153+
assertThat(sharingResponse.bodyAsJsonNode().get("hits").get("hits"), equalTo(expectedHits(resourceId, "sample_read_write"))); // with
154+
// custom
155+
// access-level
155156
}
156157
}
157158

@@ -194,6 +195,19 @@ public void testMigrateAPIWithRestAdmin_noSourceIndex() {
194195
}
195196
}
196197

198+
@Test
199+
public void testMigrateAPIWithRestAdmin_noDefaultAccessLevel() {
200+
createSampleResource();
201+
202+
try (TestRestClient client = cluster.getRestClient(cluster.getAdminCertificate())) {
203+
TestRestClient.HttpResponse migrateResponse = client.postJson(
204+
RESOURCE_SHARING_MIGRATION_ENDPOINT,
205+
migrationPayload_missingDefaultAccessLevel()
206+
);
207+
assertThat(migrateResponse, RestMatchers.isBadRequest("/missing_mandatory_keys/keys", "default_access_level"));
208+
}
209+
}
210+
197211
private String createSampleResource() {
198212
try (TestRestClient client = cluster.getRestClient(MIGRATION_USER)) {
199213
String sampleResource = """

spi/README.md

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -349,6 +349,25 @@ resourceSharingClient.getAccessibleResourceIds(RESOURCE_INDEX_NAME, ActionListen
349349
```
350350
> **Use Case:** Helps a user identify **which shareableResources they can interact with**.
351351

352+
---
353+
354+
#### **5. `isFeatureEnabledForType`**
355+
**Add as code-control to execute resource-sharing code only if the feature is enabled for the given type.**
356+
357+
##### **Method Signature:**
358+
```java
359+
void isFeatureEnabledForType(String resourceIndex, ActionListener<Set<String>> listener);
360+
```
361+
362+
##### **Example Usage:**
363+
```java
364+
public static boolean shouldUseResourceAuthz(String resourceType) {
365+
var client = ResourceSharingClientAccessor.getInstance().getResourceSharingClient();
366+
return client != null && client.isFeatureEnabledForType(resourceType);
367+
}
368+
```
369+
> **Use Case:** Helps control code-path for cases where global feature flag is enabled but given resource-type is (not) marked as protected**.
370+
352371
##### **Sample Request Flow:**
353372

354373
```mermaid

src/main/java/org/opensearch/security/resources/migrate/MigrateResourceSharingInfoApiAction.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@
7676
* source_index: "abc", // name of plugin index
7777
* username_path: "/path/to/username/node", // path to user-name in resource document in the plugin index
7878
* backend_roles_path: "/path/to/user_backend-roles/node" // path to backend-roles in resource document in the plugin index
79-
* default_access_level: "<some-default-access-level>" // default value that should replace the otherwise ResourceAccessLevels.PLACE_HOLDER assigned to the new ResourceSharing object
79+
* default_access_level: "<some-default-access-level>" // default access-level at which sharing records should be created
8080
* }
8181
* - Response:
8282
* 200 OK Migration Complete. Migrate X, skippedNoUser Y, failed Z // migrate -> successful migration count, skippedNoUser -> records with no creator info, failed -> records that failed to migrate
@@ -140,7 +140,7 @@ private ValidationResult<Triple<String, String, List<SourceDoc>>> loadCurrentSha
140140
String sourceIndex = body.get("source_index").asText();
141141
String userNamePath = body.get("username_path").asText();
142142
String backendRolesPath = body.get("backend_roles_path").asText();
143-
String defaultAccessLevel = body.has("default_access_level") ? body.get("default_access_level").asText() : "default";
143+
String defaultAccessLevel = body.get("default_access_level").asText();
144144

145145
List<SourceDoc> results = new ArrayList<>();
146146

@@ -307,7 +307,7 @@ public Settings settings() {
307307

308308
@Override
309309
public Set<String> mandatoryKeys() {
310-
return ImmutableSet.of("source_index", "username_path", "backend_roles_path");
310+
return ImmutableSet.of("source_index", "username_path", "backend_roles_path", "default_access_level");
311311
}
312312

313313
@Override

0 commit comments

Comments
 (0)