Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support intersecting multi-sets of queries with DocumentPermissions #91151

Merged
merged 12 commits into from
Nov 2, 2022
Merged

Support intersecting multi-sets of queries with DocumentPermissions #91151

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
887241b
wip: production code
ywangd Oct 27, 2022
8e56512
wip: fix test compliation, not running yet
ywangd Oct 27, 2022
d3e4ba9
Fix tests
ywangd Oct 27, 2022
6b87003
tweak
ywangd Oct 27, 2022
42822c5
address feedback
ywangd Oct 27, 2022
8283578
address feedback
ywangd Oct 27, 2022
641c5cd
Merge remote-tracking branch 'origin/main' into multi-intersection-dl…
ywangd Oct 28, 2022
4db2f6e
address feedback
ywangd Oct 28, 2022
6266edf
Merge remote-tracking branch 'origin/main' into multi-intersection-dl…
ywangd Oct 28, 2022
df16c1b
fix test
ywangd Oct 28, 2022
4c5ce97
Merge branch 'main' into multi-intersection-dls-fls
elasticmachine Nov 2, 2022
4c95ae7
Merge remote-tracking branch 'origin/main' into multi-intersection-dl…
ywangd Nov 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
143 changes: 58 additions & 85 deletions ...main/java/org/elasticsearch/xpack/core/security/authz/permission/DocumentPermissions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.lucene.search.Queries;
import org.elasticsearch.core.Nullable;
import org.elasticsearch.index.mapper.NestedLookup;
import org.elasticsearch.index.query.QueryBuilder;
import org.elasticsearch.index.query.QueryRewriteContext;
Expand All @@ -36,6 +37,7 @@
import java.util.SortedSet;
import java.util.TreeSet;
import java.util.function.Function;
import java.util.stream.Stream;

import static org.apache.lucene.search.BooleanClause.Occur.FILTER;
import static org.apache.lucene.search.BooleanClause.Occur.SHOULD;
Expand All @@ -46,58 +48,56 @@
* queries are used as an additional filter.
*/
public final class DocumentPermissions implements CacheKey {
// SortedSet because orders are important when they get serialised for request cache key
private final SortedSet<BytesReference> queries;
private final SortedSet<BytesReference> limitedByQueries;
private List<String> evaluatedQueries;
private List<String> evaluatedLimitedByQueries;

private static DocumentPermissions ALLOW_ALL = new DocumentPermissions();
@Nullable
private final List<Set<BytesReference>> listOfQueries;
@Nullable
private List<List<String>> listOfEvaluatedQueries;

DocumentPermissions() {
this.queries = null;
this.limitedByQueries = null;
private static final DocumentPermissions ALLOW_ALL = new DocumentPermissions();

private DocumentPermissions() {
this.listOfQueries = null;
}

DocumentPermissions(Set<BytesReference> queries) {
this(queries, null);
private DocumentPermissions(Set<BytesReference> queries) {
assert queries != null && false == queries.isEmpty() : "null or empty queries not permitted";
this.listOfQueries = List.of(new TreeSet<>(queries));
}

DocumentPermissions(Set<BytesReference> queries, Set<BytesReference> scopedByQueries) {
if (queries == null && scopedByQueries == null) {
throw new IllegalArgumentException("one of the queries or scoped queries must be provided");
}
this.queries = (queries != null) ? new TreeSet<>(queries) : null;
this.limitedByQueries = (scopedByQueries != null) ? new TreeSet<>(scopedByQueries) : null;
private DocumentPermissions(List<Set<BytesReference>> listOfQueries) {
assert listOfQueries != null && false == listOfQueries.isEmpty() : "null or empty list of queries not permitted";
assert listOfQueries.stream().allMatch(queries -> queries != null && false == queries.isEmpty())
: "null or empty queries not permitted";
// SortedSet because orders are important when they get serialised for request cache key
this.listOfQueries = listOfQueries.stream()
.map(queries -> queries instanceof SortedSet<BytesReference> ? queries : new TreeSet<>(queries))
.toList();
}

public Set<BytesReference> getQueries() {
return queries == null ? null : Set.copyOf(queries);
public List<Set<BytesReference>> getListOfQueries() {
return listOfQueries;
}

public Set<BytesReference> getLimitedByQueries() {
return limitedByQueries == null ? null : Set.copyOf(limitedByQueries);
public Set<BytesReference> getSingleSetOfQueries() {
assert listOfQueries != null && listOfQueries.size() == 1 : "the list of queries does not have a single member";
return listOfQueries.get(0);
}

/**
* @return {@code true} if either queries or scoped queries are present for document level security else returns {@code false}
*/
public boolean hasDocumentLevelPermissions() {
return queries != null || limitedByQueries != null;
return listOfQueries != null;
}

public boolean hasStoredScript() throws IOException {
if (queries != null) {
for (BytesReference q : queries) {
if (DLSRoleQueryValidator.hasStoredScript(q, NamedXContentRegistry.EMPTY)) {
return true;
}
}
}
if (limitedByQueries != null) {
for (BytesReference q : limitedByQueries) {
if (DLSRoleQueryValidator.hasStoredScript(q, NamedXContentRegistry.EMPTY)) {
return true;
if (listOfQueries != null) {
for (Set<BytesReference> queries : listOfQueries) {
for (BytesReference q : queries) {
if (DLSRoleQueryValidator.hasStoredScript(q, NamedXContentRegistry.EMPTY)) {
return true;
}
}
}
}
Expand Down Expand Up @@ -125,35 +125,25 @@ public BooleanQuery filter(
) throws IOException {
if (hasDocumentLevelPermissions()) {
evaluateQueries(SecurityQueryTemplateEvaluator.wrap(user, scriptService));
BooleanQuery.Builder filter;
if (evaluatedQueries != null && evaluatedLimitedByQueries != null) {
filter = new BooleanQuery.Builder();
BooleanQuery.Builder scopedFilter = new BooleanQuery.Builder();
buildRoleQuery(shardId, searchExecutionContextProvider, evaluatedLimitedByQueries, scopedFilter);
filter.add(scopedFilter.build(), FILTER);
assert listOfEvaluatedQueries != null : "evaluated queries must not be null";
assert false == listOfEvaluatedQueries.isEmpty() : "evaluated queries must not be empty";

buildRoleQuery(shardId, searchExecutionContextProvider, evaluatedQueries, filter);
} else if (evaluatedQueries != null) {
filter = new BooleanQuery.Builder();
buildRoleQuery(shardId, searchExecutionContextProvider, evaluatedQueries, filter);
} else if (evaluatedLimitedByQueries != null) {
filter = new BooleanQuery.Builder();
buildRoleQuery(shardId, searchExecutionContextProvider, evaluatedLimitedByQueries, filter);
} else {
assert false : "one of queries and limited-by queries must be non-null";
return null;
BooleanQuery.Builder filter = new BooleanQuery.Builder();
for (int i = listOfEvaluatedQueries.size() - 1; i > 0; i--) {
final BooleanQuery.Builder scopedFilter = new BooleanQuery.Builder();
buildRoleQuery(shardId, searchExecutionContextProvider, listOfEvaluatedQueries.get(i), scopedFilter);
filter.add(scopedFilter.build(), FILTER);
}
// TODO: All role queries can be filters
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure i understand this TODO

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing logic is to have the limiting role (user) as filter. But the key's role as "should". So the query is something like:

{
  "bool": {
    "should": [ _key_queries ],
    "filter": [ { "bool": { "should": [ _Limiting_queries_ ] } } ]
  }
}

But the top level should clause is not necessary. Since intersection does not really care about order and does not treat any set of the queries specially, the TODO is proposing to change the above to:

{
  "bool": {
    "filter": [ 
      { "bool": { "should": [ _key_queries ] } } 
      { "bool": { "should": [ _Limiting_queries_ ] } } 
     ]
  }
}

This also helps simplifying the query building logic, i.e. not treating the 1st set any specially..

buildRoleQuery(shardId, searchExecutionContextProvider, listOfEvaluatedQueries.get(0), filter);
return filter.build();
}
return null;
}

private void evaluateQueries(DlsQueryEvaluationContext context) {
if (queries != null && evaluatedQueries == null) {
evaluatedQueries = queries.stream().map(context::evaluate).toList();
}
if (limitedByQueries != null && evaluatedLimitedByQueries == null) {
evaluatedLimitedByQueries = limitedByQueries.stream().map(context::evaluate).toList();
if (listOfQueries != null && listOfEvaluatedQueries == null) {
listOfEvaluatedQueries = listOfQueries.stream().map(queries -> queries.stream().map(context::evaluate).toList()).toList();
}
}

Expand Down Expand Up @@ -216,18 +206,9 @@ static void failIfQueryUsesClient(QueryBuilder queryBuilder, QueryRewriteContext
* @return {@link DocumentPermissions}
*/
public static DocumentPermissions filteredBy(Set<BytesReference> queries) {
if (queries == null || queries.isEmpty()) {
throw new IllegalArgumentException("null or empty queries not permitted");
}
return new DocumentPermissions(queries);
}

/**
* Create {@link DocumentPermissions} with no restriction. The {@link #getQueries()}
* will return {@code null} in this case and {@link #hasDocumentLevelPermissions()}
* will be {@code false}
* @return {@link DocumentPermissions}
*/
public static DocumentPermissions allowAll() {
return ALLOW_ALL;
}
Expand All @@ -240,49 +221,41 @@ public static DocumentPermissions allowAll() {
* @return instance of {@link DocumentPermissions}
*/
public DocumentPermissions limitDocumentPermissions(DocumentPermissions limitedByDocumentPermissions) {
assert limitedByQueries == null && limitedByDocumentPermissions.limitedByQueries == null
: "nested scoping for document permissions is not permitted";
if (queries == null && limitedByDocumentPermissions.queries == null) {
if (hasDocumentLevelPermissions() && limitedByDocumentPermissions.hasDocumentLevelPermissions()) {
return new DocumentPermissions(
Stream.concat(getListOfQueries().stream(), limitedByDocumentPermissions.getListOfQueries().stream()).toList()
);
} else if (hasDocumentLevelPermissions()) {
return new DocumentPermissions(getListOfQueries());
} else if (limitedByDocumentPermissions.hasDocumentLevelPermissions()) {
return new DocumentPermissions(limitedByDocumentPermissions.getListOfQueries());
} else {
return DocumentPermissions.allowAll();
}
// TODO: should we apply the same logic here as FieldPermissions#limitFieldPermissions,
// i.e. treat limited-by as queries if original queries is null?
return new DocumentPermissions(queries, limitedByDocumentPermissions.queries);
}

@Override
public String toString() {
return "DocumentPermissions [queries=" + queries + ", scopedByQueries=" + limitedByQueries + "]";
return "DocumentPermissions [listOfQueries=" + listOfQueries + "]";
}

@Override
public void buildCacheKey(StreamOutput out, DlsQueryEvaluationContext context) throws IOException {
assert false == (queries == null && limitedByQueries == null) : "one of queries and limited-by queries must be non-null";
assert hasDocumentLevelPermissions() : "document permissions should not contribute to cache key when there is no DLS query";
evaluateQueries(context);
if (evaluatedQueries != null) {
out.writeBoolean(true);
out.writeCollection(evaluatedQueries, StreamOutput::writeString);
} else {
out.writeBoolean(false);
}
if (evaluatedLimitedByQueries != null) {
out.writeBoolean(true);
out.writeCollection(evaluatedLimitedByQueries, StreamOutput::writeString);
} else {
out.writeBoolean(false);
}
out.writeCollection(listOfEvaluatedQueries, StreamOutput::writeStringCollection);
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
DocumentPermissions that = (DocumentPermissions) o;
return Objects.equals(queries, that.queries) && Objects.equals(limitedByQueries, that.limitedByQueries);
return Objects.equals(listOfQueries, that.listOfQueries);
}

@Override
public int hashCode() {
return Objects.hash(queries, limitedByQueries);
return Objects.hash(listOfQueries);
}
}
28 changes: 19 additions & 9 deletions ...src/main/java/org/elasticsearch/xpack/core/termsenum/action/TransportTermsEnumAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@

import java.io.Closeable;
import java.io.IOException;
import java.io.UncheckedIOException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
Expand Down Expand Up @@ -429,13 +430,17 @@ private boolean canAccess(
Collections.emptyMap()
);

// Unfiltered access is allowed when both base role and limiting role allow it.
return hasMatchAllEquivalent(indexAccessControl.getDocumentPermissions().getQueries(), securityContext, queryShardContext)
&& hasMatchAllEquivalent(
indexAccessControl.getDocumentPermissions().getLimitedByQueries(),
securityContext,
queryShardContext
);
// Current user has potentially many roles and therefore potentially many queries
// defining sets of docs accessible
final List<Set<BytesReference>> listOfQueries = indexAccessControl.getDocumentPermissions().getListOfQueries();

// When the user is an API Key, its role is a limitedRole and its effective document permissions
// are intersections of the two sets of queries, one belongs to the API key itself and the other belongs
// to the owner user. To allow unfiltered access to termsDict, both sets of the queries must have
// the "all" permission, i.e. the query can be rewritten into a MatchAll query.
// The following code loop through both sets queries and returns true only when both of them
// have the "all" permission.
return listOfQueries.stream().allMatch(queries -> hasMatchAllEquivalent(queries, securityContext, queryShardContext));
}
}
return true;
Expand All @@ -445,7 +450,7 @@ private boolean hasMatchAllEquivalent(
Set<BytesReference> queries,
SecurityContext securityContext,
SearchExecutionContext queryShardContext
) throws IOException {
) {
if (queries == null) {
return true;
}
Expand All @@ -458,7 +463,12 @@ private boolean hasMatchAllEquivalent(
queryShardContext.getParserConfig().registry(),
securityContext.getUser()
);
QueryBuilder rewrittenQueryBuilder = Rewriteable.rewrite(queryBuilder, queryShardContext);
QueryBuilder rewrittenQueryBuilder;
try {
rewrittenQueryBuilder = Rewriteable.rewrite(queryBuilder, queryShardContext);
} catch (IOException e) {
throw new UncheckedIOException(e);
}
if (rewrittenQueryBuilder instanceof MatchAllQueryBuilder) {
// One of the roles assigned has "all" permissions - allow unfettered access to termsDict
return true;
Expand Down
Loading