-
Notifications
You must be signed in to change notification settings - Fork 24
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
feat(IamAssumeAuthenticator): add new authentication type for iam assume #287
base: main
Are you sure you want to change the base?
Conversation
* Authorization: Bearer \<bearer-token\> | ||
*/ | ||
export class IamAssumeAuthenticator extends IamRequestBasedAuthenticator { | ||
protected tokenManager: IamAssumeTokenManager; |
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.
The way that the inheritance is set up in this core made it fairly easy to implement everything, because the shared IAM-related logic is all handled automatically.
I decided not to include any state in this class, because it didn't seem necessary and I wanted to keep it lightweight. We can revisit that decision if need be. It can also be added later in a compatible way, if need be.
@@ -115,7 +115,7 @@ export class ContainerTokenManager extends IamRequestBasedTokenManager { | |||
/** | |||
* Request an IAM token using a compute resource token. | |||
*/ | |||
protected async requestToken(): Promise<any> { |
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.
This must have been a typo or copy/paste error. It doesn't change behavior whatsoever, the function is still "asynchronous", but we don't use the await
keyword in this function so it's unnecessary to include the async
keyword (it's only purpose is to provide context when using await
).
// Set the grant type and user agent for this flavor of authentication. | ||
this.formData.grant_type = 'urn:ibm:params:oauth:grant-type:assume'; | ||
this.userAgent = buildUserAgent('iam-assume-authenticator'); | ||
} |
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.
Like the authenticator, most of the logic is already taken care of elsewhere, so this was easy to implement. It's similar to the Container authenticator in that way.
Unlike the Container authenticator, I decided not to include any setters/getters for the member variables. We don't include them in the IamAuthenticator
and I thought consistency with that class was most important. It also didn't seem necessary, but we can add them compatibly later if need be.
export function onlyOne(a: any, b: any): boolean { | ||
return Boolean((a && !b) || (b && !a)); | ||
} | ||
|
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.
I just moved this to be further down in the file, next to the other related functions.
export function atLeastOne(a: any, b: any): boolean { | ||
return Boolean(a || b); | ||
export function onlyOne(...args: any): boolean { | ||
return countDefinedArgs(args) === 1; |
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.
I changed all of these functions to accept spread arguments instead of binary options.
The check needed for this authenticator (exactly one out of three) prompted me to think these functions would be more valuable down the road if they weren't limited to looking at only two options. So, now they can take any number of options and still perform the same checks. They are perfectly compatible with their old implementations, though they've technically "changed" in the API.
@@ -13,10 +13,10 @@ import { OutgoingHttpHeaders } from 'http'; | |||
import { Stream } from 'stream'; | |||
|
|||
// @public | |||
export function atLeastOne(a: any, b: any): boolean; | |||
export function atLeastOne(...args: any): boolean; |
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.
Here we see the API change but note that they are compatible: ...args: any
can very well equal a: any, b: any
.
This commit introduces the new IamAssumeAuthenticator which will fetch an IAM access token using the IAM getToken operation's "assume" grant type. The resulting access token allows the application to assume the identity of a trusted profile, similar to the "sudo" feature of Linux. Signed-off-by: Dustin Popp <[email protected]>
f6ede06
to
3f614fc
Compare
This commit introduces the new IamAssumeAuthenticator which will fetch an IAM access token using the IAM getToken operation's "assume" grant type. The resulting access token allows the application to assume the identity of a trusted profile, similar to the "sudo" feature of Linux.
Checklist
npm test
passes (tip:npm run lint-fix
can correct most style issues)