From 23c34c43a62e75bf922ee7db800cee43dd230c7f Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 25 Jun 2024 09:17:50 -0500 Subject: [PATCH 1/6] chore: add organization_id filter to audit logs --- coderd/database/queries.sql.go | 40 ++++++++++++++++----------- coderd/database/queries/auditlogs.sql | 6 ++++ 2 files changed, 30 insertions(+), 16 deletions(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 4f113323a024f..84e3439250db7 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -500,52 +500,58 @@ WHERE resource_id = $4 ELSE true END + -- Filter organization_id + AND CASE + WHEN $5 :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN + audit_logs.organization_id = $5 + ELSE true + END -- Filter by resource_target AND CASE - WHEN $5 :: text != '' THEN - resource_target = $5 + WHEN $6 :: text != '' THEN + resource_target = $6 ELSE true END -- Filter action AND CASE - WHEN $6 :: text != '' THEN - action = $6 :: audit_action + WHEN $7 :: text != '' THEN + action = $7 :: audit_action ELSE true END -- Filter by user_id AND CASE - WHEN $7 :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN - user_id = $7 + WHEN $8 :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN + user_id = $8 ELSE true END -- Filter by username AND CASE - WHEN $8 :: text != '' THEN - user_id = (SELECT id FROM users WHERE lower(username) = lower($8) AND deleted = false) + WHEN $9 :: text != '' THEN + user_id = (SELECT id FROM users WHERE lower(username) = lower($9) AND deleted = false) ELSE true END -- Filter by user_email AND CASE - WHEN $9 :: text != '' THEN - users.email = $9 + WHEN $10 :: text != '' THEN + users.email = $10 ELSE true END -- Filter by date_from AND CASE - WHEN $10 :: timestamp with time zone != '0001-01-01 00:00:00Z' THEN - "time" >= $10 + WHEN $11 :: timestamp with time zone != '0001-01-01 00:00:00Z' THEN + "time" >= $11 ELSE true END -- Filter by date_to AND CASE - WHEN $11 :: timestamp with time zone != '0001-01-01 00:00:00Z' THEN - "time" <= $11 + WHEN $12 :: timestamp with time zone != '0001-01-01 00:00:00Z' THEN + "time" <= $12 ELSE true END -- Filter by build_reason AND CASE - WHEN $12::text != '' THEN - workspace_builds.reason::text = $12 + WHEN $13::text != '' THEN + workspace_builds.reason::text = $13 ELSE true END ORDER BY @@ -561,6 +567,7 @@ type GetAuditLogsOffsetParams struct { Offset int32 `db:"offset" json:"offset"` ResourceType string `db:"resource_type" json:"resource_type"` ResourceID uuid.UUID `db:"resource_id" json:"resource_id"` + OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"` ResourceTarget string `db:"resource_target" json:"resource_target"` Action string `db:"action" json:"action"` UserID uuid.UUID `db:"user_id" json:"user_id"` @@ -611,6 +618,7 @@ func (q *sqlQuerier) GetAuditLogsOffset(ctx context.Context, arg GetAuditLogsOff arg.Offset, arg.ResourceType, arg.ResourceID, + arg.OrganizationID, arg.ResourceTarget, arg.Action, arg.UserID, diff --git a/coderd/database/queries/auditlogs.sql b/coderd/database/queries/auditlogs.sql index d05b5bbe371e0..653082bd69fee 100644 --- a/coderd/database/queries/auditlogs.sql +++ b/coderd/database/queries/auditlogs.sql @@ -59,6 +59,12 @@ WHERE resource_id = @resource_id ELSE true END + -- Filter organization_id + AND CASE + WHEN @organization_id :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN + audit_logs.organization_id = @organization_id + ELSE true + END -- Filter by resource_target AND CASE WHEN @resource_target :: text != '' THEN From 2950d0490d93ce70f3b2e26b7152bb8c5482655c Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 25 Jun 2024 09:31:29 -0500 Subject: [PATCH 2/6] dbauthz --- coderd/database/dbauthz/dbauthz.go | 22 +++++++++++++++++----- coderd/searchquery/search.go | 1 + 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index f32e176754fa1..c4b9a0733e7d3 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1200,12 +1200,24 @@ func (q *querier) GetApplicationName(ctx context.Context) (string, error) { } func (q *querier) GetAuditLogsOffset(ctx context.Context, arg database.GetAuditLogsOffsetParams) ([]database.GetAuditLogsOffsetRow, error) { - // To optimize audit logs, we only check the global audit log permission once. - // This is because we expect a large unbounded set of audit logs, and applying a SQL - // filter would slow down the query for no benefit. - if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceAuditLog); err != nil { - return nil, err + // To optimize the authz checks for audit logs, do not run an authorize + // check on each individual audit log row. In practice, audit logs are either + // fetched from a global or an organization scope. + // Applying a SQL filter would slow down the query for no benefit on how this query is + // actually used. + + if arg.OrganizationID != uuid.Nil { + // Organization scoped logs + if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceAuditLog.InOrg(arg.OrganizationID)); err != nil { + return nil, err + } + } else { + // Site wide scoped logs + if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceAuditLog); err != nil { + return nil, err + } } + return q.db.GetAuditLogsOffset(ctx, arg) } diff --git a/coderd/searchquery/search.go b/coderd/searchquery/search.go index cef971a731cbd..ab44530cc7c4c 100644 --- a/coderd/searchquery/search.go +++ b/coderd/searchquery/search.go @@ -30,6 +30,7 @@ func AuditLogs(query string) (database.GetAuditLogsOffsetParams, []codersdk.Vali const dateLayout = "2006-01-02" parser := httpapi.NewQueryParamParser() filter := database.GetAuditLogsOffsetParams{ + OrganizationID: parser.UUID(values, uuid.Nil, "organization_id"), ResourceID: parser.UUID(values, uuid.Nil, "resource_id"), ResourceTarget: parser.String(values, "", "resource_target"), Username: parser.String(values, "", "username"), From 555b045465afe3ec6edd0dfb6aaf5a11271ea823 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 25 Jun 2024 09:50:06 -0500 Subject: [PATCH 3/6] chore: implement organization scoped audit log requests --- coderd/audit.go | 10 +++++++- coderd/audit_test.go | 43 ++++++++++++++++++++++++++++++++++ coderd/database/dbmem/dbmem.go | 3 +++ coderd/members.go | 9 +++---- codersdk/audit.go | 1 + 5 files changed, 61 insertions(+), 5 deletions(-) diff --git a/coderd/audit.go b/coderd/audit.go index a818f681038ed..37966ead7e916 100644 --- a/coderd/audit.go +++ b/coderd/audit.go @@ -18,6 +18,7 @@ import ( "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/db2sdk" + "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/coder/v2/coderd/httpmw" "github.com/coder/coder/v2/coderd/searchquery" @@ -61,6 +62,10 @@ func (api *API) auditLogs(rw http.ResponseWriter, r *http.Request) { } dblogs, err := api.Database.GetAuditLogsOffset(ctx, filter) + if dbauthz.IsNotAuthorizedError(err) { + httpapi.Forbidden(rw) + return + } if err != nil { httpapi.InternalServerError(rw, err) return @@ -139,6 +144,9 @@ func (api *API) generateFakeAuditLog(rw http.ResponseWriter, r *http.Request) { if len(params.AdditionalFields) == 0 { params.AdditionalFields = json.RawMessage("{}") } + if params.OrganizationID == uuid.Nil { + params.OrganizationID = uuid.New() + } _, err = api.Database.InsertAuditLog(ctx, database.InsertAuditLogParams{ ID: uuid.New(), @@ -155,7 +163,7 @@ func (api *API) generateFakeAuditLog(rw http.ResponseWriter, r *http.Request) { AdditionalFields: params.AdditionalFields, RequestID: uuid.Nil, // no request ID to attach this to ResourceIcon: "", - OrganizationID: uuid.New(), + OrganizationID: params.OrganizationID, }) if err != nil { httpapi.InternalServerError(rw, err) diff --git a/coderd/audit_test.go b/coderd/audit_test.go index 9de6f65071a02..a0520ea954ab4 100644 --- a/coderd/audit_test.go +++ b/coderd/audit_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "net/http" "strconv" "testing" "time" @@ -11,6 +12,7 @@ import ( "github.com/google/uuid" "github.com/stretchr/testify/require" + "cdr.dev/slog/sloggers/slogtest" "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/coderdtest" "github.com/coder/coder/v2/coderd/database" @@ -135,6 +137,47 @@ func TestAuditLogs(t *testing.T) { require.Equal(t, auditLogs.AuditLogs[0].ResourceLink, fmt.Sprintf("/@%s/%s/builds/%s", workspace.OwnerName, workspace.Name, buildNumberString)) }) + + t.Run("Organization", func(t *testing.T) { + t.Parallel() + + logger := slogtest.Make(t, &slogtest.Options{ + IgnoreErrors: true, + }) + ctx := context.Background() + client := coderdtest.New(t, &coderdtest.Options{ + Logger: &logger, + }) + owner := coderdtest.CreateFirstUser(t, client) + orgAdmin, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID, rbac.ScopedRoleOrgAdmin(owner.OrganizationID)) + + err := client.CreateTestAuditLog(ctx, codersdk.CreateTestAuditLogRequest{ + ResourceID: owner.UserID, + OrganizationID: owner.OrganizationID, + }) + require.NoError(t, err) + + // Add an extra audit log in another organization + err = client.CreateTestAuditLog(ctx, codersdk.CreateTestAuditLogRequest{ + ResourceID: owner.UserID, + OrganizationID: uuid.New(), + }) + require.NoError(t, err) + + // Fetching audit logs without an organization selector should fail + _, err = orgAdmin.AuditLogs(ctx, codersdk.AuditLogsRequest{}) + var sdkError *codersdk.Error + require.Error(t, err) + require.ErrorAsf(t, err, &sdkError, "error should be of type *codersdk.Error") + require.Equal(t, http.StatusForbidden, sdkError.StatusCode()) + + // Using the organization selector allows the org admin to fetch audit logs + alogs, err := orgAdmin.AuditLogs(ctx, codersdk.AuditLogsRequest{ + SearchQuery: fmt.Sprintf("organization_id:%s", owner.OrganizationID.String()), + }) + require.NoError(t, err) + require.Len(t, alogs.AuditLogs, 1) + }) } func TestAuditLogsFilter(t *testing.T) { diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 04eecd5d86355..1b019cf94eccf 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -1927,6 +1927,9 @@ func (q *FakeQuerier) GetAuditLogsOffset(_ context.Context, arg database.GetAudi arg.Offset-- continue } + if arg.OrganizationID != uuid.Nil && arg.OrganizationID != alog.OrganizationID { + continue + } if arg.Action != "" && !strings.Contains(string(alog.Action), arg.Action) { continue } diff --git a/coderd/members.go b/coderd/members.go index e15aa9d4821f9..24f712b8154c7 100644 --- a/coderd/members.go +++ b/coderd/members.go @@ -176,10 +176,11 @@ func (api *API) putMemberRoles(rw http.ResponseWriter, r *http.Request) { apiKey = httpmw.APIKey(r) auditor = api.Auditor.Load() aReq, commitAudit = audit.InitRequest[database.AuditableOrganizationMember](rw, &audit.RequestParams{ - Audit: *auditor, - Log: api.Logger, - Request: r, - Action: database.AuditActionWrite, + OrganizationID: organization.ID, + Audit: *auditor, + Log: api.Logger, + Request: r, + Action: database.AuditActionWrite, }) ) aReq.Old = member.OrganizationMember.Auditable(member.Username) diff --git a/codersdk/audit.go b/codersdk/audit.go index b1d525d69179f..683db5406c13f 100644 --- a/codersdk/audit.go +++ b/codersdk/audit.go @@ -161,6 +161,7 @@ type CreateTestAuditLogRequest struct { AdditionalFields json.RawMessage `json:"additional_fields,omitempty"` Time time.Time `json:"time,omitempty" format:"date-time"` BuildReason BuildReason `json:"build_reason,omitempty" enums:"autostart,autostop,initiator"` + OrganizationID uuid.UUID `json:"organization_id,omitempty" format:"uuid"` } // AuditLogs retrieves audit logs from the given page. From b9ccc5a2f220b9afe38ed126458c41ba230a5dce Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 25 Jun 2024 09:55:09 -0500 Subject: [PATCH 4/6] fixup --- coderd/database/dbauthz/dbauthz.go | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index c4b9a0733e7d3..ef5a3bc3185db 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1206,16 +1206,13 @@ func (q *querier) GetAuditLogsOffset(ctx context.Context, arg database.GetAuditL // Applying a SQL filter would slow down the query for no benefit on how this query is // actually used. + object := rbac.ResourceAuditLog if arg.OrganizationID != uuid.Nil { - // Organization scoped logs - if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceAuditLog.InOrg(arg.OrganizationID)); err != nil { - return nil, err - } - } else { - // Site wide scoped logs - if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceAuditLog); err != nil { - return nil, err - } + object = object.InOrg(arg.OrganizationID) + } + + if err := q.authorizeContext(ctx, policy.ActionRead, object); err != nil { + return nil, err } return q.db.GetAuditLogsOffset(ctx, arg) From e6d415aaf1ac2fea7842697bea02fdc7cfe801bb Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 25 Jun 2024 11:55:55 -0500 Subject: [PATCH 5/6] make gen --- coderd/apidoc/docs.go | 4 ++++ coderd/apidoc/swagger.json | 4 ++++ docs/api/schemas.md | 2 ++ site/src/api/typesGenerated.ts | 1 + 4 files changed, 11 insertions(+) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 1e2f8fa7b1d22..56031db76d535 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -8716,6 +8716,10 @@ const docTemplate = `{ } ] }, + "organization_id": { + "type": "string", + "format": "uuid" + }, "resource_id": { "type": "string", "format": "uuid" diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index c58e0a9de10f5..06c30c1044c98 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -7759,6 +7759,10 @@ } ] }, + "organization_id": { + "type": "string", + "format": "uuid" + }, "resource_id": { "type": "string", "format": "uuid" diff --git a/docs/api/schemas.md b/docs/api/schemas.md index 6329b49ee820e..76da4e3d71196 100644 --- a/docs/api/schemas.md +++ b/docs/api/schemas.md @@ -1179,6 +1179,7 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in "action": "create", "additional_fields": [0], "build_reason": "autostart", + "organization_id": "7c60d51f-b44e-4682-87d6-449835ea4de6", "resource_id": "4d5215ed-38bb-48ed-879a-fdb9ca58522f", "resource_type": "template", "time": "2019-08-24T14:15:22Z" @@ -1192,6 +1193,7 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in | `action` | [codersdk.AuditAction](#codersdkauditaction) | false | | | | `additional_fields` | array of integer | false | | | | `build_reason` | [codersdk.BuildReason](#codersdkbuildreason) | false | | | +| `organization_id` | string | false | | | | `resource_id` | string | false | | | | `resource_type` | [codersdk.ResourceType](#codersdkresourcetype) | false | | | | `time` | string | false | | | diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index ef3e997f1a1e4..167b7d28f8edf 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -281,6 +281,7 @@ export interface CreateTestAuditLogRequest { readonly additional_fields?: Record; readonly time?: string; readonly build_reason?: BuildReason; + readonly organization_id?: string; } // From codersdk/apikey.go From 1cb0c20217a5ddd84fba620d2cb82f20bbc7af14 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 26 Jun 2024 08:48:06 -0500 Subject: [PATCH 6/6] fix pagination --- coderd/audit_test.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/coderd/audit_test.go b/coderd/audit_test.go index a0520ea954ab4..198b792c44ba8 100644 --- a/coderd/audit_test.go +++ b/coderd/audit_test.go @@ -165,7 +165,11 @@ func TestAuditLogs(t *testing.T) { require.NoError(t, err) // Fetching audit logs without an organization selector should fail - _, err = orgAdmin.AuditLogs(ctx, codersdk.AuditLogsRequest{}) + _, err = orgAdmin.AuditLogs(ctx, codersdk.AuditLogsRequest{ + Pagination: codersdk.Pagination{ + Limit: 5, + }, + }) var sdkError *codersdk.Error require.Error(t, err) require.ErrorAsf(t, err, &sdkError, "error should be of type *codersdk.Error") @@ -174,6 +178,9 @@ func TestAuditLogs(t *testing.T) { // Using the organization selector allows the org admin to fetch audit logs alogs, err := orgAdmin.AuditLogs(ctx, codersdk.AuditLogsRequest{ SearchQuery: fmt.Sprintf("organization_id:%s", owner.OrganizationID.String()), + Pagination: codersdk.Pagination{ + Limit: 5, + }, }) require.NoError(t, err) require.Len(t, alogs.AuditLogs, 1)