-
Notifications
You must be signed in to change notification settings - Fork 25.7k
Update policy parser to allow static methods for entitlement creation #121706
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
Changes from all commits
8d0f2d0
d99dbec
6847428
dcc6265
f0ed569
0511e17
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,6 +40,35 @@ public ManyConstructorsEntitlement(String s) {} | |
| public ManyConstructorsEntitlement(int i) {} | ||
| } | ||
|
|
||
| public static class ManyMethodsEntitlement implements Entitlement { | ||
| @ExternalEntitlement | ||
| public static ManyMethodsEntitlement create(String s) { | ||
| return new ManyMethodsEntitlement(); | ||
| } | ||
|
|
||
| @ExternalEntitlement | ||
| public static ManyMethodsEntitlement create(int i) { | ||
| return new ManyMethodsEntitlement(); | ||
| } | ||
| } | ||
|
|
||
| public static class ConstructorAndMethodEntitlement implements Entitlement { | ||
| @ExternalEntitlement | ||
| public static ConstructorAndMethodEntitlement create(String s) { | ||
| return new ConstructorAndMethodEntitlement(s); | ||
| } | ||
|
|
||
| @ExternalEntitlement | ||
| public ConstructorAndMethodEntitlement(String s) {} | ||
| } | ||
|
|
||
| public static class NonStaticMethodEntitlement implements Entitlement { | ||
| @ExternalEntitlement | ||
| public NonStaticMethodEntitlement create() { | ||
| return new NonStaticMethodEntitlement(); | ||
| } | ||
| } | ||
|
|
||
| public void testGetEntitlementTypeName() { | ||
| assertEquals("create_class_loader", PolicyParser.getEntitlementTypeName(CreateClassLoaderEntitlement.class)); | ||
|
|
||
|
|
@@ -55,7 +84,7 @@ public void testPolicyBuilder() throws IOException { | |
| .parsePolicy(); | ||
| Policy expected = new Policy( | ||
| "test-policy.yaml", | ||
| List.of(new Scope("entitlement-module-name", List.of(new FileEntitlement("test/path/to/file", "read_write")))) | ||
| List.of(new Scope("entitlement-module-name", List.of(FileEntitlement.create("test/path/to/file", "read_write")))) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's probably worth having explicit test(s) that check the behavior when eg a ctor and method are both annotated, similar to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will add these. |
||
| ); | ||
| assertEquals(expected, parsedPolicy); | ||
| } | ||
|
|
@@ -65,7 +94,7 @@ public void testPolicyBuilderOnExternalPlugin() throws IOException { | |
| .parsePolicy(); | ||
| Policy expected = new Policy( | ||
| "test-policy.yaml", | ||
| List.of(new Scope("entitlement-module-name", List.of(new FileEntitlement("test/path/to/file", "read_write")))) | ||
| List.of(new Scope("entitlement-module-name", List.of(FileEntitlement.create("test/path/to/file", "read_write")))) | ||
| ); | ||
| assertEquals(expected, parsedPolicy); | ||
| } | ||
|
|
@@ -174,4 +203,60 @@ public void testMultipleConstructorsAnnotated() throws IOException { | |
| ) | ||
| ); | ||
| } | ||
|
|
||
| public void testMultipleMethodsAnnotated() throws IOException { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we also have a error test for a non-static method having the annotation?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes! Good call. |
||
| var parser = new PolicyParser(new ByteArrayInputStream(""" | ||
| entitlement-module-name: | ||
| - many_methods | ||
| """.getBytes(StandardCharsets.UTF_8)), "test-policy.yaml", true, Map.of("many_methods", ManyMethodsEntitlement.class)); | ||
|
|
||
| var e = expectThrows(IllegalStateException.class, parser::parsePolicy); | ||
| assertThat( | ||
| e.getMessage(), | ||
| equalTo( | ||
| "entitlement class " | ||
| + "[org.elasticsearch.entitlement.runtime.policy.PolicyParserTests$ManyMethodsEntitlement]" | ||
| + " has more than one constructor and/or method annotated with ExternalEntitlement" | ||
| ) | ||
| ); | ||
| } | ||
|
|
||
| public void testConstructorAndMethodAnnotated() throws IOException { | ||
| var parser = new PolicyParser( | ||
| new ByteArrayInputStream(""" | ||
| entitlement-module-name: | ||
| - constructor_and_method | ||
| """.getBytes(StandardCharsets.UTF_8)), | ||
| "test-policy.yaml", | ||
| true, | ||
| Map.of("constructor_and_method", ConstructorAndMethodEntitlement.class) | ||
| ); | ||
|
|
||
| var e = expectThrows(IllegalStateException.class, parser::parsePolicy); | ||
| assertThat( | ||
| e.getMessage(), | ||
| equalTo( | ||
| "entitlement class " | ||
| + "[org.elasticsearch.entitlement.runtime.policy.PolicyParserTests$ConstructorAndMethodEntitlement]" | ||
| + " has more than one constructor and/or method annotated with ExternalEntitlement" | ||
| ) | ||
| ); | ||
| } | ||
|
|
||
| public void testNonStaticMethodAnnotated() throws IOException { | ||
| var parser = new PolicyParser(new ByteArrayInputStream(""" | ||
| entitlement-module-name: | ||
| - non_static | ||
| """.getBytes(StandardCharsets.UTF_8)), "test-policy.yaml", true, Map.of("non_static", NonStaticMethodEntitlement.class)); | ||
|
|
||
| var e = expectThrows(IllegalStateException.class, parser::parsePolicy); | ||
| assertThat( | ||
| e.getMessage(), | ||
| equalTo( | ||
| "entitlement class " | ||
| + "[org.elasticsearch.entitlement.runtime.policy.PolicyParserTests$NonStaticMethodEntitlement]" | ||
| + " has non-static method annotated with ExternalEntitlement" | ||
| ) | ||
| ); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be simpler to keep a MethodHandle and invoke that? Then there would be no difference between the ctor and static method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a quick prototype of this, but it adds additional complexity for error handling, and doesn't save much in lookup. I'm still open to this, but think I would want to do it as a different PR where we look at simplifying the entirety of the method a bit more.