added OAuth2 options in http client

[yesoreyeram]

This PR adds support for creating OAuth2 based clients when creating new http client.

[cletter7]

Do we need tests?

[yesoreyeram]

Do we need tests?

added 7c344a3

[yesoreyeram]

Can I request someone to review and approve from @grafana/plugins-platform-backend?

[andresmgot]

Thanks @yesoreyeram, I think we need more context about this to know why it's necessary. After #612, data sources configured to forward OAuth2 credentials will automatically inject the Authentication and other configured cookies.

What's the use case for this PR? Why should be at the http_client level ? (this is, for all the backend data sources)

[yesoreyeram]

This is not forward oauth. This is for plugins to have its own oauth format. Say example, target api protected by some oauth but grafana not necessarily configured with oauth. We are planning to implement this is servicenow. This is already implemented and used in infinity datasource.

[andresmgot]

I see. This sounds to me that should be an http middleware (or two) like the basic_auth among other reasons, to keep the ctx, but I am not familiar enough with the code to give a final response. Will let other people in the @grafana/plugins-platform-backend to give a more helpful review.

[yesoreyeram]

Thanks @andresmgot .. to give more context, The oauth2 library we use using does provide the http client itself and manages the token itself within its context. So the token refresh done by itself. I am not sure how we implement middleware here as we don't deal with individual request directly. But happy to get more thoughts.

[wbrowne]

I think in general this makes sense - I'm not sure about how widely used this would be from the SDK, and therefore whether or not it's more appropriate for something like https://github.com/grafana/grafana-enterprise-sdk 🤔

That aside, rather than introducing this directly into the constructor - a different approach might be adding the functionality via a ConfigureClientFunc IE

func OAuthConfiguration() ConfigureClientFunc {
	return func(opts Options, client *http.Client) {
		_, err := getHTTPClientOAuth2(client, opts.OAuth2Options)
		if err != nil {
			//
		}
	}
}

func createClient() (*http.Client, error) {
        settings := DataSourceInstanceSettings{}
	opts, err := settings.HTTPClientOptions()
	if err != nil {
		panic("http client options: " + err.Error())
	}
        opts.ConfigureClient = httpclient.OAuthConfiguration()
	c, err := httpclient.New(opts)
       // ...

The main issue I see with this approach though is how to propagate errors back without just using a panic. It might be worth updating the API to support that better. WDYT?

[yesoreyeram]

@wbrowne - I will check with the team if we want to move this to other SDK.

Regarding ConfigureClient option, I don't understand the benefit of doing. From the code, what I see is that it is calling the method before returning the http client.

grafana-plugin-sdk-go/backend/httpclient/http_client.go

Lines 34 to 42 in 7c344a3

	if clientOpts.ConfigureClient != nil {
	clientOpts.ConfigureClient(clientOpts, c)
	}
	if clientOpts.AuthenticationMethod == AuthenticationMethodOAuth2 {
	return getHTTPClientOAuth2(c, clientOpts)
	}
	return c, nil

Also with that approach, the caller have to implement the ConfigureClient method. whereas with the new approach, user with pass the Auth type and options and the rest will be abstracted from the caller. Am I missing any benefits of using ConfigureClient?

[wbrowne]

@wbrowne - I will check with the team if we want to move this to other SDK.

Regarding ConfigureClient option, I don't understand the benefit of doing. From the code, what I see is that it is calling the method before returning the http client.

grafana-plugin-sdk-go/backend/httpclient/http_client.go

Lines 34 to 42 in 7c344a3

	if clientOpts.ConfigureClient != nil {
	clientOpts.ConfigureClient(clientOpts, c)
	}
	if clientOpts.AuthenticationMethod == AuthenticationMethodOAuth2 {
	return getHTTPClientOAuth2(c, clientOpts)
	}
	return c, nil

Also with that approach, the caller have to implement the ConfigureClient method. whereas with the new approach, user with pass the Auth type and options and the rest will be abstracted from the caller. Am I missing any benefits of using ConfigureClient?

It means we wouldn't need to add anything more "under the hood" of the HTTP client and there would be no need to have an authentication type, instead making use of the existing func patterns to configure the client as necessary. What I was picturing is the OAuth specific ConfigureClient function can still be exported in the SDK - which can be then re-used by consumers. Having played with that a bit locally also feels kind of strange - due to the hidden side effects.

I think what is mostly throwing me is having to configure two separate fields to configure the OAuth currently.

Maybe a few options then:

1. Keep in other SDK to configure and maintain as suited best to your needs
2. Remove the AuthenticationMethod field and instead just check for the existence of OAuth2Options
3. Exporting the getHTTPClientOAuth2 function instead as ConfigureHTTPClientWithOAuth2? 🤔 so, a HTTP client is created as usual and then can be configured once created

I tend to operate on the basis where if it is necessary for the community, then we add to the SDK as that is something the plugins platform will own and support, so would lean to option 1 in this case. I think once @marefr is back in office next week, he may have a stronger opinion on this, considering he's been the primary maintainer for this particular package.

[yesoreyeram]

I am keen to have authentication type property. Example of why I want.. in theory, client options may have forward oauth enabled and valid oauth settings. But only one of them can be applied to the api. So to avoid such confusion, clear auth type may be helpful. Other wise we will end up too many assumptions. Other example is that a user might have custom Authorization header and basic auth enabled where in the end only one will be in effect.

This can wait for @marefr as you mentioned.I can workaround this in the meantime.

Cc @scottlepp

[yesoreyeram]

What I was picturing is the OAuth specific ConfigureClient function can still be exported in the SDK - which can be then re-used by consumers

That is a good option too.

[yesoreyeram]

@scott earlier pointed out that, We can't move this enterprise SDK as we may want to use this oss plugins as well.

[andresmgot]

After reading more about this, my two cents would be to create a different package (maybe a subpackage of httpclient?) for the oauth2 HTTP client. The same way that the upstream http.Client doesn't have configuration options for oauth2 and you need to use a different package.

That package can be built on top of the httpclient and use a httpclient.ConfigureClientFunc to configure the client. It also removes the necessity of AuthenticationMethod since the caller would be explicity calling the function to create an OAuth2 client.

In any case, let's wait for @marefr as suggested, just in case there is a better proposal :)

[wbrowne]

LGTM

[yesoreyeram]

Thanks everyone for reviewing providing your thoughts. ( I moved this into experimental to avoid confusions )

[yesoreyeram]

Remove the AuthenticationMethod field and instead just check for the existence of OAuth2Options

@wbrowne - regarding your comments above, I have following comments. ( but not necessarily need to address in this PR and can wait for @marefr )

Why we need authentication method property ( follow up )

Currently the httpclient.New method having some confusing behaviour. For example, in the UI the user have following three things

• Basic auth enabled with user name and password
• Custom Authorization header defined
• Forward OAuth enabled

All the above three options will eventually set the Authorization header. So it causes confusion to the user which of the three auth methods will be used. So the proposal is to have clear authentication method defined in the httpclient.

[cletter7]

Also, as a part of datasource config redesign project we are going to work on the new Auth component that will allow users to select only one auth method. From what I understand multiple auth options cannot be used at the same time, right? So it probably makes sense to make it clear in the options which auth method is used.

[andresmgot]

Thanks for the changes @yesoreyeram! Only minor things here, the only comment is that I believe that as the code is now, the AuthMethod is not needed. See the comment inline.

[andresmgot]

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 normal httpClient.New or this oauth2Client.New.

If in the future, if there is a third auth type, then you can create the abstraction of an authclient that can either instantiate an httpClient, an oauth2Client or something else but for the case of only 2 types I don't think it's necessary this extra complexity here.

[yesoreyeram]

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 New for every single auth types such as oauthclient.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.

[andresmgot]

I am not a fan of merging code that is meant for the future but since this is still experimental it's not a blocker 👍