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

feat(IamAuthenticator): support refresh token flow in IamAuthenticator #146

Merged
merged 1 commit into from
Nov 29, 2021

Conversation

padamstx
Copy link
Member

This commit updates the IamAuthenticator so that it supports
the "refresh token" auth flow. In this scenario, the user constructs
an IamAuthenticator with a refresh token (and client id/secret) instead
of an apikey. The authenticator will use the "POST /identity/token"
operation with grant_type "refresh_token" to obtain an IAM access token.

In this commit, I also added a new "builder" for the IamAuthenticator
as well, and deprecated the legacy ctor function although it is still supported.

)

// NewIamAuthenticator constructs a new IamAuthenticator instance.
func NewIamAuthenticator(apikey string, url string, clientId string, clientSecret string,
Copy link
Member Author

Choose a reason for hiding this comment

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

This legacy ctor was simply moved down a bit.

@padamstx padamstx force-pushed the refresh-token branch 2 times, most recently from 27ba218 to 6fe42b5 Compare November 19, 2021 22:18
Copy link
Member

@pyrooka pyrooka left a comment

Choose a reason for hiding this comment

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

It looks good, just a few variable naming suggestions.

v5/core/iam_authenticator.go Outdated Show resolved Hide resolved
v5/core/iam_authenticator.go Outdated Show resolved Hide resolved
v5/core/iam_authenticator_test.go Outdated Show resolved Hide resolved
This commit updates the IamAuthenticator so that it supports
the "refresh token" auth flow.  In this scenario, the user constructs
an IamAuthenticator with a refresh token (and client id/secret) instead
of an apikey.  The authenticator will use the "POST /identity/token"
operation with grant_type "refresh_token" to obtain an IAM access token.

In this commit, I also added a new "builder" for the IamAuthenticator
as well, and deprecated the legacy ctor function although it is still supported.
Copy link
Member

@dpopp07 dpopp07 left a comment

Choose a reason for hiding this comment

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

Looks good! Just have one question but I'll approve since it's probably something you already have an answer to

// Validate ClientId and ClientSecret.
// If RefreshToken is not specified, then both or neither should be specified.
// If RefreshToken is specified, then both must be specified.
if this.ClientId == "" && this.ClientSecret == "" && this.RefreshToken == "" {
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible that the API key was created without either the client ID or secret set? I guess there's a default that's used in that case but would the refresh token path use the default as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Great questions...
When obtaining an access token for an apikey, there are no "default" values for client id/secret that are used if they're not specified. In this scenario, you won't get back a valid refresh token.

In order to obtain a valid refresh token, you must specify "bx/bx" for the client id/secret values along with the apikey.
So, if this refresh token is then going to be used to construct the IAM Authenticator, the same client id/secret values must be used as well. I didn't want to hardcode client id/secret to be "bx/bx" in the authenticator when a refresh token is being used because that seems a bit hackish. I discussed this with Martin Smolny and we settled on "the client id/secret values must be the same values that were used to initially obtain the refresh token", even though in the IBM IAM implementation those values an only be "bx/bx". However, in the future, this constraint might be relaxed to support additional values for those properties, but the "same as" constraint would still apply... presumably :)

@padamstx padamstx merged commit 97f89dd into main Nov 29, 2021
ibm-devx-sdk pushed a commit that referenced this pull request Nov 29, 2021
# [5.9.0](v5.8.2...v5.9.0) (2021-11-29)

### Features

* **IamAuthenticator:** support refresh token flow in IamAuthenticator ([#146](#146)) ([97f89dd](97f89dd))
@ibm-devx-sdk
Copy link

🎉 This PR is included in version 5.9.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@padamstx padamstx deleted the refresh-token branch November 30, 2021 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants