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

Create/Update role mapping API #34171

Merged
merged 10 commits into from
Oct 16, 2018
Merged

Conversation

bizybot
Copy link
Contributor

@bizybot bizybot commented Oct 1, 2018

We added support for role mapper expression DSL in #33745,
that allows us to build the role mapper expression used in the
role mapping (as rules for determining user roles based on what
the boolean expression resolves to).

This change now adds support for create/update role mapping
API to the high-level rest client.

This commit adds create/update role mapping API.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

Yogesh Gaikwad added 2 commits October 1, 2018 18:10

public PutRoleMappingRequest(final String name, final boolean enabled, final List<String> roles, final RoleMapperExpression rules,
@Nullable final Map<String, Object> metadata, @Nullable final RefreshPolicy refreshPolicy) {
super();
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed this, Thank you.

Copy link
Member

@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 with some minor comments @bizybot

private boolean created;

public PutRoleMappingResponse(boolean created) {
super();
Copy link
Member

Choose a reason for hiding this comment

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

is this needed?

- also added few missing cases
*/
public final class PutRoleMappingResponse {

private boolean created;
Copy link
Member

Choose a reason for hiding this comment

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

make it final

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, Thanks.

*/
public final class PutRoleMappingRequest implements Validatable, ToXContentObject {

private String name;
Copy link
Member

Choose a reason for hiding this comment

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

make all of the class member variables final

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, Thanks.

private List<String> roles;
private RoleMapperExpression rules;

@Nullable private Map<String, Object> metadata;
Copy link
Member

Choose a reason for hiding this comment

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

metadata cannot be null based on the constructor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, Thank you.

private RoleMapperExpression rules;

@Nullable private Map<String, Object> metadata;
@Nullable private RefreshPolicy refreshPolicy;
Copy link
Member

Choose a reason for hiding this comment

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

refreshPolicy cannot be null based on the constructor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, Thank you.

}
final RefreshPolicy refreshPolicy = randomFrom(randomFrom(RefreshPolicy.values()), null);

if (Strings.hasText(name) == false) {
Copy link
Member

Choose a reason for hiding this comment

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

rather than use randomization for these branches can we just test the valid case and invalid cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Separated into cases, Thank you.

@jaymode
Copy link
Member

jaymode commented Oct 2, 2018

Also @bizybot can you please add more detail to the commit message. A specific item you could include would be how this builds on the work done to the expression dsl in #33745

@rjernst rjernst removed the review label Oct 10, 2018
Copy link
Member

@jaymode jaymode left a comment

Choose a reason for hiding this comment

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

I left one comment. Otherwise LGTM

bizybot added a commit that referenced this pull request Oct 16, 2018
Correct the tags mapping with the documentation.
This was added in #34171
bizybot added a commit that referenced this pull request Oct 16, 2018
We added support for role mapper expression DSL in #33745,
that allows us to build the role mapper expression used in the
role mapping (as rules for determining user roles based on what
the boolean expression resolves to).

This change now adds support for create/update role mapping
API to the high-level rest client.
bizybot added a commit that referenced this pull request Oct 16, 2018
Correct the tags mapping with the documentation.
This was added in #34171
kcm pushed a commit that referenced this pull request Oct 30, 2018
We added support for role mapper expression DSL in #33745,
that allows us to build the role mapper expression used in the
role mapping (as rules for determining user roles based on what
the boolean expression resolves to).

This change now adds support for create/update role mapping
API to the high-level rest client.
kcm pushed a commit that referenced this pull request Oct 30, 2018
Correct the tags mapping with the documentation.
This was added in #34171
@colings86 colings86 removed the :Security/Security Security issues without another label label Nov 2, 2018
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.

None yet

6 participants