refactor(iam): split 8,018-line iam_service.rs into sub-modules#283
Merged
vieiralucas merged 2 commits intomainfrom Apr 12, 2026
Merged
refactor(iam): split 8,018-line iam_service.rs into sub-modules#283vieiralucas merged 2 commits intomainfrom
vieiralucas merged 2 commits intomainfrom
Conversation
Split monolithic iam_service.rs into 8 files following the SSM pattern: - mod.rs: struct, dispatch, helpers, tests (2,326 lines) - users.rs: user CRUD, access keys, login profiles (1,565 lines) - account.rs: account summary, aliases, password policy (1,043 lines) - roles.rs: role CRUD, assume role policy (945 lines) - oidc.rs: OIDC/SAML providers, server certificates (773 lines) - policies.rs: managed policy CRUD, versions (657 lines) - groups.rs: group CRUD, membership (605 lines) - instance_profiles.rs: instance profile CRUD (425 lines)
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
8 issues found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/fakecloud-iam/src/iam_service/groups.rs">
<violation number="1" location="crates/fakecloud-iam/src/iam_service/groups.rs:511">
P2: `AttachGroupPolicy` is missing policy-existence validation, so nonexistent custom policy ARNs can be attached successfully.</violation>
</file>
<file name="crates/fakecloud-iam/src/iam_service/oidc.rs">
<violation number="1" location="crates/fakecloud-iam/src/iam_service/oidc.rs:50">
P1: `CreateSAMLProvider` overwrites an existing provider instead of rejecting duplicates.</violation>
<violation number="2" location="crates/fakecloud-iam/src/iam_service/oidc.rs:346">
P2: Escape client IDs before embedding them in XML to avoid malformed responses.</violation>
</file>
<file name="crates/fakecloud-iam/src/iam_service/users.rs">
<violation number="1" location="crates/fakecloud-iam/src/iam_service/users.rs:263">
P2: Partition is hardcoded to `"aws"` here, but `create_user` derives it from `partition_for_region(&req.region)`. Updating a user's path or name will silently rewrite the ARN partition.</violation>
<violation number="2" location="crates/fakecloud-iam/src/iam_service/users.rs:287">
P1: `signing_certificates` and `ssh_public_keys` are not re-keyed when a user is renamed, so they become orphaned and inaccessible after the rename. Add the same remove-and-reinsert pattern used for the other maps.</violation>
</file>
<file name="crates/fakecloud-iam/src/iam_service/policies.rs">
<violation number="1" location="crates/fakecloud-iam/src/iam_service/policies.rs:334">
P2: `xml_escape` is used here to encode the policy document, but `get_policy_version` and `list_policy_versions` use `url_encode` for the same `<Document>` field. This inconsistency means CreatePolicyVersion returns an XML-entity-escaped document while the other endpoints return a percent-encoded one. Use `url_encode` here to match the other policy version responses (and real AWS behavior).</violation>
</file>
<file name="crates/fakecloud-iam/src/iam_service/roles.rs">
<violation number="1" location="crates/fakecloud-iam/src/iam_service/roles.rs:603">
P2: Pre-existing bug (non-blocking note): AWS managed policies attached via `attach_role_policy` are silently omitted from the listing. The `attach` path allows ARNs matching `:aws:policy/` to bypass the `state.policies` existence check, but this listing resolves every ARN through `state.policies.get(arn)`, so AWS managed policies are always filtered out. A fix would be to fall back to synthesizing a `PolicyName`/`PolicyArn` entry from the ARN itself when the policy isn't found in state.</violation>
</file>
<file name="crates/fakecloud-iam/src/iam_service/mod.rs">
<violation number="1" location="crates/fakecloud-iam/src/iam_service/mod.rs:592">
P2: Tag-key duplicate check uses case-insensitive comparison, but AWS IAM tag keys are case-sensitive. `"Env"` and `"env"` should be treated as distinct keys. Use `tag.key.clone()` instead of `tag.key.to_lowercase()`, and update the error message accordingly.
(Based on your team's feedback about treating tag keys as case-sensitive.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| arn, req.request_id | ||
| ); | ||
|
|
||
| state.saml_providers.insert(arn, provider); |
There was a problem hiding this comment.
P1: CreateSAMLProvider overwrites an existing provider instead of rejecting duplicates.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/fakecloud-iam/src/iam_service/oidc.rs, line 50:
<comment>`CreateSAMLProvider` overwrites an existing provider instead of rejecting duplicates.</comment>
<file context>
@@ -0,0 +1,773 @@
+ arn, req.request_id
+ );
+
+ state.saml_providers.insert(arn, provider);
+ Ok(AwsResponse::xml(StatusCode::OK, xml))
+ }
</file context>
Comment on lines
+287
to
+290
| if let Some(profile) = state.login_profiles.remove(&user_name) { | ||
| state | ||
| .login_profiles | ||
| .insert(actual_new_name.clone(), profile); |
There was a problem hiding this comment.
P1: signing_certificates and ssh_public_keys are not re-keyed when a user is renamed, so they become orphaned and inaccessible after the rename. Add the same remove-and-reinsert pattern used for the other maps.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/fakecloud-iam/src/iam_service/users.rs, line 287:
<comment>`signing_certificates` and `ssh_public_keys` are not re-keyed when a user is renamed, so they become orphaned and inaccessible after the rename. Add the same remove-and-reinsert pattern used for the other maps.</comment>
<file context>
@@ -0,0 +1,1565 @@
+ .user_inline_policies
+ .insert(actual_new_name.clone(), policies);
+ }
+ if let Some(profile) = state.login_profiles.remove(&user_name) {
+ state
+ .login_profiles
</file context>
Suggested change
| if let Some(profile) = state.login_profiles.remove(&user_name) { | |
| state | |
| .login_profiles | |
| .insert(actual_new_name.clone(), profile); | |
| if let Some(profile) = state.login_profiles.remove(&user_name) { | |
| state | |
| .login_profiles | |
| .insert(actual_new_name.clone(), profile); | |
| } | |
| if let Some(certs) = state.signing_certificates.remove(&user_name) { | |
| state | |
| .signing_certificates | |
| .insert(actual_new_name.clone(), certs); | |
| } | |
| if let Some(keys) = state.ssh_public_keys.remove(&user_name) { | |
| state | |
| .ssh_public_keys | |
| .insert(actual_new_name.clone(), keys); | |
| } |
| ) | ||
| })?; | ||
|
|
||
| if !group.attached_policies.contains(&policy_arn) { |
There was a problem hiding this comment.
P2: AttachGroupPolicy is missing policy-existence validation, so nonexistent custom policy ARNs can be attached successfully.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/fakecloud-iam/src/iam_service/groups.rs, line 511:
<comment>`AttachGroupPolicy` is missing policy-existence validation, so nonexistent custom policy ARNs can be attached successfully.</comment>
<file context>
@@ -0,0 +1,605 @@
+ )
+ })?;
+
+ if !group.attached_policies.contains(&policy_arn) {
+ group.attached_policies.push(policy_arn.clone());
+ if let Some(p) = state.policies.get_mut(&policy_arn) {
</file context>
| let client_ids: String = provider | ||
| .client_id_list | ||
| .iter() | ||
| .map(|id| format!(" <member>{id}</member>")) |
There was a problem hiding this comment.
P2: Escape client IDs before embedding them in XML to avoid malformed responses.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/fakecloud-iam/src/iam_service/oidc.rs, line 346:
<comment>Escape client IDs before embedding them in XML to avoid malformed responses.</comment>
<file context>
@@ -0,0 +1,773 @@
+ let client_ids: String = provider
+ .client_id_list
+ .iter()
+ .map(|id| format!(" <member>{id}</member>"))
+ .collect::<Vec<_>>()
+ .join("\n");
</file context>
| let actual_new_name = new_user_name.unwrap_or_else(|| user_name.clone()); | ||
| user.user_name = actual_new_name.clone(); | ||
| user.arn = format!( | ||
| "arn:aws:iam::{}:user{}{}", |
There was a problem hiding this comment.
P2: Partition is hardcoded to "aws" here, but create_user derives it from partition_for_region(&req.region). Updating a user's path or name will silently rewrite the ARN partition.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/fakecloud-iam/src/iam_service/users.rs, line 263:
<comment>Partition is hardcoded to `"aws"` here, but `create_user` derives it from `partition_for_region(&req.region)`. Updating a user's path or name will silently rewrite the ARN partition.</comment>
<file context>
@@ -0,0 +1,1565 @@
+ let actual_new_name = new_user_name.unwrap_or_else(|| user_name.clone());
+ user.user_name = actual_new_name.clone();
+ user.arn = format!(
+ "arn:aws:iam::{}:user{}{}",
+ state.account_id,
+ if user.path == "/" { "/" } else { &user.path },
</file context>
Suggested change
| "arn:aws:iam::{}:user{}{}", | |
| "arn:{}:iam::{}:user{}{}", | |
| partition_for_region(&req.region), |
| </CreatePolicyVersionResponse>"#, | ||
| version.version_id, | ||
| version.is_default, | ||
| xml_escape(&version.document), |
There was a problem hiding this comment.
P2: xml_escape is used here to encode the policy document, but get_policy_version and list_policy_versions use url_encode for the same <Document> field. This inconsistency means CreatePolicyVersion returns an XML-entity-escaped document while the other endpoints return a percent-encoded one. Use url_encode here to match the other policy version responses (and real AWS behavior).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/fakecloud-iam/src/iam_service/policies.rs, line 334:
<comment>`xml_escape` is used here to encode the policy document, but `get_policy_version` and `list_policy_versions` use `url_encode` for the same `<Document>` field. This inconsistency means CreatePolicyVersion returns an XML-entity-escaped document while the other endpoints return a percent-encoded one. Use `url_encode` here to match the other policy version responses (and real AWS behavior).</comment>
<file context>
@@ -0,0 +1,657 @@
+</CreatePolicyVersionResponse>"#,
+ version.version_id,
+ version.is_default,
+ xml_escape(&version.document),
+ version.created_at.format("%Y-%m-%dT%H:%M:%SZ"),
+ req.request_id
</file context>
|
|
||
| let members: String = policy_arns | ||
| .iter() | ||
| .filter_map(|arn| { |
There was a problem hiding this comment.
P2: Pre-existing bug (non-blocking note): AWS managed policies attached via attach_role_policy are silently omitted from the listing. The attach path allows ARNs matching :aws:policy/ to bypass the state.policies existence check, but this listing resolves every ARN through state.policies.get(arn), so AWS managed policies are always filtered out. A fix would be to fall back to synthesizing a PolicyName/PolicyArn entry from the ARN itself when the policy isn't found in state.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/fakecloud-iam/src/iam_service/roles.rs, line 603:
<comment>Pre-existing bug (non-blocking note): AWS managed policies attached via `attach_role_policy` are silently omitted from the listing. The `attach` path allows ARNs matching `:aws:policy/` to bypass the `state.policies` existence check, but this listing resolves every ARN through `state.policies.get(arn)`, so AWS managed policies are always filtered out. A fix would be to fall back to synthesizing a `PolicyName`/`PolicyArn` entry from the ARN itself when the policy isn't found in state.</comment>
<file context>
@@ -0,0 +1,945 @@
+
+ let members: String = policy_arns
+ .iter()
+ .filter_map(|arn| {
+ state.policies.get(arn).map(|p| {
+ format!(
</file context>
| // Check for duplicate keys | ||
| let mut seen_keys = std::collections::HashSet::new(); | ||
| for tag in tags { | ||
| let lower = tag.key.to_lowercase(); |
There was a problem hiding this comment.
P2: Tag-key duplicate check uses case-insensitive comparison, but AWS IAM tag keys are case-sensitive. "Env" and "env" should be treated as distinct keys. Use tag.key.clone() instead of tag.key.to_lowercase(), and update the error message accordingly.
(Based on your team's feedback about treating tag keys as case-sensitive.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/fakecloud-iam/src/iam_service/mod.rs, line 592:
<comment>Tag-key duplicate check uses case-insensitive comparison, but AWS IAM tag keys are case-sensitive. `"Env"` and `"env"` should be treated as distinct keys. Use `tag.key.clone()` instead of `tag.key.to_lowercase()`, and update the error message accordingly.
(Based on your team's feedback about treating tag keys as case-sensitive.) </comment>
<file context>
@@ -0,0 +1,2326 @@
+ // Check for duplicate keys
+ let mut seen_keys = std::collections::HashSet::new();
+ for tag in tags {
+ let lower = tag.key.to_lowercase();
+ if !seen_keys.insert(lower) {
+ return Err(AwsServiceError::aws_error(
</file context>
Suggested change
| let lower = tag.key.to_lowercase(); | |
| let lower = tag.key.clone(); |
The conformance audit scanner now tries both `<service>/mod.rs` and `<service>.rs` paths so it works whether a service is a single file or has been split into a sub-module directory.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/fakecloud-conformance/src/audit.rs">
<violation number="1" location="crates/fakecloud-conformance/src/audit.rs:15">
P3: Adding fallback source paths without changing existence handling causes noisy false warnings for expected-missing alternate files.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| ("sns", "sns", &["service.rs"]), | ||
| ("events", "eventbridge", &["service.rs"]), | ||
| ("iam", "iam", &["iam_service.rs"]), | ||
| ("iam", "iam", &["iam_service/mod.rs", "iam_service.rs"]), |
There was a problem hiding this comment.
P3: Adding fallback source paths without changing existence handling causes noisy false warnings for expected-missing alternate files.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/fakecloud-conformance/src/audit.rs, line 15:
<comment>Adding fallback source paths without changing existence handling causes noisy false warnings for expected-missing alternate files.</comment>
<file context>
@@ -6,22 +6,24 @@ use std::path::{Path, PathBuf};
("sns", "sns", &["service.rs"]),
("events", "eventbridge", &["service.rs"]),
- ("iam", "iam", &["iam_service.rs"]),
+ ("iam", "iam", &["iam_service/mod.rs", "iam_service.rs"]),
("sts", "iam", &["sts_service.rs"]),
- ("ssm", "ssm", &["service.rs"]),
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
iam_service.rsinto 8 focused sub-modulesmod.rs(2,326 lines): struct, dispatch, helpers, testsusers.rs(1,565 lines): user CRUD, access keys, login profilesaccount.rs(1,043 lines): account summary, aliases, password policyroles.rs(945 lines): role CRUD, assume role policyoidc.rs(773 lines): OIDC/SAML providers, server certificatespolicies.rs(657 lines): managed policy CRUD, versionsgroups.rs(605 lines): group CRUD, membershipinstance_profiles.rs(425 lines): instance profile operationsTest plan
cargo clippy --workspace --all-targets -- -D warningspasses cleanSummary by cubic
Split the 8,018-line
iam_service.rsinto focused submodules underiam_serviceto improve readability and maintainability. Updated the conformance audit scanner to detect split services; no IAM API behavior changes.Refactors
mod.rs,users.rs,account.rs,roles.rs,oidc.rs,policies.rs,groups.rs,instance_profiles.rs.mod.rs; moved resource-specific CRUD and tagging into dedicated files.cargo clippy(no warnings).Bug Fixes
src/<service>/mod.rsandsrc/<service>.rs(includingiam_service/mod.rs) so audits work for split services.Written for commit d427588. Summary will update on new commits.