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

HLRC: Add get users action #36332

Merged
merged 15 commits into from
Dec 13, 2018
Merged

HLRC: Add get users action #36332

merged 15 commits into from
Dec 13, 2018

Conversation

nknize
Copy link
Contributor

@nknize nknize commented Dec 6, 2018

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

Relates #29827

This commit adds get user action to the high level rest client.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features

@nknize nknize added the WIP label Dec 6, 2018
@nknize
Copy link
Contributor Author

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
Copy link
Contributor Author

nknize commented Dec 7, 2018

ping: @elastic/es-security

@jaymode
Copy link
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
Copy link
Member

jaymode commented Dec 7, 2018

I did not see anything jump out as missing to me

@albertzaharovits
Copy link
Contributor

@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
Copy link
Contributor Author

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
Copy link
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
Copy link
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
Copy link
Contributor Author

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.

…e User parser to GetUsersResponse, and remove enabled flag from user
@nknize
Copy link
Contributor Author

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?

@albertzaharovits
Copy link
Contributor

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
Copy link
Contributor Author

nknize commented Dec 12, 2018

retest this please

@nknize
Copy link
Contributor Author

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
Copy link
Member

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.

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.

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
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Dec 13, 2018
* 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
Copy link
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
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants