-
Notifications
You must be signed in to change notification settings - Fork 67
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
added OAuth2 options in http client #615
Merged
Merged
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
535b327
added oauth2 options in httpclient
yesoreyeram d9c6679
Apply suggestions from code review
yesoreyeram 7c344a3
added tests for oauth2 http client
yesoreyeram abbdf8a
authclient package moved experimental
yesoreyeram da2e7e8
Merge remote-tracking branch 'origin' into httpclient_oauth2
yesoreyeram 2d2100a
addressed review comments
yesoreyeram 02c6d70
Merge remote-tracking branch 'origin' into httpclient_oauth2
yesoreyeram File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| package authclient | ||
|
|
||
| import ( | ||
| "errors" | ||
| "net/http" | ||
|
|
||
| "github.com/grafana/grafana-plugin-sdk-go/backend/httpclient" | ||
| ) | ||
|
|
||
| // New auth client which is basically a http client but specific functionalities implemented depends on AuthMethod | ||
| func New(httpOptions httpclient.Options, options AuthOptions) (client *http.Client, err error) { | ||
| defaultClient, err := httpclient.New(httpOptions) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| switch options.AuthMethod { | ||
| case AuthMethodOAuth2: | ||
| if options.OAuth2Options == nil { | ||
| return nil, errors.New("invalid options for OAuth2 client") | ||
| } | ||
| return getOAuth2Client(defaultClient, *options.OAuth2Options) | ||
| default: | ||
| return defaultClient, nil | ||
| } | ||
| } | ||
|
|
||
| // AuthOptions Auth client options. Based on the AuthenticationMethod, further properties will be validated | ||
| type AuthOptions struct { | ||
| // AuthMethod ... | ||
| AuthMethod AuthMethod | ||
| // OAuth2Options ... | ||
| OAuth2Options *OAuth2Options | ||
| } | ||
|
|
||
| // AuthMethod defines the type of authentication method that needs to be use. | ||
| type AuthMethod string | ||
|
|
||
| const ( | ||
| // AuthMethodOAuth2 is oauth2 type authentication. | ||
| // Currently support client credentials and JWT type OAuth2 workflows. | ||
| AuthMethodOAuth2 AuthMethod = "oauth2" | ||
| ) | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,157 @@ | ||
| package authclient_test | ||
|
|
||
| import ( | ||
| "crypto/rand" | ||
| "crypto/rsa" | ||
| "crypto/x509" | ||
| "encoding/pem" | ||
| "fmt" | ||
| "io" | ||
| "net/http" | ||
| "net/http/httptest" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/grafana/grafana-plugin-sdk-go/backend/httpclient" | ||
| "github.com/grafana/grafana-plugin-sdk-go/experimental/authclient" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestNew(t *testing.T) { | ||
| t.Run("oauth2", func(t *testing.T) { | ||
| server := getOAuthServer(t) | ||
| defer server.Close() | ||
| t.Run("client credentials", func(t *testing.T) { | ||
| t.Run("valid client credentials", func(t *testing.T) { | ||
| hc, err := authclient.New(httpclient.Options{ | ||
| Headers: map[string]string{"h1": "v1"}, | ||
| }, authclient.AuthOptions{ | ||
| AuthMethod: authclient.AuthMethodOAuth2, | ||
| OAuth2Options: &authclient.OAuth2Options{ | ||
| OAuth2Type: authclient.OAuth2TypeClientCredentials, | ||
| TokenURL: server.URL + "/token", | ||
| }, | ||
| }) | ||
| require.Nil(t, err) | ||
| require.NotNil(t, hc) | ||
| res, err := hc.Get(server.URL + "/foo") | ||
| require.Nil(t, err) | ||
| require.NotNil(t, res) | ||
| if res != nil && res.Body != nil { | ||
| defer res.Body.Close() | ||
| } | ||
| bodyBytes, err := io.ReadAll(res.Body) | ||
| require.Nil(t, err) | ||
| assert.Equal(t, http.StatusOK, res.StatusCode) | ||
| assert.Equal(t, `"hello world"`, string(bodyBytes)) | ||
| }) | ||
| t.Run("valid client credentials with basic auth settings", func(t *testing.T) { | ||
| hc, err := authclient.New(httpclient.Options{ | ||
| Headers: map[string]string{"h1": "v1"}, | ||
| BasicAuth: &httpclient.BasicAuthOptions{User: "userFoo", Password: "pwdBar"}, | ||
| }, authclient.AuthOptions{ | ||
| AuthMethod: authclient.AuthMethodOAuth2, | ||
| OAuth2Options: &authclient.OAuth2Options{ | ||
| OAuth2Type: authclient.OAuth2TypeClientCredentials, | ||
| TokenURL: server.URL + "/token", | ||
| }, | ||
| }) | ||
| require.Nil(t, err) | ||
| require.NotNil(t, hc) | ||
| res, err := hc.Get(server.URL + "/foo") | ||
| require.Nil(t, err) | ||
| require.NotNil(t, res) | ||
| if res != nil && res.Body != nil { | ||
| defer res.Body.Close() | ||
| } | ||
| bodyBytes, err := io.ReadAll(res.Body) | ||
| require.Nil(t, err) | ||
| assert.Equal(t, http.StatusOK, res.StatusCode) | ||
| assert.Equal(t, `"hello world"`, string(bodyBytes)) | ||
| }) | ||
| }) | ||
| t.Run("jwt", func(t *testing.T) { | ||
| t.Run("invalid private key", func(t *testing.T) { | ||
| privateKey := generateKey(t, true) | ||
| hc, err := authclient.New(httpclient.Options{ | ||
| Headers: map[string]string{"h1": "v1"}, | ||
| }, authclient.AuthOptions{ | ||
| AuthMethod: authclient.AuthMethodOAuth2, | ||
| OAuth2Options: &authclient.OAuth2Options{ | ||
| OAuth2Type: authclient.OAuth2TypeJWT, | ||
| TokenURL: server.URL + "/token", | ||
| PrivateKey: privateKey, | ||
| }, | ||
| }) | ||
| require.Nil(t, err) | ||
| require.NotNil(t, hc) | ||
| res, err := hc.Get(server.URL + "/foo") | ||
| if res != nil && res.Body != nil { | ||
| defer res.Body.Close() | ||
| } | ||
| require.NotNil(t, err) | ||
| assert.True(t, strings.Contains(err.Error(), "private key should be a PEM or plain PKCS1 or PKCS8; parse error: asn1: structure error")) | ||
| require.Nil(t, res) | ||
| }) | ||
| t.Run("valid private key", func(t *testing.T) { | ||
| privateKey := generateKey(t, false) | ||
| hc, err := authclient.New(httpclient.Options{ | ||
| Headers: map[string]string{"h1": "v1"}, | ||
| }, authclient.AuthOptions{ | ||
| AuthMethod: authclient.AuthMethodOAuth2, | ||
| OAuth2Options: &authclient.OAuth2Options{ | ||
| OAuth2Type: authclient.OAuth2TypeJWT, | ||
| TokenURL: server.URL + "/token", | ||
| PrivateKey: privateKey, | ||
| }, | ||
| }) | ||
| require.Nil(t, err) | ||
| require.NotNil(t, hc) | ||
| res, err := hc.Get(server.URL + "/foo") | ||
| require.Nil(t, err) | ||
| require.NotNil(t, res) | ||
| if res != nil && res.Body != nil { | ||
| defer res.Body.Close() | ||
| } | ||
| bodyBytes, err := io.ReadAll(res.Body) | ||
| require.Nil(t, err) | ||
| assert.Equal(t, http.StatusOK, res.StatusCode) | ||
| assert.Equal(t, `"hello world"`, string(bodyBytes)) | ||
| }) | ||
| }) | ||
| }) | ||
| } | ||
|
|
||
| func getOAuthServer(t *testing.T) *httptest.Server { | ||
| t.Helper() | ||
| return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| oAuth2TokenValue := "foo" | ||
| t.Run("ensure custom headers propagated correctly", func(t *testing.T) { | ||
| require.Equal(t, "v1", r.Header.Get("h1")) | ||
| }) | ||
| if r.URL.String() == "/token" { | ||
| w.Header().Set("Content-Type", "application/json") | ||
| _, _ = io.WriteString(w, fmt.Sprintf(`{"access_token": "%s", "refresh_token": "bar"}`, oAuth2TokenValue)) | ||
| return | ||
| } | ||
| t.Run("ensure oauth token correctly sets to the authorization header", func(t *testing.T) { | ||
| require.Equal(t, fmt.Sprintf("Bearer %s", oAuth2TokenValue), r.Header.Get("Authorization")) | ||
| }) | ||
| w.Header().Set("Content-Type", "application/json") | ||
| _, _ = io.WriteString(w, `"hello world"`) | ||
| })) | ||
| } | ||
|
|
||
| func generateKey(t *testing.T, incorrectKey bool) (privateKey []byte) { | ||
| t.Helper() | ||
| if incorrectKey { | ||
| return []byte("invalid private key") | ||
| } | ||
| key, err := rsa.GenerateKey(rand.Reader, 4096) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| privateKey = pem.EncodeToMemory(&pem.Block{Type: "RSA PRIVATE KEY", Bytes: x509.MarshalPKCS1PrivateKey(key)}) | ||
| return privateKey | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| package authclient | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "net/http" | ||
| "net/url" | ||
| "strings" | ||
|
|
||
| "golang.org/x/oauth2" | ||
| "golang.org/x/oauth2/clientcredentials" | ||
| "golang.org/x/oauth2/jwt" | ||
| ) | ||
|
|
||
| // OAuth2Type defines type of oauth2 grant type | ||
| type OAuth2Type string | ||
|
|
||
| const ( | ||
| OAuth2TypeClientCredentials OAuth2Type = "client_credentials" | ||
| OAuth2TypeJWT OAuth2Type = "jwt" | ||
| ) | ||
|
|
||
| // OAuth2Options defines options for OAuth2 Client | ||
| type OAuth2Options struct { | ||
| OAuth2Type OAuth2Type | ||
| TokenURL string | ||
| Scopes []string | ||
| ClientID string | ||
| ClientSecret string | ||
| EndpointParams map[string]string | ||
| Subject string | ||
| Email string | ||
| PrivateKey []byte | ||
| PrivateKeyID string | ||
| } | ||
|
|
||
| func getOAuth2Client(client *http.Client, oAuth2Options OAuth2Options) (o *http.Client, err error) { | ||
| switch oAuth2Options.OAuth2Type { | ||
| case OAuth2TypeClientCredentials: | ||
| return getOAuth2ClientCredentialsClient(client, oAuth2Options) | ||
| case OAuth2TypeJWT: | ||
| return getOAuth2JWTClient(client, oAuth2Options) | ||
| } | ||
| return client, fmt.Errorf("invalid/empty oauth2 type (%s)", oAuth2Options.OAuth2Type) | ||
| } | ||
|
|
||
| func getOAuth2ClientCredentialsClient(client *http.Client, options OAuth2Options) (*http.Client, error) { | ||
| config := clientcredentials.Config{ | ||
| TokenURL: options.TokenURL, | ||
| Scopes: sanitizeOAuth2Scopes(options), | ||
| EndpointParams: sanitizeOAuth2EndpointParams(options), | ||
| ClientID: options.ClientID, | ||
| ClientSecret: options.ClientSecret, | ||
| } | ||
| if client == nil { | ||
| return config.Client(context.Background()), nil | ||
| } | ||
| ctx := context.WithValue(context.Background(), oauth2.HTTPClient, client) | ||
| return config.Client(ctx), nil | ||
| } | ||
|
|
||
| func getOAuth2JWTClient(client *http.Client, options OAuth2Options) (*http.Client, error) { | ||
| config := jwt.Config{ | ||
| TokenURL: options.TokenURL, | ||
| Scopes: sanitizeOAuth2Scopes(options), | ||
| PrivateKey: options.PrivateKey, | ||
| PrivateKeyID: options.PrivateKeyID, | ||
| Email: options.Email, | ||
| Subject: options.Subject, | ||
| } | ||
| if client == nil { | ||
| return config.Client(context.Background()), nil | ||
| } | ||
| ctx := context.WithValue(context.Background(), oauth2.HTTPClient, client) | ||
| return config.Client(ctx), nil | ||
| } | ||
|
|
||
| func sanitizeOAuth2Scopes(options OAuth2Options) []string { | ||
| scopes := []string{} | ||
| for _, scope := range options.Scopes { | ||
| if scope != "" { | ||
| scopes = append(scopes, strings.TrimSpace(scope)) | ||
| } | ||
| } | ||
| return scopes | ||
| } | ||
|
|
||
| func sanitizeOAuth2EndpointParams(options OAuth2Options) url.Values { | ||
| endpointParams := url.Values{} | ||
| for k, v := range options.EndpointParams { | ||
| if k != "" && v != "" { | ||
| endpointParams.Set(strings.TrimSpace(k), strings.TrimSpace(v)) | ||
| } | ||
| } | ||
| return endpointParams | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 is what I think doesn't make sense from the code perspective. If you name this package oauth2client, you don't need the
AuthMethod. The client, rather than setting this Method based on the user input, can call the normalhttpClient.Newor thisoauth2Client.New.If in the future, if there is a third auth type, then you can create the abstraction of an
authclientthat can either instantiate anhttpClient, anoauth2Clientor something else but for the case of only 2 types I don't think it's necessary this extra complexity here.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 still believe we need authentication method.
In this PR, I am adding only support for OAuth2. But in near future, we need more ways to authenticate as seen here.
Ideally I don't like to create
Newfor every single auth types such asoauthclient.New,digestauthclient.New.. If we want to make some changes such as adding a custom header I don't want to add in every places.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 am not a fan of merging code that is meant for the future but since this is still
experimentalit's not a blocker 👍