Skip to content

[Test] Relax assertion of denial message for service account#80389

Merged
ywangd merged 3 commits intoelastic:8.0from
ywangd:denial-error-message-for-service-account-8.0
Nov 8, 2021
Merged

[Test] Relax assertion of denial message for service account#80389
ywangd merged 3 commits intoelastic:8.0from
ywangd:denial-error-message-for-service-account-8.0

Conversation

@ywangd
Copy link
Copy Markdown
Member

@ywangd ywangd commented Nov 5, 2021

Relax the assertion for error message so that v7RestCompat test does not break on non-bwc'able changes.

Relates: #79809

The error message for access denial now explicitly spells out service
account instead of generic "user" when a service account issues the
request.

Relates: elastic#79809
@ywangd ywangd added >enhancement :Security/Security Security issues without another label v8.0.0 v7.16.0 labels Nov 5, 2021
@ywangd ywangd requested a review from tvernum November 5, 2021 00:04
@elasticmachine elasticmachine added the Team:Security Meta label for security team label Nov 5, 2021
@elasticmachine
Copy link
Copy Markdown
Collaborator

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

@ywangd ywangd changed the title Improve denial message for service account [Test] Relax assertion of denial message for service account Nov 5, 2021
@ywangd ywangd added >test Issues or PRs that are addressing/adding tests and removed >enhancement labels Nov 5, 2021
@tvernum
Copy link
Copy Markdown
Contributor

tvernum commented Nov 5, 2021

Does this need to go into 8.0 or just 7.16?

- match: { "error.type": "security_exception" }
- match:
error.reason: "action [cluster:admin/xpack/security/user/delete] is unauthorized for user [elastic/fleet-server], this action is granted by the cluster privileges [manage_security,all]"
error.reason: "/action.\\[cluster:admin/xpack/security/user/delete\\].is.unauthorized.for.*\\[elastic/fleet-server\\].*this.action.is.granted.by.the.cluster.privileges.*/"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't understand why the regex needs a . in place of a space. Can you explain?

Copy link
Copy Markdown
Member Author

@ywangd ywangd Nov 5, 2021

Choose a reason for hiding this comment

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

The match regex assertion is compiled with the COMMENTS option that ignores whitespaces and permits comments (#).

We can also assert whitespace with \\ , e.g. this\\ action\\ is\\ granted\\ by\\ the\\ cluster\\ privileges. But I think escapes looks ugly and it's not all that important. Many existing yaml tests just use ..

@ywangd
Copy link
Copy Markdown
Member Author

ywangd commented Nov 5, 2021

Does this need to go into 8.0 or just 7.16?

For now probably 7.16 is fine since 8.0 is not used for v7RestCompat for 8.1. But I think it's better to update both 8.0 and 7.16 since it is a tiny change and future-proof in case we need restCompat from 8.0 to future versions.

@ywangd ywangd merged commit d10969d into elastic:8.0 Nov 8, 2021
ywangd added a commit to ywangd/elasticsearch that referenced this pull request Nov 8, 2021
…#80389)

Relax the assertion for error message so that v7RestCompat test does not
break on non-bwc'able changes.

Relates: elastic#79809
elasticsearchmachine pushed a commit that referenced this pull request Nov 8, 2021
…#80470)

Relax the assertion for error message so that v7RestCompat test does not
break on non-bwc'able changes.

Relates: #79809
ywangd added a commit to ywangd/elasticsearch that referenced this pull request Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Security/Security Security issues without another label Team:Security Meta label for security team >test Issues or PRs that are addressing/adding tests v7.16.0 v8.0.0-rc1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants