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.

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

Fix FLS for frozen tier #82521

Merged
merged 5 commits into from
Jan 13, 2022
Merged

Fix FLS for frozen tier #82521

merged 5 commits into from
Jan 13, 2022

Conversation

ywelsch
Copy link
Contributor

@ywelsch ywelsch commented Jan 13, 2022

Field level security was interacting in bad ways with the can-match phase on frozen tier shards (interaction between FieldSubsetReader and RewriteCachingDirectoryReader). This made can-match phase fail, which in the normal case would result in extra load on the frozen tier, and in the extreme case (in interaction with #51708) made searches fail.

This is a bug that was indirectly introduced by #78988.

Closes #82044

@elasticmachine elasticmachine added the Team:Security Meta label for security team label Jan 13, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security (Team:Security)

@ywelsch ywelsch added the auto-backport Automatically create backport pull requests when merged label Jan 13, 2022
@ywelsch ywelsch requested review from jpountz and removed request for jportner January 13, 2022 12:14
final Terms fieldNameTerms = super.terms(FieldNamesFieldMapper.NAME);
this.fieldNamesFilterTerms = Optional.ofNullable(
fieldNameTerms == null ? null : new FieldNamesTerms(fieldNameTerms)
);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it feels a bit weird to use Optional.ofNullable when you need to check for null already anyway, what about something like that instead?

this.fieldNamesFilterTerms = fieldNameTerms == null ? Optional.empty() : Optional.of(new FieldNamesTerms(fieldNameTerms));

return fieldNamesFilterTerms;
if (fieldNamesFilterTerms == null) {
synchronized (this) {
if (fieldNamesFilterTerms == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you leave a comment that explains why this is computed lazily?

@ywelsch ywelsch merged commit 8a7388c into elastic:master Jan 13, 2022
@ywelsch
Copy link
Contributor Author

ywelsch commented Jan 13, 2022

Thanks @jpountz!

ywelsch added a commit to ywelsch/elasticsearch that referenced this pull request Jan 13, 2022
Field level security was interacting in bad ways with the can-match phase on frozen tier shards (interaction between
FieldSubsetReader and RewriteCachingDirectoryReader). This made can-match phase fail, which in the normal case
would result in extra load on the frozen tier, and in the extreme case (in interaction with elastic#51708) made searches fail.

This is a bug that was indirectly introduced by elastic#78988.

Closes elastic#82044
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.0
7.17

ywelsch added a commit to ywelsch/elasticsearch that referenced this pull request Jan 13, 2022
Field level security was interacting in bad ways with the can-match phase on frozen tier shards (interaction between
FieldSubsetReader and RewriteCachingDirectoryReader). This made can-match phase fail, which in the normal case
would result in extra load on the frozen tier, and in the extreme case (in interaction with elastic#51708) made searches fail.

This is a bug that was indirectly introduced by elastic#78988.

Closes elastic#82044
ywelsch added a commit that referenced this pull request Jan 14, 2022
Field level security was interacting in bad ways with the can-match phase on frozen tier shards (interaction between
FieldSubsetReader and RewriteCachingDirectoryReader). This made can-match phase fail, which in the normal case
would result in extra load on the frozen tier, and in the extreme case (in interaction with #51708) made searches fail.

This is a bug that was indirectly introduced by #78988.

Closes #82044
ywelsch added a commit that referenced this pull request Jan 14, 2022
Field level security was interacting in bad ways with the can-match phase on frozen tier shards (interaction between
FieldSubsetReader and RewriteCachingDirectoryReader). This made can-match phase fail, which in the normal case
would result in extra load on the frozen tier, and in the extreme case (in interaction with #51708) made searches fail.

This is a bug that was indirectly introduced by #78988.

Closes #82044

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Automatically create backport pull requests when merged >bug :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC Team:Security Meta label for security team v7.17.0 v8.0.0 v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document Field Level Security on Frozen Tier not working correctly
4 participants