From 23c61c78b58334c7d17854d790371a714ab9988a Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Fri, 28 Jul 2023 16:28:35 +0200 Subject: [PATCH 1/7] WIP --- coderd/insights.go | 4 --- coderd/insights_test.go | 58 ++++++++++++++++++++++++++++++ enterprise/coderd/insights_test.go | 55 ++++++++++++++++++++++++++++ 3 files changed, 113 insertions(+), 4 deletions(-) create mode 100644 enterprise/coderd/insights_test.go diff --git a/coderd/insights.go b/coderd/insights.go index 2c6d22be0ab53..cca2e04a508bb 100644 --- a/coderd/insights.go +++ b/coderd/insights.go @@ -154,10 +154,6 @@ func (api *API) insightsUserLatency(rw http.ResponseWriter, r *http.Request) { // @Router /insights/templates [get] func (api *API) insightsTemplates(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() - if !api.Authorize(r, rbac.ActionRead, rbac.ResourceDeploymentValues) { - httpapi.Forbidden(rw) - return - } p := httpapi.NewQueryParamParser(). Required("start_time"). diff --git a/coderd/insights_test.go b/coderd/insights_test.go index f108882299b9a..26cb4314c09af 100644 --- a/coderd/insights_test.go +++ b/coderd/insights_test.go @@ -14,6 +14,7 @@ import ( "cdr.dev/slog/sloggers/slogtest" "github.com/coder/coder/agent" "github.com/coder/coder/coderd/coderdtest" + "github.com/coder/coder/coderd/rbac" "github.com/coder/coder/codersdk" "github.com/coder/coder/codersdk/agentsdk" "github.com/coder/coder/provisioner/echo" @@ -380,3 +381,60 @@ func TestTemplateInsights_BadRequest(t *testing.T) { }) assert.Error(t, err, "want error for bad interval") } + +func TestTemplateInsightsRBAC(t *testing.T) { + t.Parallel() + + y, m, d := time.Now().UTC().Date() + today := time.Date(y, m, d, 0, 0, 0, 0, time.UTC) + + t.Run("AsOwner", func(t *testing.T) { + t.Parallel() + + client := coderdtest.New(t, &coderdtest.Options{}) + _ = coderdtest.CreateFirstUser(t, client) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() + + _, err := client.TemplateInsights(ctx, codersdk.TemplateInsightsRequest{ + StartTime: today.AddDate(0, 0, -1), + EndTime: today, + }) + require.NoError(t, err) + }) + t.Run("AsTemplateAdmin", func(t *testing.T) { + t.Parallel() + + client := coderdtest.New(t, &coderdtest.Options{}) + admin := coderdtest.CreateFirstUser(t, client) + + templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, admin.OrganizationID, rbac.RoleTemplateAdmin()) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() + + _, err := templateAdmin.TemplateInsights(ctx, codersdk.TemplateInsightsRequest{ + StartTime: today.AddDate(0, 0, -1), + EndTime: today, + }) + require.NoError(t, err) + }) + t.Run("AsRegularUser", func(t *testing.T) { + t.Parallel() + + client := coderdtest.New(t, &coderdtest.Options{}) + admin := coderdtest.CreateFirstUser(t, client) + + regular, _ := coderdtest.CreateAnotherUser(t, client, admin.OrganizationID) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() + + _, err := regular.TemplateInsights(ctx, codersdk.TemplateInsightsRequest{ + StartTime: today.AddDate(0, 0, -1), + EndTime: today, + }) + require.Error(t, err) + }) +} diff --git a/enterprise/coderd/insights_test.go b/enterprise/coderd/insights_test.go new file mode 100644 index 0000000000000..1a00ebd211303 --- /dev/null +++ b/enterprise/coderd/insights_test.go @@ -0,0 +1,55 @@ +package coderd_test + +import ( + "context" + "testing" + "time" + + "github.com/google/uuid" + "github.com/stretchr/testify/require" + + "github.com/coder/coder/coderd/coderdtest" + "github.com/coder/coder/codersdk" + "github.com/coder/coder/enterprise/coderd/coderdenttest" + "github.com/coder/coder/enterprise/coderd/license" + "github.com/coder/coder/testutil" +) + +func TestTemplateInsightsRBAC(t *testing.T) { + t.Parallel() + + y, m, d := time.Now().UTC().Date() + today := time.Date(y, m, d, 0, 0, 0, 0, time.UTC) + + t.Run("AsRegularUserWithTemplateAdminACL", func(t *testing.T) { + t.Parallel() + + client, admin := coderdenttest.New(t, &coderdenttest.Options{LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureTemplateRBAC: 1, + }, + }}) + + version := coderdtest.CreateTemplateVersion(t, client, admin.OrganizationID, nil) + template := coderdtest.CreateTemplate(t, client, admin.OrganizationID, version.ID) + + regular, regularUser := coderdtest.CreateAnotherUser(t, client, admin.OrganizationID) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() + + err := client.UpdateTemplateACL(ctx, template.ID, codersdk.UpdateTemplateACL{ + UserPerms: map[string]codersdk.TemplateRole{ + regularUser.ID.String(): codersdk.TemplateRoleAdmin, + }, + }) + require.NoError(t, err) + + _, err = regular.TemplateInsights(ctx, codersdk.TemplateInsightsRequest{ + StartTime: today.AddDate(0, 0, -1), + EndTime: today, + TemplateIDs: []uuid.UUID{template.ID}, + }) + require.NoError(t, err) + }) +} From 9c88628d4c0108686cf86d562a9d90a78ea1a948 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Mon, 31 Jul 2023 12:46:24 +0200 Subject: [PATCH 2/7] WIP --- coderd/database/dbauthz/dbauthz.go | 36 +++++++-- coderd/insights.go | 7 ++ coderd/insights_test.go | 123 +++++++++++++++++++++-------- 3 files changed, 129 insertions(+), 37 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 9bc3a187f6d84..48e8053ed5070 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1195,17 +1195,41 @@ func (q *querier) GetTemplateDAUs(ctx context.Context, arg database.GetTemplateD } func (q *querier) GetTemplateDailyInsights(ctx context.Context, arg database.GetTemplateDailyInsightsParams) ([]database.GetTemplateDailyInsightsRow, error) { - // FIXME: this should maybe be READ rbac.ResourceTemplate or it's own resource. - if err := q.authorizeContext(ctx, rbac.ActionRead, rbac.ResourceSystem); err != nil { - return nil, err + for _, templateID := range arg.TemplateIDs { + template, err := q.db.GetTemplateByID(ctx, templateID) + if err != nil { + return nil, err + } + + if err := q.authorizeContext(ctx, rbac.ActionUpdate, template); err != nil { + return nil, err + } + } + + if len(arg.TemplateIDs) == 0 { + if err := q.authorizeContext(ctx, rbac.ActionUpdate, rbac.ResourceTemplate.All()); err != nil { + return nil, err + } } return q.db.GetTemplateDailyInsights(ctx, arg) } func (q *querier) GetTemplateInsights(ctx context.Context, arg database.GetTemplateInsightsParams) (database.GetTemplateInsightsRow, error) { - // FIXME: this should maybe be READ rbac.ResourceTemplate or it's own resource. - if err := q.authorizeContext(ctx, rbac.ActionRead, rbac.ResourceSystem); err != nil { - return database.GetTemplateInsightsRow{}, err + for _, templateID := range arg.TemplateIDs { + template, err := q.db.GetTemplateByID(ctx, templateID) + if err != nil { + return database.GetTemplateInsightsRow{}, err + } + + if err := q.authorizeContext(ctx, rbac.ActionUpdate, template); err != nil { + return database.GetTemplateInsightsRow{}, err + } + } + + if len(arg.TemplateIDs) == 0 { + if err := q.authorizeContext(ctx, rbac.ActionUpdate, rbac.ResourceTemplate.All()); err != nil { + return database.GetTemplateInsightsRow{}, err + } } return q.db.GetTemplateInsights(ctx, arg) } diff --git a/coderd/insights.go b/coderd/insights.go index cca2e04a508bb..e4e61d569570c 100644 --- a/coderd/insights.go +++ b/coderd/insights.go @@ -187,6 +187,7 @@ func (api *API) insightsTemplates(rw http.ResponseWriter, r *http.Request) { var usage database.GetTemplateInsightsRow var dailyUsage []database.GetTemplateDailyInsightsRow + // Use a transaction to ensure that we get consistent data between // the full and interval report. err := api.Database.InTx(func(db database.Store) error { @@ -214,6 +215,12 @@ func (api *API) insightsTemplates(rw http.ResponseWriter, r *http.Request) { return nil }, nil) + if httpapi.Is404Error(err) { + httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{ + Message: "Template not found or forbidden access.", + }) + return + } if err != nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error fetching template insights.", diff --git a/coderd/insights_test.go b/coderd/insights_test.go index 26cb4314c09af..234cc978146ae 100644 --- a/coderd/insights_test.go +++ b/coderd/insights_test.go @@ -388,53 +388,114 @@ func TestTemplateInsightsRBAC(t *testing.T) { y, m, d := time.Now().UTC().Date() today := time.Date(y, m, d, 0, 0, 0, 0, time.UTC) - t.Run("AsOwner", func(t *testing.T) { + t.Run("IntervalDay", func(t *testing.T) { t.Parallel() - client := coderdtest.New(t, &coderdtest.Options{}) - _ = coderdtest.CreateFirstUser(t, client) + t.Run("AsOwner", func(t *testing.T) { + t.Parallel() - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) - defer cancel() + client := coderdtest.New(t, &coderdtest.Options{}) + _ = coderdtest.CreateFirstUser(t, client) - _, err := client.TemplateInsights(ctx, codersdk.TemplateInsightsRequest{ - StartTime: today.AddDate(0, 0, -1), - EndTime: today, + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() + + _, err := client.TemplateInsights(ctx, codersdk.TemplateInsightsRequest{ + StartTime: today.AddDate(0, 0, -1), + EndTime: today, + Interval: "day", + }) + require.NoError(t, err) }) - require.NoError(t, err) - }) - t.Run("AsTemplateAdmin", func(t *testing.T) { - t.Parallel() + t.Run("AsTemplateAdmin", func(t *testing.T) { + t.Parallel() - client := coderdtest.New(t, &coderdtest.Options{}) - admin := coderdtest.CreateFirstUser(t, client) + client := coderdtest.New(t, &coderdtest.Options{}) + admin := coderdtest.CreateFirstUser(t, client) - templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, admin.OrganizationID, rbac.RoleTemplateAdmin()) + templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, admin.OrganizationID, rbac.RoleTemplateAdmin()) - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) - defer cancel() + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() - _, err := templateAdmin.TemplateInsights(ctx, codersdk.TemplateInsightsRequest{ - StartTime: today.AddDate(0, 0, -1), - EndTime: today, + _, err := templateAdmin.TemplateInsights(ctx, codersdk.TemplateInsightsRequest{ + StartTime: today.AddDate(0, 0, -1), + EndTime: today, + Interval: "day", + }) + require.NoError(t, err) + }) + t.Run("AsRegularUser", func(t *testing.T) { + t.Parallel() + + client := coderdtest.New(t, &coderdtest.Options{}) + admin := coderdtest.CreateFirstUser(t, client) + + regular, _ := coderdtest.CreateAnotherUser(t, client, admin.OrganizationID) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() + + _, err := regular.TemplateInsights(ctx, codersdk.TemplateInsightsRequest{ + StartTime: today.AddDate(0, 0, -1), + EndTime: today, + Interval: "day", + }) + require.Error(t, err) }) - require.NoError(t, err) }) - t.Run("AsRegularUser", func(t *testing.T) { + + t.Run("IntervalNone", func(t *testing.T) { t.Parallel() - client := coderdtest.New(t, &coderdtest.Options{}) - admin := coderdtest.CreateFirstUser(t, client) + t.Run("AsOwner", func(t *testing.T) { + t.Parallel() + + client := coderdtest.New(t, &coderdtest.Options{}) + _ = coderdtest.CreateFirstUser(t, client) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() + + _, err := client.TemplateInsights(ctx, codersdk.TemplateInsightsRequest{ + StartTime: today.AddDate(0, 0, -1), + EndTime: today, + }) + require.NoError(t, err) + }) + t.Run("AsTemplateAdmin", func(t *testing.T) { + t.Parallel() + + client := coderdtest.New(t, &coderdtest.Options{}) + admin := coderdtest.CreateFirstUser(t, client) + + templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, admin.OrganizationID, rbac.RoleTemplateAdmin()) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() + + _, err := templateAdmin.TemplateInsights(ctx, codersdk.TemplateInsightsRequest{ + StartTime: today.AddDate(0, 0, -1), + EndTime: today, + }) + require.NoError(t, err) + }) + t.Run("AsRegularUser", func(t *testing.T) { + t.Parallel() + + client := coderdtest.New(t, &coderdtest.Options{}) + admin := coderdtest.CreateFirstUser(t, client) - regular, _ := coderdtest.CreateAnotherUser(t, client, admin.OrganizationID) + regular, _ := coderdtest.CreateAnotherUser(t, client, admin.OrganizationID) - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) - defer cancel() + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() - _, err := regular.TemplateInsights(ctx, codersdk.TemplateInsightsRequest{ - StartTime: today.AddDate(0, 0, -1), - EndTime: today, + _, err := regular.TemplateInsights(ctx, codersdk.TemplateInsightsRequest{ + StartTime: today.AddDate(0, 0, -1), + EndTime: today, + }) + require.Error(t, err) }) - require.Error(t, err) }) } From d2cc0ffb1ee2ca7ef655ad2141d29962888149ad Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Mon, 31 Jul 2023 13:06:22 +0200 Subject: [PATCH 3/7] ACL --- coderd/insights_test.go | 141 ++++++++++------------------- enterprise/coderd/insights_test.go | 63 ++++++++----- 2 files changed, 88 insertions(+), 116 deletions(-) diff --git a/coderd/insights_test.go b/coderd/insights_test.go index 234cc978146ae..e8a6daf4ad607 100644 --- a/coderd/insights_test.go +++ b/coderd/insights_test.go @@ -2,6 +2,7 @@ package coderd_test import ( "context" + "fmt" "io" "testing" "time" @@ -388,114 +389,72 @@ func TestTemplateInsightsRBAC(t *testing.T) { y, m, d := time.Now().UTC().Date() today := time.Date(y, m, d, 0, 0, 0, 0, time.UTC) - t.Run("IntervalDay", func(t *testing.T) { - t.Parallel() - - t.Run("AsOwner", func(t *testing.T) { - t.Parallel() - - client := coderdtest.New(t, &coderdtest.Options{}) - _ = coderdtest.CreateFirstUser(t, client) + type test struct { + interval codersdk.InsightsReportInterval + } - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) - defer cancel() + tests := []test{ + {codersdk.InsightsReportIntervalDay}, + {""}, + } - _, err := client.TemplateInsights(ctx, codersdk.TemplateInsightsRequest{ - StartTime: today.AddDate(0, 0, -1), - EndTime: today, - Interval: "day", - }) - require.NoError(t, err) - }) - t.Run("AsTemplateAdmin", func(t *testing.T) { + for _, tt := range tests { + tt := tt + t.Run(fmt.Sprintf("with interval=%q", tt.interval), func(t *testing.T) { t.Parallel() - client := coderdtest.New(t, &coderdtest.Options{}) - admin := coderdtest.CreateFirstUser(t, client) + t.Run("AsOwner", func(t *testing.T) { + t.Parallel() - templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, admin.OrganizationID, rbac.RoleTemplateAdmin()) + client := coderdtest.New(t, &coderdtest.Options{}) + _ = coderdtest.CreateFirstUser(t, client) - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) - defer cancel() + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() - _, err := templateAdmin.TemplateInsights(ctx, codersdk.TemplateInsightsRequest{ - StartTime: today.AddDate(0, 0, -1), - EndTime: today, - Interval: "day", + _, err := client.TemplateInsights(ctx, codersdk.TemplateInsightsRequest{ + StartTime: today.AddDate(0, 0, -1), + EndTime: today, + Interval: tt.interval, + }) + require.NoError(t, err) }) - require.NoError(t, err) - }) - t.Run("AsRegularUser", func(t *testing.T) { - t.Parallel() + t.Run("AsTemplateAdmin", func(t *testing.T) { + t.Parallel() - client := coderdtest.New(t, &coderdtest.Options{}) - admin := coderdtest.CreateFirstUser(t, client) + client := coderdtest.New(t, &coderdtest.Options{}) + admin := coderdtest.CreateFirstUser(t, client) - regular, _ := coderdtest.CreateAnotherUser(t, client, admin.OrganizationID) + templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, admin.OrganizationID, rbac.RoleTemplateAdmin()) - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) - defer cancel() + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() - _, err := regular.TemplateInsights(ctx, codersdk.TemplateInsightsRequest{ - StartTime: today.AddDate(0, 0, -1), - EndTime: today, - Interval: "day", + _, err := templateAdmin.TemplateInsights(ctx, codersdk.TemplateInsightsRequest{ + StartTime: today.AddDate(0, 0, -1), + EndTime: today, + Interval: tt.interval, + }) + require.NoError(t, err) }) - require.Error(t, err) - }) - }) + t.Run("AsRegularUser", func(t *testing.T) { + t.Parallel() - t.Run("IntervalNone", func(t *testing.T) { - t.Parallel() + client := coderdtest.New(t, &coderdtest.Options{}) + admin := coderdtest.CreateFirstUser(t, client) - t.Run("AsOwner", func(t *testing.T) { - t.Parallel() + regular, _ := coderdtest.CreateAnotherUser(t, client, admin.OrganizationID) - client := coderdtest.New(t, &coderdtest.Options{}) - _ = coderdtest.CreateFirstUser(t, client) + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) - defer cancel() - - _, err := client.TemplateInsights(ctx, codersdk.TemplateInsightsRequest{ - StartTime: today.AddDate(0, 0, -1), - EndTime: today, + _, err := regular.TemplateInsights(ctx, codersdk.TemplateInsightsRequest{ + StartTime: today.AddDate(0, 0, -1), + EndTime: today, + Interval: tt.interval, + }) + require.Error(t, err) }) - require.NoError(t, err) }) - t.Run("AsTemplateAdmin", func(t *testing.T) { - t.Parallel() - - client := coderdtest.New(t, &coderdtest.Options{}) - admin := coderdtest.CreateFirstUser(t, client) - - templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, admin.OrganizationID, rbac.RoleTemplateAdmin()) - - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) - defer cancel() - - _, err := templateAdmin.TemplateInsights(ctx, codersdk.TemplateInsightsRequest{ - StartTime: today.AddDate(0, 0, -1), - EndTime: today, - }) - require.NoError(t, err) - }) - t.Run("AsRegularUser", func(t *testing.T) { - t.Parallel() - - client := coderdtest.New(t, &coderdtest.Options{}) - admin := coderdtest.CreateFirstUser(t, client) - - regular, _ := coderdtest.CreateAnotherUser(t, client, admin.OrganizationID) - - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) - defer cancel() - - _, err := regular.TemplateInsights(ctx, codersdk.TemplateInsightsRequest{ - StartTime: today.AddDate(0, 0, -1), - EndTime: today, - }) - require.Error(t, err) - }) - }) + } } diff --git a/enterprise/coderd/insights_test.go b/enterprise/coderd/insights_test.go index 1a00ebd211303..be74c8dad2808 100644 --- a/enterprise/coderd/insights_test.go +++ b/enterprise/coderd/insights_test.go @@ -2,6 +2,7 @@ package coderd_test import ( "context" + "fmt" "testing" "time" @@ -15,41 +16,53 @@ import ( "github.com/coder/coder/testutil" ) -func TestTemplateInsightsRBAC(t *testing.T) { +func TestTemplateInsightsWithTemplateAdminACL(t *testing.T) { t.Parallel() y, m, d := time.Now().UTC().Date() today := time.Date(y, m, d, 0, 0, 0, 0, time.UTC) - t.Run("AsRegularUserWithTemplateAdminACL", func(t *testing.T) { - t.Parallel() + type test struct { + interval codersdk.InsightsReportInterval + } - client, admin := coderdenttest.New(t, &coderdenttest.Options{LicenseOptions: &coderdenttest.LicenseOptions{ - Features: license.Features{ - codersdk.FeatureTemplateRBAC: 1, - }, - }}) + tests := []test{ + {codersdk.InsightsReportIntervalDay}, + {""}, + } - version := coderdtest.CreateTemplateVersion(t, client, admin.OrganizationID, nil) - template := coderdtest.CreateTemplate(t, client, admin.OrganizationID, version.ID) + for _, tt := range tests { + tt := tt + t.Run(fmt.Sprintf("with interval=%q", tt.interval), func(t *testing.T) { + t.Parallel() - regular, regularUser := coderdtest.CreateAnotherUser(t, client, admin.OrganizationID) + client, admin := coderdenttest.New(t, &coderdenttest.Options{LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureTemplateRBAC: 1, + }, + }}) - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) - defer cancel() + version := coderdtest.CreateTemplateVersion(t, client, admin.OrganizationID, nil) + template := coderdtest.CreateTemplate(t, client, admin.OrganizationID, version.ID) - err := client.UpdateTemplateACL(ctx, template.ID, codersdk.UpdateTemplateACL{ - UserPerms: map[string]codersdk.TemplateRole{ - regularUser.ID.String(): codersdk.TemplateRoleAdmin, - }, - }) - require.NoError(t, err) + regular, regularUser := coderdtest.CreateAnotherUser(t, client, admin.OrganizationID) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() + + err := client.UpdateTemplateACL(ctx, template.ID, codersdk.UpdateTemplateACL{ + UserPerms: map[string]codersdk.TemplateRole{ + regularUser.ID.String(): codersdk.TemplateRoleAdmin, + }, + }) + require.NoError(t, err) - _, err = regular.TemplateInsights(ctx, codersdk.TemplateInsightsRequest{ - StartTime: today.AddDate(0, 0, -1), - EndTime: today, - TemplateIDs: []uuid.UUID{template.ID}, + _, err = regular.TemplateInsights(ctx, codersdk.TemplateInsightsRequest{ + StartTime: today.AddDate(0, 0, -1), + EndTime: today, + TemplateIDs: []uuid.UUID{template.ID}, + }) + require.NoError(t, err) }) - require.NoError(t, err) - }) + } } From 08f243da2cac9236383bd258d17dbd8370477a0e Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Mon, 31 Jul 2023 13:31:34 +0200 Subject: [PATCH 4/7] User latency tests --- coderd/database/dbauthz/dbauthz.go | 18 +++++-- coderd/insights.go | 10 ++-- coderd/insights_test.go | 75 +++++++++++++++++++++++++++++- 3 files changed, 94 insertions(+), 9 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 48e8053ed5070..f3ea758dce9f9 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1205,7 +1205,6 @@ func (q *querier) GetTemplateDailyInsights(ctx context.Context, arg database.Get return nil, err } } - if len(arg.TemplateIDs) == 0 { if err := q.authorizeContext(ctx, rbac.ActionUpdate, rbac.ResourceTemplate.All()); err != nil { return nil, err @@ -1225,7 +1224,6 @@ func (q *querier) GetTemplateInsights(ctx context.Context, arg database.GetTempl return database.GetTemplateInsightsRow{}, err } } - if len(arg.TemplateIDs) == 0 { if err := q.authorizeContext(ctx, rbac.ActionUpdate, rbac.ResourceTemplate.All()); err != nil { return database.GetTemplateInsightsRow{}, err @@ -1401,8 +1399,20 @@ func (q *querier) GetUserCount(ctx context.Context) (int64, error) { } func (q *querier) GetUserLatencyInsights(ctx context.Context, arg database.GetUserLatencyInsightsParams) ([]database.GetUserLatencyInsightsRow, error) { - if err := q.authorizeContext(ctx, rbac.ActionRead, rbac.ResourceSystem); err != nil { - return nil, err + for _, templateID := range arg.TemplateIDs { + template, err := q.db.GetTemplateByID(ctx, templateID) + if err != nil { + return nil, err + } + + if err := q.authorizeContext(ctx, rbac.ActionUpdate, template); err != nil { + return nil, err + } + } + if len(arg.TemplateIDs) == 0 { + if err := q.authorizeContext(ctx, rbac.ActionUpdate, rbac.ResourceTemplate.All()); err != nil { + return nil, err + } } return q.db.GetUserLatencyInsights(ctx, arg) } diff --git a/coderd/insights.go b/coderd/insights.go index e4e61d569570c..5d2644a75a17c 100644 --- a/coderd/insights.go +++ b/coderd/insights.go @@ -64,10 +64,6 @@ func (api *API) deploymentDAUs(rw http.ResponseWriter, r *http.Request) { // @Router /insights/user-latency [get] func (api *API) insightsUserLatency(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() - if !api.Authorize(r, rbac.ActionRead, rbac.ResourceDeploymentValues) { - httpapi.Forbidden(rw) - return - } p := httpapi.NewQueryParamParser(). Required("start_time"). @@ -100,6 +96,12 @@ func (api *API) insightsUserLatency(rw http.ResponseWriter, r *http.Request) { TemplateIDs: templateIDs, }) if err != nil { + if httpapi.Is404Error(err) { + httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{ + Message: "Template not found or forbidden access.", + }) + return + } httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error fetching user latency.", Detail: err.Error(), diff --git a/coderd/insights_test.go b/coderd/insights_test.go index e8a6daf4ad607..86f240e2a8446 100644 --- a/coderd/insights_test.go +++ b/coderd/insights_test.go @@ -383,7 +383,7 @@ func TestTemplateInsights_BadRequest(t *testing.T) { assert.Error(t, err, "want error for bad interval") } -func TestTemplateInsightsRBAC(t *testing.T) { +func TestTemplateInsights_RBAC(t *testing.T) { t.Parallel() y, m, d := time.Now().UTC().Date() @@ -458,3 +458,76 @@ func TestTemplateInsightsRBAC(t *testing.T) { }) } } + +func TestUserLatencyInsights_RBAC(t *testing.T) { + t.Parallel() + + y, m, d := time.Now().UTC().Date() + today := time.Date(y, m, d, 0, 0, 0, 0, time.UTC) + + type test struct { + interval codersdk.InsightsReportInterval + } + + tests := []test{ + {codersdk.InsightsReportIntervalDay}, + {""}, + } + + for _, tt := range tests { + tt := tt + t.Run(fmt.Sprintf("with interval=%q", tt.interval), func(t *testing.T) { + t.Parallel() + + t.Run("AsOwner", func(t *testing.T) { + t.Parallel() + + client := coderdtest.New(t, &coderdtest.Options{}) + _ = coderdtest.CreateFirstUser(t, client) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() + + _, err := client.UserLatencyInsights(ctx, codersdk.UserLatencyInsightsRequest{ + StartTime: today, + EndTime: time.Now().UTC().Truncate(time.Hour).Add(time.Hour), // Round up to include the current hour. + }) + require.NoError(t, err) + }) + t.Run("AsTemplateAdmin", func(t *testing.T) { + t.Parallel() + + client := coderdtest.New(t, &coderdtest.Options{}) + admin := coderdtest.CreateFirstUser(t, client) + + templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, admin.OrganizationID, rbac.RoleTemplateAdmin()) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() + + _, err := templateAdmin.UserLatencyInsights(ctx, codersdk.UserLatencyInsightsRequest{ + StartTime: today, + EndTime: time.Now().UTC().Truncate(time.Hour).Add(time.Hour), // Round up to include the current hour. + }) + require.NoError(t, err) + }) + t.Run("AsRegularUser", func(t *testing.T) { + t.Parallel() + + client := coderdtest.New(t, &coderdtest.Options{}) + admin := coderdtest.CreateFirstUser(t, client) + + regular, _ := coderdtest.CreateAnotherUser(t, client, admin.OrganizationID) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() + + _, err := regular.UserLatencyInsights(ctx, codersdk.UserLatencyInsightsRequest{ + StartTime: today, + EndTime: time.Now().UTC().Truncate(time.Hour).Add(time.Hour), // Round up to include the current hour. + }) + require.Error(t, err) + }) + }) + } +} From 8c5a834c5c895c898074d6cadaf0b63b7e05806f Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Mon, 31 Jul 2023 13:42:18 +0200 Subject: [PATCH 5/7] withTemplate --- coderd/insights_test.go | 54 +++++++++++++++++++++++++++++++---------- 1 file changed, 41 insertions(+), 13 deletions(-) diff --git a/coderd/insights_test.go b/coderd/insights_test.go index 86f240e2a8446..103ff147c8296 100644 --- a/coderd/insights_test.go +++ b/coderd/insights_test.go @@ -390,16 +390,20 @@ func TestTemplateInsights_RBAC(t *testing.T) { today := time.Date(y, m, d, 0, 0, 0, 0, time.UTC) type test struct { - interval codersdk.InsightsReportInterval + interval codersdk.InsightsReportInterval + withTemplate bool } tests := []test{ - {codersdk.InsightsReportIntervalDay}, - {""}, + {codersdk.InsightsReportIntervalDay, true}, + {codersdk.InsightsReportIntervalDay, false}, + {"", true}, + {"", false}, } for _, tt := range tests { tt := tt + t.Run(fmt.Sprintf("with interval=%q", tt.interval), func(t *testing.T) { t.Parallel() @@ -407,15 +411,23 @@ func TestTemplateInsights_RBAC(t *testing.T) { t.Parallel() client := coderdtest.New(t, &coderdtest.Options{}) - _ = coderdtest.CreateFirstUser(t, client) + admin := coderdtest.CreateFirstUser(t, client) ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) defer cancel() + var templateIDs []uuid.UUID + if tt.withTemplate { + version := coderdtest.CreateTemplateVersion(t, client, admin.OrganizationID, nil) + template := coderdtest.CreateTemplate(t, client, admin.OrganizationID, version.ID) + templateIDs = append(templateIDs, template.ID) + } + _, err := client.TemplateInsights(ctx, codersdk.TemplateInsightsRequest{ - StartTime: today.AddDate(0, 0, -1), - EndTime: today, - Interval: tt.interval, + StartTime: today.AddDate(0, 0, -1), + EndTime: today, + Interval: tt.interval, + TemplateIDs: templateIDs, }) require.NoError(t, err) }) @@ -430,10 +442,18 @@ func TestTemplateInsights_RBAC(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) defer cancel() + var templateIDs []uuid.UUID + if tt.withTemplate { + version := coderdtest.CreateTemplateVersion(t, client, admin.OrganizationID, nil) + template := coderdtest.CreateTemplate(t, client, admin.OrganizationID, version.ID) + templateIDs = append(templateIDs, template.ID) + } + _, err := templateAdmin.TemplateInsights(ctx, codersdk.TemplateInsightsRequest{ - StartTime: today.AddDate(0, 0, -1), - EndTime: today, - Interval: tt.interval, + StartTime: today.AddDate(0, 0, -1), + EndTime: today, + Interval: tt.interval, + TemplateIDs: templateIDs, }) require.NoError(t, err) }) @@ -448,10 +468,18 @@ func TestTemplateInsights_RBAC(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) defer cancel() + var templateIDs []uuid.UUID + if tt.withTemplate { + version := coderdtest.CreateTemplateVersion(t, client, admin.OrganizationID, nil) + template := coderdtest.CreateTemplate(t, client, admin.OrganizationID, version.ID) + templateIDs = append(templateIDs, template.ID) + } + _, err := regular.TemplateInsights(ctx, codersdk.TemplateInsightsRequest{ - StartTime: today.AddDate(0, 0, -1), - EndTime: today, - Interval: tt.interval, + StartTime: today.AddDate(0, 0, -1), + EndTime: today, + Interval: tt.interval, + TemplateIDs: templateIDs, }) require.Error(t, err) }) From 39d691cd0de0ee31682403027b7baa248acddf7f Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Mon, 31 Jul 2023 13:44:17 +0200 Subject: [PATCH 6/7] withTemplate 2 --- coderd/insights_test.go | 47 ++++++++++++++++++++++++++++++++--------- 1 file changed, 37 insertions(+), 10 deletions(-) diff --git a/coderd/insights_test.go b/coderd/insights_test.go index 103ff147c8296..d0854016d3d54 100644 --- a/coderd/insights_test.go +++ b/coderd/insights_test.go @@ -494,12 +494,15 @@ func TestUserLatencyInsights_RBAC(t *testing.T) { today := time.Date(y, m, d, 0, 0, 0, 0, time.UTC) type test struct { - interval codersdk.InsightsReportInterval + interval codersdk.InsightsReportInterval + withTemplate bool } tests := []test{ - {codersdk.InsightsReportIntervalDay}, - {""}, + {codersdk.InsightsReportIntervalDay, true}, + {codersdk.InsightsReportIntervalDay, false}, + {"", true}, + {"", false}, } for _, tt := range tests { @@ -511,14 +514,22 @@ func TestUserLatencyInsights_RBAC(t *testing.T) { t.Parallel() client := coderdtest.New(t, &coderdtest.Options{}) - _ = coderdtest.CreateFirstUser(t, client) + admin := coderdtest.CreateFirstUser(t, client) ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) defer cancel() + var templateIDs []uuid.UUID + if tt.withTemplate { + version := coderdtest.CreateTemplateVersion(t, client, admin.OrganizationID, nil) + template := coderdtest.CreateTemplate(t, client, admin.OrganizationID, version.ID) + templateIDs = append(templateIDs, template.ID) + } + _, err := client.UserLatencyInsights(ctx, codersdk.UserLatencyInsightsRequest{ - StartTime: today, - EndTime: time.Now().UTC().Truncate(time.Hour).Add(time.Hour), // Round up to include the current hour. + StartTime: today, + EndTime: time.Now().UTC().Truncate(time.Hour).Add(time.Hour), // Round up to include the current hour. + TemplateIDs: templateIDs, }) require.NoError(t, err) }) @@ -533,9 +544,17 @@ func TestUserLatencyInsights_RBAC(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) defer cancel() + var templateIDs []uuid.UUID + if tt.withTemplate { + version := coderdtest.CreateTemplateVersion(t, client, admin.OrganizationID, nil) + template := coderdtest.CreateTemplate(t, client, admin.OrganizationID, version.ID) + templateIDs = append(templateIDs, template.ID) + } + _, err := templateAdmin.UserLatencyInsights(ctx, codersdk.UserLatencyInsightsRequest{ - StartTime: today, - EndTime: time.Now().UTC().Truncate(time.Hour).Add(time.Hour), // Round up to include the current hour. + StartTime: today, + EndTime: time.Now().UTC().Truncate(time.Hour).Add(time.Hour), // Round up to include the current hour. + TemplateIDs: templateIDs, }) require.NoError(t, err) }) @@ -550,9 +569,17 @@ func TestUserLatencyInsights_RBAC(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) defer cancel() + var templateIDs []uuid.UUID + if tt.withTemplate { + version := coderdtest.CreateTemplateVersion(t, client, admin.OrganizationID, nil) + template := coderdtest.CreateTemplate(t, client, admin.OrganizationID, version.ID) + templateIDs = append(templateIDs, template.ID) + } + _, err := regular.UserLatencyInsights(ctx, codersdk.UserLatencyInsightsRequest{ - StartTime: today, - EndTime: time.Now().UTC().Truncate(time.Hour).Add(time.Hour), // Round up to include the current hour. + StartTime: today, + EndTime: time.Now().UTC().Truncate(time.Hour).Add(time.Hour), // Round up to include the current hour. + TemplateIDs: templateIDs, }) require.Error(t, err) }) From a5e01a82263846f3c318bdf0ee956488d530dfcc Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Mon, 31 Jul 2023 15:20:42 +0200 Subject: [PATCH 7/7] Address PR comments --- coderd/insights.go | 8 ++------ coderd/insights_test.go | 7 +++++++ 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/coderd/insights.go b/coderd/insights.go index 5d2644a75a17c..9152f14983814 100644 --- a/coderd/insights.go +++ b/coderd/insights.go @@ -97,9 +97,7 @@ func (api *API) insightsUserLatency(rw http.ResponseWriter, r *http.Request) { }) if err != nil { if httpapi.Is404Error(err) { - httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{ - Message: "Template not found or forbidden access.", - }) + httpapi.ResourceNotFound(rw) return } httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ @@ -218,9 +216,7 @@ func (api *API) insightsTemplates(rw http.ResponseWriter, r *http.Request) { return nil }, nil) if httpapi.Is404Error(err) { - httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{ - Message: "Template not found or forbidden access.", - }) + httpapi.ResourceNotFound(rw) return } if err != nil { diff --git a/coderd/insights_test.go b/coderd/insights_test.go index d0854016d3d54..8f518e0746076 100644 --- a/coderd/insights_test.go +++ b/coderd/insights_test.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "io" + "net/http" "testing" "time" @@ -482,6 +483,9 @@ func TestTemplateInsights_RBAC(t *testing.T) { TemplateIDs: templateIDs, }) require.Error(t, err) + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusNotFound, apiErr.StatusCode()) }) }) } @@ -582,6 +586,9 @@ func TestUserLatencyInsights_RBAC(t *testing.T) { TemplateIDs: templateIDs, }) require.Error(t, err) + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusNotFound, apiErr.StatusCode()) }) }) }