-
Notifications
You must be signed in to change notification settings - Fork 25.3k
Feature/prevent create api with cloud api key #129966
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
base: main
Are you sure you want to change the base?
Feature/prevent create api with cloud api key #129966
Conversation
…ud-api-key-authentication
…ud-api-key-authentication # Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java
…authentication # Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java
…ate-api-with-cloud-api-key # Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java # x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/Authentication.java # x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java
if (authentication.isCloudApiKey()) { | ||
listener.onFailure(new IllegalArgumentException("creating elasticsearch api keys using cloud api keys is not supported")); | ||
return; | ||
} |
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'd suggest to add this check in ApiKeyService#createApiKey
method.
Reason is to cover all possible cases, including creating cross-cluster API keys and granting API keys.
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.
Also, let's add a unit test for this change in ApiKeyServiceTests
.
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.
done!
…ate-api-with-cloud-api-key
…cloud-api-key' into feature/prevent-create-api-with-cloud-api-key
Pinging @elastic/es-security (Team:Security) |
assertEquals(true, future.isDone()); | ||
assertThrows(ExecutionException.class, future::get); | ||
try { | ||
future.get(); | ||
} catch (ExecutionException ex) { | ||
assertEquals( | ||
"java.lang.IllegalArgumentException: creating elasticsearch api keys using cloud api keys is not supported", | ||
ex.getMessage() | ||
); | ||
} |
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.
optional: The ESTestCase
has the expectThrows
method that accepts an ActionFuture
and which can make this check a bit simpler:
assertEquals(true, future.isDone()); | |
assertThrows(ExecutionException.class, future::get); | |
try { | |
future.get(); | |
} catch (ExecutionException ex) { | |
assertEquals( | |
"java.lang.IllegalArgumentException: creating elasticsearch api keys using cloud api keys is not supported", | |
ex.getMessage() | |
); | |
} | |
final IllegalArgumentException iae = expectThrows(IllegalArgumentException.class, future); | |
assertThat(iae.getMessage(), equalTo("creating elasticsearch api keys using cloud api keys is not supported")); |
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.
LGTM 👍
(left a non blocking suggestion)
@@ -2557,6 +2558,25 @@ public void testCreationWillFailIfHashingThreadPoolIsSaturated() { | |||
assertThat(e, is(rejectedExecutionException)); | |||
} | |||
|
|||
@Test |
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.
Oh, I just saw this. The @Test
annotation is forbidden. You'll have to remove it.
Forbidden annotation use: org.junit.Test [defaultMessage Just name your test method testFooBar]
@Test |
Not supporting this at this time per TDD