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 4 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
147 changes: 70 additions & 77 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,67 @@
* 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;

private static final DocumentPermissions ALLOW_ALL = new DocumentPermissions();

DocumentPermissions() {
this.queries = null;
this.limitedByQueries = null;
this.listOfQueries = null;
}

DocumentPermissions(Set<BytesReference> queries) {
this(queries, null);
if (queries == null || queries.isEmpty()) {
throw new IllegalArgumentException("null or empty queries not permitted");
Copy link
Contributor

Choose a reason for hiding this comment

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

should these (here and below) be assertions instead ? I tend to favor asserting over throwing if you are confident it will never happen without a programmer's bug.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think so, i.e. it can only be programming bug. The throwing code is carried over from existing code (the filteredBy static method). In general, I also favor assertions. That said, null check is often done with Objects.requireNonNull which is a runtime logic. Empty check is closely related. So I don't mind them to be runtime check. I can also add assert false inside the if block so we can have both. What do you think?

Separately, I'll also further lock down the constructors by making them private. They are only used in tests and should be replaced by convenient methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

We synced offline and agreed to go with assertions. I also made all constructors private so that instantiation is now only possible with the static methods of filteredBy and limitDocumentPermissions.

}
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");
DocumentPermissions(List<Set<BytesReference>> listOfQueries) {
if (listOfQueries == null || listOfQueries.isEmpty()) {
throw new IllegalArgumentException("null or empty list of queries not permitted");
}
this.queries = (queries != null) ? new TreeSet<>(queries) : null;
this.limitedByQueries = (scopedByQueries != null) ? new TreeSet<>(scopedByQueries) : null;
listOfQueries.forEach(queries -> {
if (queries == null || queries.isEmpty()) {
throw new IllegalArgumentException("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() {
if (listOfQueries != null && listOfQueries.size() == 1) {
return listOfQueries.get(0);
} else {
assert false;
throw new IllegalArgumentException("the list of queries does not have a single member: [" + listOfQueries + "]");
}
}

/**
* @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 +136,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,14 +217,11 @@ 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()}
* Create {@link DocumentPermissions} with no restriction. The {@link #getListOfQueries()}
Copy link
Contributor

Choose a reason for hiding this comment

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

not related to the change here ...but i think this javadoc is wrong, would you mind nuking it ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why do you think it is wrong? The word Create is not particularly accurate because the method always returns the same instance. But otherwise it is still accurate?

Copy link
Contributor

Choose a reason for hiding this comment

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

The javadoc is not really for this method since it describes how other methods work. It's not a big deal, just confusing.

IMO the javadoc should read

Returns a {@link DocumentPermissions} with no restrictions. This is equivalent to not having any document level permissions.

(or just not exist since it kinda self evident)
Also, outside the scope of this change, so feel free to disregard this comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah ok. Thanks for explaining this further. I see your point now. I removed the javadoc since it is indeed self evident.

* will return {@code null} in this case and {@link #hasDocumentLevelPermissions()}
* will be {@code false}
* @return {@link DocumentPermissions}
Expand All @@ -240,49 +238,44 @@ 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";
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);
if (false == hasDocumentLevelPermissions()) {
assert false;
Copy link
Contributor

Choose a reason for hiding this comment

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

as mentioned in person , probably more of style thing, but I not a fan of both assertions and throwing. If you keep both, can you add a comment to the assertion.

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed it to just the assertion since the existing code was just an assertion. Thanks!

throw new IllegalArgumentException("document permissions should not contribute to cache key when there is no DLS query");
}
evaluateQueries(context);
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);
}
}
37 changes: 23 additions & 14 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 @@ -431,21 +432,29 @@ private boolean canAccess(

// Current user has potentially many roles and therefore potentially many queries
// defining sets of docs accessible
Set<BytesReference> queries = indexAccessControl.getDocumentPermissions().getQueries();
for (BytesReference querySource : queries) {
QueryBuilder queryBuilder = DLSRoleQueryValidator.evaluateAndVerifyRoleQuery(
querySource,
scriptService,
queryShardContext.getParserConfig().registry(),
securityContext.getUser()
);
QueryBuilder rewrittenQueryBuilder = Rewriteable.rewrite(queryBuilder, queryShardContext);
if (rewrittenQueryBuilder instanceof MatchAllQueryBuilder) {
// One of the roles assigned has "all" permissions - allow unfettered access to termsDict
return true;
final List<Set<BytesReference>> listOfRawQueries = indexAccessControl.getDocumentPermissions().getListOfQueries();

return listOfRawQueries.stream().allMatch(queries -> {
for (BytesReference querySource : queries) {
QueryBuilder queryBuilder = DLSRoleQueryValidator.evaluateAndVerifyRoleQuery(
querySource,
scriptService,
queryShardContext.getParserConfig().registry(),
securityContext.getUser()
);
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;
}
}
}
return false;
return false;
});
Copy link
Member Author

@ywangd ywangd Oct 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The change here is also a bug fix. The existing code only checks getQueries() but not getLimitedByQueries. So it can make wrong decisions if the authentication subject is an API key. It is possible to maitain the existing behaviour so that the PR is more of a pure refactoring. But with the changes to DocumentPermissions, maintaining the existing behaviour is rather odd because all queries are now return by a single method.

It is probably helpful to move this logic to DocumentPermissions because it is closely related and better for reuse. But I am leaving it as is now to avoid cascading changes.

Please let me if you prefer it otherwise.

Copy link
Member Author

Choose a reason for hiding this comment

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

I raised a separate PR for the bug fix #91170 to keep this PR a pure refactoring.

}
}
return true;
Expand Down
Loading