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

IdToken as part of PluginContext.User #579

Open
kostrse opened this issue Dec 13, 2022 · 6 comments · Fixed by grafana/azure-data-explorer-datasource#533
Open

IdToken as part of PluginContext.User #579

kostrse opened this issue Dec 13, 2022 · 6 comments · Fixed by grafana/azure-data-explorer-datasource#533
Labels
enhancement New feature or request

Comments

@kostrse
Copy link

kostrse commented Dec 13, 2022

What would you like to be added:

Extend the backend.User struct with ID token (here):

// User represents a Grafana user.
type User struct {
	Login   string
	Name    string
	Email   string
	Role    string
	IdToken string
}

So it could be accessed like this:

func (adx *AzureDataExplorer) QueryData(ctx context.Context, req *backend.QueryDataRequest) (*backend.QueryDataResponse, error) {
	idToken := req.PluginContext.User.IdToken
	...
}

Why is this needed:

Currently ID tokens are being passed to plugins via Headers.

The existing approach has the following problems:

  1. CheckHealth signature doesn't have headers. Currently there's no way to obtain ID token from the backend test func. This breaks scenarios like All requests should use user identity auth if user identity configured azure-data-explorer-datasource#509
  2. If ID token would be a part of the User struct then it would be possible to pass User across as a single entity (e.g. attach it to ctx and pass from business layer to transport layer etc.). This leads to a need to repackage the user identity into another custom entity to work with (example here).
  3. ID token never originates from request, so using Headers collection for this purpose is misleading. QueryDataRequest and CallResourceRequest even differ in definition of Headers (map[string]string vs map[string][]string). It is not a strongly-typed and it's error prone.

Other things to consider

  1. Please consider adding AccessToken as well. But I don't have opinion on this, IdToken alone should be enough for purposes of identification of the user, while AccssToken useful for token pass-through scenarios.
  2. Please consider including the User struct into ctx context.Context when calling QueryData, CallResource, CheckHealth or any other backend plugin func.
@kostrse
Copy link
Author

kostrse commented Dec 20, 2022

Related #506

@marefr
Copy link
Member

marefr commented Jan 24, 2023

  1. CheckHealth signature doesn't have headers.

It's been supported since Grafana v9.2 https://grafana.com/docs/grafana/v9.2/developers/plugins/add-authentication-for-data-source-plugins/#forward-oauth-identity-for-the-logged-in-user

Starting from Grafana v9.4 there's some helpers available to operate on forwarded HTTP headers https://grafana.com/docs/grafana/next/developers/plugins/add-authentication-for-data-source-plugins/#forward-oauth-identity-for-the-logged-in-user

@kostrse
Copy link
Author

kostrse commented Jan 27, 2023

  1. CheckHealth signature doesn't have headers.

It's been supported since Grafana v9.2 https://grafana.com/docs/grafana/v9.2/developers/plugins/add-authentication-for-data-source-plugins/#forward-oauth-identity-for-the-logged-in-user

Thanks, this unblocks scenarios when ID token needed.

Starting from Grafana v9.4 there's some helpers available to operate on forwarded HTTP headers https://grafana.com/docs/grafana/next/developers/plugins/add-authentication-for-data-source-plugins/#forward-oauth-identity-for-the-logged-in-user

Main reasons why I submitted this issue:

  • ID Token is part of user identity (alongside with login, email, role), actually claims of ID Token describe the user (full name, username, email, ID, timestamps etc.). It should belong to backend.User, it has nothing to do with incoming query request since it didn't originate from there. In most web frameworks I know ID Token (and collection of parsed ID Token claims) is part of the user object. In my opinion it's not very clean design choice to use Headers as a container to pass ID Token. Could attacker send an incoming HTTP request with actual header name X-ID-Token and mislead the code?
  • Practical reason. There are three different types of request struct: query request, resource call and health check. When one component of business logic calls another component and wants to pass user information, it would be sufficient to just pass backend.User. It could be added to context.Context for example. I saw actual code where the request object was passed across business logic just because ID token was in the Headers collection.
  • I had to implement own CurrentUserContext object in Grafana Azure SDK (here and here) only because backend.User lacking IdToken and AccessToken to be used for this purpose.

The Helpers make it cleaner but they still attached to request.

@marefr
Copy link
Member

marefr commented Jan 31, 2023

Main reasons why I submitted this issue:

  • ID Token is part of user identity (alongside with login, email, role), actually claims of ID Token describe the user (full name, username, email, ID, timestamps etc.). It should belong to backend.User, it has nothing to do with incoming query request since it didn't originate from there. In most web frameworks I know ID Token (and collection of parsed ID Token claims) is part of the user object. In my opinion it's not very clean design choice to use Headers as a container to pass ID Token. Could attacker send an incoming HTTP request with actual header name X-ID-Token and mislead the code?

Only in specific scenarios such as when OAuth provider is used for login

  • Practical reason. There are three different types of request struct: query request, resource call and health check. When one component of business logic calls another component and wants to pass user information, it would be sufficient to just pass backend.User. It could be added to context.Context for example. I saw actual code where the request object was passed across business logic just because ID token was in the Headers collection.

With the latest changes in the grafana-plugin-sdk-go you can pass backend.ForwardHTTPHeaders interface instead of actual request struct.

  • I had to implement own CurrentUserContext object in Grafana Azure SDK (here and here) only because backend.User lacking IdToken and AccessToken to be used for this purpose.

The Helpers make it cleaner but they still attached to request.

With the latest changes in the grafana-plugin-sdk-go you can do something like this:

func WithUserFromReq(ctx context.Context, user *backend.User, req backend.ForwardHTTPHeaders) context.Context {
	if req == nil {
		return ctx
	}

	idToken := req.GetHTTPHeader(backend.OAuthIdentityIDTokenHeaderName)
	token := strings.Fields(req.GetHTTPHeader(backend.OAuthIdentityTokenHeaderName))
	var (
		tokenType   = token[0]
		accessToken = token[1]
	)

	currentUser := CurrentUserContext{
		User:        user,
		IdToken:     idToken,
		AccessToken: accessToken,
	}

	return WithCurrentUser(ctx, currentUser)
}

Thanks for the valuable points. We'll consider this for the future, but not at the moment since we're not sure related things to user and context are about to change. You also have workaround in place.

@marefr
Copy link
Member

marefr commented Jan 31, 2023

@kostrse
Copy link
Author

kostrse commented Jan 31, 2023

Additional proposal from the #506:

  • IdTokenClaims map[string][]string - a parsed list of claims from the given ID token.

Since ID token is a key/value bag of claims describing the user, it could be parsed in advance for user convenience as a map of claims. In some casees the original token is useful (IdToken field), in other cases are claims themselves (IdTokenClaims field).

The token would be set if Grafana authentication allows for ID token (e.g. OAuth), otherwise it would be set to "".

@marefr marefr removed their assignment Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: 💡 Ideation
Development

Successfully merging a pull request may close this issue.

3 participants