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

HLRC: Add get users action #36332

Merged
merged 15 commits into from Dec 13, 2018

Conversation

@nknize
Member

nknize commented Dec 6, 2018

This commit adds get users action to the high level rest client.

Relates #29827

HLRC: Add get users action
This commit adds get user action to the high level rest client.
@elasticmachine

This comment has been minimized.

elasticmachine commented Dec 6, 2018

@nknize nknize added the WIP label Dec 6, 2018

@nknize

This comment has been minimized.

Member

nknize commented Dec 6, 2018

Labeling this as WIP as its not entire complete but I wanted to get some immediate feedback to keep this task moving toward completion. I have one key question:

  • The example in the docs have "enabled" : true in the response. But the user class didn't have an enabled field, only the AuthenticateResponse class did. I added boolean enabled to Users but I don't know if this is the right way to go. Furthermore, its causing a test failure now in SecurityIT#testAuthenticate

Other than that I'd like someone to have a quick look to verify I'm not missing anything. Once the above question is answered I think I just need to add an explicit test for calls to the user endpoint without any usernames explicitly provided.

@nknize

This comment has been minimized.

Member

nknize commented Dec 7, 2018

@jaymode

This comment has been minimized.

Member

jaymode commented Dec 7, 2018

But the user class didn't have an enabled field, only the AuthenticateResponse class did. I added boolean enabled to Users but I don't know if this is the right way to go.

@albertzaharovits had originally put the enabled field on the user and then removed it from the object, see #33552 (comment) for the reasoning.

@jaymode

This comment has been minimized.

Member

jaymode commented Dec 7, 2018

I did not see anything jump out as missing to me

@jaymode jaymode requested a review from albertzaharovits Dec 7, 2018

@albertzaharovits

This comment has been minimized.

Contributor

albertzaharovits commented Dec 10, 2018

@nknize I would much prefer not to reintroduce enabled on the User on the client side. I don't believe this a view of a user that the client should be concerned about. It doesn't sound good to have two users that are otherwise equal but they have the enabled flag different. Are the users actually the same one or not? I would say they are, but something has changed on the server that makes the same user to behave differently at different time points; like changing his roles does not really alter the identity.

I think GetUsers should be returning two sets of users, the set of all users and the set of enabled users.

@nknize

This comment has been minimized.

Member

nknize commented Dec 11, 2018

@albertzaharovits awesome.. thx for the feedback. I'll remove the enabled flag from user.

Can you clarify one thing for me? If we don't want the client to be concerned about enabled, then why would we return all users and enabled users? Wouldn't we just want to return either all users (if a username is not specified) or the requested user?

@tvernum

This comment has been minimized.

Contributor

tvernum commented Dec 11, 2018

I think GetUsers should be returning two sets of users, the set of all users and the set of enabled users.

I think that creates a strange API for clients that just want to iterate across all users (they'd have to loop twice, or we'd have to do some fancy merging of Iterables).

I feel it would be a nicer API to just add a isEnabled(User user) (or (String principal)) method to the response object, and have a Map or Set backing that.

@albertzaharovits

This comment has been minimized.

Contributor

albertzaharovits commented Dec 11, 2018

If we don't want the client to be concerned about enabled, then why would we return all users and enabled users? Wouldn't we just want to return either all users (if a username is not specified) or the requested user?

What I meant was that the current enabled status is not part of the user's identity. We still want the client to be able to check it, preferably without another round trip to the server so it should be exposed in the GetUsersResponse.

I think that creates a strange API for clients that just want to iterate across all users (they'd have to loop twice, or we'd have to do some fancy merging of Iterables).

I was thinking of the following client experience:

for (User user : response.getUsers()) {
  if (response.getEnabledUsers().contains(user)) {
    // do smth with enabled user
  } else {
    // do smth with disabled user
  }
}

The enabled check might as well be response.isEnabled(User user), as Tim suggested, IMO both are about the same.

@nknize

This comment has been minimized.

Member

nknize commented Dec 11, 2018

I guess the main question I have here is: how do I know if a user is enabled or disabled if I do not add the enabled variable to the User class? It looks like only AuthenticateResponse and PutUserRequest have the enabled variable.

convert users to a Set, add enabledUsers set to GetUsersResponse, mov…
…e User parser to GetUsersResponse, and remove enabled flag from user
@nknize

This comment has been minimized.

Member

nknize commented Dec 11, 2018

@albertzaharovits @tvernum I removed the enabled flag from the User. I also moved the User parser to the GetUserResponse class and added an enabledUsers set. Is this closer to what y'all were thinking?

nknize added some commits Dec 11, 2018

@albertzaharovits

This comment has been minimized.

Contributor

albertzaharovits commented Dec 12, 2018

I removed the enabled flag from the User. I also moved the User parser to the GetUserResponse class and added an enabledUsers set. Is this closer to what y'all were thinking?

Looks good, thank you!
I will take another look when you're ready for merge.

nknize added some commits Dec 12, 2018

@nknize

This comment has been minimized.

Member

nknize commented Dec 12, 2018

retest this please

@nknize

This comment has been minimized.

Member

nknize commented Dec 12, 2018

This is probably okay to merge. The failures look unrelated to this change. Will keep trying to reach a passing CI result.

@jkakavas

This comment has been minimized.

Contributor

jkakavas commented Dec 13, 2018

00:08:42 Tests with failures:
00:08:42   - org.elasticsearch.client.SecurityRequestConvertersTests.testGetUsers

I left a suggestion inline for

00:08:40 FAILURE 0.02s J7 | SecurityRequestConvertersTests.testGetUsers <<< FAILURES!
00:08:40    > Throwable #1: org.junit.ComparisonFailure: expected:</[]_security/user/test> but was:</[/]_security/user/test>

The org.elasticsearch.xpack.security.transport.netty4.SimpleSecurityNetty4ServerTransportTests.testHandshakeWithIncompatVersion seems unrelated.

@albertzaharovits

If you'd be so kind I would like a test in SecurityIT as well, but it's up to you.
LGTM besides the lack of documentation

@nknize nknize removed the WIP label Dec 13, 2018

@nknize nknize merged commit 4b17055 into elastic:master Dec 13, 2018

7 of 8 checks passed

elasticsearch-ci-1 Build finished.
Details
CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci-2 Build finished.
Details
elasticsearch-ci/default-distro Build finished.
Details
elasticsearch-ci/docbldesx Build finished.
Details
elasticsearch-ci/docs-check Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Dec 13, 2018

Merge remote-tracking branch 'elastic/master' into ccr-logstash-template
* elastic/master:
  Remove deprecated `useDisMax` from MultiMatchQuery (elastic#36488)
  HLRC: Add get users action (elastic#36332)
  fix MultiValuesSourceFieldConfig toXContent (elastic#36525)
  Add ILM-specific security privileges (elastic#36493)
  Remove usages of `MockTcpTransport` from zen tests (elastic#36579)
@javanna

This comment has been minimized.

Member

javanna commented Dec 14, 2018

This is labelled 6.6.0 but it has not been backported to 6.x. Just a reminder to backport #36622 once this is backported.

nknize added a commit that referenced this pull request Dec 14, 2018

HLRC: Add get users action (#36332)
This commit adds get user action to the high level rest client.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment