Add manage_search_synonyms privilege by carlosdelest · Pull Request #97853 · elastic/elasticsearch

carlosdelest · 2023-07-20T16:51:25Z

Adds a manage_search_synonyms privilege for synonyms related actions.

carlosdelest · 2023-07-20T16:52:54Z

+      reason: Introduced in 8.10.0
+
+  - do:
+      security.put_user:


I wasn't able to use a roles.yml file / user setting for build.gradle. I'm sure I'm missing something, in the meanwhile adding users and roles via security API worked.

If you can help with another approach, that would be much appreciated 🙏

Using APIs is fine. But it would be great if we pari the setup with a teardown part. Please see this file as an example.

Done, thanks for the pointer!

carlosdelest · 2023-07-20T16:54:57Z

I created these tests in the core-rest-tests-with-security, as I haven't seen security related tests in the main rest-api-spec directory. Also, synonyms are part of the server module which has no security turned on by default on tests (as security is part of xpack).

Happy to move to a different directory structure if it makes more sense!

I think it's fine to add it here. Again, if feature flag is still in play, we need to exclude this test for release builds. See this for an example.

Included in the blacklist, thanks @ywangd !

elasticsearchmachine · 2023-07-20T18:49:37Z

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

elasticsearchmachine · 2023-07-20T18:49:37Z

Pinging @elastic/es-search (Team:Search)

kderusso

Overall LGTM with one question

kderusso · 2023-07-20T18:49:52Z

    private static final Set<String> READ_SLM_PATTERN = Set.of(GetSnapshotLifecycleAction.NAME, GetStatusAction.NAME);

    private static final Set<String> MANAGE_SEARCH_APPLICATION_PATTERN = Set.of("cluster:admin/xpack/application/search_application/*");
+    private static final Set<String> MANAGE_SEARCH_SYNONYMS_PATTERN = Set.of("cluster:admin/synonyms/*", "cluster:admin/synonyms_sets/*");


We want to make sure that we have search in the pattern for future "manage search" privileges.

Also, does this synonyms need two different permission patterns? I would think that if you have the permission to manage synonym sets, you have the permission to manage synonym rules, and vice versa. Would it make sense to simplify this to just something like cluster:admin/search/synonyms?

Do we need to add admin/synonym_rules/*?

We want to make sure that we have search in the pattern for future "manage search" privileges.

Is that needed? I think we can use something like CROSS_CLUSTER_REPLICATION where we use a set of privileges, and we don't need explicit naming prefix...

I think it's better to omit the search prefix as that it's not a pattern I see in other constants, and we don't need to have it in order to create a manage_search privilege. Not a strong opinion though, happy to change it.

Also, does this synonyms need two different permission patterns? I would think that if you have the permission to manage synonym sets, you have the permission to manage synonym rules, and vice versa. Would it make sense to simplify this to just something like cluster:admin/search/synonyms?

We have three different action types, regarding synonyms, synonyms sets and synonym rules. As managing synonyms grant you access to both, that's the meaning of using the set of actions here.

Do we need to add admin/synonym_rules/*?
🤦 yes, thanks!

@kderusso , @mayya-sharipova , anything else that you want addressed here? Hopefully my responses above and the changes to add admin/synonym_rules/* did address your concerns. Please LMK!

I'm OK with this. Thank you!

ywangd

Left a few comments. Thanks!

ywangd · 2023-07-25T00:54:41Z

            MANAGE_LOGSTASH_PIPELINES,
            CANCEL_TASK,
            MANAGE_SEARCH_APPLICATION,
+            MANAGE_SEARCH_SYNONYMS,


Is the synonyms feature still behind a feature flag? If so, we need to gate the new privilege with the feature flag (see 3 lines below for an example).

Updated, thanks for the catch and the example!

ywangd · 2023-07-25T00:57:56Z

+      reason: Introduced in 8.10.0
+
+  - do:
+      security.put_user:


Using APIs is fine. But it would be great if we pari the setup with a teardown part. Please see this file as an example.

ywangd · 2023-07-25T01:05:21Z

I think it's fine to add it here. Again, if feature flag is still in play, we need to exclude this test for release builds. See this for an example.

ywangd · 2023-07-25T01:07:44Z

+"Create synonyms set - with manage_search_synonyms privilege":
+  - do:
+      headers: { Authorization: "Basic c3lub255bXMtdXNlcjpzeW5vbnltcy11c2VyLXBhc3N3b3Jk" }  # synonyms-user
+      synonyms.put:
+        synonyms_set: test-synonyms
+        body:
+          synonyms_set:
+            - synonyms: "hello, hi"
+              id: "test-id"
+
+  - match: { result: "created" }


I think we should have more tests for the synonyms-user to show it can excercise most (if not all) actions similar to how non-synonyms-user is tested?

Makes sense. I've split into two test files, one for testing unprivileged access and another for privileged access. LMKWYT!

kderusso

Nice work!

kderusso · 2023-07-25T12:50:30Z

@@ -0,0 +1,111 @@
+setup:


Nice test organization 👍

ywangd

LGTM

ywangd · 2023-07-26T00:45:38Z

+  - do:
+      security.put_user:
+        username: "non-synonyms-user"
+        body:
+          password: "non-synonyms-user-password"
+          roles : [ "non-synonyms-role" ]
+
+  - do:
+      security.put_role:
+        name: "non-synonyms-role"
+        body:
+          indices:
+            - names: ["*"]
+              privileges: [ "manage", "write", "read" ]


Nit: I think these user non-synonyms user and role can be removed since we now split the tests into 2 files? The same goes for the teardown part.

🤦 Removing them, thanks for the catch!

carlosdelest · 2023-07-26T07:52:23Z

@elasticsearchmachine update branch

carlosdelest added 2 commits July 20, 2023 18:49

Add new privilege

22d10cd

Add tests

1205e84

carlosdelest added >non-issue Team:Enterprise Search Meta label for Enterprise Search team v8.10.0 labels Jul 20, 2023

carlosdelest commented Jul 20, 2023

View reviewed changes

Add new cluster privilege to test

f9fac1a

carlosdelest marked this pull request as ready for review July 20, 2023 18:39

carlosdelest requested review from a team and mayya-sharipova July 20, 2023 18:39

elasticsearchmachine added needs:triage Requires assignment of a team area label and removed Team:Enterprise Search Meta label for Enterprise Search team labels Jul 20, 2023

mayya-sharipova added :Security/Security Security issues without another label :Search Relevance/Analysis How text is split into tokens and removed needs:triage Requires assignment of a team area label labels Jul 20, 2023

elasticsearchmachine added Team:Search Meta label for search team Team:Security Meta label for security team labels Jul 20, 2023

kderusso reviewed Jul 20, 2023

View reviewed changes

Add synonym sets actions to cluster privilege

e713643

ioanatia requested a review from ywangd July 24, 2023 08:19

ywangd reviewed Jul 25, 2023

View reviewed changes

PR review comments

6af2d3e

carlosdelest requested a review from ywangd July 25, 2023 09:03

Spotless

14bef42

kderusso approved these changes Jul 25, 2023

View reviewed changes

kderusso mentioned this pull request Jul 25, 2023

Add manage search query rules permission #97941

Merged

ywangd approved these changes Jul 26, 2023

View reviewed changes

Remove unused test user and role

68a5a61

Merge branch 'main' into carlosdelest/synonyms-manage-privilege

c6d99cf

carlosdelest merged commit 103f332 into elastic:main Jul 26, 2023

carlosdelest mentioned this pull request Jul 26, 2023

Remove synonyms feature flag and related classes #97962

Merged

ioanatia mentioned this pull request Aug 2, 2023

[Fleet] Make .fleet-secrets index internal #97728

Merged

Conversation

carlosdelest commented Jul 20, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Jul 20, 2023

Uh oh!

elasticsearchmachine commented Jul 20, 2023

Uh oh!

kderusso left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywangd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kderusso left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywangd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlosdelest commented Jul 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants