From 914ab14079860a8c9907768122a7f07ce7408346 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 25 May 2022 14:35:26 -0500 Subject: [PATCH 1/5] chore: Remove org_id from provisionerdaemons --- coderd/coderd.go | 8 +++++++- coderd/coderd_test.go | 18 +++++++++++++++-- coderd/database/dump.sql | 1 - ...ovisioner_daemons_organization_id.down.sql | 1 + ...provisioner_daemons_organization_id.up.sql | 1 + coderd/database/modelmethods.go | 4 ++++ coderd/database/models.go | 11 +++++----- coderd/database/queries.sql.go | 20 +++++++------------ .../database/queries/provisionerdaemons.sql | 3 +-- coderd/provisionerdaemons.go | 6 +++++- coderd/provisionerdaemons_test.go | 7 +++---- coderd/rbac/object.go | 4 ++++ codersdk/organizations.go | 4 ++-- codersdk/provisionerdaemons.go | 11 +++++----- site/src/api/typesGenerated.ts | 7 +++---- 15 files changed, 64 insertions(+), 42 deletions(-) create mode 100644 coderd/database/migrations/000014_provisioner_daemons_organization_id.down.sql create mode 100644 coderd/database/migrations/000014_provisioner_daemons_organization_id.up.sql diff --git a/coderd/coderd.go b/coderd/coderd.go index c474aeb785215..06a234c4114d9 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -142,6 +142,13 @@ func newRouter(options *Options, a *api) chi.Router { r.Get("/{hash}", a.fileByHash) r.Post("/", a.postFile) }) + r.Route("/provisionerdaemons", func(r chi.Router) { + r.Use( + apiKeyMiddleware, + authRolesMiddleware, + ) + r.Get("/", a.provisionerDaemons) + }) r.Route("/organizations/{organization}", func(r chi.Router) { r.Use( apiKeyMiddleware, @@ -149,7 +156,6 @@ func newRouter(options *Options, a *api) chi.Router { authRolesMiddleware, ) r.Get("/", a.organization) - r.Get("/provisionerdaemons", a.provisionerDaemonsByOrganization) r.Post("/templateversions", a.postTemplateVersionsByOrganization) r.Route("/templates", func(r chi.Router) { r.Post("/", a.postTemplateByOrganization) diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index 1b1f7b45599a2..de50ea236b56b 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -6,6 +6,7 @@ import ( "net/http" "strings" "testing" + "time" "github.com/go-chi/chi/v5" "github.com/stretchr/testify/assert" @@ -45,7 +46,17 @@ func TestAuthorizeAllEndpoints(t *testing.T) { IncludeProvisionerD: true, }) admin := coderdtest.CreateFirstUser(t, client) - organization, err := client.Organization(context.Background(), admin.OrganizationID) + require.Eventually(t, func() bool { + provisionerds, err := client.ProvisionerDaemons(ctx) + require.NoError(t, err) + return len(provisionerds) > 0 + }, time.Second*10, time.Second) + + provisionerds, err := client.ProvisionerDaemons(ctx) + require.NoError(t, err, "fetch provisioners") + require.Len(t, provisionerds, 1) + + organization, err := client.Organization(ctx, admin.OrganizationID) require.NoError(t, err, "fetch org") // Setup some data in the database. @@ -118,7 +129,6 @@ func TestAuthorizeAllEndpoints(t *testing.T) { "GET:/api/v2/workspaceagents/{workspaceagent}/turn": {NoAuthorize: true}, // TODO: @emyrk these need to be fixed by adding authorize calls - "GET:/api/v2/organizations/{organization}/provisionerdaemons": {NoAuthorize: true}, "GET:/api/v2/organizations/{organization}/templates/{templatename}": {NoAuthorize: true}, "POST:/api/v2/organizations/{organization}/templateversions": {NoAuthorize: true}, "POST:/api/v2/organizations/{organization}/workspaces": {NoAuthorize: true}, @@ -251,6 +261,10 @@ func TestAuthorizeAllEndpoints(t *testing.T) { AssertAction: rbac.ActionRead, AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()), }, + "GET:/api/v2/provisionerdaemons": { + StatusCode: http.StatusOK, + AssertObject: rbac.ResourceProvisionerDaemon.WithID(provisionerds[0].ID.String()), + }, // These endpoints need payloads to get to the auth part. Payloads will be required "PUT:/api/v2/users/{user}/roles": {StatusCode: http.StatusBadRequest, NoAuthorize: true}, diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 4cd90454283d7..6cbae1af44b9d 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -194,7 +194,6 @@ CREATE TABLE provisioner_daemons ( id uuid NOT NULL, created_at timestamp with time zone NOT NULL, updated_at timestamp with time zone, - organization_id uuid, name character varying(64) NOT NULL, provisioners provisioner_type[] NOT NULL ); diff --git a/coderd/database/migrations/000014_provisioner_daemons_organization_id.down.sql b/coderd/database/migrations/000014_provisioner_daemons_organization_id.down.sql new file mode 100644 index 0000000000000..486f0f94644b0 --- /dev/null +++ b/coderd/database/migrations/000014_provisioner_daemons_organization_id.down.sql @@ -0,0 +1 @@ +ALTER TABLE provisioner_daemons ADD COLUMN organization_id uuid; \ No newline at end of file diff --git a/coderd/database/migrations/000014_provisioner_daemons_organization_id.up.sql b/coderd/database/migrations/000014_provisioner_daemons_organization_id.up.sql new file mode 100644 index 0000000000000..49f8e3d26c3b6 --- /dev/null +++ b/coderd/database/migrations/000014_provisioner_daemons_organization_id.up.sql @@ -0,0 +1 @@ +ALTER TABLE provisioner_daemons DROP COLUMN organization_id; \ No newline at end of file diff --git a/coderd/database/modelmethods.go b/coderd/database/modelmethods.go index 2718ab8eec0d4..900bfa1940acf 100644 --- a/coderd/database/modelmethods.go +++ b/coderd/database/modelmethods.go @@ -22,3 +22,7 @@ func (m OrganizationMember) RBACObject() rbac.Object { func (o Organization) RBACObject() rbac.Object { return rbac.ResourceOrganization.InOrg(o.ID).WithID(o.ID.String()) } + +func (d ProvisionerDaemon) RBACObject() rbac.Object { + return rbac.ResourceProvisionerDaemon.WithID(d.ID.String()) +} diff --git a/coderd/database/models.go b/coderd/database/models.go index 3fcf947bf4cae..581e05af75147 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -391,12 +391,11 @@ type ParameterValue struct { } type ProvisionerDaemon struct { - ID uuid.UUID `db:"id" json:"id"` - CreatedAt time.Time `db:"created_at" json:"created_at"` - UpdatedAt sql.NullTime `db:"updated_at" json:"updated_at"` - OrganizationID uuid.NullUUID `db:"organization_id" json:"organization_id"` - Name string `db:"name" json:"name"` - Provisioners []ProvisionerType `db:"provisioners" json:"provisioners"` + ID uuid.UUID `db:"id" json:"id"` + CreatedAt time.Time `db:"created_at" json:"created_at"` + UpdatedAt sql.NullTime `db:"updated_at" json:"updated_at"` + Name string `db:"name" json:"name"` + Provisioners []ProvisionerType `db:"provisioners" json:"provisioners"` } type ProvisionerJob struct { diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 57eeb67ac5814..e951047b0e599 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -1076,7 +1076,7 @@ func (q *sqlQuerier) InsertParameterValue(ctx context.Context, arg InsertParamet const getProvisionerDaemonByID = `-- name: GetProvisionerDaemonByID :one SELECT - id, created_at, updated_at, organization_id, name, provisioners + id, created_at, updated_at, name, provisioners FROM provisioner_daemons WHERE @@ -1090,7 +1090,6 @@ func (q *sqlQuerier) GetProvisionerDaemonByID(ctx context.Context, id uuid.UUID) &i.ID, &i.CreatedAt, &i.UpdatedAt, - &i.OrganizationID, &i.Name, pq.Array(&i.Provisioners), ) @@ -1099,7 +1098,7 @@ func (q *sqlQuerier) GetProvisionerDaemonByID(ctx context.Context, id uuid.UUID) const getProvisionerDaemons = `-- name: GetProvisionerDaemons :many SELECT - id, created_at, updated_at, organization_id, name, provisioners + id, created_at, updated_at, name, provisioners FROM provisioner_daemons ` @@ -1117,7 +1116,6 @@ func (q *sqlQuerier) GetProvisionerDaemons(ctx context.Context) ([]ProvisionerDa &i.ID, &i.CreatedAt, &i.UpdatedAt, - &i.OrganizationID, &i.Name, pq.Array(&i.Provisioners), ); err != nil { @@ -1139,27 +1137,24 @@ INSERT INTO provisioner_daemons ( id, created_at, - organization_id, "name", provisioners ) VALUES - ($1, $2, $3, $4, $5) RETURNING id, created_at, updated_at, organization_id, name, provisioners + ($1, $2, $3, $4) RETURNING id, created_at, updated_at, name, provisioners ` type InsertProvisionerDaemonParams struct { - ID uuid.UUID `db:"id" json:"id"` - CreatedAt time.Time `db:"created_at" json:"created_at"` - OrganizationID uuid.NullUUID `db:"organization_id" json:"organization_id"` - Name string `db:"name" json:"name"` - Provisioners []ProvisionerType `db:"provisioners" json:"provisioners"` + ID uuid.UUID `db:"id" json:"id"` + CreatedAt time.Time `db:"created_at" json:"created_at"` + Name string `db:"name" json:"name"` + Provisioners []ProvisionerType `db:"provisioners" json:"provisioners"` } func (q *sqlQuerier) InsertProvisionerDaemon(ctx context.Context, arg InsertProvisionerDaemonParams) (ProvisionerDaemon, error) { row := q.db.QueryRowContext(ctx, insertProvisionerDaemon, arg.ID, arg.CreatedAt, - arg.OrganizationID, arg.Name, pq.Array(arg.Provisioners), ) @@ -1168,7 +1163,6 @@ func (q *sqlQuerier) InsertProvisionerDaemon(ctx context.Context, arg InsertProv &i.ID, &i.CreatedAt, &i.UpdatedAt, - &i.OrganizationID, &i.Name, pq.Array(&i.Provisioners), ) diff --git a/coderd/database/queries/provisionerdaemons.sql b/coderd/database/queries/provisionerdaemons.sql index 0051c5428fb45..30ff6d9d43eda 100644 --- a/coderd/database/queries/provisionerdaemons.sql +++ b/coderd/database/queries/provisionerdaemons.sql @@ -17,12 +17,11 @@ INSERT INTO provisioner_daemons ( id, created_at, - organization_id, "name", provisioners ) VALUES - ($1, $2, $3, $4, $5) RETURNING *; + ($1, $2, $3, $4) RETURNING *; -- name: UpdateProvisionerDaemonByID :exec UPDATE diff --git a/coderd/provisionerdaemons.go b/coderd/provisionerdaemons.go index bb3fa21361bdd..e442c3edc25e8 100644 --- a/coderd/provisionerdaemons.go +++ b/coderd/provisionerdaemons.go @@ -12,6 +12,8 @@ import ( "reflect" "time" + "github.com/coder/coder/coderd/rbac" + "github.com/google/uuid" "github.com/moby/moby/pkg/namesgenerator" "github.com/tabbed/pqtype" @@ -30,7 +32,7 @@ import ( sdkproto "github.com/coder/coder/provisionersdk/proto" ) -func (api *api) provisionerDaemonsByOrganization(rw http.ResponseWriter, r *http.Request) { +func (api *api) provisionerDaemons(rw http.ResponseWriter, r *http.Request) { daemons, err := api.Database.GetProvisionerDaemons(r.Context()) if errors.Is(err, sql.ErrNoRows) { err = nil @@ -44,6 +46,8 @@ func (api *api) provisionerDaemonsByOrganization(rw http.ResponseWriter, r *http if daemons == nil { daemons = []database.ProvisionerDaemon{} } + daemons = AuthorizeFilter(api, r, rbac.ActionRead, daemons) + httpapi.Write(rw, http.StatusOK, daemons) } diff --git a/coderd/provisionerdaemons_test.go b/coderd/provisionerdaemons_test.go index 65c7cd511d6e4..7d1434ba9f24b 100644 --- a/coderd/provisionerdaemons_test.go +++ b/coderd/provisionerdaemons_test.go @@ -7,7 +7,6 @@ import ( "testing" "time" - "github.com/google/uuid" "github.com/stretchr/testify/require" "github.com/coder/coder/coderd/coderdtest" @@ -51,15 +50,15 @@ func TestProvisionerDaemonsByOrganization(t *testing.T) { t.Run("NoAuth", func(t *testing.T) { t.Parallel() client := coderdtest.New(t, nil) - _, err := client.ProvisionerDaemonsByOrganization(context.Background(), uuid.New()) + _, err := client.ProvisionerDaemons(context.Background()) require.Error(t, err) }) t.Run("Get", func(t *testing.T) { t.Parallel() client := coderdtest.New(t, nil) - user := coderdtest.CreateFirstUser(t, client) - _, err := client.ProvisionerDaemonsByOrganization(context.Background(), user.OrganizationID) + _ = coderdtest.CreateFirstUser(t, client) + _, err := client.ProvisionerDaemons(context.Background()) require.NoError(t, err) }) } diff --git a/coderd/rbac/object.go b/coderd/rbac/object.go index 9ebbb304935c2..95820ff25dad1 100644 --- a/coderd/rbac/object.go +++ b/coderd/rbac/object.go @@ -34,6 +34,10 @@ var ( Type: "file", } + ResourceProvisionerDaemon = Object{ + Type: "provisioner_daemon", + } + // ResourceOrganization CRUD. Has an org owner on all but 'create'. // create/delete = make or delete organizations // read = view org information (Can add user owner for read) diff --git a/codersdk/organizations.go b/codersdk/organizations.go index 82a0e648e75ee..f0f6bd249ef33 100644 --- a/codersdk/organizations.go +++ b/codersdk/organizations.go @@ -90,9 +90,9 @@ func (c *Client) Organization(ctx context.Context, id uuid.UUID) (Organization, } // ProvisionerDaemonsByOrganization returns provisioner daemons available for an organization. -func (c *Client) ProvisionerDaemonsByOrganization(ctx context.Context, organizationID uuid.UUID) ([]ProvisionerDaemon, error) { +func (c *Client) ProvisionerDaemons(ctx context.Context) ([]ProvisionerDaemon, error) { res, err := c.Request(ctx, http.MethodGet, - fmt.Sprintf("/api/v2/organizations/%s/provisionerdaemons", organizationID.String()), + fmt.Sprintf("/api/v2/provisionerdaemons"), nil, ) if err != nil { diff --git a/codersdk/provisionerdaemons.go b/codersdk/provisionerdaemons.go index 7d2fd3717896b..d9d232bb0ec0d 100644 --- a/codersdk/provisionerdaemons.go +++ b/codersdk/provisionerdaemons.go @@ -31,12 +31,11 @@ const ( ) type ProvisionerDaemon struct { - ID uuid.UUID `json:"id"` - CreatedAt time.Time `json:"created_at"` - UpdatedAt sql.NullTime `json:"updated_at"` - OrganizationID uuid.NullUUID `json:"organization_id"` - Name string `json:"name"` - Provisioners []ProvisionerType `json:"provisioners"` + ID uuid.UUID `json:"id"` + CreatedAt time.Time `json:"created_at"` + UpdatedAt sql.NullTime `json:"updated_at"` + Name string `json:"name"` + Provisioners []ProvisionerType `json:"provisioners"` } // ProvisionerJobStaus represents the at-time state of a job. diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index de1d984461c5b..9248dc179d4eb 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -191,12 +191,11 @@ export interface ProvisionerDaemon { readonly id: string readonly created_at: string readonly updated_at?: string - readonly organization_id?: string readonly name: string readonly provisioners: ProvisionerType[] } -// From codersdk/provisionerdaemons.go:63:6 +// From codersdk/provisionerdaemons.go:62:6 export interface ProvisionerJob { readonly id: string readonly created_at: string @@ -207,7 +206,7 @@ export interface ProvisionerJob { readonly worker_id?: string } -// From codersdk/provisionerdaemons.go:73:6 +// From codersdk/provisionerdaemons.go:72:6 export interface ProvisionerJobLog { readonly id: string readonly created_at: string @@ -466,7 +465,7 @@ export type ParameterSourceScheme = "data" | "none" // From codersdk/parameters.go:38:6 export type ParameterTypeSystem = "hcl" | "none" -// From codersdk/provisionerdaemons.go:43:6 +// From codersdk/provisionerdaemons.go:42:6 export type ProvisionerJobStatus = "canceled" | "canceling" | "failed" | "pending" | "running" | "succeeded" // From codersdk/organizations.go:14:6 From 65b8a33c3951bab5745de7a30f4f7904268a24c3 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 25 May 2022 14:42:05 -0500 Subject: [PATCH 2/5] Let users see the provisioner daemons --- coderd/coderd_test.go | 2 ++ coderd/provisionerdaemons.go | 3 +-- coderd/rbac/builtin.go | 2 ++ 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index de50ea236b56b..18faadd0eb59f 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -46,6 +46,8 @@ func TestAuthorizeAllEndpoints(t *testing.T) { IncludeProvisionerD: true, }) admin := coderdtest.CreateFirstUser(t, client) + // The provisioner will call to coderd and register itself. This is async, + // so we wait for it to occur. require.Eventually(t, func() bool { provisionerds, err := client.ProvisionerDaemons(ctx) require.NoError(t, err) diff --git a/coderd/provisionerdaemons.go b/coderd/provisionerdaemons.go index e442c3edc25e8..8896e16464211 100644 --- a/coderd/provisionerdaemons.go +++ b/coderd/provisionerdaemons.go @@ -12,8 +12,6 @@ import ( "reflect" "time" - "github.com/coder/coder/coderd/rbac" - "github.com/google/uuid" "github.com/moby/moby/pkg/namesgenerator" "github.com/tabbed/pqtype" @@ -27,6 +25,7 @@ import ( "github.com/coder/coder/coderd/database" "github.com/coder/coder/coderd/httpapi" "github.com/coder/coder/coderd/parameter" + "github.com/coder/coder/coderd/rbac" "github.com/coder/coder/provisionerd/proto" "github.com/coder/coder/provisionersdk" sdkproto "github.com/coder/coder/provisionersdk/proto" diff --git a/coderd/rbac/builtin.go b/coderd/rbac/builtin.go index b530e3bbac99e..407d827448b54 100644 --- a/coderd/rbac/builtin.go +++ b/coderd/rbac/builtin.go @@ -68,6 +68,8 @@ var ( // All users can read all other users and know they exist. ResourceUser: {ActionRead}, ResourceRoleAssignment: {ActionRead}, + // All users can see the provisioner daemons. + ResourceProvisionerDaemon: {ActionRead}, }), User: permissions(map[Object][]Action{ ResourceWildcard: {WildcardSymbol}, From 651e0bca91b8e2b3cd88b3e15fdd793b855cce88 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 25 May 2022 15:56:15 -0500 Subject: [PATCH 3/5] feat: RBAC parameters --- coderd/coderd.go | 5 +- coderd/coderd_test.go | 28 +++++- coderd/database/databasefake/databasefake.go | 9 +- coderd/parameters.go | 97 +++++++++++++++++++- 4 files changed, 125 insertions(+), 14 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 06a234c4114d9..1ff03f4ced749 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -183,7 +183,10 @@ func newRouter(options *Options, a *api) chi.Router { }) }) r.Route("/parameters/{scope}/{id}", func(r chi.Router) { - r.Use(apiKeyMiddleware) + r.Use( + apiKeyMiddleware, + authRolesMiddleware, + ) r.Post("/", a.postParameter) r.Get("/", a.parameters) r.Route("/{name}", func(r chi.Router) { diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index 18faadd0eb59f..c9956463cd468 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -61,6 +61,14 @@ func TestAuthorizeAllEndpoints(t *testing.T) { organization, err := client.Organization(ctx, admin.OrganizationID) require.NoError(t, err, "fetch org") + organizationParam, err := client.CreateParameter(ctx, codersdk.ParameterOrganization, organization.ID, codersdk.CreateParameterRequest{ + Name: "test-param", + SourceValue: "hello world", + SourceScheme: codersdk.ParameterSourceSchemeData, + DestinationScheme: codersdk.ParameterDestinationSchemeProvisionerVariable, + }) + require.NoError(t, err, "create org param") + // Setup some data in the database. version := coderdtest.CreateTemplateVersion(t, client, admin.OrganizationID, &echo.Responses{ Parse: echo.ParseComplete, @@ -135,10 +143,6 @@ func TestAuthorizeAllEndpoints(t *testing.T) { "POST:/api/v2/organizations/{organization}/templateversions": {NoAuthorize: true}, "POST:/api/v2/organizations/{organization}/workspaces": {NoAuthorize: true}, - "POST:/api/v2/parameters/{scope}/{id}": {NoAuthorize: true}, - "GET:/api/v2/parameters/{scope}/{id}": {NoAuthorize: true}, - "DELETE:/api/v2/parameters/{scope}/{id}/{name}": {NoAuthorize: true}, - "POST:/api/v2/users/{user}/organizations": {NoAuthorize: true}, "GET:/api/v2/workspaces/{workspace}/watch": {NoAuthorize: true}, @@ -268,6 +272,19 @@ func TestAuthorizeAllEndpoints(t *testing.T) { AssertObject: rbac.ResourceProvisionerDaemon.WithID(provisionerds[0].ID.String()), }, + "POST:/api/v2/parameters/{scope}/{id}": { + AssertAction: rbac.ActionUpdate, + AssertObject: rbac.ResourceOrganization.WithID(organization.ID.String()), + }, + "GET:/api/v2/parameters/{scope}/{id}": { + AssertAction: rbac.ActionRead, + AssertObject: rbac.ResourceOrganization.WithID(organization.ID.String()), + }, + "DELETE:/api/v2/parameters/{scope}/{id}/{name}": { + AssertAction: rbac.ActionUpdate, + AssertObject: rbac.ResourceOrganization.WithID(organization.ID.String()), + }, + // These endpoints need payloads to get to the auth part. Payloads will be required "PUT:/api/v2/users/{user}/roles": {StatusCode: http.StatusBadRequest, NoAuthorize: true}, "PUT:/api/v2/organizations/{organization}/members/{user}/roles": {NoAuthorize: true}, @@ -309,6 +326,9 @@ func TestAuthorizeAllEndpoints(t *testing.T) { route = strings.ReplaceAll(route, "{hash}", file.Hash) route = strings.ReplaceAll(route, "{workspaceresource}", workspaceResources[0].ID.String()) route = strings.ReplaceAll(route, "{templateversion}", version.ID.String()) + // Only checking org scoped params here + route = strings.ReplaceAll(route, "{scope}", string(organizationParam.Scope)) + route = strings.ReplaceAll(route, "{id}", organizationParam.ScopeID.String()) resp, err := client.Request(context.Background(), method, route, nil) require.NoError(t, err, "do req") diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go index 0e16107a43858..4fdbcbc275960 100644 --- a/coderd/database/databasefake/databasefake.go +++ b/coderd/database/databasefake/databasefake.go @@ -1283,11 +1283,10 @@ func (q *fakeQuerier) InsertProvisionerDaemon(_ context.Context, arg database.In defer q.mutex.Unlock() daemon := database.ProvisionerDaemon{ - ID: arg.ID, - CreatedAt: arg.CreatedAt, - OrganizationID: arg.OrganizationID, - Name: arg.Name, - Provisioners: arg.Provisioners, + ID: arg.ID, + CreatedAt: arg.CreatedAt, + Name: arg.Name, + Provisioners: arg.Provisioners, } q.provisionerDaemons = append(q.provisionerDaemons, daemon) return daemon, nil diff --git a/coderd/parameters.go b/coderd/parameters.go index 0f8afcbb2da60..36b33543a2bd1 100644 --- a/coderd/parameters.go +++ b/coderd/parameters.go @@ -6,6 +6,8 @@ import ( "fmt" "net/http" + "github.com/coder/coder/coderd/rbac" + "github.com/go-chi/chi/v5" "github.com/google/uuid" "golang.org/x/xerrors" @@ -17,14 +19,23 @@ import ( ) func (api *api) postParameter(rw http.ResponseWriter, r *http.Request) { - var createRequest codersdk.CreateParameterRequest - if !httpapi.Read(rw, r, &createRequest) { - return - } scope, scopeID, valid := readScopeAndID(rw, r) if !valid { return } + obj, ok := api.parameterRBACResource(rw, r, scope, scopeID) + if !ok { + return + } + if !api.Authorize(rw, r, rbac.ActionUpdate, obj) { + return + } + + var createRequest codersdk.CreateParameterRequest + if !httpapi.Read(rw, r, &createRequest) { + return + } + _, err := api.Database.GetParameterValueByScopeAndName(r.Context(), database.GetParameterValueByScopeAndNameParams{ Scope: scope, ScopeID: scopeID, @@ -42,6 +53,20 @@ func (api *api) postParameter(rw http.ResponseWriter, r *http.Request) { }) return } + + switch scope { + case database.ParameterScopeWorkspace: + case database.ParameterScopeUser: + case database.ParameterScopeTemplate: + case database.ParameterScopeImportJob: + case database.ParameterScopeOrganization: + default: + httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{ + Message: fmt.Sprintf("scope %q unsupported", scope), + }) + return + } + parameterValue, err := api.Database.InsertParameterValue(r.Context(), database.InsertParameterValueParams{ ID: uuid.New(), Name: createRequest.Name, @@ -68,6 +93,16 @@ func (api *api) parameters(rw http.ResponseWriter, r *http.Request) { if !valid { return } + obj, ok := api.parameterRBACResource(rw, r, scope, scopeID) + if !ok { + return + } + // Should we allow reading the params of the resource if they can read the + // resource? Will this leak secrets? + if !api.Authorize(rw, r, rbac.ActionRead, obj) { + return + } + parameterValues, err := api.Database.GetParameterValuesByScope(r.Context(), database.GetParameterValuesByScopeParams{ Scope: scope, ScopeID: scopeID, @@ -94,6 +129,15 @@ func (api *api) deleteParameter(rw http.ResponseWriter, r *http.Request) { if !valid { return } + obj, ok := api.parameterRBACResource(rw, r, scope, scopeID) + if !ok { + return + } + // A delete param is still updating the underlying resource for the scope. + if !api.Authorize(rw, r, rbac.ActionUpdate, obj) { + return + } + name := chi.URLParam(r, "name") parameterValue, err := api.Database.GetParameterValueByScopeAndName(r.Context(), database.GetParameterValueByScopeAndNameParams{ Scope: scope, @@ -168,6 +212,51 @@ func convertParameterValue(parameterValue database.ParameterValue) codersdk.Para } } +// parameterRBACResource returns the RBAC resource a parameter scope and scope +// ID is trying to update. For RBAC purposes, adding a param to a resource +// is equivalent to updating/reading the associated resource. +// This means "parameters" are not a new resource, but an extension of existing +// ones. +func (api *api) parameterRBACResource(rw http.ResponseWriter, r *http.Request, scope database.ParameterScope, scopeID uuid.UUID) (rbac.Objecter, bool) { + ctx := r.Context() + var resource rbac.Objecter + var err error + switch scope { + case database.ParameterScopeWorkspace: + resource, err = api.Database.GetWorkspaceByID(ctx, scopeID) + case database.ParameterScopeTemplate: + resource, err = api.Database.GetTemplateByID(ctx, scopeID) + case database.ParameterScopeOrganization: + resource, err = api.Database.GetOrganizationByID(ctx, scopeID) + case database.ParameterScopeUser: + user, userErr := api.Database.GetUserByID(ctx, scopeID) + err = userErr + if err != nil { + // Use the userdata resource instead of the user. This way users + // can add user scoped params. + resource = rbac.ResourceUserData.WithID(user.ID.String()).WithOwner(user.ID.String()) + } + case database.ParameterScopeImportJob: + // ?? + err = xerrors.Errorf("ImportJob scope not supported") + default: + err = xerrors.Errorf("scope %q unsupported", scope) + } + + // Write error payload to rw if we cannot find the resource for the scope + if err != nil { + if xerrors.Is(err, sql.ErrNoRows) { + httpapi.Forbidden(rw) + } else { + httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{ + Message: fmt.Sprintf("param scope resource: %s", err.Error()), + }) + } + return nil, false + } + return resource, true +} + func readScopeAndID(rw http.ResponseWriter, r *http.Request) (database.ParameterScope, uuid.UUID, bool) { var scope database.ParameterScope switch chi.URLParam(r, "scope") { From 9feb24a93e1b61c97b3bd4869256aa21cbca108b Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 25 May 2022 16:09:58 -0500 Subject: [PATCH 4/5] test: Fixup test --- coderd/coderd_test.go | 16 +++++++++------- coderd/parameters.go | 17 +++-------------- 2 files changed, 12 insertions(+), 21 deletions(-) diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index c9956463cd468..67a5e776b6e4c 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -139,13 +139,10 @@ func TestAuthorizeAllEndpoints(t *testing.T) { "GET:/api/v2/workspaceagents/{workspaceagent}/turn": {NoAuthorize: true}, // TODO: @emyrk these need to be fixed by adding authorize calls - "GET:/api/v2/organizations/{organization}/templates/{templatename}": {NoAuthorize: true}, - "POST:/api/v2/organizations/{organization}/templateversions": {NoAuthorize: true}, - "POST:/api/v2/organizations/{organization}/workspaces": {NoAuthorize: true}, - - "POST:/api/v2/users/{user}/organizations": {NoAuthorize: true}, - - "GET:/api/v2/workspaces/{workspace}/watch": {NoAuthorize: true}, + "POST:/api/v2/organizations/{organization}/workspaces": {NoAuthorize: true}, + "POST:/api/v2/users/{user}/organizations": {NoAuthorize: true}, + "GET:/api/v2/workspaces/{workspace}/watch": {NoAuthorize: true}, + "POST:/api/v2/organizations/{organization}/templateversions": {NoAuthorize: true}, // These endpoints have more assertions. This is good, add more endpoints to assert if you can! "GET:/api/v2/organizations/{organization}": {AssertObject: rbac.ResourceOrganization.InOrg(admin.OrganizationID)}, @@ -284,6 +281,10 @@ func TestAuthorizeAllEndpoints(t *testing.T) { AssertAction: rbac.ActionUpdate, AssertObject: rbac.ResourceOrganization.WithID(organization.ID.String()), }, + "GET:/api/v2/organizations/{organization}/templates/{templatename}": { + AssertAction: rbac.ActionRead, + AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()), + }, // These endpoints need payloads to get to the auth part. Payloads will be required "PUT:/api/v2/users/{user}/roles": {StatusCode: http.StatusBadRequest, NoAuthorize: true}, @@ -326,6 +327,7 @@ func TestAuthorizeAllEndpoints(t *testing.T) { route = strings.ReplaceAll(route, "{hash}", file.Hash) route = strings.ReplaceAll(route, "{workspaceresource}", workspaceResources[0].ID.String()) route = strings.ReplaceAll(route, "{templateversion}", version.ID.String()) + route = strings.ReplaceAll(route, "{templatename}", template.Name) // Only checking org scoped params here route = strings.ReplaceAll(route, "{scope}", string(organizationParam.Scope)) route = strings.ReplaceAll(route, "{id}", organizationParam.ScopeID.String()) diff --git a/coderd/parameters.go b/coderd/parameters.go index 36b33543a2bd1..e606a58c3ae7f 100644 --- a/coderd/parameters.go +++ b/coderd/parameters.go @@ -54,19 +54,6 @@ func (api *api) postParameter(rw http.ResponseWriter, r *http.Request) { return } - switch scope { - case database.ParameterScopeWorkspace: - case database.ParameterScopeUser: - case database.ParameterScopeTemplate: - case database.ParameterScopeImportJob: - case database.ParameterScopeOrganization: - default: - httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{ - Message: fmt.Sprintf("scope %q unsupported", scope), - }) - return - } - parameterValue, err := api.Database.InsertParameterValue(r.Context(), database.InsertParameterValueParams{ ID: uuid.New(), Name: createRequest.Name, @@ -237,7 +224,9 @@ func (api *api) parameterRBACResource(rw http.ResponseWriter, r *http.Request, s resource = rbac.ResourceUserData.WithID(user.ID.String()).WithOwner(user.ID.String()) } case database.ParameterScopeImportJob: - // ?? + // This scope does not make sense from this api. + // ImportJob params are created with the job, and the job id cannot + // be predicted. err = xerrors.Errorf("ImportJob scope not supported") default: err = xerrors.Errorf("scope %q unsupported", scope) From 3bff48e9f4c8ff808601fb7818f6cbde953ed541 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 25 May 2022 16:15:02 -0500 Subject: [PATCH 5/5] Cleanup --- coderd/parameters.go | 3 +-- codersdk/organizations.go | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/coderd/parameters.go b/coderd/parameters.go index e606a58c3ae7f..e041aaab33946 100644 --- a/coderd/parameters.go +++ b/coderd/parameters.go @@ -84,8 +84,7 @@ func (api *api) parameters(rw http.ResponseWriter, r *http.Request) { if !ok { return } - // Should we allow reading the params of the resource if they can read the - // resource? Will this leak secrets? + if !api.Authorize(rw, r, rbac.ActionRead, obj) { return } diff --git a/codersdk/organizations.go b/codersdk/organizations.go index f0f6bd249ef33..1ac435439267d 100644 --- a/codersdk/organizations.go +++ b/codersdk/organizations.go @@ -92,7 +92,7 @@ func (c *Client) Organization(ctx context.Context, id uuid.UUID) (Organization, // ProvisionerDaemonsByOrganization returns provisioner daemons available for an organization. func (c *Client) ProvisionerDaemons(ctx context.Context) ([]ProvisionerDaemon, error) { res, err := c.Request(ctx, http.MethodGet, - fmt.Sprintf("/api/v2/provisionerdaemons"), + "/api/v2/provisionerdaemons", nil, ) if err != nil {