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

fix(iam): withConditions overrides Principal actions #28510

Merged
merged 6 commits into from Dec 30, 2023

Conversation

lpizzinidev
Copy link
Contributor

Calling withConditions after withSessionTags will override the existing sts:TagSession action for the statement.
This implementation fixes the issue.

Closes #28426.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@github-actions github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p2 distinguished-contributor [Pilot] contributed 50+ PRs to the CDK labels Dec 28, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team December 28, 2023 15:11
@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 28, 2023
Copy link
Contributor

@kylelaker kylelaker left a comment

Choose a reason for hiding this comment

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

Very nice fix! Thank you!

@aws-cdk-automation aws-cdk-automation added pr/needs-maintainer-review This PR needs a review from a Core Team Member and removed pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Dec 29, 2023
@@ -264,6 +264,18 @@ export class PrincipalWithConditions extends PrincipalAdapter {
this.additionalConditions = conditions;
}

public addToAssumeRolePolicy(doc: PolicyDocument) {
// eslint-disable-next-line @typescript-eslint/no-require-imports
const adapter: typeof import('./private/policydoc-adapter') = require('./private/policydoc-adapter');
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I'll ask the question: why are we doing this weird import here? I assume something is stopping us from importing MutatingPolicyDocumentAdapter the normal way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To prevent a circular dependency on imports (it seems on PolicyDocument) as done here.
I added a comment to clarify.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect thanks. My next comment was going to be that if I'm confused, so will people who look at the code in the future!

kaizencc
kaizencc previously approved these changes Dec 30, 2023
Copy link
Contributor

mergify bot commented Dec 30, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 30, 2023
@mergify mergify bot dismissed kaizencc’s stale review December 30, 2023 12:57

Pull request has been modified.

Copy link
Contributor

mergify bot commented Dec 30, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: f3dae83
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit 0b345c5 into aws:main Dec 30, 2023
9 checks passed
Copy link
Contributor

mergify bot commented Dec 30, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

paulhcsun pushed a commit to paulhcsun/aws-cdk that referenced this pull request Jan 5, 2024
Calling `withConditions` after `withSessionTags` will override the existing `sts:TagSession` action for the statement.
This implementation fixes the issue.

Closes aws#28426.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. distinguished-contributor [Pilot] contributed 50+ PRs to the CDK effort/small Small work item – less than a day of effort p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IAM: ArnPrincipal withSessionTags depends on order.
5 participants