gnodet-bot commented on code in PR #28553:
URL: https://github.com/apache/camel/pull/28553#discussion_r4386360785
##########
components/camel-minio/src/main/java/org/apache/camel/component/minio/MinioProducer.java:
##########
@@ -196,6 +196,11 @@ private void doPutObject(
.object(objectName)
.userMetadata(objectMetadata);
+ MinioChecks.checkIfConfigIsNotEmptyAndSetAndConfig(
+ getConfiguration()::getServerSideEncryption,
putObjectRequest::sse);
+ MinioChecks.checkIfConfigIsNotEmptyAndSetAndConfig(
+ getConfiguration()::getServerSideEncryptionCustomerKey,
putObjectRequest::sse);
Review Comment:
🔵 **`MinioChecks` is `@Deprecated(since = "4.23")`**
`MinioProducer.java` previously had no usages of `MinioChecks`. This PR
introduces the first ones. What replaces this utility in 4.23+? If there's a
recommended alternative, use it here instead of expanding the deprecated
surface.
##########
components/camel-minio/src/main/java/org/apache/camel/component/minio/MinioProducer.java:
##########
@@ -196,6 +196,11 @@ private void doPutObject(
.object(objectName)
.userMetadata(objectMetadata);
+ MinioChecks.checkIfConfigIsNotEmptyAndSetAndConfig(
+ getConfiguration()::getServerSideEncryption,
putObjectRequest::sse);
+ MinioChecks.checkIfConfigIsNotEmptyAndSetAndConfig(
+ getConfiguration()::getServerSideEncryptionCustomerKey,
putObjectRequest::sse);
Review Comment:
⚠️ **Last-write-wins silently swallows `serverSideEncryption`**
Both `serverSideEncryption` and `serverSideEncryptionCustomerKey` are mapped
to `putObjectRequest::sse`. If both are non-null, the second call overwrites
the first — no warning, no exception, the SSE-S3 setting is silently dropped.
`ServerSideEncryption.CustomerKey` is a subtype of `ServerSideEncryption`,
so calling `sse(customerKey)` is valid SDK API for write operations. But the
precedence is undefined and invisible to callers. Add an explicit guard:
```suggestion
if (isNotEmpty(getConfiguration().getServerSideEncryption()) &&
isNotEmpty(getConfiguration().getServerSideEncryptionCustomerKey())) {
throw new IllegalArgumentException(
"Only one of serverSideEncryption or
serverSideEncryptionCustomerKey may be configured, not both");
}
MinioChecks.checkIfConfigIsNotEmptyAndSetAndConfig(
getConfiguration()::getServerSideEncryption,
putObjectRequest::sse);
MinioChecks.checkIfConfigIsNotEmptyAndSetAndConfig(
getConfiguration()::getServerSideEncryptionCustomerKey,
putObjectRequest::sse);
```
Or alternatively, pick an explicit precedence (SSE-C over SSE-S3 or
vice-versa) and document it.
##########
components/camel-minio/src/main/java/org/apache/camel/component/minio/MinioProducer.java:
##########
@@ -256,11 +266,19 @@ private void copyObject(MinioClient minioClient, Exchange
exchange) throws Excep
.bucket(bucketName)
.object(sourceKey);
+ MinioChecks.checkIfConfigIsNotEmptyAndSetAndConfig(
+ getConfiguration()::getServerSideEncryptionCustomerKey,
copySourceBuilder::ssec);
+
CopyObjectArgs.Builder copyObjectRequest = CopyObjectArgs.builder()
.bucket(destinationBucketName)
.object(destinationKey)
.source(copySourceBuilder.build());
+ MinioChecks.checkIfConfigIsNotEmptyAndSetAndConfig(
+ getConfiguration()::getServerSideEncryption,
copyObjectRequest::sse);
+ MinioChecks.checkIfConfigIsNotEmptyAndSetAndConfig(
+ getConfiguration()::getServerSideEncryptionCustomerKey,
copyObjectRequest::sse);
Review Comment:
⚠️ **Same last-write-wins issue on `copyObject` destination**
`copyObjectRequest::sse` is called for both `serverSideEncryption` and
`serverSideEncryptionCustomerKey`. The test `copyObjectWithEncryption` actually
*validates* this broken behavior — it sets both, then asserts that `ssec` wins
(because it's the second write). That means the S3 SSE configured on the
destination is silently discarded.
Apply the same explicit guard as suggested for `doPutObject`.
##########
components/camel-minio/src/test/java/org/apache/camel/component/minio/MinioProducerEncryptionTest.java:
##########
@@ -0,0 +1,221 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.component.minio;
+
+import javax.crypto.KeyGenerator;
+import javax.crypto.SecretKey;
+
+import io.minio.BucketExistsArgs;
+import io.minio.CopyObjectArgs;
+import io.minio.GetObjectArgs;
+import io.minio.GetObjectResponse;
+import io.minio.MinioClient;
+import io.minio.ObjectWriteResponse;
+import io.minio.PutObjectArgs;
+import io.minio.ServerSideEncryption;
+import org.apache.camel.Exchange;
+import org.apache.camel.test.junit6.CamelTestSupport;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.ArgumentCaptor;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+class MinioProducerEncryptionTest extends CamelTestSupport {
+
+ private MinioClient minioClient;
+ private SecretKey secretKey;
+
+ @BeforeEach
+ void init() throws Exception {
+ minioClient = mock(MinioClient.class);
+
when(minioClient.bucketExists(any(BucketExistsArgs.class))).thenReturn(true);
+ KeyGenerator generator = KeyGenerator.getInstance("AES");
+ generator.init(256);
+ secretKey = generator.generateKey();
+ }
+
+ private MinioProducer createProducer(MinioEndpoint endpoint) throws
Exception {
+ endpoint.start();
+ MinioProducer producer = new MinioProducer(endpoint);
+ producer.start();
+ return producer;
+ }
+
+ @Test
+ void putObjectWithServerSideEncryption() throws Exception {
+ ServerSideEncryption.S3 sse = new ServerSideEncryption.S3();
+ MinioComponent component = context.getComponent("minio",
MinioComponent.class);
+ MinioEndpoint endpoint = (MinioEndpoint)
component.createEndpoint("minio://mybucket?autoCreateBucket=false");
+ endpoint.getConfiguration().setMinioClient(minioClient);
+ endpoint.getConfiguration().setServerSideEncryption(sse);
+
+
when(minioClient.putObject(any(PutObjectArgs.class))).thenReturn(mock(ObjectWriteResponse.class));
+
+ MinioProducer producer = createProducer(endpoint);
+ Exchange exchange = endpoint.createExchange();
+ exchange.getIn().setHeader(MinioConstants.OBJECT_NAME, "test.txt");
+ exchange.getIn().setBody("content");
+
+ producer.process(exchange);
+
+ ArgumentCaptor<PutObjectArgs> captor =
ArgumentCaptor.forClass(PutObjectArgs.class);
+ verify(minioClient).putObject(captor.capture());
+ assertSame(sse, captor.getValue().sse());
+ producer.stop();
+ endpoint.stop();
+ }
+
+ @Test
+ void putObjectWithCustomerKey() throws Exception {
+ ServerSideEncryption.CustomerKey ssec = new
ServerSideEncryption.CustomerKey(secretKey);
+ MinioComponent component = context.getComponent("minio",
MinioComponent.class);
+ MinioEndpoint endpoint = (MinioEndpoint)
component.createEndpoint("minio://mybucket?autoCreateBucket=false");
+ endpoint.getConfiguration().setMinioClient(minioClient);
+ endpoint.getConfiguration().setServerSideEncryptionCustomerKey(ssec);
+
+
when(minioClient.putObject(any(PutObjectArgs.class))).thenReturn(mock(ObjectWriteResponse.class));
+
+ MinioProducer producer = createProducer(endpoint);
+ Exchange exchange = endpoint.createExchange();
+ exchange.getIn().setHeader(MinioConstants.OBJECT_NAME, "test.txt");
+ exchange.getIn().setBody("content");
+
+ producer.process(exchange);
+
+ ArgumentCaptor<PutObjectArgs> captor =
ArgumentCaptor.forClass(PutObjectArgs.class);
+ verify(minioClient).putObject(captor.capture());
+ assertSame(ssec, captor.getValue().sse());
+ producer.stop();
+ endpoint.stop();
+ }
+
+ @Test
+ void putObjectWithServerSideEncryptionHeader() throws Exception {
+ MinioComponent component = context.getComponent("minio",
MinioComponent.class);
+ MinioEndpoint endpoint = (MinioEndpoint)
component.createEndpoint("minio://mybucket?autoCreateBucket=false");
+ endpoint.getConfiguration().setMinioClient(minioClient);
+
+
when(minioClient.putObject(any(PutObjectArgs.class))).thenReturn(mock(ObjectWriteResponse.class));
+
+ MinioProducer producer = createProducer(endpoint);
+ Exchange exchange = endpoint.createExchange();
+ exchange.getIn().setHeader(MinioConstants.OBJECT_NAME, "test.txt");
+ exchange.getIn().setHeader(MinioConstants.SERVER_SIDE_ENCRYPTION,
"AES256");
+ exchange.getIn().setBody("content");
+
+ producer.process(exchange);
+
+ ArgumentCaptor<PutObjectArgs> captor =
ArgumentCaptor.forClass(PutObjectArgs.class);
+ verify(minioClient).putObject(captor.capture());
+ assertNotNull(captor.getValue().extraHeaders());
+ assertEquals("AES256",
captor.getValue().extraHeaders().getFirst("x-amz-server-side-encryption"));
+ producer.stop();
+ endpoint.stop();
+ }
+
+ @Test
+ void copyObjectWithEncryption() throws Exception {
+ ServerSideEncryption.S3 sse = new ServerSideEncryption.S3();
+ ServerSideEncryption.CustomerKey ssec = new
ServerSideEncryption.CustomerKey(secretKey);
+
+ MinioComponent component = context.getComponent("minio",
MinioComponent.class);
+ MinioEndpoint endpoint = (MinioEndpoint)
component.createEndpoint("minio://mybucket?autoCreateBucket=false");
+ endpoint.getConfiguration().setMinioClient(minioClient);
+ endpoint.getConfiguration().setServerSideEncryption(sse);
+ endpoint.getConfiguration().setServerSideEncryptionCustomerKey(ssec);
+
+
when(minioClient.copyObject(any(CopyObjectArgs.class))).thenReturn(mock(ObjectWriteResponse.class));
+
+ MinioProducer producer = createProducer(endpoint);
+ Exchange exchange = endpoint.createExchange();
+ exchange.getIn().setHeader(MinioConstants.MINIO_OPERATION,
MinioOperations.copyObject);
+ exchange.getIn().setHeader(MinioConstants.OBJECT_NAME, "source.txt");
+ exchange.getIn().setHeader(MinioConstants.DESTINATION_OBJECT_NAME,
"dest.txt");
+ exchange.getIn().setHeader(MinioConstants.DESTINATION_BUCKET_NAME,
"dest-bucket");
+
+ producer.process(exchange);
+
+ ArgumentCaptor<CopyObjectArgs> captor =
ArgumentCaptor.forClass(CopyObjectArgs.class);
+ verify(minioClient).copyObject(captor.capture());
+ assertSame(ssec, captor.getValue().sse());
Review Comment:
⚠️ **`copyObjectWithEncryption` test validates ambiguous last-wins behavior**
The test configures both `serverSideEncryption` (SSE-S3) and
`serverSideEncryptionCustomerKey` (SSE-C), then asserts
`captor.getValue().sse() == ssec`. This passes only because `ssec` was written
last, not because the logic is correct. The test should instead verify the two
independent scenarios:
1. `copyObject` with `serverSideEncryption` only → destination gets SSE-S3
2. `copyObject` with `serverSideEncryptionCustomerKey` only → destination
gets SSE-C
Testing the "both configured" case is fine, but it should test the
*intended* precedence, not an accidental one.
##########
components/camel-minio/src/test/java/org/apache/camel/component/minio/MinioProducerEncryptionTest.java:
##########
@@ -0,0 +1,221 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.component.minio;
+
+import javax.crypto.KeyGenerator;
+import javax.crypto.SecretKey;
+
+import io.minio.BucketExistsArgs;
+import io.minio.CopyObjectArgs;
+import io.minio.GetObjectArgs;
+import io.minio.GetObjectResponse;
+import io.minio.MinioClient;
+import io.minio.ObjectWriteResponse;
+import io.minio.PutObjectArgs;
+import io.minio.ServerSideEncryption;
+import org.apache.camel.Exchange;
+import org.apache.camel.test.junit6.CamelTestSupport;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.ArgumentCaptor;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+class MinioProducerEncryptionTest extends CamelTestSupport {
+
+ private MinioClient minioClient;
+ private SecretKey secretKey;
+
+ @BeforeEach
+ void init() throws Exception {
+ minioClient = mock(MinioClient.class);
+
when(minioClient.bucketExists(any(BucketExistsArgs.class))).thenReturn(true);
+ KeyGenerator generator = KeyGenerator.getInstance("AES");
+ generator.init(256);
+ secretKey = generator.generateKey();
+ }
+
+ private MinioProducer createProducer(MinioEndpoint endpoint) throws
Exception {
+ endpoint.start();
+ MinioProducer producer = new MinioProducer(endpoint);
+ producer.start();
+ return producer;
+ }
+
+ @Test
+ void putObjectWithServerSideEncryption() throws Exception {
+ ServerSideEncryption.S3 sse = new ServerSideEncryption.S3();
+ MinioComponent component = context.getComponent("minio",
MinioComponent.class);
+ MinioEndpoint endpoint = (MinioEndpoint)
component.createEndpoint("minio://mybucket?autoCreateBucket=false");
+ endpoint.getConfiguration().setMinioClient(minioClient);
+ endpoint.getConfiguration().setServerSideEncryption(sse);
+
+
when(minioClient.putObject(any(PutObjectArgs.class))).thenReturn(mock(ObjectWriteResponse.class));
+
+ MinioProducer producer = createProducer(endpoint);
+ Exchange exchange = endpoint.createExchange();
+ exchange.getIn().setHeader(MinioConstants.OBJECT_NAME, "test.txt");
+ exchange.getIn().setBody("content");
+
+ producer.process(exchange);
+
+ ArgumentCaptor<PutObjectArgs> captor =
ArgumentCaptor.forClass(PutObjectArgs.class);
+ verify(minioClient).putObject(captor.capture());
+ assertSame(sse, captor.getValue().sse());
+ producer.stop();
+ endpoint.stop();
+ }
+
+ @Test
+ void putObjectWithCustomerKey() throws Exception {
+ ServerSideEncryption.CustomerKey ssec = new
ServerSideEncryption.CustomerKey(secretKey);
+ MinioComponent component = context.getComponent("minio",
MinioComponent.class);
+ MinioEndpoint endpoint = (MinioEndpoint)
component.createEndpoint("minio://mybucket?autoCreateBucket=false");
+ endpoint.getConfiguration().setMinioClient(minioClient);
+ endpoint.getConfiguration().setServerSideEncryptionCustomerKey(ssec);
+
+
when(minioClient.putObject(any(PutObjectArgs.class))).thenReturn(mock(ObjectWriteResponse.class));
+
+ MinioProducer producer = createProducer(endpoint);
+ Exchange exchange = endpoint.createExchange();
+ exchange.getIn().setHeader(MinioConstants.OBJECT_NAME, "test.txt");
+ exchange.getIn().setBody("content");
+
+ producer.process(exchange);
+
+ ArgumentCaptor<PutObjectArgs> captor =
ArgumentCaptor.forClass(PutObjectArgs.class);
+ verify(minioClient).putObject(captor.capture());
+ assertSame(ssec, captor.getValue().sse());
Review Comment:
⚠️ **Pre-fix test validation**
The test uses a fully mocked `MinioClient`, which bypasses the actual
builder construction path. Confirm that `assertSame(ssec,
captor.getValue().sse())` in `putObjectWithCustomerKey` would **fail** without
the `checkIfConfigIsNotEmptyAndSetAndConfig` call in `doPutObject` — i.e., that
before the fix, `putObjectRequest.build()` would have `sse()` returning null.
If the mock captures whatever was set on the builder, the test is valid. But
if the builder's `build()` does defensive copying and the mock intercepts
before `sse` is applied, the assertion could pass vacuously.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
