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

kevluu-aws · 2024-11-21T00:40:18Z

Problem

Policy Checks currently only uses the default profile within the credentials file for the CLI tool and SDK calls made to A2.

Solution

Pass on the profile name from the current profile setup in AWS Toolkits to the CLI tool and SDK.

License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

github-actions · 2024-11-21T00:40:38Z

This pull request modifies code in src/* but no tests were added/updated.
- Confirm whether tests should be added or ensure the PR description explains why tests are not required.
This pull request implements a feat or fix, so it must include a changelog entry (unless the fix is for an unreleased feature). Review the changelog guidelines.
- Note: beta or "experiment" features that have active users should announce fixes in the changelog.
- If this is not a feature or fix, use an appropriate type from the title guidelines. For example, telemetry-only changes should use the telemetry type.

…S Toolkits instead of always default

jpinkney-aws

When someone from your team approves I'll merge it

mponaws · 2024-11-21T16:01:40Z

packages/core/src/awsService/accessanalyzer/vue/iamPolicyChecks.ts

                if (isJsonPolicyLanguage(document)) {
                    telemetry.accessanalyzer_iamPolicyChecksValidatePolicy.run((span) => {
                        span.record({
                            documentType,


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

mponaws · 2024-11-21T16:02:22Z

packages/core/src/awsService/accessanalyzer/vue/iamPolicyChecks.ts


+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]


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

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

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.

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.

mponaws · 2024-11-21T16:03:10Z

packages/core/src/test/awsService/accessanalyzer/iamPolicyChecks.test.ts

                '--template-path',
                IamPolicyChecksWebview.editedDocumentFileName,
                '--region',
                'us-east-1',


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

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

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.

justinmk3 · 2024-12-03T22:26:23Z

Does this fix a user-facing symptom? If so, please add a changelog as described in #6074 (comment)

kevluu-aws · 2025-01-08T22:22:55Z

Added changelog describing the fix

justinmk3 · 2025-01-23T23:36:37Z

Please try doing a squash-merge locally (git merge --squash, or whatever tool you prefer). That will fix the commit history here.

If this still adds 1700 lines of code then next question is why :)

kevluu-aws · 2025-01-24T00:28:14Z

Please try doing a squash-merge locally (git merge --squash, or whatever tool you prefer). That will fix the commit history here.

If this still adds 1700 lines of code then next question is why :)

Accidental rebasing caused it, just fixed it and rebased with updated changelog

justinmk3 · 2025-01-27T19:34:04Z

The jscpd ci check (for redundant code) is failing, and its logs point to code that could easily be deduplicated with minimal effort.

justinmk3 · 2025-01-28T01:59:02Z

packages/core/src/test/awsService/accessanalyzer/iamPolicyChecks.test.ts

                    '--region',
                    'us-east-1',
+                    '--profile',
+                    'undefined',


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

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.

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

Look at existing code.

justinmk3 · 2025-01-28T23:45:23Z

Merging to unblock. Please followup on the above comments.

## Problem Policy Checks currently only uses the default profile within the credentials file for the CLI tool and SDK calls made to A2. ## Solution Pass on the profile name from the current profile setup in AWS Toolkits to the CLI tool and SDK.

kevluu-aws requested a review from a team as a code owner November 21, 2024 00:40

kevluu-aws force-pushed the master branch from 5fddd7d to 858ffd6 Compare November 21, 2024 01:42

kevluu-aws requested a review from a team as a code owner November 21, 2024 01:42

fix(policychecks): Update Policy Checks to use profile selected by AW…

bf0066d

…S Toolkits instead of always default

kevluu-aws force-pushed the master branch from 858ffd6 to bf0066d Compare November 21, 2024 02:00

jpinkney-aws approved these changes Nov 21, 2024

View reviewed changes

mponaws reviewed Nov 21, 2024

View reviewed changes

nkomonen-amazon approved these changes Nov 21, 2024

View reviewed changes

mponaws approved these changes Nov 21, 2024

View reviewed changes

kevluu-aws requested review from jpinkney-aws and nkomonen-amazon November 22, 2024 19:29

kevluu-aws force-pushed the master branch from e30b97e to e3043db Compare January 8, 2025 22:13

kevluu-aws force-pushed the master branch from dad4001 to 9e8379b Compare January 23, 2025 23:22

kevluu-aws requested review from a team as code owners January 23, 2025 23:22

kevluu-aws force-pushed the master branch 2 times, most recently from 48370e4 to 8e02176 Compare January 24, 2025 00:27

kevluu-aws force-pushed the master branch from 8e02176 to 8a66915 Compare January 27, 2025 23:45

Merge branch 'aws:master' into master

24ba726

kevluu-aws force-pushed the master branch from 8a66915 to 24ba726 Compare January 27, 2025 23:49

justinmk3 reviewed Jan 28, 2025

View reviewed changes

kevluu-aws requested a review from justinmk3 January 28, 2025 22:04

justinmk3 merged commit 3f16d7d into aws:master Jan 28, 2025
25 of 26 checks passed

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

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

Uh oh!

Conversation

kevluu-aws commented Nov 21, 2024

Problem

Solution

Uh oh!

github-actions bot commented Nov 21, 2024

Uh oh!

jpinkney-aws left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmk3 commented Dec 3, 2024

Uh oh!

kevluu-aws commented Jan 8, 2025

Uh oh!

justinmk3 commented Jan 23, 2025

Uh oh!

kevluu-aws commented Jan 24, 2025

Uh oh!

justinmk3 commented Jan 27, 2025

Uh oh!

justinmk3 Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmk3 commented Jan 28, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

justinmk3 Jan 28, 2025 •

edited

Loading