Skip to content

Commit b28f0cf

Browse files
SriramGuduriSriramGuduri
committed
address review comments
1 parent 36d8071 commit b28f0cf

File tree

7 files changed

+262
-70
lines changed

7 files changed

+262
-70
lines changed

‎examples/src/main/java/com/salesforce/multicloudj/iam/Main.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ public class Main {
3737
private static final String REGION = "us-central-1";
3838
private static final String TENANT_ID = "projects/substrate-sdk-gcp-poc1";
3939
private static final String SERVICE_ACCOUNT =
40-
"serviceAccount:chameleon@substrate-sdk-gcp-poc1.iam.gserviceaccount.com";
40+
"serviceAccount:multicloudjexample@substrate-sdk-gcp-poc1.iam.gserviceaccount.com";
4141

4242
// Demo settings
4343
private static final BufferedReader reader = new BufferedReader(new InputStreamReader(System.in));

‎iam/iam-aws/src/main/java/com/salesforce/multicloudj/iam/aws/AwsIam.java‎

Lines changed: 3 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
package com.salesforce.multicloudj.iam.aws;
22

3+
import static com.salesforce.multicloudj.iam.aws.AwsIamPolicyTranslator.translateToAwsPolicy;
4+
35
import com.fasterxml.jackson.core.JsonProcessingException;
46
import com.fasterxml.jackson.databind.JsonNode;
57
import com.fasterxml.jackson.databind.ObjectMapper;
@@ -355,7 +357,7 @@ protected void doAttachInlinePolicy(AttachInlinePolicyRequest request) {
355357
}
356358

357359
String roleName = request.getIdentityName();
358-
String policyDocumentJson = buildInlinePolicyDocumentJson(request.getPolicyDocument());
360+
String policyDocumentJson = translateToAwsPolicy(request.getPolicyDocument());
359361

360362
PutRolePolicyRequest awsRequest =
361363
PutRolePolicyRequest.builder()
@@ -367,47 +369,6 @@ protected void doAttachInlinePolicy(AttachInlinePolicyRequest request) {
367369
this.iamClient.putRolePolicy(awsRequest);
368370
}
369371

370-
private static String buildInlinePolicyDocumentJson(PolicyDocument policyDocument) {
371-
String version = policyDocument.getVersion();
372-
if (StringUtils.isBlank(version)) {
373-
version = POLICY_VERSION;
374-
}
375-
Map<String, Object> doc = new LinkedHashMap<>();
376-
doc.put("Version", version);
377-
378-
List<Map<String, Object>> awsStatements = new ArrayList<>();
379-
for (Statement stmt : policyDocument.getStatements()) {
380-
Map<String, Object> awsStmt = new LinkedHashMap<>();
381-
awsStmt.put("Effect", stmt.getEffect());
382-
383-
List<String> actions = stmt.getActionsAsStrings();
384-
if (actions != null && !actions.isEmpty()) {
385-
awsStmt.put("Action", actions);
386-
}
387-
if (StringUtils.isNotBlank(stmt.getSid())) {
388-
awsStmt.put("Sid", stmt.getSid());
389-
}
390-
if (stmt.getResources() != null && !stmt.getResources().isEmpty()) {
391-
awsStmt.put("Resource", stmt.getResources());
392-
}
393-
if (stmt.getConditions() != null && !stmt.getConditions().isEmpty()) {
394-
awsStmt.put("Condition", stmt.getConditions());
395-
}
396-
if (stmt.getPrincipals() != null && !stmt.getPrincipals().isEmpty()) {
397-
awsStmt.put("Principal", stmt.getPrincipals());
398-
}
399-
400-
awsStatements.add(awsStmt);
401-
}
402-
doc.put("Statement", awsStatements);
403-
404-
try {
405-
return OBJECT_MAPPER.writeValueAsString(doc);
406-
} catch (JsonProcessingException e) {
407-
throw new InvalidArgumentException("Failed to serialize inline policy document", e);
408-
}
409-
}
410-
411372
/**
412373
* Get inline policy document attached to an IAM role.
413374
*

‎iam/iam-aws/src/main/java/com/salesforce/multicloudj/iam/aws/AwsIamPolicyTranslator.java‎

Lines changed: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import com.fasterxml.jackson.core.JsonProcessingException;
44
import com.fasterxml.jackson.databind.ObjectMapper;
5+
import com.salesforce.multicloudj.common.exceptions.InvalidArgumentException;
56
import com.salesforce.multicloudj.common.exceptions.SubstrateSdkException;
67
import com.salesforce.multicloudj.iam.model.Action;
78
import com.salesforce.multicloudj.iam.model.ComputeActions;
@@ -15,6 +16,7 @@
1516
import java.util.List;
1617
import java.util.Map;
1718
import java.util.stream.Collectors;
19+
import org.apache.commons.lang3.StringUtils;
1820

1921
/**
2022
* Translates substrate-neutral PolicyDocument to AWS IAM policy format.
@@ -89,6 +91,7 @@ public class AwsIamPolicyTranslator {
8991
Map.entry(ConditionOperator.BOOL, "Bool"),
9092
Map.entry(ConditionOperator.IP_ADDRESS, "IpAddress"),
9193
Map.entry(ConditionOperator.NOT_IP_ADDRESS, "NotIpAddress"));
94+
public static final String DEFAULT_VERSION = "2012-10-17";
9295

9396
/**
9497
* Translates a substrate-neutral PolicyDocument to AWS IAM policy JSON string.
@@ -99,7 +102,12 @@ public class AwsIamPolicyTranslator {
99102
*/
100103
public static String translateToAwsPolicy(PolicyDocument policyDocument) {
101104
Map<String, Object> awsPolicy = new LinkedHashMap<>();
102-
awsPolicy.put("Version", policyDocument.getVersion());
105+
// Default to AWS IAM policy version if not provided
106+
String version =
107+
StringUtils.isNotBlank(policyDocument.getVersion())
108+
? policyDocument.getVersion()
109+
: DEFAULT_VERSION;
110+
awsPolicy.put("Version", version);
103111

104112
List<Map<String, Object>> awsStatements = new ArrayList<>();
105113
for (Statement statement : policyDocument.getStatements()) {
@@ -122,19 +130,25 @@ public static String translateToAwsPolicy(PolicyDocument policyDocument) {
122130
* @throws SubstrateSdkException if translation fails
123131
*/
124132
private static Map<String, Object> translateStatement(Statement statement) {
133+
if (statement.getEffect() == null) {
134+
throw new InvalidArgumentException("Effect is required for AWS IAM policy statement");
135+
}
136+
125137
Map<String, Object> awsStatement = new LinkedHashMap<>();
126138

127139
// Add Sid if present
128-
if (statement.getSid() != null && !statement.getSid().isEmpty()) {
129-
awsStatement.put("Sid", statement.getSid());
140+
String sid = statement.getSid();
141+
if (StringUtils.isNotBlank(sid)) {
142+
awsStatement.put("Sid", sid);
130143
}
131144

132145
// Add Effect
133146
awsStatement.put("Effect", statement.getEffect().getValue());
134147

135148
// Translate and add Principals if present
136-
if (statement.getPrincipals() != null && !statement.getPrincipals().isEmpty()) {
137-
awsStatement.put("Principal", translatePrincipals(statement.getPrincipals()));
149+
List<String> principals = statement.getPrincipals();
150+
if (principals != null && !principals.isEmpty()) {
151+
awsStatement.put("Principal", translatePrincipals(principals));
138152
}
139153

140154
// Translate and add Actions
@@ -191,7 +205,7 @@ private static String translateAction(Action action) {
191205
case "iam":
192206
return "iam:*";
193207
default:
194-
throw new SubstrateSdkException(
208+
throw new InvalidArgumentException(
195209
"Unknown substrate-neutral service for wildcard action: "
196210
+ action.toActionString()
197211
+ ". "
@@ -202,7 +216,7 @@ private static String translateAction(Action action) {
202216
// Handle specific actions
203217
String awsAction = ACTION_MAPPINGS.get(action);
204218
if (awsAction == null) {
205-
throw new SubstrateSdkException(
219+
throw new InvalidArgumentException(
206220
"Unknown substrate-neutral action: "
207221
+ action.toActionString()
208222
+ ". "
@@ -239,7 +253,7 @@ private static String translateResource(String resource) {
239253
return resource;
240254
}
241255

242-
throw new SubstrateSdkException(
256+
throw new InvalidArgumentException(
243257
"Unknown resource format: "
244258
+ resource
245259
+ ". "
@@ -292,7 +306,7 @@ private static Map<String, Map<String, Object>> translateConditions(
292306
String awsOperator = CONDITION_MAPPINGS.get(operator);
293307

294308
if (awsOperator == null) {
295-
throw new SubstrateSdkException(
309+
throw new InvalidArgumentException(
296310
"Unsupported condition operator: "
297311
+ operator.getValue()
298312
+ ". "

‎iam/iam-aws/src/test/java/com/salesforce/multicloudj/iam/aws/AwsIamPolicyTranslatorTest.java‎

Lines changed: 75 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,12 @@
66
import static org.junit.jupiter.api.Assertions.assertNull;
77
import static org.junit.jupiter.api.Assertions.assertThrows;
88
import static org.junit.jupiter.api.Assertions.assertTrue;
9+
import static org.mockito.Mockito.mock;
10+
import static org.mockito.Mockito.when;
911

1012
import com.fasterxml.jackson.databind.JsonNode;
1113
import com.fasterxml.jackson.databind.ObjectMapper;
14+
import com.salesforce.multicloudj.common.exceptions.InvalidArgumentException;
1215
import com.salesforce.multicloudj.common.exceptions.SubstrateSdkException;
1316
import com.salesforce.multicloudj.iam.model.Action;
1417
import com.salesforce.multicloudj.iam.model.ComputeActions;
@@ -317,9 +320,9 @@ void testTranslateUnknownActionThrowsException() {
317320
.build())
318321
.build();
319322

320-
SubstrateSdkException exception =
323+
InvalidArgumentException exception =
321324
assertThrows(
322-
SubstrateSdkException.class,
325+
InvalidArgumentException.class,
323326
() -> {
324327
AwsIamPolicyTranslator.translateToAwsPolicy(policy);
325328
});
@@ -340,9 +343,9 @@ void testTranslateInvalidResourceFormatThrowsException() {
340343
.build())
341344
.build();
342345

343-
SubstrateSdkException exception =
346+
InvalidArgumentException exception =
344347
assertThrows(
345-
SubstrateSdkException.class,
348+
InvalidArgumentException.class,
346349
() -> {
347350
AwsIamPolicyTranslator.translateToAwsPolicy(policy);
348351
});
@@ -534,9 +537,9 @@ void testTranslateUnknownWildcardServiceThrowsException() {
534537
.build())
535538
.build();
536539

537-
SubstrateSdkException exception =
540+
InvalidArgumentException exception =
538541
assertThrows(
539-
SubstrateSdkException.class,
542+
InvalidArgumentException.class,
540543
() -> {
541544
AwsIamPolicyTranslator.translateToAwsPolicy(policy);
542545
});
@@ -575,4 +578,70 @@ void testTranslateMultipleWildcardActions() throws Exception {
575578
assertEquals("ec2:*", actions.get(1).asText());
576579
assertEquals("iam:*", actions.get(2).asText());
577580
}
581+
582+
@Test
583+
void testTranslateWithNullVersionDefaultsToAwsVersion() throws Exception {
584+
PolicyDocument policy =
585+
PolicyDocument.builder()
586+
.statement(
587+
Statement.builder().effect(Effect.ALLOW).action(StorageActions.GET_OBJECT).build())
588+
.build();
589+
590+
String awsPolicy = AwsIamPolicyTranslator.translateToAwsPolicy(policy);
591+
JsonNode policyJson = OBJECT_MAPPER.readTree(awsPolicy);
592+
593+
assertEquals("2012-10-17", policyJson.get("Version").asText());
594+
}
595+
596+
@Test
597+
void testTranslateWithBlankVersionDefaultsToAwsVersion() throws Exception {
598+
PolicyDocument policy =
599+
PolicyDocument.builder()
600+
.version("")
601+
.statement(
602+
Statement.builder().effect(Effect.ALLOW).action(StorageActions.GET_OBJECT).build())
603+
.build();
604+
605+
String awsPolicy = AwsIamPolicyTranslator.translateToAwsPolicy(policy);
606+
JsonNode policyJson = OBJECT_MAPPER.readTree(awsPolicy);
607+
608+
assertEquals("2012-10-17", policyJson.get("Version").asText());
609+
}
610+
611+
@Test
612+
void testTranslateWithCustomVersionPreservesIt() throws Exception {
613+
PolicyDocument policy =
614+
PolicyDocument.builder()
615+
.version("2008-10-17")
616+
.statement(
617+
Statement.builder().effect(Effect.ALLOW).action(StorageActions.GET_OBJECT).build())
618+
.build();
619+
620+
String awsPolicy = AwsIamPolicyTranslator.translateToAwsPolicy(policy);
621+
JsonNode policyJson = OBJECT_MAPPER.readTree(awsPolicy);
622+
623+
assertEquals("2008-10-17", policyJson.get("Version").asText());
624+
}
625+
626+
@Test
627+
void testTranslateNullEffectThrowsException() {
628+
Statement mockStatement = mock(Statement.class);
629+
when(mockStatement.getEffect()).thenReturn(null);
630+
when(mockStatement.getActions())
631+
.thenReturn(java.util.Collections.singletonList(StorageActions.GET_OBJECT));
632+
633+
PolicyDocument policy =
634+
PolicyDocument.builder()
635+
.statement(mockStatement)
636+
.build();
637+
638+
InvalidArgumentException exception =
639+
assertThrows(
640+
InvalidArgumentException.class,
641+
() -> {
642+
AwsIamPolicyTranslator.translateToAwsPolicy(policy);
643+
});
644+
645+
assertEquals("Effect is required for AWS IAM policy statement", exception.getMessage());
646+
}
578647
}

0 commit comments

Comments
 (0)