Add BulkPutRoles API #109339

jfreden · 2024-06-04T09:46:23Z

This PR adds a new POST _security/role API that can be used to bulk insert roles.

Example

Request

POST _security/role
{
    "roles": {
        "test": {"cluster": ["all"],"indices": [{"names": ["test_index1"],"privileges": ["all"]}],"applications": [],"run_as": []},
        "test2": {"cluster": ["all"],"indices": [{"names": ["test_index1"],"privileges": ["read"]}],"applications": [],"run_as": []},
        "test3": {"cluster": ["all"],"indices": [{"names": ["test_index1"],"privileges": ["all"]}],"applications": [],"run_as": []},
        "test4": {"cluster": ["all"],"indices": [{"names": ["test_index1"],"privileges": ["write"]}],"applications": [],"run_as": []},
        "test5": {"cluster": ["all"],"indices": [{"names": ["test_index1"],"privileges": ["all"]}],"applications": [],"run_as": []}
    }
}

Response

{
    "created": [
        "test",
        "test2",
        "test3",
        "test4",
        "test5"
    ]
}

Under the hood it uses a BulkRequest with a collection of UpdateRequest with doc_as_upsert to allow for noops against the Lucene index when no changes are detected in the payload.

This doesn't yet support serveless (set to internal) since a separate request builder need to be added to build put role requests that satisfy custom roles. This should ideally use the new manage_roles privilege when available and should no be enabled in serverless until then.

Ater Merge

Add new manage_roles privilege to allow us to use this in serverless and enhance security.
Create a separate serverless PR to handle validation for custom roles
Make this API available in serverless
Add new API spec to https://github.com/elastic/elasticsearch-specification

jfreden · 2024-06-12T11:53:27Z

...java/org/elasticsearch/xpack/core/security/action/role/BulkPutRoleRequestBuilderFactory.java

+            return new BulkPutRoleRequestBuilder(client);
+        }
+    }
+}


This is needed to allow us to inject another builder through SPI when running in serverless to enforce custom roles validation for the bulk api.

My suggestion is to make this PR simpler and not think about validation for serverless roles. Your call.

jfreden · 2024-06-12T11:54:30Z

...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/RoleDescriptor.java

+        }
+
+        public RoleDescriptor parse(String name, XContentParser parser, boolean validate) throws IOException {
+            if (validate) {


We want to postpone validation until we handle the rest of the request so we can merge actual results with validation errors and produce a single stream of results. The boolean here provides a way to skip validation that will be done later.

jfreden · 2024-06-12T11:56:20Z

...urity/src/main/java/org/elasticsearch/xpack/security/action/role/TransportPutRoleAction.java

-        }
-    }
-
-    private Exception validateRequest(final PutRoleRequest request) {


This is now handled in the roles store instead, so the logic can be shared between bulk and single inserts. The benefit of keeping it in the roles store is that we can then merge the failed validation items with successful inserts.

jfreden · 2024-06-12T11:57:20Z

.../src/test/java/org/elasticsearch/xpack/security/action/role/TransportPutRoleActionTests.java

@@ -92,109 +82,6 @@ protected NamedXContentRegistry xContentRegistry() {
        );
    }

-    public void testReservedRole() {


All these tests have been moved to NativeRolesStoreTests.

elasticsearchmachine · 2024-06-12T11:58:36Z

Pinging @elastic/es-security (Team:Security)

elasticsearchmachine · 2024-06-12T11:58:36Z

Hi @jfreden, I've created a changelog YAML for you.

jfreden · 2024-06-12T12:04:39Z

This is trying to look like the bulk index api as much as possible. Open to modifying things if some of the naming is confusing.

elasticsearchmachine · 2024-06-13T11:14:50Z

Pinging @elastic/es-docs (Team:Docs)

...java/org/elasticsearch/xpack/core/security/action/role/BulkPutRoleRequestBuilderFactory.java

shainaraskas

A couple of small notes from the docs side!

docs/reference/rest-api/security/bulk-create-roles.asciidoc

albertzaharovits

LGTM
Only minor nits that are optional to address.
I think we should discuss (in a GH issue, or the team meeting) what to do when (bulk) creating roles succeeds but cache clearing fails. Can you raise an issue to track this discussion, please?

albertzaharovits · 2024-07-02T10:52:49Z

.../src/main/java/org/elasticsearch/xpack/security/action/role/TransportBulkPutRolesAction.java

+        try {
+            rolesStore.putRoles(request.getRefreshPolicy(), request.getRoles(), listener);
+        } catch (IOException e) {
+            listener.onFailure(e);
+        }


nit: can you please make putRoles both throw an exception and take a listener parameter. It avoids code like this and should make the method itself clearer.

albertzaharovits · 2024-07-02T11:17:35Z

...in/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java

+        validationException = RoleDescriptorRequestValidator.validate(role, validationException);
+
+        if (reservedRoleNameChecker.isReserved(role.getName())) {
+            throw addValidationError("Role [" + role.getName() + "] is reserved and may not be used.", validationException);


nit: maybe not throw here, looks dissimilar to the prevailing convention around here to return the validation exception.

Yes, this is to match the existing behaviour from when this lived in the TransportPutRoleAction, but I agree this is not very nice. I'm a little worried to change existing behaviour of the PutRole so I rather keep it as is for now. WDYT?

Hmmm, OK, lets keep the existing behavior.

albertzaharovits · 2024-07-02T11:36:13Z

...in/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java

+                        Iterator<BulkItemResponse> bulkItemResponses = bulkResponse.iterator();
+                        BulkPutRolesResponse.Builder bulkPutRolesResponseBuilder = new BulkPutRolesResponse.Builder();
+
+                        roles.stream().map(RoleDescriptor::getName).map(roleName -> {


nit: can you please create a roleNames list outside the response handler here, to avoid keeping the list of RoleDescriptor around for the response for the names only? (also, I would assert that the list of role names contains unique elements).

albertzaharovits · 2024-07-02T12:01:58Z

...in/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java

+                        clearRoleCache(rolesToRefreshInCache.toArray(String[]::new), ActionListener.wrap(res -> {
+                            listener.onResponse(bulkPutRolesResponseBuilder.build());
+                        }, listener::onFailure), bulkResponse);


I think we should return the bulk response when the cache clear operation failed.
We can discuss separately, but please raise an issue to track it.

Ah yes. Hmm that's a little tricky. I'll add an item to our weekly + a jira ticket to make sure I don't forget.

jfreden · 2024-07-02T13:27:36Z

Thanks for the review @albertzaharovits !

elasticsearchmachine added the v8.15.0 label Jun 4, 2024

jfreden force-pushed the add_bulk_roles_api branch 2 times, most recently from f91d111 to cdb65b1 Compare June 10, 2024 10:55

jfreden added test-full-bwc Trigger full BWC version matrix tests test-update-serverless >enhancement labels Jun 11, 2024

jfreden force-pushed the add_bulk_roles_api branch from 622acb5 to bcb39f2 Compare June 12, 2024 11:50

Add BulkPutRoles API

d2c1751

jfreden force-pushed the add_bulk_roles_api branch from bcb39f2 to d2c1751 Compare June 12, 2024 11:51

jfreden commented Jun 12, 2024

View reviewed changes

jfreden requested a review from albertzaharovits June 12, 2024 11:58

jfreden marked this pull request as ready for review June 12, 2024 11:58

jfreden added the :Security/Security Security issues without another label label Jun 12, 2024

elasticsearchmachine added the Team:Security Meta label for security team label Jun 12, 2024

Update docs/changelog/109339.yaml

97853da

Fix doc tests

ea85ad1

albertzaharovits added the >docs General docs changes label Jun 13, 2024

elasticsearchmachine added the Team:Docs Meta label for docs team label Jun 13, 2024

jfreden and others added 4 commits June 13, 2024 13:14

Delete docs/changelog/109339.yaml

019cd25

Fix unit test

49ead79

Merge remote-tracking branch 'upstream/main' into add_bulk_roles_api

aadbfba

Change serverless scope to private

c402420

n1v0lg reviewed Jun 13, 2024

View reviewed changes

...java/org/elasticsearch/xpack/core/security/action/role/BulkPutRoleRequestBuilderFactory.java Outdated Show resolved Hide resolved

jfreden added 2 commits June 17, 2024 13:48

fixup! Path in docs

1b3adf2

Merge remote-tracking branch 'upstream/main' into add_bulk_roles_api

e989131

shainaraskas reviewed Jun 17, 2024

View reviewed changes

jfreden added 14 commits June 18, 2024 10:53

fixup! refactor after code review

a0ce4c1

fixup! docs

a6ba89f

Merge remote-tracking branch 'upstream/main' into add_bulk_roles_api

c5f07ff

fixup! operator action constants

bb3eb1c

fixup! Better test logging

2ed3475

Merge remote-tracking branch 'upstream/main' into add_bulk_roles_api

75bbafa

fixup! Use correct name

e4b8951

fixup! Prepare index before exec

d5c0a07

Merge remote-tracking branch 'upstream/main' into add_bulk_roles_api

4447b83

Merge remote-tracking branch 'upstream/main' into add_bulk_roles_api

6485dc8

Add mock data to enable prepare call in native role store

95180b3

Add api spec and fixups

066f7c8

Merge remote-tracking branch 'upstream/main' into add_bulk_roles_api

7cecd88

Merge remote-tracking branch 'upstream/main' into add_bulk_roles_api

7cc2b33

albertzaharovits self-requested a review July 2, 2024 06:43

jfreden added 3 commits July 2, 2024 11:32

Merge remote-tracking branch 'upstream/main' into add_bulk_roles_api

32987e0

fixup! new priv

d3a9c78

fixup! Add priv test

5205ed0

albertzaharovits approved these changes Jul 2, 2024

View reviewed changes

jfreden added 2 commits July 2, 2024 14:48

Change name to future proof for more tests

7d6636a

fixup! Code review comments

cc1bf50

jfreden merged commit 5547604 into elastic:main Jul 2, 2024
6 of 21 checks passed

elena-shostak mentioned this pull request Jul 2, 2024

Support bulk endpoint for creating/updating roles elastic/kibana#187427

Closed

This was referenced Jul 3, 2024

Add audit logging for bulk role APIs #110410

Merged

Add spec for bulk put roles elastic/elasticsearch-specification#2679

Closed

Add spec for bulk put roles elastic/elasticsearch-specification#2682

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add BulkPutRoles API #109339

Add BulkPutRoles API #109339

jfreden commented Jun 4, 2024 •

edited

Loading

jfreden Jun 12, 2024

albertzaharovits Jun 13, 2024

jfreden Jun 12, 2024 •

edited

Loading

jfreden Jun 12, 2024

jfreden Jun 12, 2024

elasticsearchmachine commented Jun 12, 2024

elasticsearchmachine commented Jun 12, 2024

jfreden commented Jun 12, 2024

elasticsearchmachine commented Jun 13, 2024

shainaraskas left a comment

albertzaharovits left a comment

albertzaharovits Jul 2, 2024

albertzaharovits Jul 2, 2024

jfreden Jul 2, 2024

albertzaharovits Jul 2, 2024

albertzaharovits Jul 2, 2024

albertzaharovits Jul 2, 2024

jfreden Jul 2, 2024

jfreden commented Jul 2, 2024

Add BulkPutRoles API #109339

Add BulkPutRoles API #109339

Conversation

jfreden commented Jun 4, 2024 • edited Loading

Example

Ater Merge

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfreden Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticsearchmachine commented Jun 12, 2024

elasticsearchmachine commented Jun 12, 2024

jfreden commented Jun 12, 2024

elasticsearchmachine commented Jun 13, 2024

shainaraskas left a comment

Choose a reason for hiding this comment

albertzaharovits left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfreden commented Jul 2, 2024

jfreden commented Jun 4, 2024 •

edited

Loading

jfreden Jun 12, 2024 •

edited

Loading