Skip to content

Conversation

@tvernum
Copy link
Contributor

@tvernum tvernum commented Nov 13, 2018

This adds the "hasPrivileges()" method to SecurityClient, including
request, response & async variant of the method.

Also includes API documentation.

Relates: #29827

This adds the "hasPrivileges()" method to SecurityClient, including
request, response & async variant of the method.

Also includes API documentation.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@tvernum
Copy link
Contributor Author

tvernum commented Nov 13, 2018

CC: @elastic/es-security

Copy link
Contributor

@albertzaharovits albertzaharovits left a comment

Choose a reason for hiding this comment

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

LGTM ❤️

I would've used AbstractHlrcStreamableXContentTestCase for HasPrivilegesResponseTests . It's easier to write and more valuable as it tests the serialization against the server entity. And maybe added AbstractHlrcStreamableXContentTestCase. We also should reserve some time, when all APIs are done, to write tests in SecurityIT.

Copy link
Contributor

@jkakavas jkakavas left a comment

Choose a reason for hiding this comment

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

LGTM

@tvernum
Copy link
Contributor Author

tvernum commented Nov 16, 2018

Scrap that, I worked it out.

@tvernum
Copy link
Contributor Author

tvernum commented Nov 16, 2018

@albertzaharovits I'm going to raise the AbstractHlrcStreamableXContentTestCase as a separate PR.
It requires that the XPack response inplement ToXContent, and that's not true for HasPrivilegesResponse, so it will require some refactoring that is better to do in a standalone PR.

@tvernum tvernum merged commit 87a8b99 into elastic:master Nov 16, 2018
tvernum added a commit that referenced this pull request Nov 20, 2018
This adds the "hasPrivileges()" method to SecurityClient, including
request, response & async variant of the method.

Also includes API documentation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants