feat: Implement list roles & enforce authorize examples #1273
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,122 @@ | ||
| package httpmw | ||
|
|
||
| import ( | ||
| "context" | ||
| "net/http" | ||
|
|
||
| "golang.org/x/xerrors" | ||
|
|
||
| "cdr.dev/slog" | ||
| "github.com/coder/coder/coderd/database" | ||
| "github.com/coder/coder/coderd/httpapi" | ||
| "github.com/coder/coder/coderd/rbac" | ||
| ) | ||
|
|
||
| // Authorize will enforce if the user roles can complete the action on the AuthObject. | ||
| // The organization and owner are found using the ExtractOrganization and | ||
| // ExtractUser middleware if present. | ||
| func Authorize(logger slog.Logger, auth *rbac.RegoAuthorizer, action rbac.Action) func(http.Handler) http.Handler { | ||
|
There was a problem hiding this comment.
What do you think about renaming this to There was a problem hiding this comment. I think
Another word that comes to mind is "Access". Idk, There was a problem hiding this comment. Fair enough. I'm primarily trying to display that the While it is authorizing, I'm nervous that this will get conflated with authentication really easily. There was a problem hiding this comment. yea this is classic authorization vs authentication. If you aren't familiar with it, it's easy to mix up. |
||
| return func(next http.Handler) http.Handler { | ||
| return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { | ||
| roles := UserRoles(r) | ||
| object := rbacObject(r) | ||
|
|
||
| if object.Type == "" { | ||
| panic("developer error: auth object has no type") | ||
| } | ||
|
|
||
| // First extract the object's owner and organization if present. | ||
| unknownOrg := r.Context().Value(organizationParamContextKey{}) | ||
| if organization, castOK := unknownOrg.(database.Organization); unknownOrg != nil { | ||
| if !castOK { | ||
| panic("developer error: organization param middleware not provided for authorize") | ||
| } | ||
| object = object.InOrg(organization.ID) | ||
| } | ||
|
|
||
| unknownOwner := r.Context().Value(userParamContextKey{}) | ||
| if owner, castOK := unknownOwner.(database.User); unknownOwner != nil { | ||
| if !castOK { | ||
| panic("developer error: user param middleware not provided for authorize") | ||
| } | ||
| object = object.WithOwner(owner.ID.String()) | ||
| } | ||
|
|
||
| err := auth.AuthorizeByRoleName(r.Context(), roles.ID.String(), roles.Roles, action, object) | ||
| if err != nil { | ||
| internalError := new(rbac.UnauthorizedError) | ||
| if xerrors.As(err, internalError) { | ||
| logger = logger.With(slog.F("internal", internalError.Internal())) | ||
| } | ||
| // Log information for debugging. This will be very helpful | ||
| // in the early days if we over secure endpoints. | ||
| logger.Warn(r.Context(), "unauthorized", | ||
| slog.F("roles", roles.Roles), | ||
| slog.F("user_id", roles.ID), | ||
| slog.F("username", roles.Username), | ||
| slog.F("route", r.URL.Path), | ||
| slog.F("action", action), | ||
| slog.F("object", object), | ||
| ) | ||
| httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{ | ||
| Message: err.Error(), | ||
| }) | ||
| return | ||
| } | ||
| next.ServeHTTP(rw, r) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| type authObjectKey struct{} | ||
|
|
||
| // APIKey returns the API key from the ExtractAPIKey handler. | ||
| func rbacObject(r *http.Request) rbac.Object { | ||
| obj, ok := r.Context().Value(authObjectKey{}).(rbac.Object) | ||
| if !ok { | ||
| panic("developer error: auth object middleware not provided") | ||
| } | ||
| return obj | ||
| } | ||
|
|
||
| // WithRBACObject sets the object for 'Authorize()' for all routes handled | ||
| // by this middleware. The important field to set is 'Type' | ||
| func WithRBACObject(object rbac.Object) func(http.Handler) http.Handler { | ||
|
There was a problem hiding this comment. It might be confusing that this is called |
||
| return func(next http.Handler) http.Handler { | ||
| return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { | ||
| ctx := context.WithValue(r.Context(), authObjectKey{}, object) | ||
| next.ServeHTTP(rw, r.WithContext(ctx)) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| // User roles are the 'subject' field of Authorize() | ||
| type userRolesKey struct{} | ||
|
|
||
| // UserRoles returns the API key from the ExtractUserRoles handler. | ||
| func UserRoles(r *http.Request) database.GetAllUserRolesRow { | ||
| apiKey, ok := r.Context().Value(userRolesKey{}).(database.GetAllUserRolesRow) | ||
| if !ok { | ||
| panic("developer error: user roles middleware not provided") | ||
| } | ||
| return apiKey | ||
| } | ||
|
|
||
| // ExtractUserRoles requires authentication using a valid API key. | ||
| func ExtractUserRoles(db database.Store) func(http.Handler) http.Handler { | ||
| return func(next http.Handler) http.Handler { | ||
| return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { | ||
| apiKey := APIKey(r) | ||
| role, err := db.GetAllUserRoles(r.Context(), apiKey.UserID) | ||
| if err != nil { | ||
| httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{ | ||
| Message: "roles not found", | ||
| }) | ||
| return | ||
| } | ||
|
|
||
| ctx := context.WithValue(r.Context(), userRolesKey{}, role) | ||
| next.ServeHTTP(rw, r.WithContext(ctx)) | ||
| }) | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Does this need to be exposed via options? It doesn't seem to be used outside of here.
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 was thinking of making it an interface that is "denyall" or something to trigger in unit tests. But for now, we don't, so I'll drop it from
OptionsThere 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.
Ah wait yes. @kylecarbs this will be needed if you decide to do the rbac
Authorize()check manually inside the function, instead of the middleware. So this is required.Uh oh!
There was an error while loading. Please reload this page.
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.
This could go on
apiinstead, that way tests couldn't mistakenly pass it viaOptions.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.
Right now nothing else is on the api only like that.
coder/coderd/coderd.go
Lines 333 to 338 in db04d67
I don't think it'd be bad to pass one in via options 🤷♂️
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.
Oh my bad, I thought there was precedent for this 🤦
If you feel like it's fine I'll defer, but I generally think it's hasty to expose a value unless it needs to be used elsewhere. It's really easy to expose a parameter, but much harder to hide one.