Skip to content

[ZEPPELIN-6414] Apply the authorization filter before the search cutoff - #5427

Open
dev-donghwan wants to merge 1 commit into
apache:masterfrom
dev-donghwan:ZEPPELIN-6414
Open

[ZEPPELIN-6414] Apply the authorization filter before the search cutoff#5427
dev-donghwan wants to merge 1 commit into
apache:masterfrom
dev-donghwan:ZEPPELIN-6414

Conversation

@dev-donghwan

Copy link
Copy Markdown
Contributor

What is this PR for?

/notebook/search asked the search service for results and then dropped the ones the caller may not read. Both search services cut their results down to twenty before returning them, so that removal ran on an already shortened list:

  • A caller with access to few notes is served fewer results than it is allowed to see. If the twenty highest scoring hits all belong to notes it cannot read, the search comes back empty while its own matching notes sit just below the cut.
  • In EmbeddingSearch the entries the caller cannot read are also counted into the table boost of Phase 1, so notes that never reach the caller still move the ranking of the ones that do.

The read check now travels with the query, as a predicate over the note id, and every implementation applies it before it cuts anything.

before after
EmbeddingSearch scores every entry, cuts to MAX_RESULTS, REST filters skips unreadable entries while scoring, so neither the table weights nor the cutoff see them
LuceneSearch searcher.search(query, 20), REST filters walks the hits in score order with searchAfter until a full page of readable hits is collected, or the hits run out
NotebookRestApi four permission calls per result, list mutated while iterating passes the predicate, no post-filtering

LuceneSearch reads only the id field to decide on a hit and loads the whole document for the hits it keeps, so hits that are dropped cost one stored field read and no highlighting.

query(String) is removed rather than kept next to the new method: leaving it would leave a way to search without saying who is asking, which is the defect this issue is about. SearchService is bound in ZeppelinServer to the three implementations in the repository and is not reachable as an extension point, so nothing outside the tree implements it.

Note that this changes what the result limit means: it is now the top twenty results the caller may read, rather than what is left of the top twenty overall after filtering.

What type of PR is it?

Bug Fix

What is the Jira issue?

How should this be tested?

New tests in both search services, keepsReadableResultsThatTheCutWouldHide, put twenty-eight matching notes in the index and let the caller read three of them. Each test first checks its own fixture: it queries with an allow-all predicate and asserts that the readable notes really do fall outside the cut, so that the test cannot quietly stop testing anything if the limit or the scoring changes later. It then queries with the real predicate and expects all three readable notes back and nothing else.

LuceneSearchTest.returnsNothingWhenTheCallerMayReadNothing covers the walk ending on its own when no hit is readable.

Run locally:

  • LuceneSearchTest - 12 tests, including the two new ones
  • EmbeddingSearchTest - 12 tests, including the new one, run with ZEPPELIN_EMBEDDING_TEST=true after bin/install-search-model.sh

The existing call sites in both test classes now pass id -> true explicitly.

Questions:

  • Does the license files need to update? No
  • Is there breaking changes for older versions? SearchService.query(String) is replaced by query(String, Predicate<String>). Only the three in-tree implementations and the REST resource use it.
  • Does this needs documentation? No

Possible follow-up

Indexing the note id as its own field would let LuceneSearch hand the permission filter to Lucene instead of walking the hits, but it needs an index schema change and a rebuild, so it is left out of this issue.

The REST layer asked the search service for results and then removed the ones the
caller may not read. Both search services cut their results down to twenty before
returning them, so the removal ran on an already shortened list and a caller with
access to few notes was served fewer results than it is allowed to see, down to
none at all. In EmbeddingSearch the notes the caller cannot read also fed the
table boost, so they moved the ranking of the results that were kept.

The read check now travels with the query as a predicate over the note id, and
every implementation applies it before it cuts anything: EmbeddingSearch drops
the entries while it scores them, so neither the table weights nor the cutoff see
them, and LuceneSearch walks the hits in score order until it has collected a
full page of readable ones. LuceneSearch reads only the id field to decide on a
hit and loads the rest of the document for the hits that are kept.

query(String) is gone, so there is no longer a way to search without saying who
is asking.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant