fix(iam): include policy description in GetPolicy/ListPolicies/CreatePolicy responses - #2155
Conversation
…Policy responses policyXml() built the response for CreatePolicy, GetPolicy, and ListPolicies but never emitted the Description element, even though IamPolicy stores it correctly and the sibling roleXml() includes it for roles. Callers (e.g. Terraform's AWS provider) that set a policy description see it as permanently missing on every read, which forces destroy+recreate since IAM policy descriptions are immutable. Fixes floci-io#2154
|
| Filename | Overview |
|---|---|
| src/main/java/io/github/hectorvent/floci/services/iam/IamQueryHandler.java | Serializes policy descriptions for CreatePolicy and GetPolicy while explicitly omitting them from ListPolicies. |
| src/test/java/io/github/hectorvent/floci/services/iam/IamIntegrationTest.java | Verifies description inclusion for CreatePolicy/GetPolicy and element-level omission for ListPolicies. |
Reviews (3): Last reviewed commit: "fix(iam): make listPoliciesOmitsDescript..." | Re-trigger Greptile
There was a problem hiding this comment.
Pull request overview
This PR fixes an IAM Query protocol compatibility gap by including the managed policy Description field in CreatePolicy, GetPolicy, and ListPolicies XML responses, aligning Floci’s output with AWS IAM behavior and preventing downstream drift (e.g., Terraform recreations).
Changes:
- Add
<Description>emission to the sharedpolicyXml()serializer used by multiple IAM policy response paths. - Extend existing IAM integration tests to assert that
Descriptionis returned forCreatePolicyandGetPolicy.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/main/java/io/github/hectorvent/floci/services/iam/IamQueryHandler.java |
Adds Description to the IAM policy XML serialization helper used by Create/Get/List policy responses. |
src/test/java/io/github/hectorvent/floci/services/iam/IamIntegrationTest.java |
Adds response assertions ensuring Description is present for CreatePolicy and GetPolicy integration flows. |
pgermosen
left a comment
There was a problem hiding this comment.
Reviewed in a local worktree. IAM suites pass: 129/129, 0 failures (IamIntegrationTest 49/49, IamServiceTest 60/60, IamManagedPolicyAccountScopeTest 7/7, IamConcurrencyTest 13/13) — matching every count in the description.
The bug is real, and the diagnosis is exactly right. GetPolicy must return Description; AWS's own sample response has it:
<Policy>
<PolicyName>S3-read-only-example-bucket</PolicyName>
<Description>Allows read-only access to the example bucket</Description>
...The Terraform reasoning holds too — policy descriptions are immutable, so a permanently-absent description forces destroy+recreate on every apply.
But the one-line fix lands in a helper shared with ListPolicies, and AWS documents that ListPolicies must not return Description. The model's Policy.Description member doc states it directly: "This element is included in the response to the GetPolicy operation. It is not included in the response to the ListPolicies operation." So the change fixes one divergence and introduces another — details inline.
Verified rather than assumed:
XmlBuilder.elem()really does skip nulls, so the "strict superset" claim is accurate for the two operations where the element belongs.- The three call sites are
CreatePolicy(:453),GetPolicy(:459) andListPolicies(:473), the last inside the<member>loop. CreatePolicyis fine. AWS documents neither inclusion nor exclusion, and its sample response omitsDescriptiononly because the sample request never set one.- The model and the API reference agree, so this isn't a stale-doc artifact.
** The GetPolicy half is right and worth merging — it just needs to not apply to ListPolicies. It's a small fix, and the second comment covers the test that would keep it fixed.
policyXml() is shared by CreatePolicy, GetPolicy, and ListPolicies — the previous fix added Description to all three, but AWS's own Policy model documents that Description "is included in the response to the GetPolicy operation. It is not included in the response to the ListPolicies operation." CreatePolicy documents neither inclusion nor exclusion, so it's treated the same as GetPolicy. policyXml() now takes an explicit includeDescription flag: true at the CreatePolicy/GetPolicy call sites, false in ListPolicies' member loop. Adds a ListPolicies test asserting no member response ever carries a Description element, so folding it back into the shared helper unconditionally — the exact way this would regress — fails loudly. Addresses review feedback from @pgermosen.
|
Thanks for the thorough review, @pgermosen — you're right, and I verified it directly against AWS's own Pushed a fix: All 130 IAM tests pass locally (IamIntegrationTest 50/50, IamServiceTest 60/60, IamManagedPolicyAccountScopeTest 7/7, IamConcurrencyTest 13/13). |
pgermosen
left a comment
There was a problem hiding this comment.
Looks good, just a minor thing
Both findings are fixed correctly and for the stated reason.
For the record, the resolved blocker was worth the round trip: with 58 seeded AWS-managed policies, each carrying a description, a plain aws iam list-policies would have returned 58+ members with a field real AWS omits — the same class of divergence this PR set out to remove, in the opposite direction.
…he word Match "<Description>" rather than the bare word "Description" to avoid a false failure if a future policy's name/path happens to contain that substring, and assert the created policy is actually present in the response first, so the absence check can't pass vacuously against an empty (or wrongly-filtered) member list.
|
Good catch — fixed. Both halves of the reasoning are addressed: added All 130 IAM tests still pass locally ( |
Fixes #2154.
Bug
GetPolicy,ListPolicies, andCreatePolicyall built their response via the sharedpolicyXml()helper inIamQueryHandler.java, which never emitted aDescriptionelement —even when the policy was created with one, and even though the sibling
roleXml()(right aboveit) does emit
Descriptionfor roles.IamPolicy(the model) andIamService.createPolicy()both handle the field correctly; it was only dropped on the way out to the client.
Real AWS's
GetPolicyreturnsDescription(see theAPI docs's own sample
response). Consumers that diff against real AWS behavior — e.g. Terraform's AWS provider — see the
description as permanently missing on every read, and since IAM policy descriptions are immutable
(force-new on the AWS provider), this forces a destroy+recreate of the policy on every apply, even
when nothing in the underlying config changed.
Fix
One line: add
.elem("Description", p.getDescription())topolicyXml(), matching the existingpattern in
roleXml().XmlBuilder.elem()already skips the element when the value isnull, sothis is a strict superset of the previous output — no behavior change for policies without one.
Tests
Added
Descriptionassertions to the existingcreatePolicy()andgetPolicy()integrationtests in
IamIntegrationTest— both already pass aDescriptiononCreatePolicybut weren'tasserting on it in the response, so they didn't catch this. Ran locally (Java 25):