[ZEPPELIN-6414] Apply the authorization filter before the search cutoff - #5427
Open
dev-donghwan wants to merge 1 commit into
Open
[ZEPPELIN-6414] Apply the authorization filter before the search cutoff#5427dev-donghwan wants to merge 1 commit into
dev-donghwan wants to merge 1 commit into
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is this PR for?
/notebook/searchasked 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:EmbeddingSearchthe 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.
EmbeddingSearchMAX_RESULTS, REST filtersLuceneSearchsearcher.search(query, 20), REST filterssearchAfteruntil a full page of readable hits is collected, or the hits run outNotebookRestApiLuceneSearchreads 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.SearchServiceis bound inZeppelinServerto 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.returnsNothingWhenTheCallerMayReadNothingcovers the walk ending on its own when no hit is readable.Run locally:
LuceneSearchTest- 12 tests, including the two new onesEmbeddingSearchTest- 12 tests, including the new one, run withZEPPELIN_EMBEDDING_TEST=trueafterbin/install-search-model.shThe existing call sites in both test classes now pass
id -> trueexplicitly.Questions:
SearchService.query(String)is replaced byquery(String, Predicate<String>). Only the three in-tree implementations and the REST resource use it.Possible follow-up
Indexing the note id as its own field would let
LuceneSearchhand 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.