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.

Sign up for GitHub

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

Restrict direct use of ApplicationPrivilege constructor #91176

Merged
merged 9 commits into from
Nov 2, 2022
Merged

Restrict direct use of ApplicationPrivilege constructor #91176

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
53dbe65
Lock down application privilege constructor
n1v0lg Oct 28, 2022
3b6372c
TODOs
n1v0lg Oct 28, 2022
10812c2
Clean up comment
n1v0lg Oct 28, 2022
6f95f18
Merge branch 'main' into lock-down-application-priv-constructors
n1v0lg Oct 28, 2022
3bd987c
Nits
n1v0lg Oct 28, 2022
47dd44e
Rm metadata setup
n1v0lg Oct 28, 2022
02cffd4
Imports
n1v0lg Oct 28, 2022
243626c
Merge branch 'main' into lock-down-application-priv-constructors
n1v0lg Nov 1, 2022
5c0cc17
Merge branch 'main' into lock-down-application-priv-constructors
n1v0lg Nov 2, 2022
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
9 changes: 3 additions & 6 deletions ...main/java/org/elasticsearch/xpack/core/security/authz/privilege/ApplicationPrivilege.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,16 +43,13 @@ public final class ApplicationPrivilege extends Privilege {
*/
private static final Pattern VALID_NAME_OR_ACTION = Pattern.compile("^\\p{Graph}*$");

public static final Function<String, ApplicationPrivilege> NONE = app -> new ApplicationPrivilege(app, "none", new String[0]);
public static final Function<String, ApplicationPrivilege> NONE = app -> new ApplicationPrivilege(app, Collections.singleton("none"));

private final String application;
private final String[] patterns;

public ApplicationPrivilege(String application, String privilegeName, String... patterns) {
this(application, Collections.singleton(privilegeName), patterns);
}

public ApplicationPrivilege(String application, Set<String> name, String... patterns) {
// TODO make this private once ApplicationPrivilegeTests::createPrivilege uses ApplicationPrivilege::get
ApplicationPrivilege(String application, Set<String> name, String... patterns) {
super(name, patterns);
this.application = application;
this.patterns = patterns;
Expand Down
3 changes: 2 additions & 1 deletion ...va/org/elasticsearch/xpack/core/security/authz/permission/ApplicationPermissionTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilege;
import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilegeDescriptor;
import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilegeTests;

import java.util.ArrayList;
import java.util.Arrays;
Expand Down Expand Up @@ -40,7 +41,7 @@ public class ApplicationPermissionTests extends ESTestCase {

private ApplicationPrivilege storePrivilege(String app, String name, String... patterns) {
store.add(new ApplicationPrivilegeDescriptor(app, name, Sets.newHashSet(patterns), Collections.emptyMap()));
return new ApplicationPrivilege(app, name, patterns);
return ApplicationPrivilegeTests.createPrivilege(app, name, patterns);
}

public void testCheckSimplePermission() {
Expand Down
3 changes: 2 additions & 1 deletion ...rc/test/java/org/elasticsearch/xpack/core/security/authz/permission/LimitedRoleTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.elasticsearch.xpack.core.security.authz.accesscontrol.IndicesAccessControl;
import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilege;
import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilegeDescriptor;
import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilegeTests;
import org.elasticsearch.xpack.core.security.authz.privilege.ClusterPrivilegeResolver;
import org.elasticsearch.xpack.core.security.authz.privilege.IndexPrivilege;
import org.elasticsearch.xpack.core.security.support.Automatons;
Expand Down Expand Up @@ -873,7 +874,7 @@ private ApplicationPrivilege defineApplicationPrivilege(String app, String name,
applicationPrivilegeDescriptors.add(
new ApplicationPrivilegeDescriptor(app, name, Sets.newHashSet(actions), Collections.emptyMap())
);
return new ApplicationPrivilege(app, name, actions);
return ApplicationPrivilegeTests.createPrivilege(app, name, actions);
}

private static MapBuilder<String, ResourcePrivileges> mapBuilder() {
Expand Down
6 changes: 3 additions & 3 deletions ...src/test/java/org/elasticsearch/xpack/core/security/authz/permission/SimpleRoleTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.xpack.core.security.authz.AuthorizationEngine;
import org.elasticsearch.xpack.core.security.authz.RoleDescriptor;
import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilege;
import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilegeDescriptor;
import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilegeTests;

import java.util.Arrays;
import java.util.List;
Expand Down Expand Up @@ -102,7 +102,7 @@ public void testBuildFromRoleDescriptorWithApplicationPrivileges() {
"expected grant for role with application privilege to be: " + applicationPrivilege,
role.application()
.grants(
new ApplicationPrivilege(
ApplicationPrivilegeTests.createPrivilege(
wildcardApplication ? randomAlphaOfLengthBetween(1, 10) : applicationPrivilege.getApplication(),
wildcardPrivileges ? Set.of(randomAlphaOfLengthBetween(1, 10)) : Set.of(applicationPrivilege.getPrivileges()),
wildcardPrivileges ? randomAlphaOfLengthBetween(1, 10) : allowedApplicationActionPattern
Expand All @@ -117,7 +117,7 @@ public void testBuildFromRoleDescriptorWithApplicationPrivileges() {
"expected grant for role with application privilege to be: " + applicationPrivilege,
role.application()
.grants(
new ApplicationPrivilege(
ApplicationPrivilegeTests.createPrivilege(
false == wildcardApplication
? randomValueOtherThan(applicationPrivilege.getApplication(), () -> randomAlphaOfLengthBetween(1, 10))
: randomAlphaOfLengthBetween(1, 10),
Expand Down
25 changes: 14 additions & 11 deletions ...java/org/elasticsearch/xpack/core/security/authz/privilege/ApplicationPrivilegeTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,7 @@

import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.Locale;
import java.util.Map;
import java.util.Set;
import java.util.function.Supplier;

Expand All @@ -35,6 +33,19 @@

public class ApplicationPrivilegeTests extends ESTestCase {

public static ApplicationPrivilege createPrivilege(final String applicationName, final String privilegeName, final String... patterns) {
return createPrivilege(applicationName, Collections.singleton(privilegeName), patterns);
}

public static ApplicationPrivilege createPrivilege(
final String applicationName,
final Set<String> privilegeNames,
final String... patterns
) {
// TODO rewrite this to use `ApplicationPrivilege.get()`
return new ApplicationPrivilege(applicationName, privilegeNames, patterns);
}

public void testValidationOfApplicationName() {
final String specialCharacters = ":;$#%()+='.{}[]!@^&'";
final Supplier<Character> specialCharacter = () -> specialCharacters.charAt(randomInt(specialCharacters.length() - 1));
Expand Down Expand Up @@ -203,7 +214,7 @@ public void testEqualsAndHashCode() {
final EqualsHashCodeTestUtils.MutateFunction<ApplicationPrivilege> mutate = randomFrom(
orig -> createPrivilege("x" + orig.getApplication(), getPrivilegeName(orig), orig.getPatterns()),
orig -> createPrivilege(orig.getApplication(), "x" + getPrivilegeName(orig), orig.getPatterns()),
orig -> new ApplicationPrivilege(orig.getApplication(), getPrivilegeName(orig), "*")
orig -> createPrivilege(orig.getApplication(), getPrivilegeName(orig), "*")
);
EqualsHashCodeTestUtils.checkEqualsAndHashCode(
privilege,
Expand All @@ -212,10 +223,6 @@ public void testEqualsAndHashCode() {
);
}

private ApplicationPrivilege createPrivilege(String applicationName, String privilegeName, String... patterns) {
return new ApplicationPrivilege(applicationName, privilegeName, patterns);
}

private String getPrivilegeName(ApplicationPrivilege privilege) {
if (privilege.name.size() == 1) {
return privilege.name.iterator().next();
Expand Down Expand Up @@ -257,10 +264,6 @@ private ApplicationPrivilege randomPrivilege() {
patterns[i] = randomAlphaOfLengthBetween(2, 5) + "/" + suffix;
}

final Map<String, Object> metadata = new HashMap<>();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was dead code

for (int i = randomInt(3); i > 0; i--) {
metadata.put(randomAlphaOfLengthBetween(2, 5), randomFrom(randomBoolean(), randomInt(10), randomAlphaOfLength(5)));
}
return createPrivilege(applicationName, privilegeName, patterns);
}

Expand Down
Loading