-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Updatable API keys - REST API spec and tests #88270
Conversation
x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/api_key/30_update.yml
Show resolved
Hide resolved
x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/api_key/20_query.yml
Show resolved
Hide resolved
Pinging @elastic/es-security (Team:Security) |
Hi @n1v0lg, I've created a changelog YAML for you. |
Pinging @elastic/clients-team (Team:Clients) |
@elasticmachine run elasticsearch-ci/part-2 plz. Unrelated failure. |
@n1v0lg Looks pretty good so far! Thanks for adding so many YAML tests for this API. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM pending stub doc page as suggested below.
docs/changelog/88270.yaml
Outdated
@@ -0,0 +1,5 @@ | |||
pr: 88270 | |||
summary: Updatable API keys - REST API spec and tests | |||
area: Authentication |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am curious why you picked Authentication
. I'd go with the generic Security
. It's not great either. I don't have a strong opinion, but would like to know your thoughts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really wasn't sure here. I went with authentication because API keys generally are related to authentication but it's a bit of a stretch for this PR. I think Security
might make more sense.
- do: | ||
security.clear_api_key_cache: | ||
ids: "*" | ||
- match: { _nodes.failed: 0 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should clear API key cache after each test since this does not match the real usage pattern, i.e. it is unlikely that users would clear cache every often. I understand this is copied from 10_basic.yml
. But those calls were added when API key cache clearing API was initially introduced. So they intentionally excercise the API. But outside that, there is no need (and actually better not to) call this API.
@@ -0,0 +1,355 @@ | |||
--- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The YAML tests look fine to me. That said, I just noticed two things in Java tests:
- There is a bug in
ApiKeyIntegTests.testUpdateApiKeyAutoUpdatesUserRoles
in that the role definition most often do not change because theputRoleWithClusterPrivileges
method only honors the last cluster privilege. - We don't seem to have tests for auto-update user information other than role definition changes, e.g.
full_name
oremail
changes. Role name change (as opposed to role definition changes) is not covered either.
This PR adds REST API spec and YAML test files for the UpdateApiKey
operation.