Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exclude inactive accounts in lookup cache #114

Open
wants to merge 1 commit into
base: next
Choose a base branch
from

Conversation

iainelder
Copy link

The lookup_accounts_for_ou function yields accounts in two branches. Branch 1
handles uncached accounts and branch 2 handles cached accounts.

PR #81 added a check to exclude inactive accounts in branch 1 without adding
the same check to branch 2.

In that way it solved #80 but only when the OU containing a suspended account
doesn't repeat.

This PR copies the check from branch 1 to branch 2 for consistent behavior in a
template with many assgnment groups to the same target OU.

The second assignment group no longer generates an assignment for a suspended
account, which causes CloudFormation to fail with an error like this:

Resource handler returned message: "Error occurred during operation 'Request REDACTED failed due to:
AWS SSO is unable to complete your request at this time.
Obtaining permissions to manage your AWS account 'REDACTED' is taking longer than usual.

Test the deployed macro with a template like this:

AWSTemplateFormatVersion: "2010-09-09"
Transform: AWS-SSO-Util-2020-11-08
Resources:
  Test:
    Type: SSOUtil::SSO::AssignmentGroup
    Properties:
      Name: MacroTest
      Principal:
        - Type: GROUP
          Id:
            - 11111111-1111-1111-1111-111111111111
      PermissionSet:
        - arn:aws:sso:::permissionSet/ssoins-2222222222222222/ps-3333333333333333
      Target:
        - { Type: AWS_OU, Id: r-4444 }
  Test2:
    Type: SSOUtil::SSO::AssignmentGroup
    Properties:
      Name: MacroTest2
      Principal:
        - Type: GROUP
          Id:
            - 55555555-5555-5555-5555-555555555555
      PermissionSet:
        - arn:aws:sso:::permissionSet/ssoins-2222222222222222/ps-3333333333333333
      Target:
        - { Type: AWS_OU, Id: r-4444 }

Consider an organization with active member accounts 111111111111 and
222222222222 and suspneded member account 333333333333.

Before this change, the CloudWatch logs group shows the following message for
the first assignment group. It lists just active accounts as targets.

[DEBUG] 2023-10-17T13:13:49.592Z    eeeeeeee-eeee-eeee-eeee-eeeeeeeeeeee    Got targets:
[ACCOUNT:111111111111[r-4444], ACCOUNT:222222222222[r-4444]

The group shows the following message for the second assignment group. It lists
the active and suspended accounts as targets. This is incorrect behavior and
later causes the CloudFormation error.

[DEBUG] 2023-10-17T13:13:49.593Z    eeeeeeee-eeee-eeee-eeee-eeeeeeeeeeee    Got targets:
[ACCOUNT:111111111111[r-4444], ACCOUNT:222222222222[r-4444], ACCOUNT:333333333333[r-4444]]

After this change, each assignment group logs just the active accounts as targets.

The `lookup_accounts_for_ou` function yields accounts in two branches. Branch 1
handles uncached accounts and branch 2 handles cached accounts.

PR benkehoe#81 added a check to exclude inactive accounts in branch 1 without adding
the same check to branch 2.

In that way it solved benkehoe#80 but only when the OU containing a suspended account
doesn't repeat.

This PR copies the check from branch 1 to branch 2 for consistent behavior in a
template with many assgnment groups to the same target OU.

The second assignment group no longer generates an assignment for a suspended
account, which causes CloudFormation to fail with an error like this:

```text
Resource handler returned message: "Error occurred during operation 'Request REDACTED failed due to:
AWS SSO is unable to complete your request at this time.
Obtaining permissions to manage your AWS account 'REDACTED' is taking longer than usual.
```

Test the deployed macro with a template like this:

```yaml
AWSTemplateFormatVersion: "2010-09-09"
Transform: AWS-SSO-Util-2020-11-08
Resources:
  Test:
    Type: SSOUtil::SSO::AssignmentGroup
    Properties:
      Name: MacroTest
      Principal:
        - Type: GROUP
          Id:
            - 11111111-1111-1111-1111-111111111111
      PermissionSet:
        - arn:aws:sso:::permissionSet/ssoins-2222222222222222/ps-3333333333333333
      Target:
        - { Type: AWS_OU, Id: r-4444 }
  Test2:
    Type: SSOUtil::SSO::AssignmentGroup
    Properties:
      Name: MacroTest2
      Principal:
        - Type: GROUP
          Id:
            - 55555555-5555-5555-5555-555555555555
      PermissionSet:
        - arn:aws:sso:::permissionSet/ssoins-2222222222222222/ps-3333333333333333
      Target:
        - { Type: AWS_OU, Id: r-4444 }
```

Consider an organization with active member accounts `111111111111` and `222222222222` and suspneded member account `333333333333`.

Before this change, the CloudWatch logs group shows the following message for
the first assignment group. It lists just active accounts as targets.

```text
[DEBUG]	2023-10-17T13:13:49.592Z	eeeeeeee-eeee-eeee-eeee-eeeeeeeeeeee	Got targets:
[ACCOUNT:111111111111[r-4444], ACCOUNT:222222222222[r-4444]
```

The group shows the following message for the second assignment group. It lists
the active and suspended accounts as targets. This is incorrect behavior and
later causes the CloudFormation error.

```text
[DEBUG]	2023-10-17T13:13:49.593Z	334ac992-cf53-4af0-adac-7aa15704a573	Got targets:
[ACCOUNT:111111111111[r-4444], ACCOUNT:222222222222[r-4444], ACCOUNT:333333333333[r-4444]]
```

After this change, each assignment group logs just the active accounts as targets.
@iainelder
Copy link
Author

My skip-suspended branch includes extra changes that allow me to use this change before PyPI hosts a new version of the aws-sso-lib module.

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.

1 participant