-
Notifications
You must be signed in to change notification settings - Fork 743
feat(auth): add STS credential management and mfa verification #7811
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
Conversation
b7bb5cd to
0ef9118
Compare
|
| updateCredentialsParams: { | ||
| data: 'credential-data', | ||
| }, | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does a type exist that we could do: } as ReturnTypeOfLogin so that the actual fields are type checked?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I will add this.
| sinon.stub(auth2, 'IamLogin').returns(mockIamLoginArn as any) | ||
|
|
||
| const response = await auth.loginIam( | ||
| 'accessKey', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: In the future I'd make these values a bit more unique, like MyAccessKey to reduce confusion since there is also the key which is called accessKey
packages/core/src/auth/auth2.ts
Outdated
| async updateProfile(opts: { accessKey: string; secretKey: string; sessionToken?: string; roleArn?: string }) { | ||
| if (opts.roleArn) { | ||
| const sourceProfile = this.profileName + '-source' | ||
| await this.lspAuth.updateIamProfile( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe theres a reason for this, but the interface of updateiamProfile() is a bit awkward. We probably want to use a single object as the argument here since this function takes 6 arguments. Usually after 3 args to a function, I like to make a single argument which is a typed object. Additionally, it will save you from explicitly needing to set '' for the empty fields
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right. We are wrapping this into a typed object.
| * Entered token is passed to the callback. | ||
| * If user cancels out, the callback is passed an error with a fixed message string. | ||
| * | ||
| * @param profileName Name of Credentials profile we are asking an MFA Token for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These params look out of date
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated!
packages/core/src/auth/sso/cache.ts
Outdated
| } | ||
|
|
||
| const defaultCacheDir = () => path.join(fs.getUserHomeDir(), '.aws/sso/cache') | ||
| const defaultStsCacheDir = () => path.join(fs.getUserHomeDir(), '.aws/cli/cache') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just verifying that this is the path that the AWS CLI uses for its cache as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are deleting all sts cache watcher logic in the PR as it is not working as intended. This should not be here anymore.
| accessKey: string, | ||
| secretKey: string, | ||
| sessionToken?: string | ||
| sessionToken?: string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned above, may be worth to refactor in to a single object as the argument
| profileName: string, | ||
| accessKey: string, | ||
| secretKey: string | ||
| secretKey: string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above with an object as the argument
e79313c to
8342872
Compare
…scode into feature/sts-login
…scode into feature/sts-login
Problem
The webview does not support STS credentials input (sessionToken and roleArn) and endpoint to LSP does not support STS credentials and profiles.
Solution
This is part of #7507 and is built on top of #7797.