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

CurrentUserContext to carry current user identity to the authentication layer #525

Merged
merged 7 commits into from
Dec 20, 2022

Conversation

kostrse
Copy link
Contributor

@kostrse kostrse commented Dec 13, 2022

The final goal is to completely separate authentication logic from the business layer. The token request logic should not live alongside with normal query code. This will allow to abstract out authentication from the core plugin implementation and eventually move authentication to the Grafana Azure SDK.

This is intermediate PR which introduces CurrentUserContext context abstraction which is supposed to carry current user identity though context down to the transport layer.

ADX client extracts the CurrentUserContext and uses it for authentication regardless of where the context was originated.

Helper methods adxusercontext.FromQueryReq and adxusercontext.FromResourceReq provided to create user context for query (here) and resource (here) requests. After the context was created, it can be passed to the rest of the business logic, down to ADX client without having to have any handling of access token or authorization header.

Problem with heath check

Due to current design of Plugin SDK, CheckHealth doesn't carry ID token (see here), so this PR has a workaround specifically for TestRequest to use service identity via GetServiceAccessToken(ctx) for datasource testing (here), rather than using the proper user identity.

This is incorrect behavior which already existed before this PR. This PR just isolated it to CheckHealth/TestRequest so the issue becomes more straightforward to address.

Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

I am okay with the main change, my main concern is that we have both the context and the backend.Requests in the different methods and we are choosing to add the token info to the context rather than the request (which is what I would thought include the auth info at first glance) but it's probably not a big issue if we are explicit in the code about the decission.

Also, there is also the potential problem that the tokens may be pass down to functions that should not have access to them (but that have access to the context) but at the moment I don't see any example. What do you think?

pkg/azuredx/datasource.go Outdated Show resolved Hide resolved
pkg/azuredx/datasource.go Outdated Show resolved Hide resolved
pkg/azuredx/adxauth/adxusercontext/from_req.go Outdated Show resolved Hide resolved
pkg/azuredx/adxauth/adxusercontext/from_req.go Outdated Show resolved Hide resolved
@kostrse
Copy link
Contributor Author

kostrse commented Dec 16, 2022

I created a separate issue to remove remaining headers out of the modelQuery, it should not need backend.User anymore:

@andresmgot
Copy link
Contributor

There is one comment left and some spell / lint errors, let me know if you need any help with that

@kostrse
Copy link
Contributor Author

kostrse commented Dec 20, 2022

There is one comment left and some spell / lint errors, let me know if you need any help with that

Tests fixed.

Sergey Kostrukov added 2 commits December 20, 2022 02:41
Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the changes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants