feat: add session actor middleware #897
Conversation
|
Rationale for |
Codecov Report
@@ Coverage Diff @@
## main #897 +/- ##
==========================================
+ Coverage 65.92% 66.25% +0.32%
==========================================
Files 228 231 +3
Lines 14126 14293 +167
Branches 105 105
==========================================
+ Hits 9313 9470 +157
- Misses 3868 3877 +9
- Partials 945 946 +1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
I'm not sure if this is finished, so I won't be a complete stickler. I want to point out in it's current form, this violates the no unused packages rule, which makes this a lot harder to thoroughly review.
There was a problem hiding this comment.
It'd be helpful to understand the interaction of RBAC with the Actor. I think I get it, but I'm going to write it out to ensure I can give a thorough review:
- Actor interface gets sent into RBAC.
- RBAC uses the
ID()function to abstractly lookup roles. - That way all authentication types group into a single interface!
I see how we'd add actor types in the future and they'd be covered in this system, but would a function that returned a UUID for a resource perform similarly?
| // ID is the unique ID of the actor for logging purposes. | ||
| ID() string | ||
| // Name is a friendly, but consistent, name for the actor for logging | ||
| // purposes. E.g. "deansheather" | ||
| Name() string |
There was a problem hiding this comment.
Wouldn't this either be system or the user's name? Seems like certain routes should never be system, so I'm confused when the logging would come into play.
There was a problem hiding this comment.
On each request I want to log the actor name at least, so it's easy to see which user made a request in the logs. I am removing the SystemActor though.
There was a problem hiding this comment.
is this ok @kylecarbs? The goal of this is so we can log the username without having to do a type cast because of the same reason as above (type switching just to read the username is very slow).
There was a problem hiding this comment.
If we're just going to have the UserActor do we need to generalize the actor type at all? Could we just use database.User directly? That way we wouldn't have to type-cast at all.
There was a problem hiding this comment.
We're not just going to have the UserActor type. We will also have a WorkspaceActor and eventually a SatelliteActor.
There was a problem hiding this comment.
If we just have two or three though, could we just log the username / workspace / satellite name separately? If the actor type is primarily going to be used for that, I'm hesitant to say it's worth the grouping.
I'd think we'd have separate routes for users / workspaces / satellites anyways, similar to how agents can only access specific routes right now. In that pattern, there's no opportunity to have a user call an agent route, because they'll never need to do so.
There was a problem hiding this comment.
That is one way to separate things. But that makes each actor type more distinct.
RBAC means an agent token is just a UserActor with a reduced scope. I see the benefit in having 1 type and letting rbac decide what they can and can't do.
I see value in the separating routes approach too.
Both options require their own care. One thing I wonder about is will a provisionerd ever need to query coderd for anything? Like is there a case some provisionerd will want a system_user with some RBAC role/perms to query some extra endpoints? I have no idea if there is, but having 1 system is nice in the sense all actors are equivalent and just defer to rbac for drawing lines.
| // You should probably be calling session.ExtractActor as a middleware, or | ||
| // session.RequestActor instead. |
There was a problem hiding this comment.
If this is true, could we keep this function private?
There was a problem hiding this comment.
No because then the linter got mad at me for having the tests be package session, and I have test helpers shared between the user tests and the middleware tests :(
There was a problem hiding this comment.
Wouldn't testing the middleware test this function in that case? That's how it works in httpmw right now too.
There was a problem hiding this comment.
I wanted to write tests for this function by itself instead of having a giant test file for the middleware when the middleware itself is very small.
There was a problem hiding this comment.
Our tests may be deceiving to a caller if they primarily test and exposed a function we don't recommend they use. The middleware is only valuable if it calls this other function, so I'd be hesitant to say the middleware's expectations are small.
That's not to say decomposition isn't right for this case, but I do think our tests should primarily test the primary functions we expose.
There was a problem hiding this comment.
I'll do the internal tests for the entire package in that case then
There was a problem hiding this comment.
is this ok? @kylecarbs
There was a problem hiding this comment.
I think it's best to test the HTTP handler that's being exposed to the API; not being able to test that handler will likely result in a similar experience for callers. Is there a reason we can't test the handler with similar levels of functionality?
There was a problem hiding this comment.
The middleware handler is tested to handle all possible output states from the user actor function. The user actor tests additionally also test each possible failure state individually.
There was a problem hiding this comment.
@kylecarbs I disagree that we should only test a package api imo.
I am all for testing individual functions in a package. I understand unit tests can help api design, and those tests should still exist in the xxxx_test.go file. But having internal tests to test more granular functions is a great way to test edge cases without excess state.
If the internal test fails, that is way more information for debugging than if an api level function that hides the internal functions fails.
So I think internal tests are great. But should not replace package_test functions that better test pkg api design
| // You should probably be calling session.ExtractActor as a middleware, or | ||
| // session.RequestActor instead. |
There was a problem hiding this comment.
@deansheather you can do user_internal_test.go for package session tests
| } | ||
|
|
||
| // UserActorFromRequest tries to get a UserActor from the API key supplied in | ||
| // the request cookies. If the cookie doesn't exist, nil is returned. If there |
There was a problem hiding this comment.
Just a note in the future we need to also support a custom Header for cli auth. Cookie auth will need to do CSRF in future.
| // ID is the unique ID of the actor for logging purposes. | ||
| ID() string | ||
| // Name is a friendly, but consistent, name for the actor for logging | ||
| // purposes. E.g. "deansheather" | ||
| Name() string |
There was a problem hiding this comment.
That is one way to separate things. But that makes each actor type more distinct.
RBAC means an agent token is just a UserActor with a reduced scope. I see the benefit in having 1 type and letting rbac decide what they can and can't do.
I see value in the separating routes approach too.
Both options require their own care. One thing I wonder about is will a provisionerd ever need to query coderd for anything? Like is there a case some provisionerd will want a system_user with some RBAC role/perms to query some extra endpoints? I have no idea if there is, but having 1 system is nice in the sense all actors are equivalent and just defer to rbac for drawing lines.
| "github.com/stretchr/testify/require" | ||
|
|
||
| "github.com/coder/coder/coderd/database/databasefake" | ||
| "github.com/coder/coder/coderd/httpapi" |
There was a problem hiding this comment.
Just curious, are we still splitting coder and other github imports?
| // You should probably be calling session.ExtractActor as a middleware, or | ||
| // session.RequestActor instead. |
There was a problem hiding this comment.
@kylecarbs I disagree that we should only test a package api imo.
I am all for testing individual functions in a package. I understand unit tests can help api design, and those tests should still exist in the xxxx_test.go file. But having internal tests to test more granular functions is a great way to test edge cases without excess state.
If the internal test fails, that is way more information for debugging than if an api level function that hides the internal functions fails.
So I think internal tests are great. But should not replace package_test functions that better test pkg api design
| return NewUserActor(u, key), true | ||
| } | ||
|
|
||
| func parseAPIKey(apiKey string) (id, secret string, hashed []byte, err error) { |
There was a problem hiding this comment.
Just and example @kylecarbs of a great function to unit test with an easy tabled test. To do this using the middleware would be such a pain because invalid apiKey strings might not even get far enough in the function to get to this parse.
Regardless if the invalid strings reach this in prod, I still think it's of value to know this function works as intended incase it's ever moved/reused.
There was a problem hiding this comment.
If this function is well-tested outside of the middleware, it could be argued the middleware doesn't have to be. If both are expected to be well-tested, then tests become duplicative. I'm of the opinion testing the user-expected behavior is much more important than if the code actually works, because the user just wants it to work for them.
There was a problem hiding this comment.
The middleware unit test will not test this one function to all of it's edge cases. For one, the middleware does some basic validation above this usage, so any tokens that fall those checks won't make it to parseAPIKey.
The code might be able to be refactored to ensure all keys hit parseAPIKey, but I'm speaking more generally than this one case.
I'm of the opinion testing the user-expected behavior is much more important than if the code actually works, because the user just wants it to work for them.
I don't see how these are not related. These functions are the building blocks, and often they will be reused/adapted by a developer in the future (months out).
I am of the opinion if I write helper functions like this, it's very easy to build a tabled test to confirm my implementation is correct. And if someone comes in later, the tabled tests can often serve better than a comment for what should happen in given cases, because let's be honest sometimes comments are vague/ambiguous. A tabled test does not have this ambiguity. Worse case the tabled test is missing an edge case, in which case the new developer can easily add it.
I do agree if you sufficiently test all the building blocks of a function, then you do not need to test the, in this case, the middle function as exhaustively.
There was a problem hiding this comment.
The functions build the API surface, but the most important part of the code is that the API works as expected. I'm not arguing against testing the internals, but we should try to get away with testing via externals first to reduce complexity.
| func RequestActor(r *http.Request) session.Actor { | ||
| return session.RequestActor(r) | ||
| } | ||
|
|
||
| // ExtractActor reexports session.ExtractActor for your convenience. | ||
| func ExtractActor(db database.Store) func(http.Handler) http.Handler { | ||
| return session.ExtractActor(db) | ||
| } |
There was a problem hiding this comment.
Isn't this also defined in the access package? why is there 2 ExtractActor functions?
| @@ -76,6 +78,7 @@ func New(options *Options) (http.Handler, func()) { | |||
| }) | |||
| r.Route("/files", func(r chi.Router) { | |||
| r.Use( | |||
| httpmw.RequireAuthentication(), | |||
There was a problem hiding this comment.
Can we do this at a top level? I'd hate to forget it in one of these routes.
|
Superseded by #1050 |
Adds the base actor interface types (without RBAC methods for now, Steven will add them later):
And their basic implementations. Also adds middleware for extracting the actor from the request, which is implemented for UserActor and falls back to AnonymousActor if no cookie was set.
The existing apikey middleware was left in, and I will be making the transition to in a separate PR. I removed the OIDC/Oauth2 code when I copied the apikey middleware across and replaced it with a TODO.
#782