Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,14 @@ public void provision(StackResource r, JsonNode props, ProvisionContext ctx) {
throw new AwsException("ValidationError",
"Updating RoleName requires resource replacement, which is not supported.", 400);
}
String assumeDoc = props != null && props.has("AssumeRolePolicyDocument")
? props.get("AssumeRolePolicyDocument").toString()
: "{\"Version\":\"2012-10-17\",\"Statement\":[]}";
String assumeDoc = props == null
? null
: ctx.engine().resolveJsonAttribute(props.path("AssumeRolePolicyDocument"));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Fn::If corrupts policy objects

When an AssumeRolePolicyDocument contains an Fn::If whose selected branch is an object or array, resolveJsonAttribute routes that branch through scalar resolution and replaces it with an empty string, causing the role to store a malformed trust policy.


if (assumeDoc == null) {
assumeDoc = "{\"Version\":\"2012-10-17\",\"Statement\":[]}";
}

String path = ctx.resolveOptional(props, "Path");
if (path == null) {
path = "/";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import java.util.Map;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
Expand Down Expand Up @@ -44,17 +45,21 @@ class IamRoleCfnProvisionerTest {

private ProvisionContext ctx() {
CloudFormationTemplateEngine engine = mock(CloudFormationTemplateEngine.class);

when(engine.resolve(any())).thenAnswer(inv -> {
JsonNode node = inv.getArgument(0);
return node == null ? null : node.asText();
JsonNode node = inv.getArgument(0);
return node == null ? null : node.asText();
});

when(engine.resolveNode(any())).thenAnswer(inv -> inv.getArgument(0));

when(engine.resolveJsonAttribute(any())).thenAnswer(inv -> {
JsonNode node = inv.getArgument(0);
return node != null && node.isTextual() ? node.asText() : node.toString();
JsonNode node = inv.getArgument(0);
return node != null && node.isTextual() ? node.asText() : node.toString();
});

return new ProvisionContext(engine, "us-east-1", ACCOUNT_ID, "test-stack");
}
}

private StackResource resource() {
StackResource r = new StackResource();
Expand Down Expand Up @@ -364,4 +369,84 @@ void deletePropagatesUnexpectedLookupFailures() {
assertEquals("AccessDenied", failure.getErrorCode());
verify(iam, never()).deleteRole("denied-role");
}
@Test
void assumeRolePolicyIntrinsicsAreResolved() {
Comment on lines +372 to +373

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Test indentation breaks convention

The new test annotation and method declaration are placed at column one rather than using the class-member indentation established throughout this file, while the adjacent ctx() helper also receives unrelated inconsistent indentation. Aligning these lines with the existing test structure keeps this focused fix readable and avoids formatting churn.

Context Used: AGENTS.md (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

stubCreate("app-role");
StackResource r = resource();

CloudFormationTemplateEngine engine = mock(CloudFormationTemplateEngine.class);

when(engine.resolve(any())).thenAnswer(inv -> {
JsonNode node = inv.getArgument(0);
return node == null ? null : node.asText();
});

JsonNode assumeRolePolicy = props("""
{
"Version": "2012-10-17",
"Statement": [{
"Effect": "Allow",
"Principal": {
"AWS": {
"Fn::Sub": "arn:aws:iam::${AWS::AccountId}:root"
}
},
"Action": "sts:AssumeRole"
}]
}
""");

when(engine.resolveJsonAttribute(any())).thenReturn(
"{\"Version\":\"2012-10-17\",\"Statement\":[{\"Effect\":\"Allow\","
+ "\"Principal\":{\"AWS\":\"arn:aws:iam::" + ACCOUNT_ID + ":root\"},"
+ "\"Action\":\"sts:AssumeRole\"}]}"
);

ProvisionContext ctx = new ProvisionContext(
engine,
"us-east-1",
ACCOUNT_ID,
"test-stack"
);

JsonNode roleProps = props("""
{
"RoleName": "app-role",
"AssumeRolePolicyDocument": {
"Version": "2012-10-17",
"Statement": [{
"Effect": "Allow",
"Principal": {
"AWS": {
"Fn::Sub": "arn:aws:iam::${AWS::AccountId}:root"
}
},
"Action": "sts:AssumeRole"
}]
}
}
""");

provisioner.provision(r, roleProps, ctx);

ArgumentCaptor<String> docCaptor = ArgumentCaptor.forClass(String.class);

verify(iam).createRole(
eq("app-role"),
eq("/"),
docCaptor.capture(),
any(),
eq(3600),
eq(Map.of())
);

String storedDoc = docCaptor.getValue();

assertFalse(storedDoc.contains("Fn::Sub"));
assertFalse(storedDoc.contains("\"Ref\""));
assertTrue(storedDoc.contains(
"arn:aws:iam::" + ACCOUNT_ID + ":root"
));
}

}
Loading