Integrate tenant authentication #92
Conversation
|
This pull request has been linked to Shortcut Story #10289: Implement Quarterdeck Authentication, Authorization, and CSRF Middleware. |
| srv.Shutdown() | ||
| <-stopped | ||
| logger.ResetLogger() | ||
| }) |
There was a problem hiding this comment.
Weird deadlock that only occurred when this test errored out. Previously the <-stopped was declared in the cleanup earlier in the method which caused a hang because the channel was never written to if the test errored.
| // API Keys management | ||
| ReadAPIKey = "read:key" | ||
| WriteAPIKey = "write:key" | ||
| DeleteAPIKey = "delete:key" |
There was a problem hiding this comment.
First crack at the permissions but do we want to have create and update as well?
There was a problem hiding this comment.
Nice! I'll post the permissions that quarterdeck has right now into the engineering channel; then we can figure out what is needed here.
Do you think Quarterdeck needs an API endpoint (possibly unauthenticated) that lists its permissions?
There was a problem hiding this comment.
Looks great! The endpoints are very well organized now and I think it will make it more maintainable in the future. Potentially we should move the setupRoutes function to a routes.go file as an easier reference in the future, but that's not something we have to do now.
Also thank you for going above and beyond and dealing with the tests as well - I really appreciate your hard work on this story!
| // SetCSRFProtect is a helper function to set CSRF cookies on the client. This is not | ||
| // possible in a browser because of the HttpOnly flag. This method should only be used | ||
| // for testing purposes and an error is returned if the URL is not localhost. For live | ||
| // clients - the server should set these cookies. If protect is false, then the cookies | ||
| // are removed from the client by setting the cookies to an empty slice. | ||
| func (c *APIv1) SetCSRFProtect(protect bool) error { |
There was a problem hiding this comment.
Is this implementation from the Admin API or is it different?
There was a problem hiding this comment.
It's from the BFF API
| // SetCredentials is a helper function for external users to override credentials at | ||
| // runtime by directly passing in the token, which is useful for testing. | ||
| // TODO: Pass in a credentials interface instead of the token string. | ||
| func (c *APIv1) SetCredentials(token string) { | ||
| c.creds = token | ||
| } |
| // API Keys management | ||
| ReadAPIKey = "read:key" | ||
| WriteAPIKey = "write:key" | ||
| DeleteAPIKey = "delete:key" |
There was a problem hiding this comment.
Nice! I'll post the permissions that quarterdeck has right now into the engineering channel; then we can figure out what is needed here.
Do you think Quarterdeck needs an API endpoint (possibly unauthenticated) that lists its permissions?
| srv.Shutdown() | ||
| <-stopped | ||
| logger.ResetLogger() | ||
| }) |
pkg/tenant/tenant.go
Outdated
| // In maintenance mode authentication is disabled | ||
| var authenticator gin.HandlerFunc | ||
| if !s.conf.Maintenance { |
There was a problem hiding this comment.
Were you getting a test error with authenticate in maintenance mode? I was hoping that we wouldn't have to worry about that here because the Available middleware comes before the Authenticate middleware, so it should stop processing before any authenticated endpoint. I'm worried that this logic might create a nil middleware accidentally in the future.
There was a problem hiding this comment.
I think the problem was that the maintenance mode test wasn't starting an authtest server so the code here was returning an error. However if we're worried about the nil middleware I can just update the test instead of having the check here.
There was a problem hiding this comment.
Perfect, I'd prefer that we update the test - thank you!
| { | ||
| apikeys.GET("", mw.Authorize(ReadAPIKey), s.APIKeyList) | ||
| apikeys.POST("", csrf, mw.Authorize(WriteAPIKey), s.APIKeyCreate) | ||
| apikeys.GET("/:apiKeyID", mw.Authorize(ReadAPIKey), s.APIKeyDetail) |
There was a problem hiding this comment.
The linter seems to be complaining about missing methods TopicCreate and APIKeyCreate would you mind either creating stub handlers that return http.StatusNotImplemented to appease the linter?
There was a problem hiding this comment.
Sure, I think we just ran into some merge conflicts from Danielle's PR.
pkg/tenant/tenant.go
Outdated
| // Set the maximum age of login protection cookies. | ||
| const doubleCookiesMaxAge = time.Minute * 10 | ||
|
|
||
| // ProtectLogin prepares the front-end for login by setting the double cookie | ||
| // tokens for CSRF protection. | ||
| func (s *Server) ProtectLogin(c *gin.Context) { | ||
| expiresAt := time.Now().Add(doubleCookiesMaxAge) | ||
| if err := mw.SetDoubleCookieToken(c, s.conf.Auth.CookieDomain, expiresAt); err != nil { | ||
| log.Error().Err(err).Msg("could not set cookies") | ||
| c.JSON(http.StatusInternalServerError, api.ErrorResponse("could not set cookies")) | ||
| return | ||
| } | ||
| c.JSON(http.StatusOK, &api.Reply{Success: true}) | ||
| } |
There was a problem hiding this comment.
Thank you for porting this over! Would you mind adding this to an auth.go file? Tenant will have login and register routes in the future so that file will be the starting place for it.
Scope of changes
This integrates the quarterdeck authentication and authorization middlewares implemented in a previous PR into the tenant server.
There is a fair bit of related changes made, mostly to get the tests to a working state again after the integration.
Fixes SC-10289
Type of change
Acceptance criteria
Tests should still run with the configured middleware and I believe the docker compose should at least start all the services.
Author checklist
Reviewer(s) checklist