-
Notifications
You must be signed in to change notification settings - Fork 40
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
Conversation
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 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?
I created a separate issue to remove remaining headers out of the |
There is one comment left and some spell / lint errors, let me know if you need any help with that |
Tests fixed. |
# Conflicts: # pkg/azuredx/resource_handler_test.go
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.
LGTM, thanks for the changes!
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
andadxusercontext.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 forTestRequest
to use service identity viaGetServiceAccessToken(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.