Skip to content

Add permission set -> iam policy relationship - #134

Merged
ggreer merged 1 commit into
mainfrom
ggreer/policy-permission-set
Jul 14, 2026
Merged

Add permission set -> iam policy relationship#134
ggreer merged 1 commit into
mainfrom
ggreer/policy-permission-set

Conversation

@ggreer

@ggreer ggreer commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@ggreer
ggreer requested a review from a team July 14, 2026 22:11

resp, err := o.ssoAdminClient.ListManagedPoliciesInPermissionSet(ctx, input)
if err != nil {
return nil, nil, wrapAWSError(fmt.Errorf("baton-aws: ssoadmin.ListManagedPoliciesInPermissionSet failed: %w", err))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Suggestion: This new sso:ListManagedPoliciesInPermissionSet call (and GetInlinePolicyForPermissionSet in inline_policy.go) fails the whole sync on AccessDenied. The existing IAM inline-policy path degrades gracefully (see getInlinePolicyDocument's isAccessDeniedError handling). Existing permission-set-opted-in installs that haven't added the two new SSO scopes will now hard-fail rather than skip. Consider matching the graceful-skip pattern for these new scopes. (medium confidence)

@github-actions

Copy link
Copy Markdown
Contributor

Connector PR Review: Add permission set -> iam policy relationship

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 805e8bf9140c.
Review mode: full
View review run

Review Summary

The full PR diff was scanned for security and correctness. This PR makes the permission_set resource emit its managed-policy composition as structural grants against the iam_policy "attached" entitlement, adds Identity Center inline-policy support under the inline_policy builder (with a nil-safe permission-set branch), and flips the annotation from SkipEntitlementsAndGrants to SkipEntitlements so Grants now runs. Nil-safety of the new ssoAdminClient/identityInstance fields holds because those builders only run the permission-set path when SSO+orgs are enabled; pagination, entity-source direction, idempotent Revoke (Unimplemented for permission sets, InvalidArgument default), and test coverage all look correct. No blocking issues found.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

  • pkg/connector/permission_set.go:125 (and pkg/connector/inline_policy.go:157) — new sso:ListManagedPoliciesInPermissionSet / sso:GetInlinePolicyForPermissionSet calls don't degrade gracefully on AccessDenied, unlike the existing IAM inline-policy path; opted-in installs missing the new scopes will hard-fail the sync.
Prompt for AI agents
Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/connector/permission_set.go`:
- Around line 123-126: The ListManagedPoliciesInPermissionSet call returns the
  wrapped error on any failure, so a missing sso:ListManagedPoliciesInPermissionSet
  scope hard-fails the permission_set Grants phase. Consider detecting AccessDenied
  (via the existing isAccessDeniedError helper), logging a Warn, and returning
  empty grants so the sync degrades gracefully — matching the pattern already used in
  inline_policy.go's getInlinePolicyDocument.

In `pkg/connector/inline_policy.go`:
- Around line 151-158: listPermissionSetInlinePolicy returns the wrapped error from
  GetInlinePolicyForPermissionSet on any failure. Apply the same graceful AccessDenied
  handling as above so a missing sso:GetInlinePolicyForPermissionSet scope skips the
  inline policy rather than failing the whole List.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No blocking issues found.

@ggreer
ggreer force-pushed the ggreer/policy-permission-set branch from d3f483f to 3f29976 Compare July 14, 2026 22:26
@ggreer
ggreer force-pushed the ggreer/policy-permission-set branch from 3f29976 to 6f12bd1 Compare July 14, 2026 22:27
@github-actions

Copy link
Copy Markdown
Contributor

Connector PR Review: Add permission set -> iam policy relationship

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 805e8bf9140c.
Review mode: full
View review run

Review Summary

Scanned the full PR diff for security and correctness. This change adds two Identity Center relationships: permission-set Grants now emit the managed-policy composition against the iam_policy attached entitlement, and the inline-policy builder gains a permission-set parent branch backed by GetInlinePolicyForPermissionSet. Both new SSO calls degrade gracefully on ResourceNotFound and AccessDenied (Warn plus return nil), which resolves the prior review concern about opted-in installs hard-failing on missing scopes. Nil-safety of identityInstance holds because the permission-set builder, and thus any permission-set parent, is only registered under orgsEnabled and ssoEnabled. Pagination in permissionSetResourceType.Grants passes the raw AWS NextToken through and guards against empty tokens, consistent with permission_set_assignment.go. Tests cover both new paths, including pagination and the empty-document case.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

  • docs/connector.mdx — the required-permissions IAM policy JSON does not list the two new scopes (sso:ListManagedPoliciesInPermissionSet, sso:GetInlinePolicyForPermissionSet). Customers who opt in will silently get empty results (graceful AccessDenied) until their IAM policy is updated.
Prompt for AI agents
Verify each finding against the current code and only fix it if needed.

## Suggestions

In docs/connector.mdx:
- The required-permissions IAM policy JSON blocks (around lines 348-352 and 725-729) list
  the sso:* scopes the connector needs but omit the two new scopes added in this PR:
  sso:ListManagedPoliciesInPermissionSet and sso:GetInlinePolicyForPermissionSet. Add both
  scopes to the SSO permission blocks so opted-in installs get the new permission-set
  managed-policy and inline-policy data instead of silently receiving empty results due to
  graceful AccessDenied handling.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No blocking issues found.

@ggreer
ggreer merged commit a81d622 into main Jul 14, 2026
9 checks passed
@ggreer
ggreer deleted the ggreer/policy-permission-set branch July 14, 2026 22:43
luisina-santos added a commit that referenced this pull request Aug 3, 2026
…XH-1152, CXP-785) (#148)

* docs: document IAM policy resources and their required permissions (CXH-1152)

The connector has synced IAM managed policies and inline policies as
resources with their policy documents since #132 / #134 / #145, but
docs/connector.mdx was never updated. README.md carries the new
permissions; the customer-facing page does not.

Because the document fetch degrades AccessDenied to a warn-and-skip, a
customer who builds their IAM role from the published policy gets policy
resources with no policy_document and no error — the feature is silently
inert for anyone onboarding from the docs.

- Add a SyncIAMPolicyDocuments statement to all three setup paths (IAM
  setup, Identity Center setup, Terraform) covering the 12 IAM actions
  the policy resource types actually call.
- Add "IAM policies as resources" describing both resource types, the
  policy document attribute, and the degrade-on-missing-permission
  behaviour.
- Add IAM managed policies / Inline policies to the capabilities table.
- Add an explanation bullet group in the IAM setup tab, matching the
  existing style of that section.

Not included: sso:ListCustomerManagedPolicyReferencesInPermissionSet is
also missing from this page. That gap belongs to CXP-785 and is left
alone here.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* docs: add sso:ListCustomerManagedPolicyReferencesInPermissionSet (CXP-785)

Folds in the one surviving item from CXP-785. Auditing that ticket's five
claimed actions against main:

- sso:GetInlinePolicyForPermissionSet and
  sso:ListManagedPoliciesInPermissionSet are already present in this file.
- sso:GetPermissionsBoundaryForPermissionSet and
  sso:DescribePermissionSetProvisioningStatus are not in
  baton_capabilities.json and are not called by the connector, so they are
  not required permissions.
- sso:ListCustomerManagedPolicyReferencesInPermissionSet is genuinely
  missing, and is called at permission_set_assignment.go:382.

Added to the Identity Center and Terraform read policies. Every sso:
action declared in baton_capabilities.json is now present in both.

Also note CXP-785's stated impact is stale: that call degrades
AccessDenied to a warn-and-skip (permission_set_assignment.go:392-397),
so the sync does not fail — customer-managed policy grants on permission
sets are silently missing instead, the same failure class as the iam:
gaps in this PR.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants