Update api keys to include acl filter index #159840

seanstory · 2023-06-15T19:39:31Z

Summary

Part of https://github.com/elastic/enterprise-search-team/issues/4304

Ensures that new API keys generated from our application include permissions not only to the document index but also to the .search-acl-filter-* index that corresponds to it.

For maintainers

This was checked for breaking API changes and was labeled appropriately

ioanatia · 2023-06-16T08:04:22Z

x-pack/plugins/enterprise_search/server/lib/indices/create_api_key.ts

@@ -17,14 +17,22 @@ export const createApiKey = async (
  indexName: string,
  keyName: string
 ) => {
+  let aclIndexName: string;
+  if (indexName.startsWith('search-')) {
+    const suffix = indexName.replace('search-', '');


why do we have to replace the prefix?

I'm wondering the same thing. Seems to me that this could create conflicting index names.

The index name isn't getting modified, we're just figuring out the suffix.

If the user uses our UI to create an index:

they type test

the index name is search-test

the permissions index name is .search-acl-filter-test

I don't want the permissions index name to be .search-acl-filter-search-test, so I have to strip off the search- from the index name to determine the suffix.

Right, my question is really: why don't we want it to be .search-acl-filter-search-test?

because then you have search in the prefix twice and it's redundant

Not really? The second search isn't part of the prefix, it's the actual index name that just happens to start with search. What happens when people want to use their own indices that don't start with search (which we don't really support, but people can easily do)? We may accidentally create conflicting ACL index names (eg: search-test and test would have the same ACL name).

++ @sphilipse - I am not too worried about the redundancy, more about consistency in the future where we don't require the search- prefix.

🤦 totally missed that I had replies on here when I asked again for an approve and then merged. Sorry both, did not mean to ignore this concern.

I do think that we're in an OK place right now. If we get rid of using search-*, we could switch to using .acl-filter as our prefix anyway.

sphilipse · 2023-06-19T08:52:08Z

x-pack/plugins/enterprise_search/server/lib/indices/create_api_key.ts

@@ -17,14 +17,22 @@ export const createApiKey = async (
  indexName: string,
  keyName: string
 ) => {
+  let aclIndexName: string;
+  if (indexName.startsWith('search-')) {
+    const suffix = indexName.replace('search-', '');


I'm wondering the same thing. Seems to me that this could create conflicting index names.

sphilipse · 2023-06-19T08:54:14Z

x-pack/plugins/enterprise_search/server/lib/indices/create_api_key.ts

@@ -17,14 +17,22 @@ export const createApiKey = async (
  indexName: string,
  keyName: string
 ) => {
+  let aclIndexName: string;


nitpick: more idiomatic for our codebase would be

const aclIndexName = indexName.startsWith('search-') ? `.search-acl-filter-${indexName.slice(7)}` : `.search-acl-filter-${indexName}`;

I can make that change. I hadn't considered using a substring method so I could inline the suffix extraction with the prefix concatenation.

If I had to have a block of logic, what's the idiomatic way to do this to avoid the let? (I assume that's what you're trying to avoid?)

maybe the closest thing you can get to a block/scope that returns a value for assignment is an IIFE.

with this code it would look something like this:

const aclIndexName: string = (() => { if (indexName.startsWith('search-')) { const suffix = indexName.replace('search-', ''); return `.search-acl-filter-${suffix}`; } else { return `.search-acl-filter-${indexName}`; } })()

An IIFE is possible but isn't really idiomatic, we're more likely to extract it to a function elsewhere in the file and call it here (or to a shared utility function that's used in multiple places, which might be useful here).

seanstory · 2023-06-20T13:55:37Z

@elasticmachine merge upstream

sloanelybutsurely · 2023-06-20T14:25:54Z

x-pack/plugins/enterprise_search/server/lib/indices/create_api_key.ts

+  const aclIndexName = indexName.startsWith('search-')
+    ? `.search-acl-filter-${indexName.slice(7)}`
+    : `.search-acl-filter-${indexName}`;


okay i know this code has been rewritten for you already but i got a bit nerdsniped and thought "could we do this with a regular expression?

Suggested change

const aclIndexName = indexName.startsWith('search-')

? `.search-acl-filter-${indexName.slice(7)}`

: `.search-acl-filter-${indexName}`;

const aclIndexName = indexName.replace(/^(?:search-)?(.*)$/, '.search-acl-filter-$1')

this regexp matches a string with an optional search- prefix and adds it to the end of the desired .search-acl-filter- without any search- prefix.

again, this is so minor, please feel free to disregard.

TIL "Nerd Sniping" 😂
I actually like this better, because the slice(7) didn't seem explicitly clear, but this regex isn't too hard to follow.

…-filter-index

kibana-ci · 2023-06-20T19:19:10Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 80f8620

Failed CI Steps

FTR Configs #39

Test Failures

[job] [logs] FTR Configs #39 / endpoint For each artifact list under management "after all" hook in "For each artifact list under management"
[job] [logs] FTR Configs #39 / endpoint For each artifact list under management "before all" hook in "For each artifact list under management"

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	13	15	+2
`securitySolution`	411	415	+4
total			+6

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	14	16	+2
`securitySolution`	494	498	+4
total			+6

History

💚 Build #136649 succeeded 44fefbc
💚 Build #136547 succeeded c0072a6
💚 Build #135779 succeeded a0cb7ba

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…unction (#160457) ## Summary I'd added this logic in #159840 and turns out that was the wrong place to influence connector api keys. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

…unction (elastic#160457) ## Summary I'd added this logic in elastic#159840 and turns out that was the wrong place to influence connector api keys. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) (cherry picked from commit 8251481)

…tion function (#160457) (#160465) # Backport This will backport the following commits from `main` to `8.9`: - [move .search-acl-filter-* permissions to the right api key creation function (#160457)](#160457)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Sean Story <[email protected]>

seanstory added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Team:EnterpriseSearch v8.9.0 labels Jun 15, 2023

seanstory requested a review from a team June 15, 2023 19:39

ioanatia reviewed Jun 16, 2023

View reviewed changes

sphilipse reviewed Jun 19, 2023

View reviewed changes

sloanelybutsurely reviewed Jun 20, 2023

View reviewed changes

seanstory added 4 commits June 20, 2023 11:08

Update api keys to include acl filter index

1de3efc

Refactor conditional block to ternary operator

a6c8344

Add a test case for search- prefixed indices

b7545b0

Refactor to use @sloanelybutsurel's regex

44fefbc

seanstory force-pushed the seanstory/4304-update-api-key-to-include-acl-filter-index branch from c0072a6 to 44fefbc Compare June 20, 2023 16:09

Merge branch 'main' into seanstory/4304-update-api-key-to-include-acl…

80f8620

…-filter-index

sphilipse approved these changes Jun 21, 2023

View reviewed changes

seanstory merged commit 9382687 into main Jun 21, 2023

seanstory deleted the seanstory/4304-update-api-key-to-include-acl-filter-index branch June 21, 2023 14:18

seanstory mentioned this pull request Jun 23, 2023

move .search-acl-filter-* permissions to the right api key creation function #160457

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update api keys to include acl filter index #159840

Update api keys to include acl filter index #159840

seanstory commented Jun 15, 2023

ioanatia Jun 16, 2023

sphilipse Jun 19, 2023

seanstory Jun 20, 2023

sphilipse Jun 20, 2023

seanstory Jun 20, 2023

sphilipse Jun 21, 2023

ioanatia Jun 21, 2023

seanstory Jun 21, 2023

sphilipse Jun 19, 2023

sphilipse Jun 19, 2023

seanstory Jun 20, 2023

sloanelybutsurely Jun 20, 2023

sphilipse Jun 20, 2023

seanstory commented Jun 20, 2023

sloanelybutsurely Jun 20, 2023

seanstory Jun 20, 2023

kibana-ci commented Jun 20, 2023

ESLint disabled line counts

Total ESLint disabled count

Update api keys to include acl filter index #159840

Update api keys to include acl filter index #159840

Conversation

seanstory commented Jun 15, 2023

Summary

For maintainers

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

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

seanstory commented Jun 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Jun 20, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

ESLint disabled line counts

Total ESLint disabled count

History