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(policychecks): Update Policy Checks to use profile selected by AWS Toolkits instead of always default #6074

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

Conversation

kevluu-aws
Copy link
Contributor

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.

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

  • 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.

Copy link
Contributor

@jpinkney-aws jpinkney-aws left a comment

Choose a reason for hiding this comment

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

When someone from your team approves I'll merge it

@@ -179,7 +179,9 @@ export class IamPolicyChecksWebview extends VueWebview {
documentType,
Copy link

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! 🙌

@@ -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

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

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

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

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.

@@ -155,6 +155,8 @@ describe('validatePolicy', function () {
'us-east-1',
Copy link

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

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

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.

@justinmk3
Copy link
Contributor

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

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.

5 participants