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

Security: remove put privilege API #32879

Merged
merged 10 commits into from
Aug 17, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import java.io.IOException;
import java.io.InputStream;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Objects;

Expand All @@ -34,32 +33,6 @@ public PutPrivilegesRequestBuilder(ElasticsearchClient client, PutPrivilegesActi
super(client, action, new PutPrivilegesRequest());
}

/**
* Populate the put privileges request using the given source, application name and privilege name
* The source must contain a single privilege object which matches the application and privilege names.
*/
public PutPrivilegesRequestBuilder source(String applicationName, String expectedName,
BytesReference source, XContentType xContentType)
throws IOException {
Objects.requireNonNull(xContentType);
// EMPTY is ok here because we never call namedObject
try (InputStream stream = source.streamInput();
XContentParser parser = xContentType.xContent()
.createParser(NamedXContentRegistry.EMPTY, LoggingDeprecationHandler.INSTANCE, stream)) {
XContentParser.Token token = parser.currentToken();
if (token == null) {
token = parser.nextToken();
}
if (token == XContentParser.Token.START_OBJECT) {
final ApplicationPrivilegeDescriptor privilege = parsePrivilege(parser, applicationName, expectedName);
this.request.setPrivileges(Collections.singleton(privilege));
} else {
throw new ElasticsearchParseException("expected an object but found {} instead", token);
}
}
return this;
}

ApplicationPrivilegeDescriptor parsePrivilege(XContentParser parser, String applicationName, String privilegeName) throws IOException {
ApplicationPrivilegeDescriptor privilege = ApplicationPrivilegeDescriptor.parse(parser, applicationName, privilegeName, false);
checkPrivilegeName(privilege, applicationName, privilegeName);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -292,12 +292,6 @@ public GetPrivilegesRequestBuilder prepareGetPrivileges(String applicationName,
return new GetPrivilegesRequestBuilder(client, GetPrivilegesAction.INSTANCE).application(applicationName).privileges(privileges);
}

public PutPrivilegesRequestBuilder preparePutPrivilege(String applicationName, String privilegeName,
BytesReference bytesReference, XContentType xContentType) throws IOException {
return new PutPrivilegesRequestBuilder(client, PutPrivilegesAction.INSTANCE)
.source(applicationName, privilegeName, bytesReference, xContentType);
}

public PutPrivilegesRequestBuilder preparePutPrivileges(BytesReference bytesReference, XContentType xContentType) throws IOException {
return new PutPrivilegesRequestBuilder(client, PutPrivilegesAction.INSTANCE).source(bytesReference, xContentType);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,6 @@
import org.elasticsearch.xpack.security.rest.action.oauth2.RestInvalidateTokenAction;
import org.elasticsearch.xpack.security.rest.action.privilege.RestDeletePrivilegesAction;
import org.elasticsearch.xpack.security.rest.action.privilege.RestGetPrivilegesAction;
import org.elasticsearch.xpack.security.rest.action.privilege.RestPutPrivilegeAction;
import org.elasticsearch.xpack.security.rest.action.privilege.RestPutPrivilegesAction;
import org.elasticsearch.xpack.security.rest.action.realm.RestClearRealmCacheAction;
import org.elasticsearch.xpack.security.rest.action.role.RestClearRolesCacheAction;
Expand Down Expand Up @@ -762,7 +761,6 @@ public List<RestHandler> getRestHandlers(Settings settings, RestController restC
new RestSamlInvalidateSessionAction(settings, restController, getLicenseState()),
new RestGetPrivilegesAction(settings, restController, getLicenseState()),
new RestPutPrivilegesAction(settings, restController, getLicenseState()),
new RestPutPrivilegeAction(settings, restController, getLicenseState()),
new RestDeletePrivilegesAction(settings, restController, getLicenseState())
);
}
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import java.util.Map;

import static org.elasticsearch.rest.RestRequest.Method.POST;
import static org.elasticsearch.rest.RestRequest.Method.PUT;

/**
* Rest endpoint to add one or more {@link ApplicationPrivilege} objects to the security index
Expand All @@ -37,6 +38,7 @@ public class RestPutPrivilegesAction extends SecurityBaseRestHandler {

public RestPutPrivilegesAction(Settings settings, RestController controller, XPackLicenseState licenseState) {
super(settings, licenseState);
controller.registerHandler(PUT, "/_xpack/security/privilege/", this);
controller.registerHandler(POST, "/_xpack/security/privilege/", this);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,36 +52,6 @@ private ApplicationPrivilegeDescriptor descriptor(String app, String name, Strin
return new ApplicationPrivilegeDescriptor(app, name, Sets.newHashSet(actions), Collections.emptyMap());
}

public void testBuildRequestFromJsonObject() throws Exception {
final PutPrivilegesRequestBuilder builder = new PutPrivilegesRequestBuilder(null, PutPrivilegesAction.INSTANCE);
builder.source("foo", "read", new BytesArray(
"{ \"application\":\"foo\", \"name\":\"read\", \"actions\":[ \"data:/read/*\", \"admin:/read/*\" ] }"
), XContentType.JSON);
final List<ApplicationPrivilegeDescriptor> privileges = builder.request().getPrivileges();
assertThat(privileges, iterableWithSize(1));
assertThat(privileges, contains(descriptor("foo", "read", "data:/read/*", "admin:/read/*")));
}

public void testPrivilegeNameValidationOfSingleElement() throws Exception {
final PutPrivilegesRequestBuilder builder = new PutPrivilegesRequestBuilder(null, PutPrivilegesAction.INSTANCE);
final IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () ->
builder.source("foo", "write", new BytesArray(
"{ \"application\":\"foo\", \"name\":\"read\", \"actions\":[ \"data:/read/*\", \"admin:/read/*\" ] }"
), XContentType.JSON));
assertThat(exception.getMessage(), containsString("write"));
assertThat(exception.getMessage(), containsString("read"));
}

public void testApplicationNameValidationOfSingleElement() throws Exception {
final PutPrivilegesRequestBuilder builder = new PutPrivilegesRequestBuilder(null, PutPrivilegesAction.INSTANCE);
final IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () ->
builder.source("bar", "read", new BytesArray(
"{ \"application\":\"foo\", \"name\":\"read\", \"actions\":[ \"data:/read/*\", \"admin:/read/*\" ] }"
), XContentType.JSON));
assertThat(exception.getMessage(), containsString("foo"));
assertThat(exception.getMessage(), containsString("bar"));
}

public void testPrivilegeNameValidationOfMultipleElement() throws Exception {
final PutPrivilegesRequestBuilder builder = new PutPrivilegesRequestBuilder(null, PutPrivilegesAction.INSTANCE);
final IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () ->
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"xpack.security.put_privileges": {
"documentation": "TODO",
"methods": [ "POST" ],
"methods": [ "PUT", "POST" ],
"url": {
"path": "/_xpack/security/privilege/",
"paths": [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,24 +30,26 @@ teardown:
ignore: 404
---
"Test put and get privileges":
# Single privilege, with names in URL
# Single privilege
- do:
xpack.security.put_privilege:
application: app
name: p1
xpack.security.put_privileges:
body: >
{
"application": "app",
"name": "p1",
"actions": [ "data:read/*" , "action:login" ],
"metadata": {
"key1" : "val1a",
"key2" : "val2a"
"app": {
Copy link
Member

Choose a reason for hiding this comment

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

Whats the relation between app and p1 and them being later declared again using application: "app" and name: "p1" do these need to be equal?

If we are taking a Map<AppName, Map<PrivilegeName, Privilege>> I think we should omit application and name or we have to take Array<Privilege>

Copy link
Member Author

Choose a reason for hiding this comment

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

They are equivalent as far as I know and I agree that it is redundant. @tvernum can you chime in about this aspect?

Copy link
Contributor

Choose a reason for hiding this comment

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

They're redundant and optional, but if they exist they need to match the keys from the containing object.
They're allowed for three reasons

  1. Historical, originally I had the body just being an array, but there are a few problems with array bodies, including that rest tests can't handle it, so I switched to objects, and at Albert's suggestion made the inner fields optional.
  2. It means that the objects that come from the GET API can be fed back into the PUT/POST without needing to remove fields
  3. The format for index storage needs the fields and we re-use the parser, so we needed to do something and optional seemed better than rejection.

Copy link
Member Author

Choose a reason for hiding this comment

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

Optional makes sense. I updated the rest test to use it both ways

"p1": {
"application": "app",
"name": "p1",
"actions": [ "data:read/*" , "action:login" ],
"metadata": {
"key1" : "val1a",
"key2" : "val2a"
}
}
}
}
- match: { "app.p1" : { created: true } }

# Multiple privileges, no names in URL
# Multiple privileges
- do:
xpack.security.put_privileges:
body: >
Expand Down Expand Up @@ -84,18 +86,18 @@ teardown:
- match: { "app.p3" : { created: true } }
- match: { "app2.p1" : { created: true } }

# Update existing privilege, with names in URL
# Update existing privilege
- do:
xpack.security.put_privilege:
application: app
name: p1
xpack.security.put_privileges:
body: >
{
"application": "app",
"name": "p1",
"actions": [ "data:read/*" , "action:login" ],
"metadata": {
"key3" : "val3"
"app": {
"p1": {
"actions": [ "data:read/*" , "action:login" ],
"metadata": {
"key3" : "val3"
}
}
}
}
- match: { "app.p1" : { created: false } }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,21 +31,25 @@ setup:
}

- do:
xpack.security.put_privilege:
application: app-allow
name: read
xpack.security.put_privileges:
body: >
{
"actions": [ "data:read/*" ]
"app-allow": {
"read": {
"actions": [ "data:read/*" ]
}
}
}

- do:
xpack.security.put_privilege:
application: app_deny
name: read
xpack.security.put_privileges:
body: >
{
"actions": [ "data:read/*" ]
"app-deny": {
"read": {
"actions": [ "data:read/*" ]
}
}
}

---
Expand Down Expand Up @@ -82,12 +86,14 @@ teardown:

- do:
headers: { Authorization: "Basic dGVzdF91c2VyOngtcGFjay10ZXN0LXBhc3N3b3Jk" } # test_user
xpack.security.put_privilege:
application: app
name: read
xpack.security.put_privileges:
body: >
{
"actions": [ "data:read/*" ]
"app": {
"read": {
"actions": [ "data:read/*" ]
}
}
}
- match: { "app.read" : { created: true } }

Expand All @@ -112,12 +118,14 @@ teardown:
"Test put application privileges when not allowed":
- do:
headers: { Authorization: "Basic dGVzdF91c2VyOngtcGFjay10ZXN0LXBhc3N3b3Jk" } # test_user
xpack.security.put_privilege:
application: app_deny
name: write
xpack.security.put_privileges:
body: >
{
"actions": [ "data:write/*" ]
"app_deny": {
"write": {
"actions": [ "data:write/*" ]
}
}
}
catch: forbidden

Expand Down