Add authentication #38
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
pkg/influx/api.go
Outdated
| @@ -27,8 +27,15 @@ type API struct { | |||
| recorder Recorder | |||
| } | |||
|
|
|||
| func (a *API) Register(server *server.Server, authMiddleware middleware.Interface) { | |||
| server.HTTP.Handle("/api/v1/push/influx/write", authMiddleware.Wrap(http.HandlerFunc(a.handleSeriesPush))) | |||
| func (a *API) Register(server *server.Server, enableAuth bool) { | |||
There was a problem hiding this comment.
Could you add tests to check enabling and disabling auth work as expected?
There was a problem hiding this comment.
Since this is a small change that will unblock other work, we think it's ok to merge this now and add the test in a later PR (since it will require some refactoring to create the foundation necessary to do so)
pkg/influx/api.go
Outdated
| httpAuthMiddleware := middleware.AuthenticateUser | ||
| httpAuthMiddleware.Wrap(handler) |
There was a problem hiding this comment.
the wrap function returns a wrapped result, so this needs to change to work:
| httpAuthMiddleware := middleware.AuthenticateUser | |
| httpAuthMiddleware.Wrap(handler) | |
| handler = middleware.AuthenticateUser.Wrap(handler) |
There was a problem hiding this comment.
(I think a small unit test could be written using InjectOrgIDIntoHTTPRequest -- the wrapper is here: https://github.com/grafana/mimir/blob/main/vendor/github.com/weaveworks/common/middleware/http_auth.go#L10 and makes a call to this code: https://github.com/grafana/mimir/blob/main/vendor/github.com/weaveworks/common/user/http.go#L23. A minimal artificial httprequest could be created to exercise this code)
There was a problem hiding this comment.
(I think a small unit test could be written using InjectOrgIDIntoHTTPRequest -- the wrapper is here: https://github.com/grafana/mimir/blob/main/vendor/github.com/weaveworks/common/middleware/http_auth.go#L10 and makes a call to this code: https://github.com/grafana/mimir/blob/main/vendor/github.com/weaveworks/common/user/http.go#L23. A minimal artificial httprequest could be created to exercise this code)
There was a problem hiding this comment.
Do you mean a unit test that creates a req with a userID injected into it, then verifies that its extracted properly (or errors as expected)?
There was a problem hiding this comment.
yeah that's what I mean -- again not worth totally blocking this if it's > 1/2 day of poking
carrieedwards
force-pushed
the
cedwards/add-authentication
branch
from
b328194
to
0fd4e7b
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
pkg/influx/auth_test.go
Outdated
| url: "/write", | ||
| data: "measurement,t1=v1 f1=2 1465839830100400200", | ||
| enableAuth: false, | ||
| orgID: "fake", |
There was a problem hiding this comment.
one more case to detect -- no orgid with auth off is also a success
There was a problem hiding this comment.
Yeah, it does test this because in auth is enabled, the test doesn't inject the org ID into the request, and relies on the middleware.HttpFakeAuth{} to inject the "fake" org ID into the request's context. This orgID for this test is just used to verify the "fake" orgID is added to the request context. But if there is a way to make this more clear, let me know!
|
Go coverage report: Click to expand.
Go lint report: No issues found. 😎 |
This PR replaces the fakeAuth middleware with middleware.Authenticate user, so that authentication can be done. The enableAuth flag is still available.