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

Validate `query` field when creating roles #46275

Open
wants to merge 10 commits into
base: master
from

Conversation

@bizybot
Copy link
Contributor

commented Sep 3, 2019

In the current implementation, the validation of the role query
occurs at runtime when the query is being executed.

This commit adds validation for the role query when creating a role
but not for the template query as we do not have the runtime
information required for evaluating the template query (eg. authenticated user's
information). This is similar to the scripts that we
store but do not evaluate or parse if they are valid queries or not.

For validation, the query is evaluated (if not a template), parsed to build the
QueryBuilder and verify if the query type is allowed.

Closes #34252

@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Sep 3, 2019

@bizybot bizybot force-pushed the bizybot:34252-fix branch 10 times, most recently from 406f3a1 to 6efafea Sep 4, 2019

@bizybot

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

unrelated failure
@elasticmachine run elasticsearch-ci/2

Validate `query` field when creating roles
As of now the validation occurs at runtime when the query is being
executed. I think the reason was due to the use of template queries
which need runtime information as they need to be evaluated like
user information.

This commit adds validation for the role query but **not for the
template query** as we do not have the runtime information required
for evaluating the template query.
This also corrects some tests and roles.yml files where the `query` field was not
populated correctly.
For validation, the query is evaluated (if not a template), parsed to build the
`QueryBuilder` and verify if the query type is allowed.

Closes #34252

@bizybot bizybot force-pushed the bizybot:34252-fix branch from 6efafea to 6e1c8b6 Sep 5, 2019

@bizybot

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

@elasticmachine run elasticsearch-ci/2

@bizybot bizybot requested review from jkakavas and tvernum Sep 5, 2019

@bizybot bizybot marked this pull request as ready for review Sep 5, 2019

@jkakavas
Copy link
Contributor

left a comment

Looks good @bizybot , left some comments

}
}
} catch (XContentParseException | IOException e) {
throw new ElasticsearchParseException("failed to parse field 'query' from the role descriptor", e);

This comment has been minimized.

Copy link
@jkakavas

jkakavas Sep 10, 2019

Contributor

consider changing the error message here, we wouldn't trying parse the query. WDYT ?

This comment has been minimized.

Copy link
@bizybot

bizybot Sep 10, 2019

Author Contributor

changed the message here, Thank you.

import java.util.List;

/**
* This class helps in evaluating the query field if it is template

This comment has been minimized.

Copy link
@jkakavas

jkakavas Sep 10, 2019

Contributor

it evaluates non template queries also, right ?

This comment has been minimized.

Copy link
@bizybot

bizybot Sep 10, 2019

Author Contributor

So there are two validation methods one that validates query (validateQueryField) and another one that evaluates a query if it is a template and then performs validation(validateAndVerifyRoleQuery).
validateQueryField cannot evaluate the template query so skips the validation for it as the runtime information is not available.
I have corrected the documentation for validateQueryField which was incorrect. Thank you.

bizybot added 6 commits Sep 10, 2019

@bizybot bizybot requested a review from jkakavas Sep 10, 2019

@jkakavas
Copy link
Contributor

left a comment

LGTM

@legrego

This comment has been minimized.

Copy link

commented Sep 11, 2019

I'm very excited for this, as I can use it to resolve elastic/kibana#36529.

I started testing against this PR, and I can see the validation working:

image

image

(aside: what's the difference between a parse_exception and a parsing_exception?)

My question for you: Is there a way I can programmatically determine if the query validation failed, or if there was a different cause for the failure? I'd rather surface a more user-friendly message in the UI, as there are potentially a lot of fields in play on the Role Management screen, and the errors returned from ES in this scenario don't always offer enough context to make them actionable. For example, if there are two or more DLS queries defined on the role, is there a way to know which one failed?

If not, then I have another question: would it be possible to extract this new validation logic to a dedicated API call as well, so that I can use it independently of the create/update role APIs? That would allow me to highlight the offending query in the UI, so that it's immediately obvious to the user that a query is malformed, and even which query is malformed. I realize this might not be your preferred route, as it'd really only be useful to Kibana...I'm open to other suggestions as well!

@bizybot

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2019

I'm very excited for this, as I can use it to resolve elastic/kibana#36529.

I started testing against this PR, and I can see the validation working:

Thank you for testing this, appreciate your feedback!

(aside: what's the difference between a parse_exception and a parsing_exception?)

It's a little confusing for me too. We have been using at places for parsing errors and sometimes parsing_exception wrapping the parse_exception. I think it is not intentional but I have asked in the channel. I think both are same though used interchangeably at places.
But they should be wrapped by ElasticsearchParseException so it is translated to HTTP error code 400 over Rest.

My question for you: Is there a way I can programmatically determine if the query validation failed, or if there was a different cause for the failure? I'd rather surface a more user-friendly message in the UI, as there are potentially a lot of fields in play on the Role Management screen, and the errors returned from ES in this scenario don't always offer enough context to make them actionable. For example, if there are two or more DLS queries defined on the role, is there a way to know which one failed?

Right now there is no way programmatically to determine if the query validation failed.
The validation is not just validation of the query field but also checks if those query types are supported in DLS queries.

For the position of index privilege where the query validation failed we can do that as it is an array.
We can return the array index within the indices privileges in the error response that UI can parse.
I have updated the PR with more information in the error message.

For example:

  1. For Query: { "match_all": { "unknown_field": "" } } error message would look like
    failed to parse field 'query' for [0]th index privilege from role descriptor; nested: [1:18] [match_all] unknown field [unknown_field], parser not found;...
  2. For Query: {}
    failed to parse field 'query' for [0]th index privilege from role descriptor; nested: [1:2] expected [FIELD_NAME] with value a query name or 'template' but found [END_OBJECT] instead;..
  3. For unsupported query:
    failed to parse field 'query' for [0]th index privilege from role descriptor; nested: IllegalArgumentException[has_parent query isn't supported as part of a role query;..

If not, then I have another question: would it be possible to extract this new validation logic to a dedicated API call as well, so that I can use it independently of the create/update role APIs? That would allow me to highlight the offending query in the UI, so that it's immediately obvious to the user that a query is malformed, and even which query is malformed. I realize this might not be your preferred route, as it'd really only be useful to Kibana...I'm open to other suggestions as well!

I think adding a new API only for validation is something we might not want to do as it is very specific to the presentation layer and we have avoided doing that till now.

Few things UI could do more:

  • validate if the query JSON is a valid JSON. For example, if the query is { malformed }
    ES would have an error message like
    [Unexpected character ('m' (code 109)): was expecting double-quote to start field name at [Source: java.io.StringReader@35a88b96; line: 1, column: 4]] which definitely is not going to help users.
  • another suggestion would be to use the query completion which is present on the dev tools? I think this could be useful, though the query completion may allow query names that are not supported for DLS but you will get an error from the backend.
bizybot added 2 commits Sep 12, 2019
Address review comments
- add index privileg index to the error message to denote query of which
index privilege failed to validate.
- add more information to the error message where we know the expected values
@@ -190,7 +190,7 @@ private static Role randomRole(String roleName) {
.name(roleName)
.clusterPrivileges(randomSubsetOf(randomInt(3), Role.ClusterPrivilegeName.ALL_ARRAY))
.indicesPrivileges(
randomArray(3, IndicesPrivileges[]::new, () -> IndicesPrivilegesTests.createNewRandom(randomAlphaOfLength(3))))
randomArray(3, IndicesPrivileges[]::new, () -> IndicesPrivilegesTests.createNewRandom("{\"match_all\": {}}")))

This comment has been minimized.

Copy link
@tvernum

tvernum Sep 12, 2019

Contributor

If we really want this to be a random role then may this would be better as

Suggested change
randomArray(3, IndicesPrivileges[]::new, () -> IndicesPrivilegesTests.createNewRandom("{\"match_all\": {}}")))
randomArray(3, IndicesPrivileges[]::new, () -> IndicesPrivilegesTests.createNewRandom(
"{\"term\":{ \"" + randomAlphaOfLength(5) + "\" : \"" + randomAlphaOfLength(3) + "\"}}")))

But maybe that's overkill, I didn't really look at where we use this method.

This comment has been minimized.

Copy link
@bizybot

bizybot Sep 12, 2019

Author Contributor

Since this was an IT testing put role, we were failing due to validation, earlier the query was invalid.
I don't think we need to create a random query here, the createNewRandom is being used in unit tests mostly. But if you feel that we should have a random query string generated then we can do something here. Thank you.

}
} catch (ParsingException | IllegalArgumentException | IOException e) {
throw new ElasticsearchParseException("failed to parse field 'query' for [" + i + "]th index privilege " +
"from role descriptor", e);

This comment has been minimized.

Copy link
@tvernum

tvernum Sep 12, 2019

Contributor

This seems like a verbose error without much useful information.
What does [0]th index privilege mean to a user?

It does tell you the query, or the role, or the index pattern, just a zero-based index. I think we can do better.

@@ -58,6 +67,15 @@ protected void doExecute(Task task, HasPrivilegesRequest request, ActionListener
return;
}

if (request.indexPrivileges() != null) {
try {
DLSRoleQueryValidator.validateQueryField(request.indexPrivileges(), xContentRegistry);

This comment has been minimized.

Copy link
@tvernum

tvernum Sep 12, 2019

Contributor

I don't think this is right. has-privileges shouldn't be doing anything with DLS...

This comment has been minimized.

Copy link
@bizybot

bizybot Sep 12, 2019

Author Contributor

has-privileges does not use the query field but it does parse the role descriptor with index privileges. We can skip this validation in case of has-privileges if we think it's okay to do that. Is it what you are suggesting? or to remove the parsing of role query in case of has-privileges?

@legrego

This comment has been minimized.

Copy link

commented Sep 12, 2019

I think adding a new API only for validation is something we might not want to do as it is very specific to the presentation layer and we have avoided doing that till now.

Yep, I assumed that was the case, but there's no harm in asking 😄

Few things UI could do more:
validate if the query JSON is a valid JSON. For example, if the query is { malformed }

Yeah, this is a quick win for us, good suggestion.

another suggestion would be to use the query completion which is present on the dev tools? I think this could be useful, though the query completion may allow query names that are not supported for DLS but you will get an error from the backend.

I think this would be ideal, but that would be quite a bit of work for us, and not something I could take on in the near future.

Address review comments
- public to package protected method
- return immediately for simpler methods
- correct the logic in file roles and add tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.