Skip to content

feat: improve RBAC preconditions for Insights endpoint #8794

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Jul 31, 2023
Merged

feat: improve RBAC preconditions for Insights endpoint #8794

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
23c61c7
WIP
mtojek Jul 28, 2023
5a8e4fb
Merge branch 'main' into 8763-insights-rbac
mtojek Jul 31, 2023
9c88628
WIP
mtojek Jul 31, 2023
d2cc0ff
ACL
mtojek Jul 31, 2023
08f243d
User latency tests
mtojek Jul 31, 2023
8c5a834
withTemplate
mtojek Jul 31, 2023
39d691c
withTemplate 2
mtojek Jul 31, 2023
a5e01a8
Address PR comments
mtojek Jul 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 42 additions & 8 deletions coderd/database/dbauthz/dbauthz.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1195,17 +1195,39 @@ 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
}
}
Comment on lines +1208 to 1212
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't align with our documented permissions here.

Is the reason for making the required permission "update" to ensure that only template owners can view template insights? If so, we should make a separate TemplateInsights resource and make appropriate modifications to our built-in roles.

Otherwise, why not just allow anyone with read permissions to view this data?

Copy link
Member Author

@mtojek mtojek Jul 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the reason for making the required permission "update" to ensure that only template owners can view template insights?

Otherwise, why not just allow anyone with read permissions to view this data?

I suppose that we don't want to share user details for template usage and latency with regular users.

This doesn't align with our documented permissions here.

The Roles table does not describe the use case when a regular user can be granted admin access for a single template using Template ACLs (enterprise feature). This is why I picked the for-loop with "update template" check to see if the user can modify a template, ergo can access insights.

Please let me know what is your recommendation in this case.

Copy link
Member

@johnstcn johnstcn Jul 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the moment, this is OK as a bandage to stop the FE spinner issue. However, the current situation with regard to ACLs is not ideal in terms of integration with the rest of the RBAC package.

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)
}
Expand Down Expand Up @@ -1377,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)
}
Expand Down
17 changes: 9 additions & 8 deletions coderd/insights.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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").
Expand Down Expand Up @@ -100,6 +96,10 @@ func (api *API) insightsUserLatency(rw http.ResponseWriter, r *http.Request) {
TemplateIDs: templateIDs,
})
if err != nil {
if httpapi.Is404Error(err) {
httpapi.ResourceNotFound(rw)
return
}
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
Message: "Internal error fetching user latency.",
Detail: err.Error(),
Expand Down Expand Up @@ -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").
Expand Down Expand Up @@ -191,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 {
Expand Down Expand Up @@ -218,6 +215,10 @@ func (api *API) insightsTemplates(rw http.ResponseWriter, r *http.Request) {

return nil
}, nil)
if httpapi.Is404Error(err) {
httpapi.ResourceNotFound(rw)
return
}
if err != nil {
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
Message: "Internal error fetching template insights.",
Expand Down
213 changes: 213 additions & 0 deletions coderd/insights_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ package coderd_test

import (
"context"
"fmt"
"io"
"net/http"
"testing"
"time"

Expand All @@ -14,6 +16,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"
Expand Down Expand Up @@ -380,3 +383,213 @@ func TestTemplateInsights_BadRequest(t *testing.T) {
})
assert.Error(t, err, "want error for bad interval")
}

func TestTemplateInsights_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
withTemplate bool
}

tests := []test{
{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()

t.Run("AsOwner", func(t *testing.T) {
t.Parallel()

client := coderdtest.New(t, &coderdtest.Options{})
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,
TemplateIDs: templateIDs,
})
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()

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,
TemplateIDs: templateIDs,
})
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()

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,
TemplateIDs: templateIDs,
})
require.Error(t, err)
var apiErr *codersdk.Error
require.ErrorAs(t, err, &apiErr)
require.Equal(t, http.StatusNotFound, apiErr.StatusCode())
})
})
}
}

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
withTemplate bool
}

tests := []test{
{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()

t.Run("AsOwner", func(t *testing.T) {
t.Parallel()

client := coderdtest.New(t, &coderdtest.Options{})
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.
TemplateIDs: templateIDs,
})
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()

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.
TemplateIDs: templateIDs,
})
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()

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.
TemplateIDs: templateIDs,
})
require.Error(t, err)
var apiErr *codersdk.Error
require.ErrorAs(t, err, &apiErr)
require.Equal(t, http.StatusNotFound, apiErr.StatusCode())
})
})
}
}
Loading