From 30e203196965ebcfc0b9bfd640c326fc76870ee7 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 2 May 2022 23:12:56 -0500 Subject: [PATCH 01/15] feat: First draft of adding authorize to an http endpoint --- coderd/coderd.go | 42 +++++++- coderd/database/querier.go | 1 + coderd/database/queries.sql.go | 40 ++++++++ coderd/database/queries/users.sql | 12 +++ coderd/httpmw/authorize.go | 160 ++++++++++++++++++++++++++++++ coderd/rbac/builtin.go | 37 +++++++ coderd/rbac/object.go | 7 ++ coderd/roles.go | 24 +++++ 8 files changed, 318 insertions(+), 5 deletions(-) create mode 100644 coderd/httpmw/authorize.go create mode 100644 coderd/roles.go diff --git a/coderd/coderd.go b/coderd/coderd.go index b1cb613fc891b..0c36e40ce1ed9 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -11,6 +11,7 @@ import ( "github.com/go-chi/chi/v5" "github.com/pion/webrtc/v3" + "golang.org/x/xerrors" "google.golang.org/api/idtoken" chitrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/go-chi/chi.v5" @@ -22,6 +23,7 @@ import ( "github.com/coder/coder/coderd/gitsshkey" "github.com/coder/coder/coderd/httpapi" "github.com/coder/coder/coderd/httpmw" + "github.com/coder/coder/coderd/rbac" "github.com/coder/coder/coderd/turnconn" "github.com/coder/coder/codersdk" "github.com/coder/coder/site" @@ -47,6 +49,7 @@ type Options struct { SecureAuthCookie bool SSHKeygenAlgorithm gitsshkey.Algorithm TURNServer *turnconn.Server + Authorizer *rbac.RegoAuthorizer } // New constructs the Coder API into an HTTP handler. @@ -60,6 +63,15 @@ func New(options *Options) (http.Handler, func()) { if options.APIRateLimit == 0 { options.APIRateLimit = 512 } + if options.Authorizer == nil { + var err error + options.Authorizer, err = rbac.NewAuthorizer() + if err != nil { + // This should never happen, as the unit tests would fail if the + // default built in authorizer failed. + panic(xerrors.Errorf("rego authorize panic: %w", err)) + } + } api := &api{ Options: options, } @@ -67,6 +79,12 @@ func New(options *Options) (http.Handler, func()) { Github: options.GithubOAuth2Config, }) + authRolesMiddleware := httpmw.ExtractUserRoles(options.Database) + + authorize := func(actions ...rbac.Action) func(http.Handler) http.Handler { + return httpmw.Authorize(api.Logger, api.Authorizer, actions...) + } + r := chi.NewRouter() r.Route("/api/v2", func(r chi.Router) { r.Use( @@ -102,6 +120,9 @@ func New(options *Options) (http.Handler, func()) { r.Use( apiKeyMiddleware, httpmw.ExtractOrganizationParam(options.Database), + authRolesMiddleware, + // All authorize() functions will be scoped to this organization + httpmw.InOrg(httpmw.OrganizationParam), ) r.Get("/", api.organization) r.Get("/provisionerdaemons", api.provisionerDaemonsByOrganization) @@ -121,6 +142,9 @@ func New(options *Options) (http.Handler, func()) { }) }) r.Route("/members", func(r chi.Router) { + r.Route("/roles", func(r chi.Router) { + r.With(httpmw.Object(rbac.ResourceUserRole), authorize(rbac.ActionCreate, rbac.ActionDelete)).Get("/", api.assignableOrgRoles) + }) r.Route("/{user}", func(r chi.Router) { r.Use( httpmw.ExtractUserParam(options.Database), @@ -183,20 +207,28 @@ func New(options *Options) (http.Handler, func()) { }) }) r.Group(func(r chi.Router) { - r.Use(apiKeyMiddleware) + r.Use( + apiKeyMiddleware, + authRolesMiddleware, + ) r.Post("/", api.postUser) r.Get("/", api.users) + // These routes query information about site wide roles. + r.Route("/roles", func(r chi.Router) { + // Can create/delete all roles to view this endpoint + r.With(httpmw.Object(rbac.ResourceUserRole), authorize(rbac.ActionCreate, rbac.ActionDelete)).Get("/", api.assignableSiteRoles) + }) r.Route("/{user}", func(r chi.Router) { r.Use(httpmw.ExtractUserParam(options.Database)) r.Get("/", api.userByName) r.Put("/profile", api.putUserProfile) r.Put("/suspend", api.putUserSuspend) - // TODO: @emyrk Might want to move these to a /roles group instead of /user. - // As we include more roles like org roles, it makes less sense to scope these here. - r.Put("/roles", api.putUserRoles) - r.Get("/roles", api.userRoles) r.Get("/organizations", api.organizationsByUser) r.Post("/organizations", api.postOrganizationsByUser) + // These roles apply to the site wide permissions. + r.Put("/roles", api.putUserRoles) + r.Get("/roles", api.userRoles) + r.Post("/keys", api.postAPIKey) r.Route("/organizations", func(r chi.Router) { r.Post("/", api.postOrganizationsByUser) diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 24c459d9d3744..172c64873c218 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -13,6 +13,7 @@ type querier interface { DeleteGitSSHKey(ctx context.Context, userID uuid.UUID) error DeleteParameterValueByID(ctx context.Context, id uuid.UUID) error GetAPIKeyByID(ctx context.Context, id string) (APIKey, error) + GetAllUserRoles(ctx context.Context, userID uuid.UUID) ([]GetAllUserRolesRow, error) GetFileByHash(ctx context.Context, hash string) (File, error) GetGitSSHKey(ctx context.Context, userID uuid.UUID) (GitSSHKey, error) GetOrganizationByID(ctx context.Context, id uuid.UUID) (Organization, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 93c20c7095b7c..2b53799c18196 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -1888,6 +1888,46 @@ func (q *sqlQuerier) UpdateTemplateVersionByID(ctx context.Context, arg UpdateTe return err } +const getAllUserRoles = `-- name: GetAllUserRoles :many +SELECT + id, username, array_cat(users.rbac_roles, organization_members.roles) :: text[] AS roles +FROM + users +LEFT JOIN organization_members + ON id = user_id +WHERE + id = $1 +` + +type GetAllUserRolesRow struct { + ID uuid.UUID `db:"id" json:"id"` + Username string `db:"username" json:"username"` + Roles []string `db:"roles" json:"roles"` +} + +func (q *sqlQuerier) GetAllUserRoles(ctx context.Context, userID uuid.UUID) ([]GetAllUserRolesRow, error) { + rows, err := q.db.QueryContext(ctx, getAllUserRoles, userID) + if err != nil { + return nil, err + } + defer rows.Close() + var items []GetAllUserRolesRow + for rows.Next() { + var i GetAllUserRolesRow + if err := rows.Scan(&i.ID, &i.Username, pq.Array(&i.Roles)); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Close(); err != nil { + return nil, err + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const getUserByEmailOrUsername = `-- name: GetUserByEmailOrUsername :one SELECT id, email, username, hashed_password, created_at, updated_at, status, rbac_roles diff --git a/coderd/database/queries/users.sql b/coderd/database/queries/users.sql index aa94117ec2aff..2389de7a1fab4 100644 --- a/coderd/database/queries/users.sql +++ b/coderd/database/queries/users.sql @@ -122,3 +122,15 @@ SET updated_at = $3 WHERE id = $1 RETURNING *; + + +-- name: GetAllUserRoles :many +SELECT + -- username is returned just to help for logging purposes + id, username, array_cat(users.rbac_roles, organization_members.roles) :: text[] AS roles +FROM + users +LEFT JOIN organization_members + ON id = user_id +WHERE + id = @user_id; \ No newline at end of file diff --git a/coderd/httpmw/authorize.go b/coderd/httpmw/authorize.go new file mode 100644 index 0000000000000..fa1fe2578119a --- /dev/null +++ b/coderd/httpmw/authorize.go @@ -0,0 +1,160 @@ +package httpmw + +import ( + "context" + "fmt" + "net/http" + + "github.com/google/uuid" + + "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" +) + +type AuthObject struct { + // WithUser sets the owner of the object to the value returned by the func + WithUser func(r *http.Request) uuid.UUID + + // InOrg sets the org owner of the object to the value returned by the func + InOrg func(r *http.Request) uuid.UUID + + // WithOwner sets the object id to the value returned by the func + WithOwner func(r *http.Request) uuid.UUID + + // Object is that base static object the above functions can modify. + Object rbac.Object + //// Actions are the various actions the middleware will check can be done on the object. + //Actions []rbac.Action +} + +func WithOwner(owner func(r *http.Request) database.User) func(http.Handler) http.Handler { + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + ao := GetAuthObject(r) + ao.WithOwner = func(r *http.Request) uuid.UUID { + return owner(r).ID + } + + ctx := context.WithValue(r.Context(), authObjectKey{}, ao) + next.ServeHTTP(rw, r.WithContext(ctx)) + }) + } +} + +func InOrg(org func(r *http.Request) database.Organization) func(http.Handler) http.Handler { + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + ao := GetAuthObject(r) + ao.InOrg = func(r *http.Request) uuid.UUID { + return org(r).ID + } + + ctx := context.WithValue(r.Context(), authObjectKey{}, ao) + next.ServeHTTP(rw, r.WithContext(ctx)) + }) + } +} + +// Authorize allows for static object & action authorize checking. If the object is a static object, this is an easy way +// to enforce the route. +func Authorize(logger slog.Logger, auth *rbac.RegoAuthorizer, actions ...rbac.Action) func(http.Handler) http.Handler { + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + roles := UserRoles(r) + args := GetAuthObject(r) + + object := args.Object + if args.InOrg != nil { + object.InOrg(args.InOrg(r)) + } + if args.WithUser != nil { + object.WithOwner(args.InOrg(r).String()) + } + if args.WithOwner != nil { + object.WithID(args.InOrg(r).String()) + } + + // Error on the first action that fails + for _, act := range actions { + err := auth.AuthorizeByRoleName(r.Context(), roles.ID.String(), roles.Roles, act, object) + if err != nil { + var internalError *rbac.UnauthorizedError + if xerrors.As(err, internalError) { + logger = logger.With(slog.F("internal", internalError.Internal())) + } + 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", act), + 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 GetAuthObject(r *http.Request) AuthObject { + obj, ok := r.Context().Value(authObjectKey{}).(AuthObject) + if !ok { + return AuthObject{} + } + return obj +} + +func Object(object rbac.Object) func(http.Handler) http.Handler { + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + ao := GetAuthObject(r) + ao.Object = object + + ctx := context.WithValue(r.Context(), authObjectKey{}, ao) + next.ServeHTTP(rw, r.WithContext(ctx)) + }) + } +} + +// User roles are the 'subject' field of Authorize() +type userRolesKey struct{} + +// APIKey returns the API key from the ExtractAPIKey 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: fmt.Sprintf("roles not found", AuthCookie), + }) + return + } + + ctx := context.WithValue(r.Context(), userRolesKey{}, role) + next.ServeHTTP(rw, r.WithContext(ctx)) + }) + } +} diff --git a/coderd/rbac/builtin.go b/coderd/rbac/builtin.go index 4d659e6e8e39f..1aeb2535e7851 100644 --- a/coderd/rbac/builtin.go +++ b/coderd/rbac/builtin.go @@ -145,6 +145,43 @@ func IsOrgRole(roleName string) (string, bool) { return "", false } +// ListOrgRoles lists all roles that can be applied to an organization user +// in the given organization. +// Note: This should be a list in a database, but until then we build +// the list from the builtins. +func ListOrgRoles(organizationID uuid.UUID) []string { + var roles []string + for role := range builtInRoles { + _, scope, err := roleSplit(role) + if err != nil { + // This should never happen + continue + } + if scope == organizationID.String() { + roles = append(roles, role) + } + } + return roles +} + +// ListSiteRoles lists all roles that can be applied to a user. +// Note: This should be a list in a database, but until then we build +// the list from the builtins. +func ListSiteRoles() []string { + var roles []string + for role := range builtInRoles { + _, scope, err := roleSplit(role) + if err != nil { + // This should never happen + continue + } + if scope == "" { + roles = append(roles, role) + } + } + return roles +} + // roleName is a quick helper function to return // role_name:scopeID // If no scopeID is required, only 'role_name' is returned diff --git a/coderd/rbac/object.go b/coderd/rbac/object.go index 1e86165f24cf4..a4be9b1edab5b 100644 --- a/coderd/rbac/object.go +++ b/coderd/rbac/object.go @@ -17,6 +17,13 @@ var ( Type: "template", } + // ResourceUserRole might be expanded later to allow more granular permissions + // to modifying roles. For now, this covers all possible roles, so having this permission + // allows granting/deleting **ALL** roles. + ResourceUserRole = Object{ + Type: "user_role", + } + // ResourceWildcard represents all resource types ResourceWildcard = Object{ Type: WildcardSymbol, diff --git a/coderd/roles.go b/coderd/roles.go new file mode 100644 index 0000000000000..fea73a922ca26 --- /dev/null +++ b/coderd/roles.go @@ -0,0 +1,24 @@ +package coderd + +import ( + "net/http" + + "github.com/coder/coder/coderd/httpapi" + "github.com/coder/coder/coderd/rbac" +) + +// assignableSiteRoles returns all site wide roles that can be assigned. +func (api *api) assignableSiteRoles(rw http.ResponseWriter, r *http.Request) { + // TODO: @emyrk in the future, allow granular subsets of roles to be returned based on the + // role of the user. + roles := rbac.ListSiteRoles() + httpapi.Write(rw, http.StatusOK, roles) +} + +// assignableSiteRoles returns all site wide roles that can be assigned. +func (api *api) assignableOrgRoles(rw http.ResponseWriter, r *http.Request) { + // TODO: @emyrk in the future, allow granular subsets of roles to be returned based on the + // role of the user. + roles := rbac.ListSiteRoles() + httpapi.Write(rw, http.StatusOK, roles) +} From 2161f846af25c6ad09b4ca334f42fea0c1a14c65 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 3 May 2022 08:22:55 -0500 Subject: [PATCH 02/15] WIP: Using middleware to change auth object params Authorize can be changed dynamically with middlewares. --- coderd/coderd.go | 9 +++--- coderd/httpmw/authorize.go | 49 +++++++++++++++--------------- coderd/httpmw/organizationparam.go | 4 +++ coderd/rbac/authz.go | 1 + 4 files changed, 34 insertions(+), 29 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 0c36e40ce1ed9..600e1247bf7e2 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -81,8 +81,8 @@ func New(options *Options) (http.Handler, func()) { authRolesMiddleware := httpmw.ExtractUserRoles(options.Database) - authorize := func(actions ...rbac.Action) func(http.Handler) http.Handler { - return httpmw.Authorize(api.Logger, api.Authorizer, actions...) + authorize := func(f http.HandlerFunc, actions rbac.Action) func(http.Handler) http.Handler { + return httpmw.Authorize(api.Logger, api.Authorizer, actions) } r := chi.NewRouter() @@ -121,8 +121,6 @@ func New(options *Options) (http.Handler, func()) { apiKeyMiddleware, httpmw.ExtractOrganizationParam(options.Database), authRolesMiddleware, - // All authorize() functions will be scoped to this organization - httpmw.InOrg(httpmw.OrganizationParam), ) r.Get("/", api.organization) r.Get("/provisionerdaemons", api.provisionerDaemonsByOrganization) @@ -143,7 +141,8 @@ func New(options *Options) (http.Handler, func()) { }) r.Route("/members", func(r chi.Router) { r.Route("/roles", func(r chi.Router) { - r.With(httpmw.Object(rbac.ResourceUserRole), authorize(rbac.ActionCreate, rbac.ActionDelete)).Get("/", api.assignableOrgRoles) + r.Use(httpmw.Object(rbac.ResourceUserRole)) + r.Get("/", authorize(api.assignableOrgRoles, rbac.ActionCreate)) }) r.Route("/{user}", func(r chi.Router) { r.Use( diff --git a/coderd/httpmw/authorize.go b/coderd/httpmw/authorize.go index fa1fe2578119a..34efc301b27ce 100644 --- a/coderd/httpmw/authorize.go +++ b/coderd/httpmw/authorize.go @@ -27,11 +27,9 @@ type AuthObject struct { // Object is that base static object the above functions can modify. Object rbac.Object - //// Actions are the various actions the middleware will check can be done on the object. - //Actions []rbac.Action } -func WithOwner(owner func(r *http.Request) database.User) func(http.Handler) http.Handler { +func RBACWithOwner(owner func(r *http.Request) database.User) func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { ao := GetAuthObject(r) @@ -45,7 +43,7 @@ func WithOwner(owner func(r *http.Request) database.User) func(http.Handler) htt } } -func InOrg(org func(r *http.Request) database.Organization) func(http.Handler) http.Handler { +func RBACInOrg(org func(r *http.Request) database.Organization) func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { ao := GetAuthObject(r) @@ -61,13 +59,18 @@ func InOrg(org func(r *http.Request) database.Organization) func(http.Handler) h // Authorize allows for static object & action authorize checking. If the object is a static object, this is an easy way // to enforce the route. -func Authorize(logger slog.Logger, auth *rbac.RegoAuthorizer, actions ...rbac.Action) func(http.Handler) http.Handler { +func Authorize(logger slog.Logger, auth *rbac.RegoAuthorizer, action rbac.Action) func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { roles := UserRoles(r) args := GetAuthObject(r) object := args.Object + organization, ok := r.Context().Value(organizationParamContextKey{}).(database.Organization) + if ok { + object = object.InOrg(organization.ID) + } + if args.InOrg != nil { object.InOrg(args.InOrg(r)) } @@ -79,26 +82,24 @@ func Authorize(logger slog.Logger, auth *rbac.RegoAuthorizer, actions ...rbac.Ac } // Error on the first action that fails - for _, act := range actions { - err := auth.AuthorizeByRoleName(r.Context(), roles.ID.String(), roles.Roles, act, object) - if err != nil { - var internalError *rbac.UnauthorizedError - if xerrors.As(err, internalError) { - logger = logger.With(slog.F("internal", internalError.Internal())) - } - 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", act), - slog.F("object", object), - ) - httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{ - Message: err.Error(), - }) - return + err := auth.AuthorizeByRoleName(r.Context(), roles.ID.String(), roles.Roles, action, object) + if err != nil { + var internalError *rbac.UnauthorizedError + if xerrors.As(err, internalError) { + logger = logger.With(slog.F("internal", internalError.Internal())) } + 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) }) diff --git a/coderd/httpmw/organizationparam.go b/coderd/httpmw/organizationparam.go index d66e49236672e..e91a2fd2f8e8d 100644 --- a/coderd/httpmw/organizationparam.go +++ b/coderd/httpmw/organizationparam.go @@ -77,6 +77,10 @@ func ExtractOrganizationParam(db database.Store) func(http.Handler) http.Handler ctx := context.WithValue(r.Context(), organizationParamContextKey{}, organization) ctx = context.WithValue(ctx, organizationMemberParamContextKey{}, organizationMember) + + next = RBACInOrg(func(r *http.Request) database.Organization { + return organization + })(next) next.ServeHTTP(rw, r.WithContext(ctx)) }) } diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go index f11508ad2b099..8ed2832102329 100644 --- a/coderd/rbac/authz.go +++ b/coderd/rbac/authz.go @@ -50,6 +50,7 @@ func (a RegoAuthorizer) AuthorizeByRoleName(ctx context.Context, subjectID strin } roles = append(roles, r) } + return a.Authorize(ctx, subjectID, roles, action, object) } From 54bc0543b153f33a07219eae3a6f68ab13adccf2 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 3 May 2022 09:26:55 -0500 Subject: [PATCH 03/15] feat: Implement basic authorize and unit test --- coderd/coderd.go | 10 +- coderd/database/databasefake/databasefake.go | 31 +++++ coderd/database/querier.go | 2 +- coderd/database/queries.sql.go | 29 +--- coderd/database/queries/users.sql | 2 +- coderd/httpmw/authorize.go | 67 +++------- coderd/httpmw/organizationparam.go | 3 - coderd/roles_test.go | 132 +++++++++++++++++++ codersdk/roles.go | 36 +++++ 9 files changed, 229 insertions(+), 83 deletions(-) create mode 100644 coderd/roles_test.go create mode 100644 codersdk/roles.go diff --git a/coderd/coderd.go b/coderd/coderd.go index 600e1247bf7e2..1c97fb140b215 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -81,8 +81,8 @@ func New(options *Options) (http.Handler, func()) { authRolesMiddleware := httpmw.ExtractUserRoles(options.Database) - authorize := func(f http.HandlerFunc, actions rbac.Action) func(http.Handler) http.Handler { - return httpmw.Authorize(api.Logger, api.Authorizer, actions) + authorize := func(f http.HandlerFunc, actions rbac.Action) http.HandlerFunc { + return httpmw.Authorize(api.Logger, api.Authorizer, actions)(f).ServeHTTP } r := chi.NewRouter() @@ -141,7 +141,7 @@ func New(options *Options) (http.Handler, func()) { }) r.Route("/members", func(r chi.Router) { r.Route("/roles", func(r chi.Router) { - r.Use(httpmw.Object(rbac.ResourceUserRole)) + r.Use(httpmw.RBACObject(rbac.ResourceUserRole)) r.Get("/", authorize(api.assignableOrgRoles, rbac.ActionCreate)) }) r.Route("/{user}", func(r chi.Router) { @@ -214,8 +214,8 @@ func New(options *Options) (http.Handler, func()) { r.Get("/", api.users) // These routes query information about site wide roles. r.Route("/roles", func(r chi.Router) { - // Can create/delete all roles to view this endpoint - r.With(httpmw.Object(rbac.ResourceUserRole), authorize(rbac.ActionCreate, rbac.ActionDelete)).Get("/", api.assignableSiteRoles) + r.Use(httpmw.RBACObject(rbac.ResourceUserRole)) + r.Get("/", authorize(api.assignableSiteRoles, rbac.ActionCreate)) }) r.Route("/{user}", func(r chi.Router) { r.Use(httpmw.ExtractUserParam(options.Database)) diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go index 827bdeed5fcbb..6486d4cb9e00f 100644 --- a/coderd/database/databasefake/databasefake.go +++ b/coderd/database/databasefake/databasefake.go @@ -242,6 +242,37 @@ func (q *fakeQuerier) GetUsers(_ context.Context, params database.GetUsersParams return tmp, nil } +func (q *fakeQuerier) GetAllUserRoles(ctx context.Context, userID uuid.UUID) (database.GetAllUserRolesRow, error) { + q.mutex.RLock() + defer q.mutex.RUnlock() + + var user *database.User + roles := make([]string, 0) + for _, u := range q.users { + if u.ID == userID { + roles = append(roles, u.RBACRoles...) + user = &u + break + } + } + + for _, mem := range q.organizationMembers { + if mem.UserID == userID { + roles = append(roles, mem.Roles...) + } + } + + if user == nil { + return database.GetAllUserRolesRow{}, sql.ErrNoRows + } + + return database.GetAllUserRolesRow{ + ID: userID, + Username: user.Username, + Roles: roles, + }, nil +} + func (q *fakeQuerier) GetWorkspacesByTemplateID(_ context.Context, arg database.GetWorkspacesByTemplateIDParams) ([]database.Workspace, error) { q.mutex.RLock() defer q.mutex.RUnlock() diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 172c64873c218..0d7b21ed85b26 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -13,7 +13,7 @@ type querier interface { DeleteGitSSHKey(ctx context.Context, userID uuid.UUID) error DeleteParameterValueByID(ctx context.Context, id uuid.UUID) error GetAPIKeyByID(ctx context.Context, id string) (APIKey, error) - GetAllUserRoles(ctx context.Context, userID uuid.UUID) ([]GetAllUserRolesRow, error) + GetAllUserRoles(ctx context.Context, userID uuid.UUID) (GetAllUserRolesRow, error) GetFileByHash(ctx context.Context, hash string) (File, error) GetGitSSHKey(ctx context.Context, userID uuid.UUID) (GitSSHKey, error) GetOrganizationByID(ctx context.Context, id uuid.UUID) (Organization, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 2b53799c18196..7e2c919b43dcc 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -1888,8 +1888,9 @@ func (q *sqlQuerier) UpdateTemplateVersionByID(ctx context.Context, arg UpdateTe return err } -const getAllUserRoles = `-- name: GetAllUserRoles :many +const getAllUserRoles = `-- name: GetAllUserRoles :one SELECT + -- username is returned just to help for logging purposes id, username, array_cat(users.rbac_roles, organization_members.roles) :: text[] AS roles FROM users @@ -1905,27 +1906,11 @@ type GetAllUserRolesRow struct { Roles []string `db:"roles" json:"roles"` } -func (q *sqlQuerier) GetAllUserRoles(ctx context.Context, userID uuid.UUID) ([]GetAllUserRolesRow, error) { - rows, err := q.db.QueryContext(ctx, getAllUserRoles, userID) - if err != nil { - return nil, err - } - defer rows.Close() - var items []GetAllUserRolesRow - for rows.Next() { - var i GetAllUserRolesRow - if err := rows.Scan(&i.ID, &i.Username, pq.Array(&i.Roles)); err != nil { - return nil, err - } - items = append(items, i) - } - if err := rows.Close(); err != nil { - return nil, err - } - if err := rows.Err(); err != nil { - return nil, err - } - return items, nil +func (q *sqlQuerier) GetAllUserRoles(ctx context.Context, userID uuid.UUID) (GetAllUserRolesRow, error) { + row := q.db.QueryRowContext(ctx, getAllUserRoles, userID) + var i GetAllUserRolesRow + err := row.Scan(&i.ID, &i.Username, pq.Array(&i.Roles)) + return i, err } const getUserByEmailOrUsername = `-- name: GetUserByEmailOrUsername :one diff --git a/coderd/database/queries/users.sql b/coderd/database/queries/users.sql index 2389de7a1fab4..2f0ecb3709b9b 100644 --- a/coderd/database/queries/users.sql +++ b/coderd/database/queries/users.sql @@ -124,7 +124,7 @@ WHERE id = $1 RETURNING *; --- name: GetAllUserRoles :many +-- name: GetAllUserRoles :one SELECT -- username is returned just to help for logging purposes id, username, array_cat(users.rbac_roles, organization_members.roles) :: text[] AS roles diff --git a/coderd/httpmw/authorize.go b/coderd/httpmw/authorize.go index 34efc301b27ce..69d75d71c6b4f 100644 --- a/coderd/httpmw/authorize.go +++ b/coderd/httpmw/authorize.go @@ -5,8 +5,6 @@ import ( "fmt" "net/http" - "github.com/google/uuid" - "golang.org/x/xerrors" "cdr.dev/slog" @@ -15,48 +13,13 @@ import ( "github.com/coder/coder/coderd/rbac" ) +// AuthObject wraps the rbac object type for middleware to customize this value +// before being passed to Authorize(). type AuthObject struct { - // WithUser sets the owner of the object to the value returned by the func - WithUser func(r *http.Request) uuid.UUID - - // InOrg sets the org owner of the object to the value returned by the func - InOrg func(r *http.Request) uuid.UUID - - // WithOwner sets the object id to the value returned by the func - WithOwner func(r *http.Request) uuid.UUID - // Object is that base static object the above functions can modify. Object rbac.Object } -func RBACWithOwner(owner func(r *http.Request) database.User) func(http.Handler) http.Handler { - return func(next http.Handler) http.Handler { - return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { - ao := GetAuthObject(r) - ao.WithOwner = func(r *http.Request) uuid.UUID { - return owner(r).ID - } - - ctx := context.WithValue(r.Context(), authObjectKey{}, ao) - next.ServeHTTP(rw, r.WithContext(ctx)) - }) - } -} - -func RBACInOrg(org func(r *http.Request) database.Organization) func(http.Handler) http.Handler { - return func(next http.Handler) http.Handler { - return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { - ao := GetAuthObject(r) - ao.InOrg = func(r *http.Request) uuid.UUID { - return org(r).ID - } - - ctx := context.WithValue(r.Context(), authObjectKey{}, ao) - next.ServeHTTP(rw, r.WithContext(ctx)) - }) - } -} - // Authorize allows for static object & action authorize checking. If the object is a static object, this is an easy way // to enforce the route. func Authorize(logger slog.Logger, auth *rbac.RegoAuthorizer, action rbac.Action) func(http.Handler) http.Handler { @@ -66,25 +29,27 @@ func Authorize(logger slog.Logger, auth *rbac.RegoAuthorizer, action rbac.Action args := GetAuthObject(r) object := args.Object - organization, ok := r.Context().Value(organizationParamContextKey{}).(database.Organization) - if ok { + + 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) } - if args.InOrg != nil { - object.InOrg(args.InOrg(r)) - } - if args.WithUser != nil { - object.WithOwner(args.InOrg(r).String()) - } - if args.WithOwner != nil { - object.WithID(args.InOrg(r).String()) + 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()) } // Error on the first action that fails err := auth.AuthorizeByRoleName(r.Context(), roles.ID.String(), roles.Roles, action, object) if err != nil { - var internalError *rbac.UnauthorizedError + internalError := new(rbac.UnauthorizedError) if xerrors.As(err, internalError) { logger = logger.With(slog.F("internal", internalError.Internal())) } @@ -117,7 +82,7 @@ func GetAuthObject(r *http.Request) AuthObject { return obj } -func Object(object rbac.Object) func(http.Handler) http.Handler { +func RBACObject(object rbac.Object) func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { ao := GetAuthObject(r) diff --git a/coderd/httpmw/organizationparam.go b/coderd/httpmw/organizationparam.go index e91a2fd2f8e8d..668980a56324a 100644 --- a/coderd/httpmw/organizationparam.go +++ b/coderd/httpmw/organizationparam.go @@ -78,9 +78,6 @@ func ExtractOrganizationParam(db database.Store) func(http.Handler) http.Handler ctx := context.WithValue(r.Context(), organizationParamContextKey{}, organization) ctx = context.WithValue(ctx, organizationMemberParamContextKey{}, organizationMember) - next = RBACInOrg(func(r *http.Request) database.Organization { - return organization - })(next) next.ServeHTTP(rw, r.WithContext(ctx)) }) } diff --git a/coderd/roles_test.go b/coderd/roles_test.go new file mode 100644 index 0000000000000..fec407d8122fd --- /dev/null +++ b/coderd/roles_test.go @@ -0,0 +1,132 @@ +package coderd_test + +import ( + "context" + "net/http" + "testing" + + "github.com/coder/coder/coderd/rbac" + + "github.com/google/uuid" + + "github.com/coder/coder/codersdk" + + "github.com/stretchr/testify/require" + + "github.com/coder/coder/coderd/coderdtest" +) + +func TestListRoles(t *testing.T) { + t.Parallel() + + requireUnauthorized := func(t *testing.T, err error) { + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusUnauthorized, apiErr.StatusCode()) + require.Contains(t, apiErr.Message, "unauthorized") + } + + ctx := context.Background() + client := coderdtest.New(t, nil) + // Create admin, member, and org admin + admin := coderdtest.CreateFirstUser(t, client) + member := coderdtest.CreateAnotherUser(t, client, admin.OrganizationID) + + orgAdmin := coderdtest.CreateAnotherUser(t, client, admin.OrganizationID) + orgAdminUser, err := orgAdmin.User(ctx, codersdk.Me) + require.NoError(t, err) + + // TODO: @emyrk switch this to the admin when getting non-personal users is + // supported. `client.UpdateOrganizationMemberRoles(...)` + _, err = orgAdmin.UpdateOrganizationMemberRoles(ctx, admin.OrganizationID, orgAdminUser.ID, + codersdk.UpdateRoles{ + Roles: []string{rbac.RoleOrgMember(admin.OrganizationID), rbac.RoleOrgAdmin(admin.OrganizationID)}, + }, + ) + require.NoError(t, err) + + testCases := []struct { + Name string + Client *codersdk.Client + APICall func() ([]string, error) + ExpectedRoles []string + Authorized bool + }{ + { + Name: "MemberListSite", + APICall: func() ([]string, error) { + x, err := member.ListSiteRoles(ctx) + return x, err + }, + Authorized: false, + }, + { + Name: "OrgMemberListOrg", + APICall: func() ([]string, error) { + return member.ListOrgRoles(ctx, admin.OrganizationID) + }, + Authorized: false, + }, + { + Name: "NonOrgMemberListOrg", + APICall: func() ([]string, error) { + return member.ListOrgRoles(ctx, uuid.New()) + }, + Authorized: false, + }, + // Org admin + { + Name: "OrgAdminListSite", + APICall: func() ([]string, error) { + return orgAdmin.ListSiteRoles(ctx) + }, + Authorized: false, + }, + { + Name: "OrgAdminListOrg", + APICall: func() ([]string, error) { + return orgAdmin.ListOrgRoles(ctx, admin.OrganizationID) + }, + Authorized: true, + ExpectedRoles: rbac.ListOrgRoles(admin.OrganizationID), + }, + { + Name: "OrgAdminListOtherOrg", + APICall: func() ([]string, error) { + return orgAdmin.ListOrgRoles(ctx, uuid.New()) + }, + Authorized: false, + }, + // Admin + { + Name: "AdminListSite", + APICall: func() ([]string, error) { + return client.ListSiteRoles(ctx) + }, + Authorized: true, + ExpectedRoles: rbac.ListSiteRoles(), + }, + { + Name: "AdminListOrg", + APICall: func() ([]string, error) { + return client.ListOrgRoles(ctx, admin.OrganizationID) + }, + Authorized: true, + ExpectedRoles: rbac.ListOrgRoles(admin.OrganizationID), + }, + } + + for _, c := range testCases { + c := c + t.Run(c.Name, func(t *testing.T) { + t.Parallel() + roles, err := c.APICall() + if !c.Authorized { + requireUnauthorized(t, err) + } else { + require.NoError(t, err) + require.Equal(t, c.ExpectedRoles, roles) + } + }) + } +} diff --git a/codersdk/roles.go b/codersdk/roles.go new file mode 100644 index 0000000000000..f33d8c8afe9aa --- /dev/null +++ b/codersdk/roles.go @@ -0,0 +1,36 @@ +package codersdk + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + + "github.com/google/uuid" +) + +func (c *Client) ListSiteRoles(ctx context.Context) ([]string, error) { + res, err := c.request(ctx, http.MethodGet, "/api/v2/users/roles", nil) + if err != nil { + return nil, err + } + defer res.Body.Close() + if res.StatusCode != http.StatusOK { + return nil, readBodyAsError(res) + } + var roles []string + return roles, json.NewDecoder(res.Body).Decode(&roles) +} + +func (c *Client) ListOrgRoles(ctx context.Context, org uuid.UUID) ([]string, error) { + res, err := c.request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/organizations/%s/members/roles", org.String()), nil) + if err != nil { + return nil, err + } + defer res.Body.Close() + if res.StatusCode != http.StatusOK { + return nil, readBodyAsError(res) + } + var roles []string + return roles, json.NewDecoder(res.Body).Decode(&roles) +} From d083a7c87ecf363c009f61ec2674dff192cf28f9 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 3 May 2022 09:34:50 -0500 Subject: [PATCH 04/15] Some cleanup --- coderd/coderd.go | 5 +++-- coderd/httpmw/authorize.go | 16 ++++++++++++---- coderd/httpmw/organizationparam.go | 1 - coderd/rbac/authz.go | 1 - coderd/roles_test.go | 8 ++------ 5 files changed, 17 insertions(+), 14 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 1c97fb140b215..e59eeae658b07 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -79,6 +79,7 @@ func New(options *Options) (http.Handler, func()) { Github: options.GithubOAuth2Config, }) + // TODO: @emyrk we should just move this into 'ExtractAPIKey'. authRolesMiddleware := httpmw.ExtractUserRoles(options.Database) authorize := func(f http.HandlerFunc, actions rbac.Action) http.HandlerFunc { @@ -142,7 +143,7 @@ func New(options *Options) (http.Handler, func()) { r.Route("/members", func(r chi.Router) { r.Route("/roles", func(r chi.Router) { r.Use(httpmw.RBACObject(rbac.ResourceUserRole)) - r.Get("/", authorize(api.assignableOrgRoles, rbac.ActionCreate)) + r.Get("/", authorize(api.assignableOrgRoles, rbac.ActionRead)) }) r.Route("/{user}", func(r chi.Router) { r.Use( @@ -215,7 +216,7 @@ func New(options *Options) (http.Handler, func()) { // These routes query information about site wide roles. r.Route("/roles", func(r chi.Router) { r.Use(httpmw.RBACObject(rbac.ResourceUserRole)) - r.Get("/", authorize(api.assignableSiteRoles, rbac.ActionCreate)) + r.Get("/", authorize(api.assignableSiteRoles, rbac.ActionRead)) }) r.Route("/{user}", func(r chi.Router) { r.Use(httpmw.ExtractUserParam(options.Database)) diff --git a/coderd/httpmw/authorize.go b/coderd/httpmw/authorize.go index 69d75d71c6b4f..985b2cbaf5e9b 100644 --- a/coderd/httpmw/authorize.go +++ b/coderd/httpmw/authorize.go @@ -20,8 +20,9 @@ type AuthObject struct { Object rbac.Object } -// Authorize allows for static object & action authorize checking. If the object is a static object, this is an easy way -// to enforce the route. +// 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 { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { @@ -29,7 +30,11 @@ func Authorize(logger slog.Logger, auth *rbac.RegoAuthorizer, action rbac.Action args := GetAuthObject(r) object := args.Object + 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 { @@ -46,13 +51,14 @@ func Authorize(logger slog.Logger, auth *rbac.RegoAuthorizer, action rbac.Action object = object.WithOwner(owner.ID.String()) } - // Error on the first action that fails 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), @@ -77,11 +83,13 @@ type authObjectKey struct{} func GetAuthObject(r *http.Request) AuthObject { obj, ok := r.Context().Value(authObjectKey{}).(AuthObject) if !ok { - return AuthObject{} + panic("developer error: auth object middleware not provided") } return obj } +// RBACObject sets the object for 'Authorize()' for all routes handled +// by this middleware. The important field to set is 'Type' func RBACObject(object rbac.Object) func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { diff --git a/coderd/httpmw/organizationparam.go b/coderd/httpmw/organizationparam.go index 668980a56324a..d66e49236672e 100644 --- a/coderd/httpmw/organizationparam.go +++ b/coderd/httpmw/organizationparam.go @@ -77,7 +77,6 @@ func ExtractOrganizationParam(db database.Store) func(http.Handler) http.Handler ctx := context.WithValue(r.Context(), organizationParamContextKey{}, organization) ctx = context.WithValue(ctx, organizationMemberParamContextKey{}, organizationMember) - next.ServeHTTP(rw, r.WithContext(ctx)) }) } diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go index 8ed2832102329..f11508ad2b099 100644 --- a/coderd/rbac/authz.go +++ b/coderd/rbac/authz.go @@ -50,7 +50,6 @@ func (a RegoAuthorizer) AuthorizeByRoleName(ctx context.Context, subjectID strin } roles = append(roles, r) } - return a.Authorize(ctx, subjectID, roles, action, object) } diff --git a/coderd/roles_test.go b/coderd/roles_test.go index fec407d8122fd..43e690f3c56f1 100644 --- a/coderd/roles_test.go +++ b/coderd/roles_test.go @@ -5,15 +5,11 @@ import ( "net/http" "testing" + "github.com/coder/coder/coderd/coderdtest" "github.com/coder/coder/coderd/rbac" - - "github.com/google/uuid" - "github.com/coder/coder/codersdk" - + "github.com/google/uuid" "github.com/stretchr/testify/require" - - "github.com/coder/coder/coderd/coderdtest" ) func TestListRoles(t *testing.T) { From 1498dcd6dc5bbd68e985649d7c99c1be2e536b0c Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 3 May 2022 11:04:55 -0500 Subject: [PATCH 05/15] Expand 'orgs' to 'organizations' in func namings --- coderd/rbac/builtin.go | 4 ++-- coderd/roles_test.go | 14 +++++++------- codersdk/roles.go | 2 +- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/coderd/rbac/builtin.go b/coderd/rbac/builtin.go index 1aeb2535e7851..0206e87dc5e7b 100644 --- a/coderd/rbac/builtin.go +++ b/coderd/rbac/builtin.go @@ -145,11 +145,11 @@ func IsOrgRole(roleName string) (string, bool) { return "", false } -// ListOrgRoles lists all roles that can be applied to an organization user +// ListOrganizationRoles lists all roles that can be applied to an organization user // in the given organization. // Note: This should be a list in a database, but until then we build // the list from the builtins. -func ListOrgRoles(organizationID uuid.UUID) []string { +func ListOrganizationRoles(organizationID uuid.UUID) []string { var roles []string for role := range builtInRoles { _, scope, err := roleSplit(role) diff --git a/coderd/roles_test.go b/coderd/roles_test.go index 43e690f3c56f1..7390636fff24a 100644 --- a/coderd/roles_test.go +++ b/coderd/roles_test.go @@ -59,14 +59,14 @@ func TestListRoles(t *testing.T) { { Name: "OrgMemberListOrg", APICall: func() ([]string, error) { - return member.ListOrgRoles(ctx, admin.OrganizationID) + return member.ListOrganizationRoles(ctx, admin.OrganizationID) }, Authorized: false, }, { Name: "NonOrgMemberListOrg", APICall: func() ([]string, error) { - return member.ListOrgRoles(ctx, uuid.New()) + return member.ListOrganizationRoles(ctx, uuid.New()) }, Authorized: false, }, @@ -81,15 +81,15 @@ func TestListRoles(t *testing.T) { { Name: "OrgAdminListOrg", APICall: func() ([]string, error) { - return orgAdmin.ListOrgRoles(ctx, admin.OrganizationID) + return orgAdmin.ListOrganizationRoles(ctx, admin.OrganizationID) }, Authorized: true, - ExpectedRoles: rbac.ListOrgRoles(admin.OrganizationID), + ExpectedRoles: rbac.ListOrganizationRoles(admin.OrganizationID), }, { Name: "OrgAdminListOtherOrg", APICall: func() ([]string, error) { - return orgAdmin.ListOrgRoles(ctx, uuid.New()) + return orgAdmin.ListOrganizationRoles(ctx, uuid.New()) }, Authorized: false, }, @@ -105,10 +105,10 @@ func TestListRoles(t *testing.T) { { Name: "AdminListOrg", APICall: func() ([]string, error) { - return client.ListOrgRoles(ctx, admin.OrganizationID) + return client.ListOrganizationRoles(ctx, admin.OrganizationID) }, Authorized: true, - ExpectedRoles: rbac.ListOrgRoles(admin.OrganizationID), + ExpectedRoles: rbac.ListOrganizationRoles(admin.OrganizationID), }, } diff --git a/codersdk/roles.go b/codersdk/roles.go index f33d8c8afe9aa..a8235a884c592 100644 --- a/codersdk/roles.go +++ b/codersdk/roles.go @@ -22,7 +22,7 @@ func (c *Client) ListSiteRoles(ctx context.Context) ([]string, error) { return roles, json.NewDecoder(res.Body).Decode(&roles) } -func (c *Client) ListOrgRoles(ctx context.Context, org uuid.UUID) ([]string, error) { +func (c *Client) ListOrganizationRoles(ctx context.Context, org uuid.UUID) ([]string, error) { res, err := c.request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/organizations/%s/members/roles", org.String()), nil) if err != nil { return nil, err From f36ae37fa9ffc09b95c8d6625c6dfb9e6c44976b Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 3 May 2022 11:19:51 -0500 Subject: [PATCH 06/15] Renamings --- coderd/coderd.go | 4 ++-- coderd/httpmw/authorize.go | 6 +++--- coderd/rbac/builtin.go | 12 ++++++------ coderd/roles.go | 4 ++-- coderd/roles_test.go | 6 +++--- 5 files changed, 16 insertions(+), 16 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 5c6dac4a8df4e..191d3339aa2b1 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -159,7 +159,7 @@ func New(options *Options) (http.Handler, func()) { }) r.Route("/members", func(r chi.Router) { r.Route("/roles", func(r chi.Router) { - r.Use(httpmw.RBACObject(rbac.ResourceUserRole)) + r.Use(httpmw.WithRBACObject(rbac.ResourceUserRole)) r.Get("/", authorize(api.assignableOrgRoles, rbac.ActionRead)) }) r.Route("/{user}", func(r chi.Router) { @@ -232,7 +232,7 @@ func New(options *Options) (http.Handler, func()) { r.Get("/", api.users) // These routes query information about site wide roles. r.Route("/roles", func(r chi.Router) { - r.Use(httpmw.RBACObject(rbac.ResourceUserRole)) + r.Use(httpmw.WithRBACObject(rbac.ResourceUserRole)) r.Get("/", authorize(api.assignableSiteRoles, rbac.ActionRead)) }) r.Route("/{user}", func(r chi.Router) { diff --git a/coderd/httpmw/authorize.go b/coderd/httpmw/authorize.go index 985b2cbaf5e9b..ef76fab01bf9c 100644 --- a/coderd/httpmw/authorize.go +++ b/coderd/httpmw/authorize.go @@ -88,9 +88,9 @@ func GetAuthObject(r *http.Request) AuthObject { return obj } -// RBACObject sets the object for 'Authorize()' for all routes handled +// WithRBACObject sets the object for 'Authorize()' for all routes handled // by this middleware. The important field to set is 'Type' -func RBACObject(object rbac.Object) func(http.Handler) http.Handler { +func WithRBACObject(object rbac.Object) func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { ao := GetAuthObject(r) @@ -105,7 +105,7 @@ func RBACObject(object rbac.Object) func(http.Handler) http.Handler { // User roles are the 'subject' field of Authorize() type userRolesKey struct{} -// APIKey returns the API key from the ExtractAPIKey handler. +// 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 { diff --git a/coderd/rbac/builtin.go b/coderd/rbac/builtin.go index 0206e87dc5e7b..04a67568cf64c 100644 --- a/coderd/rbac/builtin.go +++ b/coderd/rbac/builtin.go @@ -145,11 +145,11 @@ func IsOrgRole(roleName string) (string, bool) { return "", false } -// ListOrganizationRoles lists all roles that can be applied to an organization user +// OrganizationRoles lists all roles that can be applied to an organization user // in the given organization. -// Note: This should be a list in a database, but until then we build +// This should be a list in a database, but until then we build // the list from the builtins. -func ListOrganizationRoles(organizationID uuid.UUID) []string { +func OrganizationRoles(organizationID uuid.UUID) []string { var roles []string for role := range builtInRoles { _, scope, err := roleSplit(role) @@ -164,10 +164,10 @@ func ListOrganizationRoles(organizationID uuid.UUID) []string { return roles } -// ListSiteRoles lists all roles that can be applied to a user. -// Note: This should be a list in a database, but until then we build +// SiteRoles lists all roles that can be applied to a user. +// This should be a list in a database, but until then we build // the list from the builtins. -func ListSiteRoles() []string { +func SiteRoles() []string { var roles []string for role := range builtInRoles { _, scope, err := roleSplit(role) diff --git a/coderd/roles.go b/coderd/roles.go index fea73a922ca26..ca526638fde8a 100644 --- a/coderd/roles.go +++ b/coderd/roles.go @@ -11,7 +11,7 @@ import ( func (api *api) assignableSiteRoles(rw http.ResponseWriter, r *http.Request) { // TODO: @emyrk in the future, allow granular subsets of roles to be returned based on the // role of the user. - roles := rbac.ListSiteRoles() + roles := rbac.SiteRoles() httpapi.Write(rw, http.StatusOK, roles) } @@ -19,6 +19,6 @@ func (api *api) assignableSiteRoles(rw http.ResponseWriter, r *http.Request) { func (api *api) assignableOrgRoles(rw http.ResponseWriter, r *http.Request) { // TODO: @emyrk in the future, allow granular subsets of roles to be returned based on the // role of the user. - roles := rbac.ListSiteRoles() + roles := rbac.SiteRoles() httpapi.Write(rw, http.StatusOK, roles) } diff --git a/coderd/roles_test.go b/coderd/roles_test.go index 7390636fff24a..1f6f1c05a76c4 100644 --- a/coderd/roles_test.go +++ b/coderd/roles_test.go @@ -84,7 +84,7 @@ func TestListRoles(t *testing.T) { return orgAdmin.ListOrganizationRoles(ctx, admin.OrganizationID) }, Authorized: true, - ExpectedRoles: rbac.ListOrganizationRoles(admin.OrganizationID), + ExpectedRoles: rbac.OrganizationRoles(admin.OrganizationID), }, { Name: "OrgAdminListOtherOrg", @@ -100,7 +100,7 @@ func TestListRoles(t *testing.T) { return client.ListSiteRoles(ctx) }, Authorized: true, - ExpectedRoles: rbac.ListSiteRoles(), + ExpectedRoles: rbac.SiteRoles(), }, { Name: "AdminListOrg", @@ -108,7 +108,7 @@ func TestListRoles(t *testing.T) { return client.ListOrganizationRoles(ctx, admin.OrganizationID) }, Authorized: true, - ExpectedRoles: rbac.ListOrganizationRoles(admin.OrganizationID), + ExpectedRoles: rbac.OrganizationRoles(admin.OrganizationID), }, } From b831260332af2956d66fa56f73e5828a3a00ff9d Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 3 May 2022 12:02:34 -0500 Subject: [PATCH 07/15] Use rbac.object directly --- coderd/httpmw/authorize.go | 19 ++++--------------- coderd/roles_test.go | 2 +- 2 files changed, 5 insertions(+), 16 deletions(-) diff --git a/coderd/httpmw/authorize.go b/coderd/httpmw/authorize.go index ef76fab01bf9c..d5e20a8aa4640 100644 --- a/coderd/httpmw/authorize.go +++ b/coderd/httpmw/authorize.go @@ -13,13 +13,6 @@ import ( "github.com/coder/coder/coderd/rbac" ) -// AuthObject wraps the rbac object type for middleware to customize this value -// before being passed to Authorize(). -type AuthObject struct { - // Object is that base static object the above functions can modify. - Object rbac.Object -} - // 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. @@ -27,9 +20,8 @@ func Authorize(logger slog.Logger, auth *rbac.RegoAuthorizer, action rbac.Action return func(next http.Handler) http.Handler { return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { roles := UserRoles(r) - args := GetAuthObject(r) + object := authObject(r) - object := args.Object if object.Type == "" { panic("developer error: auth object has no type") } @@ -80,8 +72,8 @@ func Authorize(logger slog.Logger, auth *rbac.RegoAuthorizer, action rbac.Action type authObjectKey struct{} // APIKey returns the API key from the ExtractAPIKey handler. -func GetAuthObject(r *http.Request) AuthObject { - obj, ok := r.Context().Value(authObjectKey{}).(AuthObject) +func authObject(r *http.Request) rbac.Object { + obj, ok := r.Context().Value(authObjectKey{}).(rbac.Object) if !ok { panic("developer error: auth object middleware not provided") } @@ -93,10 +85,7 @@ func GetAuthObject(r *http.Request) AuthObject { func WithRBACObject(object rbac.Object) func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { - ao := GetAuthObject(r) - ao.Object = object - - ctx := context.WithValue(r.Context(), authObjectKey{}, ao) + ctx := context.WithValue(r.Context(), authObjectKey{}, object) next.ServeHTTP(rw, r.WithContext(ctx)) }) } diff --git a/coderd/roles_test.go b/coderd/roles_test.go index 1f6f1c05a76c4..1f7a0bddd0ba2 100644 --- a/coderd/roles_test.go +++ b/coderd/roles_test.go @@ -39,7 +39,7 @@ func TestListRoles(t *testing.T) { Roles: []string{rbac.RoleOrgMember(admin.OrganizationID), rbac.RoleOrgAdmin(admin.OrganizationID)}, }, ) - require.NoError(t, err) + require.NoError(t, err, "update org member roles") testCases := []struct { Name string From db04d67fc7465a608aaa4bc2f113fc68f56845ea Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 3 May 2022 12:32:09 -0500 Subject: [PATCH 08/15] Fix broken tests --- coderd/coderd_test.go | 14 ++++++++++++ coderd/rbac/builtin.go | 3 ++- coderd/roles.go | 5 ++++- coderd/roles_test.go | 50 +++++++++++++++++++++--------------------- codersdk/roles.go | 2 +- 5 files changed, 46 insertions(+), 28 deletions(-) diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index 73d3c3d308def..1060db3a65921 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -2,8 +2,13 @@ package coderd_test import ( "context" + "fmt" + "net/http" "testing" + "github.com/coder/coder/coderd" + "github.com/go-chi/chi/v5" + "go.uber.org/goleak" "github.com/stretchr/testify/require" @@ -24,3 +29,12 @@ func TestBuildInfo(t *testing.T) { require.Equal(t, buildinfo.ExternalURL(), buildInfo.ExternalURL, "external URL") require.Equal(t, buildinfo.Version(), buildInfo.Version, "version") } + +func TestWalk(t *testing.T) { + r, _ := coderd.New(&coderd.Options{}) + chiRouter := r.(chi.Router) + chi.Walk(chiRouter, func(method string, route string, handler http.Handler, middlewares ...func(http.Handler) http.Handler) error { + fmt.Println(method, route) + return nil + }) +} diff --git a/coderd/rbac/builtin.go b/coderd/rbac/builtin.go index 04a67568cf64c..2bc25a084525a 100644 --- a/coderd/rbac/builtin.go +++ b/coderd/rbac/builtin.go @@ -151,7 +151,8 @@ func IsOrgRole(roleName string) (string, bool) { // the list from the builtins. func OrganizationRoles(organizationID uuid.UUID) []string { var roles []string - for role := range builtInRoles { + for _, roleF := range builtInRoles { + role := roleF(organizationID.String()).Name _, scope, err := roleSplit(role) if err != nil { // This should never happen diff --git a/coderd/roles.go b/coderd/roles.go index ca526638fde8a..11d1569a7f7f3 100644 --- a/coderd/roles.go +++ b/coderd/roles.go @@ -3,6 +3,8 @@ package coderd import ( "net/http" + "github.com/coder/coder/coderd/httpmw" + "github.com/coder/coder/coderd/httpapi" "github.com/coder/coder/coderd/rbac" ) @@ -19,6 +21,7 @@ func (api *api) assignableSiteRoles(rw http.ResponseWriter, r *http.Request) { func (api *api) assignableOrgRoles(rw http.ResponseWriter, r *http.Request) { // TODO: @emyrk in the future, allow granular subsets of roles to be returned based on the // role of the user. - roles := rbac.SiteRoles() + organization := httpmw.OrganizationParam(r) + roles := rbac.OrganizationRoles(organization.ID) httpapi.Write(rw, http.StatusOK, roles) } diff --git a/coderd/roles_test.go b/coderd/roles_test.go index 1f7a0bddd0ba2..e306c8e6e6fbc 100644 --- a/coderd/roles_test.go +++ b/coderd/roles_test.go @@ -8,20 +8,12 @@ import ( "github.com/coder/coder/coderd/coderdtest" "github.com/coder/coder/coderd/rbac" "github.com/coder/coder/codersdk" - "github.com/google/uuid" "github.com/stretchr/testify/require" ) func TestListRoles(t *testing.T) { t.Parallel() - requireUnauthorized := func(t *testing.T, err error) { - var apiErr *codersdk.Error - require.ErrorAs(t, err, &apiErr) - require.Equal(t, http.StatusUnauthorized, apiErr.StatusCode()) - require.Contains(t, apiErr.Message, "unauthorized") - } - ctx := context.Background() client := coderdtest.New(t, nil) // Create admin, member, and org admin @@ -41,12 +33,20 @@ func TestListRoles(t *testing.T) { ) require.NoError(t, err, "update org member roles") + otherOrg, err := client.CreateOrganization(ctx, admin.UserID, codersdk.CreateOrganizationRequest{ + Name: "other", + }) + require.NoError(t, err, "create org") + + const unauth = "unauthorized" + const notMember = "not a member of the organization" + testCases := []struct { - Name string - Client *codersdk.Client - APICall func() ([]string, error) - ExpectedRoles []string - Authorized bool + Name string + Client *codersdk.Client + APICall func() ([]string, error) + ExpectedRoles []string + AuthorizedError string }{ { Name: "MemberListSite", @@ -54,21 +54,21 @@ func TestListRoles(t *testing.T) { x, err := member.ListSiteRoles(ctx) return x, err }, - Authorized: false, + AuthorizedError: unauth, }, { Name: "OrgMemberListOrg", APICall: func() ([]string, error) { return member.ListOrganizationRoles(ctx, admin.OrganizationID) }, - Authorized: false, + AuthorizedError: unauth, }, { Name: "NonOrgMemberListOrg", APICall: func() ([]string, error) { - return member.ListOrganizationRoles(ctx, uuid.New()) + return member.ListOrganizationRoles(ctx, otherOrg.ID) }, - Authorized: false, + AuthorizedError: notMember, }, // Org admin { @@ -76,22 +76,21 @@ func TestListRoles(t *testing.T) { APICall: func() ([]string, error) { return orgAdmin.ListSiteRoles(ctx) }, - Authorized: false, + AuthorizedError: unauth, }, { Name: "OrgAdminListOrg", APICall: func() ([]string, error) { return orgAdmin.ListOrganizationRoles(ctx, admin.OrganizationID) }, - Authorized: true, ExpectedRoles: rbac.OrganizationRoles(admin.OrganizationID), }, { Name: "OrgAdminListOtherOrg", APICall: func() ([]string, error) { - return orgAdmin.ListOrganizationRoles(ctx, uuid.New()) + return orgAdmin.ListOrganizationRoles(ctx, otherOrg.ID) }, - Authorized: false, + AuthorizedError: notMember, }, // Admin { @@ -99,7 +98,6 @@ func TestListRoles(t *testing.T) { APICall: func() ([]string, error) { return client.ListSiteRoles(ctx) }, - Authorized: true, ExpectedRoles: rbac.SiteRoles(), }, { @@ -107,7 +105,6 @@ func TestListRoles(t *testing.T) { APICall: func() ([]string, error) { return client.ListOrganizationRoles(ctx, admin.OrganizationID) }, - Authorized: true, ExpectedRoles: rbac.OrganizationRoles(admin.OrganizationID), }, } @@ -117,8 +114,11 @@ func TestListRoles(t *testing.T) { t.Run(c.Name, func(t *testing.T) { t.Parallel() roles, err := c.APICall() - if !c.Authorized { - requireUnauthorized(t, err) + if c.AuthorizedError != "" { + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusUnauthorized, apiErr.StatusCode()) + require.Contains(t, apiErr.Message, c.AuthorizedError) } else { require.NoError(t, err) require.Equal(t, c.ExpectedRoles, roles) diff --git a/codersdk/roles.go b/codersdk/roles.go index a8235a884c592..5a03f9e02bf05 100644 --- a/codersdk/roles.go +++ b/codersdk/roles.go @@ -23,7 +23,7 @@ func (c *Client) ListSiteRoles(ctx context.Context) ([]string, error) { } func (c *Client) ListOrganizationRoles(ctx context.Context, org uuid.UUID) ([]string, error) { - res, err := c.request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/organizations/%s/members/roles", org.String()), nil) + res, err := c.request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/organizations/%s/members/roles/", org.String()), nil) if err != nil { return nil, err } From b76f373ae1dc276110b71f3b701ccfaf102e970d Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 3 May 2022 12:37:47 -0500 Subject: [PATCH 09/15] Add some comments --- coderd/httpmw/authorize.go | 4 ++-- coderd/rbac/builtin.go | 10 +++++++--- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/coderd/httpmw/authorize.go b/coderd/httpmw/authorize.go index d5e20a8aa4640..d9f3efe6f70cd 100644 --- a/coderd/httpmw/authorize.go +++ b/coderd/httpmw/authorize.go @@ -20,7 +20,7 @@ func Authorize(logger slog.Logger, auth *rbac.RegoAuthorizer, action rbac.Action return func(next http.Handler) http.Handler { return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { roles := UserRoles(r) - object := authObject(r) + object := rbacObject(r) if object.Type == "" { panic("developer error: auth object has no type") @@ -72,7 +72,7 @@ func Authorize(logger slog.Logger, auth *rbac.RegoAuthorizer, action rbac.Action type authObjectKey struct{} // APIKey returns the API key from the ExtractAPIKey handler. -func authObject(r *http.Request) rbac.Object { +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") diff --git a/coderd/rbac/builtin.go b/coderd/rbac/builtin.go index 2bc25a084525a..f388f52cc128b 100644 --- a/coderd/rbac/builtin.go +++ b/coderd/rbac/builtin.go @@ -146,9 +146,11 @@ func IsOrgRole(roleName string) (string, bool) { } // OrganizationRoles lists all roles that can be applied to an organization user -// in the given organization. +// in the given organization. This is the list of available roles, +// and specific to an organization. +// // This should be a list in a database, but until then we build -// the list from the builtins. +// the list from the builtins. func OrganizationRoles(organizationID uuid.UUID) []string { var roles []string for _, roleF := range builtInRoles { @@ -166,8 +168,10 @@ func OrganizationRoles(organizationID uuid.UUID) []string { } // SiteRoles lists all roles that can be applied to a user. +// This is the list of available roles, and not specific to a user +// // This should be a list in a database, but until then we build -// the list from the builtins. +// the list from the builtins. func SiteRoles() []string { var roles []string for role := range builtInRoles { From 117f838c5c527e983a667d70b6d3a0e565d49f48 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 3 May 2022 13:31:38 -0500 Subject: [PATCH 10/15] Linting --- coderd/coderd_test.go | 14 -------------- coderd/database/databasefake/databasefake.go | 3 ++- coderd/httpmw/authorize.go | 3 +-- coderd/roles.go | 4 ++-- coderd/roles_test.go | 3 ++- 5 files changed, 7 insertions(+), 20 deletions(-) diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index 1060db3a65921..73d3c3d308def 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -2,13 +2,8 @@ package coderd_test import ( "context" - "fmt" - "net/http" "testing" - "github.com/coder/coder/coderd" - "github.com/go-chi/chi/v5" - "go.uber.org/goleak" "github.com/stretchr/testify/require" @@ -29,12 +24,3 @@ func TestBuildInfo(t *testing.T) { require.Equal(t, buildinfo.ExternalURL(), buildInfo.ExternalURL, "external URL") require.Equal(t, buildinfo.Version(), buildInfo.Version, "version") } - -func TestWalk(t *testing.T) { - r, _ := coderd.New(&coderd.Options{}) - chiRouter := r.(chi.Router) - chi.Walk(chiRouter, func(method string, route string, handler http.Handler, middlewares ...func(http.Handler) http.Handler) error { - fmt.Println(method, route) - return nil - }) -} diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go index 6f466fe015b9a..5726c9f7befe5 100644 --- a/coderd/database/databasefake/databasefake.go +++ b/coderd/database/databasefake/databasefake.go @@ -245,7 +245,7 @@ func (q *fakeQuerier) GetUsers(_ context.Context, params database.GetUsersParams return tmp, nil } -func (q *fakeQuerier) GetAllUserRoles(ctx context.Context, userID uuid.UUID) (database.GetAllUserRolesRow, error) { +func (q *fakeQuerier) GetAllUserRoles(_ context.Context, userID uuid.UUID) (database.GetAllUserRolesRow, error) { q.mutex.RLock() defer q.mutex.RUnlock() @@ -253,6 +253,7 @@ func (q *fakeQuerier) GetAllUserRoles(ctx context.Context, userID uuid.UUID) (da roles := make([]string, 0) for _, u := range q.users { if u.ID == userID { + u := u roles = append(roles, u.RBACRoles...) user = &u break diff --git a/coderd/httpmw/authorize.go b/coderd/httpmw/authorize.go index d9f3efe6f70cd..2eb221f1893eb 100644 --- a/coderd/httpmw/authorize.go +++ b/coderd/httpmw/authorize.go @@ -2,7 +2,6 @@ package httpmw import ( "context" - "fmt" "net/http" "golang.org/x/xerrors" @@ -111,7 +110,7 @@ func ExtractUserRoles(db database.Store) func(http.Handler) http.Handler { role, err := db.GetAllUserRoles(r.Context(), apiKey.UserID) if err != nil { httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{ - Message: fmt.Sprintf("roles not found", AuthCookie), + Message: "roles not found", }) return } diff --git a/coderd/roles.go b/coderd/roles.go index 11d1569a7f7f3..64bd7353b4293 100644 --- a/coderd/roles.go +++ b/coderd/roles.go @@ -10,7 +10,7 @@ import ( ) // assignableSiteRoles returns all site wide roles that can be assigned. -func (api *api) assignableSiteRoles(rw http.ResponseWriter, r *http.Request) { +func (*api) assignableSiteRoles(rw http.ResponseWriter, _ *http.Request) { // TODO: @emyrk in the future, allow granular subsets of roles to be returned based on the // role of the user. roles := rbac.SiteRoles() @@ -18,7 +18,7 @@ func (api *api) assignableSiteRoles(rw http.ResponseWriter, r *http.Request) { } // assignableSiteRoles returns all site wide roles that can be assigned. -func (api *api) assignableOrgRoles(rw http.ResponseWriter, r *http.Request) { +func (*api) assignableOrgRoles(rw http.ResponseWriter, r *http.Request) { // TODO: @emyrk in the future, allow granular subsets of roles to be returned based on the // role of the user. organization := httpmw.OrganizationParam(r) diff --git a/coderd/roles_test.go b/coderd/roles_test.go index e306c8e6e6fbc..a05e20f3bea7d 100644 --- a/coderd/roles_test.go +++ b/coderd/roles_test.go @@ -5,10 +5,11 @@ import ( "net/http" "testing" + "github.com/stretchr/testify/require" + "github.com/coder/coder/coderd/coderdtest" "github.com/coder/coder/coderd/rbac" "github.com/coder/coder/codersdk" - "github.com/stretchr/testify/require" ) func TestListRoles(t *testing.T) { From 42b42abc73c57d591a991d0bb532b40ec2a86076 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 3 May 2022 13:34:26 -0500 Subject: [PATCH 11/15] Handle out of order lists --- coderd/rbac/builtin.go | 7 ++++--- coderd/roles_test.go | 2 +- codersdk/roles.go | 4 ++++ 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/coderd/rbac/builtin.go b/coderd/rbac/builtin.go index f388f52cc128b..26ec0a1afa1cc 100644 --- a/coderd/rbac/builtin.go +++ b/coderd/rbac/builtin.go @@ -174,14 +174,15 @@ func OrganizationRoles(organizationID uuid.UUID) []string { // the list from the builtins. func SiteRoles() []string { var roles []string - for role := range builtInRoles { - _, scope, err := roleSplit(role) + for _, roleF := range builtInRoles { + role := roleF("random") + _, scope, err := roleSplit(role.Name) if err != nil { // This should never happen continue } if scope == "" { - roles = append(roles, role) + roles = append(roles, role.Name) } } return roles diff --git a/coderd/roles_test.go b/coderd/roles_test.go index a05e20f3bea7d..523439883e33d 100644 --- a/coderd/roles_test.go +++ b/coderd/roles_test.go @@ -122,7 +122,7 @@ func TestListRoles(t *testing.T) { require.Contains(t, apiErr.Message, c.AuthorizedError) } else { require.NoError(t, err) - require.Equal(t, c.ExpectedRoles, roles) + require.ElementsMatch(t, c.ExpectedRoles, roles) } }) } diff --git a/codersdk/roles.go b/codersdk/roles.go index 5a03f9e02bf05..d7d6d0fe2b8bc 100644 --- a/codersdk/roles.go +++ b/codersdk/roles.go @@ -9,6 +9,8 @@ import ( "github.com/google/uuid" ) +// ListSiteRoles lists all available site wide roles. +// This is not user specific. func (c *Client) ListSiteRoles(ctx context.Context) ([]string, error) { res, err := c.request(ctx, http.MethodGet, "/api/v2/users/roles", nil) if err != nil { @@ -22,6 +24,8 @@ func (c *Client) ListSiteRoles(ctx context.Context) ([]string, error) { return roles, json.NewDecoder(res.Body).Decode(&roles) } +// ListOrganizationRoles lists all available roles for a given organization. +// This is not user specific. func (c *Client) ListOrganizationRoles(ctx context.Context, org uuid.UUID) ([]string, error) { res, err := c.request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/organizations/%s/members/roles/", org.String()), nil) if err != nil { From 0efe72cfd76443ba4cbae7a38dd2771bd97a59aa Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 3 May 2022 13:52:18 -0500 Subject: [PATCH 12/15] Add unit test --- coderd/rbac/builtin_test.go | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/coderd/rbac/builtin_test.go b/coderd/rbac/builtin_test.go index 849179dc78893..d8b937f78ac53 100644 --- a/coderd/rbac/builtin_test.go +++ b/coderd/rbac/builtin_test.go @@ -1,6 +1,7 @@ package rbac_test import ( + "fmt" "testing" "github.com/google/uuid" @@ -60,3 +61,23 @@ func TestIsOrgRole(t *testing.T) { }) } } + +func TestListRoles(t *testing.T) { + t.Parallel() + + // If this test is ever failing, just update the list to the roles + // expected from the builtin set. + require.ElementsMatch(t, []string{ + "admin", + "member", + "auditor", + }, + rbac.SiteRoles()) + + orgID := uuid.New() + require.ElementsMatch(t, []string{ + fmt.Sprintf("organization-admin:%s", orgID.String()), + fmt.Sprintf("organization-member:%s", orgID.String()), + }, + rbac.OrganizationRoles(orgID)) +} From dba617db1d0bc6084a9eddeffca14bf7b18209b5 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 3 May 2022 14:13:40 -0500 Subject: [PATCH 13/15] Add unit test for mw --- coderd/httpmw/authorize_test.go | 128 ++++++++++++++++++++++++++++++++ 1 file changed, 128 insertions(+) create mode 100644 coderd/httpmw/authorize_test.go diff --git a/coderd/httpmw/authorize_test.go b/coderd/httpmw/authorize_test.go new file mode 100644 index 0000000000000..95b429b9ba5e2 --- /dev/null +++ b/coderd/httpmw/authorize_test.go @@ -0,0 +1,128 @@ +package httpmw_test + +import ( + "context" + "crypto/sha256" + "fmt" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/coder/coder/coderd/rbac" + + "github.com/coder/coder/coderd/database" + "github.com/google/uuid" + + "github.com/coder/coder/coderd/database/databasefake" + "github.com/coder/coder/coderd/httpmw" + "github.com/go-chi/chi/v5" + "github.com/stretchr/testify/require" +) + +func TestExtractUserRoles(t *testing.T) { + testCases := []struct { + Name string + AddUser func(db database.Store) (database.User, []string, string) + }{ + { + Name: "Member", + AddUser: func(db database.Store) (database.User, []string, string) { + roles := []string{rbac.RoleMember()} + user, token := addUser(t, db, roles...) + return user, roles, token + }, + }, + { + Name: "Admin", + AddUser: func(db database.Store) (database.User, []string, string) { + roles := []string{rbac.RoleMember(), rbac.RoleAdmin()} + user, token := addUser(t, db, roles...) + return user, roles, token + }, + }, + { + Name: "OrgMember", + AddUser: func(db database.Store) (database.User, []string, string) { + roles := []string{rbac.RoleMember()} + user, token := addUser(t, db, roles...) + org, err := db.InsertOrganization(context.Background(), database.InsertOrganizationParams{ + ID: uuid.New(), + Name: "testorg", + Description: "test", + CreatedAt: time.Now(), + UpdatedAt: time.Now(), + }) + require.NoError(t, err) + + orgRoles := []string{rbac.RoleOrgMember(org.ID)} + _, err = db.InsertOrganizationMember(context.Background(), database.InsertOrganizationMemberParams{ + OrganizationID: org.ID, + UserID: user.ID, + CreatedAt: time.Now(), + UpdatedAt: time.Now(), + Roles: orgRoles, + }) + require.NoError(t, err) + return user, append(roles, orgRoles...), token + }, + }, + } + + for _, c := range testCases { + c := c + t.Run(c.Name, func(t *testing.T) { + t.Parallel() + var ( + db = databasefake.New() + user, expRoles, token = c.AddUser(db) + rw = httptest.NewRecorder() + rtr = chi.NewRouter() + ) + rtr.Use( + httpmw.ExtractAPIKey(db, &httpmw.OAuth2Configs{}), + httpmw.ExtractUserRoles(db), + ) + rtr.Get("/", func(_ http.ResponseWriter, r *http.Request) { + roles := httpmw.UserRoles(r) + require.ElementsMatch(t, user.ID, roles.ID) + require.ElementsMatch(t, expRoles, roles.Roles) + }) + + req := httptest.NewRequest("GET", "/", nil) + req.AddCookie(&http.Cookie{ + Name: httpmw.AuthCookie, + Value: token, + }) + + rtr.ServeHTTP(rw, req) + require.Equal(t, http.StatusOK, rw.Result().StatusCode) + }) + } +} + +func addUser(t *testing.T, db database.Store, roles ...string) (database.User, string) { + var ( + id, secret = randomAPIKeyParts() + hashed = sha256.Sum256([]byte(secret)) + ) + + user, err := db.InsertUser(context.Background(), database.InsertUserParams{ + ID: uuid.New(), + Email: "admin@email.com", + Username: "admin", + RBACRoles: roles, + }) + require.NoError(t, err) + + _, err = db.InsertAPIKey(context.Background(), database.InsertAPIKeyParams{ + ID: id, + UserID: user.ID, + HashedSecret: hashed[:], + LastUsed: database.Now(), + ExpiresAt: database.Now().Add(time.Minute), + }) + require.NoError(t, err) + + return user, fmt.Sprintf("%s-%s", id, secret) +} From 190940f3d214bb98880caf7605e8efbccdd9afda Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 3 May 2022 14:15:54 -0500 Subject: [PATCH 14/15] parallel unit test --- coderd/httpmw/authorize_test.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/coderd/httpmw/authorize_test.go b/coderd/httpmw/authorize_test.go index 95b429b9ba5e2..59ee50c80d9be 100644 --- a/coderd/httpmw/authorize_test.go +++ b/coderd/httpmw/authorize_test.go @@ -11,16 +11,19 @@ import ( "github.com/coder/coder/coderd/rbac" - "github.com/coder/coder/coderd/database" "github.com/google/uuid" - "github.com/coder/coder/coderd/database/databasefake" - "github.com/coder/coder/coderd/httpmw" + "github.com/coder/coder/coderd/database" + "github.com/go-chi/chi/v5" "github.com/stretchr/testify/require" + + "github.com/coder/coder/coderd/database/databasefake" + "github.com/coder/coder/coderd/httpmw" ) func TestExtractUserRoles(t *testing.T) { + t.Parallel() testCases := []struct { Name string AddUser func(db database.Store) (database.User, []string, string) From c86c67c840a74c7b848a8efdc0fe4490e53a841d Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 3 May 2022 14:34:40 -0500 Subject: [PATCH 15/15] style order --- coderd/database/querier.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 34c07f555d551..db2f3f0d49987 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -21,10 +21,10 @@ type querier interface { DeleteGitSSHKey(ctx context.Context, userID uuid.UUID) error DeleteParameterValueByID(ctx context.Context, id uuid.UUID) error GetAPIKeyByID(ctx context.Context, id string) (APIKey, error) + GetAllUserRoles(ctx context.Context, userID uuid.UUID) (GetAllUserRolesRow, error) // GetAuditLogsBefore retrieves `limit` number of audit logs before the provided // ID. GetAuditLogsBefore(ctx context.Context, arg GetAuditLogsBeforeParams) ([]AuditLog, error) - GetAllUserRoles(ctx context.Context, userID uuid.UUID) (GetAllUserRolesRow, error) GetFileByHash(ctx context.Context, hash string) (File, error) GetGitSSHKey(ctx context.Context, userID uuid.UUID) (GitSSHKey, error) GetOrganizationByID(ctx context.Context, id uuid.UUID) (Organization, error)