Skip to content

fix(policychecks): Update Policy Checks to use profile selected by AWS Toolkits instead of always default #6074

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jan 28, 2025
Merged

fix(policychecks): Update Policy Checks to use profile selected by AWS Toolkits instead of always default #6074

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bf0066d
fix(policychecks): Update Policy Checks to use profile selected by AW…
kevluu-aws Nov 21, 2024
24ba726
Merge branch 'aws:master' into master
kevluu-aws Jan 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 31 additions & 2 deletions packages/core/src/awsService/accessanalyzer/vue/iamPolicyChecks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,9 @@ export class IamPolicyChecksWebview extends VueWebview {
documentType,
Copy link

@mponaws mponaws Nov 21, 2024

Choose a reason for hiding this comment

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

Firstly, thank you @kevluu-aws for fixing it! 🙌

inputPolicyType: policyType ? policyType : 'None',
})
this.client.config.credentials = new SharedIniFileCredentials() // We need to detect changes in the user's credentials
this.client.config.credentials = new SharedIniFileCredentials({
profile: `${getProfileName()}`,
}) // We need to detect changes in the user's credentials
this.client.validatePolicy(
{
policyDocument: IamPolicyChecksWebview.editedDocument,
Expand Down Expand Up @@ -276,6 +278,8 @@ export class IamPolicyChecksWebview extends VueWebview {
`${this.region}`,
'--config',
`${globals.context.asAbsolutePath(defaultTerraformConfigPath)}`,
'--profile',
`${getProfileName()}`,
]
this.executeValidatePolicyCommand({
command,
Expand All @@ -296,7 +300,15 @@ export class IamPolicyChecksWebview extends VueWebview {
case 'CloudFormation': {
if (isCloudFormationTemplate(document)) {
const command = 'cfn-policy-validator'
const args = ['validate', '--template-path', `${document}`, '--region', `${this.region}`]
const args = [
'validate',
'--template-path',
`${document}`,
'--region',
`${this.region}`,
'--profile',
`${getProfileName()}`,
]
if (cfnParameterPath !== '') {
args.push('--template-configuration-file', `${cfnParameterPath}`)
}
Expand Down Expand Up @@ -356,6 +368,8 @@ export class IamPolicyChecksWebview extends VueWebview {
`${tempFilePath}`,
'--reference-policy-type',
`${policyType}`,
'--profile',
`${getProfileName()}`,
]
this.executeCustomPolicyChecksCommand({
command,
Expand Down Expand Up @@ -387,6 +401,8 @@ export class IamPolicyChecksWebview extends VueWebview {
`${tempFilePath}`,
'--reference-policy-type',
`${policyType}`,
'--profile',
`${getProfileName()}`,
]
if (cfnParameterPath !== '') {
args.push('--template-configuration-file', `${cfnParameterPath}`)
Expand Down Expand Up @@ -447,6 +463,8 @@ export class IamPolicyChecksWebview extends VueWebview {
`${this.region}`,
'--config',
`${globals.context.asAbsolutePath(defaultTerraformConfigPath)}`,
'--profile',
`${getProfileName()}`,
]
if (actions !== '') {
args.push('--actions', `${actions}`)
Expand Down Expand Up @@ -479,6 +497,8 @@ export class IamPolicyChecksWebview extends VueWebview {
`${document}`,
'--region',
`${this.region}`,
'--profile',
`${getProfileName()}`,
]
if (actions !== '') {
args.push('--actions', `${actions}`)
Expand Down Expand Up @@ -524,6 +544,8 @@ export class IamPolicyChecksWebview extends VueWebview {
`${this.region}`,
'--config',
`${globals.context.asAbsolutePath(defaultTerraformConfigPath)}`,
'--profile',
`${getProfileName()}`,
]
this.executeCustomPolicyChecksCommand({
command,
Expand All @@ -550,6 +572,8 @@ export class IamPolicyChecksWebview extends VueWebview {
`${document}`,
'--region',
`${this.region}`,
'--profile',
`${getProfileName()}`,
]
if (cfnParameterPath !== '') {
args.push('--template-configuration-file', `${cfnParameterPath}`)
Expand Down Expand Up @@ -919,6 +943,11 @@ export function isJsonPolicyLanguage(document: string) {
return policyLanguageFileTypes.some((t) => document.endsWith(t))
}

export function getProfileName(): string | undefined {
// We neeed to split the name on 'profile:' to extract the correct profile name
return globals.awsContext.getCredentialProfileName()?.split('profile:')[1]
Copy link

@mponaws mponaws Nov 21, 2024

Choose a reason for hiding this comment

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

Is this a constant format? I am trying to understand who enforces this format?

Copy link

@mponaws mponaws Nov 21, 2024

Choose a reason for hiding this comment

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

Also, can we add an error message on the backend to determine any incorrect format here?

Copy link
Contributor Author

@kevluu-aws kevluu-aws Nov 21, 2024

Choose a reason for hiding this comment

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

Toolkits uses the format in displaying the profile name to users after authenticating through the extension. It's stored with profile: prepended in the Credentials object.

Copy link
Contributor

@nkomonen-amazon nkomonen-amazon Nov 21, 2024

Choose a reason for hiding this comment

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

Agreed we should probably refactor the profile name in to a data object instead. It doesnt need to be done in this PR.

Also, can we add an error message on the backend to determine any incorrect format here?

There shouldn't be a need to determine incorrect formats as the format of this data should have been validated here and then right after we create the name with this format.

}

export class PolicyChecksError extends ToolkitError {
constructor(message: string, code: PolicyChecksErrorCode) {
super(message, { code })
Expand Down
12 changes: 12 additions & 0 deletions packages/core/src/test/awsService/accessanalyzer/iamPolicyChecks.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,8 @@ describe('validatePolicy', function () {
'us-east-1',
Copy link

@mponaws mponaws Nov 21, 2024

Choose a reason for hiding this comment

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

Can we add a test that will help us determine if there is any change to the expected format in the getProfileName() function?

Copy link
Contributor Author

@kevluu-aws kevluu-aws Nov 21, 2024

Choose a reason for hiding this comment

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

The CLI tool tests check for a profile to be passed as undefined. This will check if the 'profile:' part gets changed

Copy link

@mponaws mponaws Nov 21, 2024

Choose a reason for hiding this comment

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

Had a chat with Kevin offline regarding if there is a way for us to catch the change in the format of the profile name, and he confirmed that it should be caught in the unit tests.

'--config',
`${globals.context.asAbsolutePath(defaultTerraformConfigPath)}`,
'--profile',
'undefined',
],
cfnParameterPathExists: false,
documentType,
Expand All @@ -180,6 +182,8 @@ describe('validatePolicy', function () {
IamPolicyChecksWebview.editedDocumentFileName,
'--region',
'us-east-1',
'--profile',
'undefined',
'--template-configuration-file',
cfnParameterPath,
],
Expand Down Expand Up @@ -449,6 +453,8 @@ describe('customChecks', function () {
'us-east-1',
'--config',
`${globals.context.asAbsolutePath(defaultTerraformConfigPath)}`,
'--profile',
'undefined',
'--actions',
'action1action2',
'--resources',
Expand Down Expand Up @@ -486,6 +492,8 @@ describe('customChecks', function () {
document,
'--region',
'us-east-1',
'--profile',
'undefined',
'--actions',
'action1action2',
'--resources',
Expand Down Expand Up @@ -548,6 +556,8 @@ describe('customChecks', function () {
'us-east-1',
'--config',
`${globals.context.asAbsolutePath(defaultTerraformConfigPath)}`,
'--profile',
'undefined',
],
cfnParameterPathExists: !!cfnParameterPath,
documentType,
Expand Down Expand Up @@ -579,6 +589,8 @@ describe('customChecks', function () {
document,
'--region',
'us-east-1',
'--profile',
'undefined',
Copy link
Contributor

@justinmk3 justinmk3 Jan 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

All of the tests assert "undefined". If this PR is intended to support the non-undefined case, at least one test should assert that case

Copy link
Contributor Author

@kevluu-aws kevluu-aws Jan 28, 2025

Choose a reason for hiding this comment

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

These test cases do not actually ever use this argument, as we stub the actual execution of our CLI tool here. It simply looks at the global globals.awsContext.getCredentialProfileName(), which is undefined in the test execution context.

Is there a way to change the current profile in the test execution context? If we are able to, we can assert it is propagated properly.

Copy link
Contributor

@justinmk3 justinmk3 Jan 28, 2025

Choose a reason for hiding this comment

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

Is there a way to change the current profile in the test execution context?

Look at existing code.

'--template-configuration-file',
cfnParameterPath,
],
Expand Down
Loading