Skip to content

Commit a27ac30

Browse files
EmyrkEmyrk
authored
chore: add sql filter to fetching audit logs (coder#14070)
* chore: add sql filter to fetching audit logs * use sqlc.embed for audit logs * fix sql query matcher
1 parent d23670a commit a27ac30

File tree

16 files changed

+562
-245
lines changed

16 files changed

+562
-245
lines changed

‎coderd/audit.go

Lines changed: 43 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -182,17 +182,17 @@ func (api *API) convertAuditLogs(ctx context.Context, dblogs []database.GetAudit
182182
}
183183

184184
func (api *API) convertAuditLog(ctx context.Context, dblog database.GetAuditLogsOffsetRow) codersdk.AuditLog {
185-
ip, _ := netip.AddrFromSlice(dblog.Ip.IPNet.IP)
185+
ip, _ := netip.AddrFromSlice(dblog.AuditLog.Ip.IPNet.IP)
186186

187187
diff := codersdk.AuditDiff{}
188-
_ = json.Unmarshal(dblog.Diff, &diff)
188+
_ = json.Unmarshal(dblog.AuditLog.Diff, &diff)
189189

190190
var user *codersdk.User
191191
if dblog.UserUsername.Valid {
192192
// Leaving the organization IDs blank for now; not sure they are useful for
193193
// the audit query anyway?
194194
sdkUser := db2sdk.User(database.User{
195-
ID: dblog.UserID,
195+
ID: dblog.AuditLog.UserID,
196196
Email: dblog.UserEmail.String,
197197
Username: dblog.UserUsername.String,
198198
CreatedAt: dblog.UserCreatedAt.Time,
@@ -211,7 +211,7 @@ func (api *API) convertAuditLog(ctx context.Context, dblog database.GetAuditLogs
211211
}
212212

213213
var (
214-
additionalFieldsBytes = []byte(dblog.AdditionalFields)
214+
additionalFieldsBytes = []byte(dblog.AuditLog.AdditionalFields)
215215
additionalFields audit.AdditionalFields
216216
err = json.Unmarshal(additionalFieldsBytes, &additionalFields)
217217
)
@@ -224,7 +224,7 @@ func (api *API) convertAuditLog(ctx context.Context, dblog database.GetAuditLogs
224224
WorkspaceOwner: "unknown",
225225
}
226226

227-
dblog.AdditionalFields, err = json.Marshal(resourceInfo)
227+
dblog.AuditLog.AdditionalFields, err = json.Marshal(resourceInfo)
228228
api.Logger.Error(ctx, "marshal additional fields", slog.Error(err))
229229
}
230230

@@ -239,30 +239,30 @@ func (api *API) convertAuditLog(ctx context.Context, dblog database.GetAuditLogs
239239
}
240240

241241
alog := codersdk.AuditLog{
242-
ID: dblog.ID,
243-
RequestID: dblog.RequestID,
244-
Time: dblog.Time,
242+
ID: dblog.AuditLog.ID,
243+
RequestID: dblog.AuditLog.RequestID,
244+
Time: dblog.AuditLog.Time,
245245
// OrganizationID is deprecated.
246-
OrganizationID: dblog.OrganizationID,
246+
OrganizationID: dblog.AuditLog.OrganizationID,
247247
IP: ip,
248-
UserAgent: dblog.UserAgent.String,
249-
ResourceType: codersdk.ResourceType(dblog.ResourceType),
250-
ResourceID: dblog.ResourceID,
251-
ResourceTarget: dblog.ResourceTarget,
252-
ResourceIcon: dblog.ResourceIcon,
253-
Action: codersdk.AuditAction(dblog.Action),
248+
UserAgent: dblog.AuditLog.UserAgent.String,
249+
ResourceType: codersdk.ResourceType(dblog.AuditLog.ResourceType),
250+
ResourceID: dblog.AuditLog.ResourceID,
251+
ResourceTarget: dblog.AuditLog.ResourceTarget,
252+
ResourceIcon: dblog.AuditLog.ResourceIcon,
253+
Action: codersdk.AuditAction(dblog.AuditLog.Action),
254254
Diff: diff,
255-
StatusCode: dblog.StatusCode,
256-
AdditionalFields: dblog.AdditionalFields,
255+
StatusCode: dblog.AuditLog.StatusCode,
256+
AdditionalFields: dblog.AuditLog.AdditionalFields,
257257
User: user,
258258
Description: auditLogDescription(dblog),
259259
ResourceLink: resourceLink,
260260
IsDeleted: isDeleted,
261261
}
262262

263-
if dblog.OrganizationID != uuid.Nil {
263+
if dblog.AuditLog.OrganizationID != uuid.Nil {
264264
alog.Organization = &codersdk.MinimalOrganization{
265-
ID: dblog.OrganizationID,
265+
ID: dblog.AuditLog.OrganizationID,
266266
Name: dblog.OrganizationName,
267267
DisplayName: dblog.OrganizationDisplayName,
268268
Icon: dblog.OrganizationIcon,
@@ -276,32 +276,32 @@ func auditLogDescription(alog database.GetAuditLogsOffsetRow) string {
276276
b := strings.Builder{}
277277
// NOTE: WriteString always returns a nil error, so we never check it
278278
_, _ = b.WriteString("{user} ")
279-
if alog.StatusCode >= 400 {
279+
if alog.AuditLog.StatusCode >= 400 {
280280
_, _ = b.WriteString("unsuccessfully attempted to ")
281-
_, _ = b.WriteString(string(alog.Action))
281+
_, _ = b.WriteString(string(alog.AuditLog.Action))
282282
} else {
283-
_, _ = b.WriteString(codersdk.AuditAction(alog.Action).Friendly())
283+
_, _ = b.WriteString(codersdk.AuditAction(alog.AuditLog.Action).Friendly())
284284
}
285285

286286
// API Key resources (used for authentication) do not have targets and follow the below format:
287287
// "User {logged in | logged out | registered}"
288-
if alog.ResourceType == database.ResourceTypeApiKey &&
289-
(alog.Action == database.AuditActionLogin || alog.Action == database.AuditActionLogout || alog.Action == database.AuditActionRegister) {
288+
if alog.AuditLog.ResourceType == database.ResourceTypeApiKey &&
289+
(alog.AuditLog.Action == database.AuditActionLogin || alog.AuditLog.Action == database.AuditActionLogout || alog.AuditLog.Action == database.AuditActionRegister) {
290290
return b.String()
291291
}
292292

293293
// We don't display the name (target) for git ssh keys. It's fairly long and doesn't
294294
// make too much sense to display.
295-
if alog.ResourceType == database.ResourceTypeGitSshKey {
295+
if alog.AuditLog.ResourceType == database.ResourceTypeGitSshKey {
296296
_, _ = b.WriteString(" the ")
297-
_, _ = b.WriteString(codersdk.ResourceType(alog.ResourceType).FriendlyString())
297+
_, _ = b.WriteString(codersdk.ResourceType(alog.AuditLog.ResourceType).FriendlyString())
298298
return b.String()
299299
}
300300

301301
_, _ = b.WriteString(" ")
302-
_, _ = b.WriteString(codersdk.ResourceType(alog.ResourceType).FriendlyString())
302+
_, _ = b.WriteString(codersdk.ResourceType(alog.AuditLog.ResourceType).FriendlyString())
303303

304-
if alog.ResourceType == database.ResourceTypeConvertLogin {
304+
if alog.AuditLog.ResourceType == database.ResourceTypeConvertLogin {
305305
_, _ = b.WriteString(" to")
306306
}
307307

@@ -311,9 +311,9 @@ func auditLogDescription(alog database.GetAuditLogsOffsetRow) string {
311311
}
312312

313313
func (api *API) auditLogIsResourceDeleted(ctx context.Context, alog database.GetAuditLogsOffsetRow) bool {
314-
switch alog.ResourceType {
314+
switch alog.AuditLog.ResourceType {
315315
case database.ResourceTypeTemplate:
316-
template, err := api.Database.GetTemplateByID(ctx, alog.ResourceID)
316+
template, err := api.Database.GetTemplateByID(ctx, alog.AuditLog.ResourceID)
317317
if err != nil {
318318
if xerrors.Is(err, sql.ErrNoRows) {
319319
return true
@@ -322,7 +322,7 @@ func (api *API) auditLogIsResourceDeleted(ctx context.Context, alog database.Get
322322
}
323323
return template.Deleted
324324
case database.ResourceTypeUser:
325-
user, err := api.Database.GetUserByID(ctx, alog.ResourceID)
325+
user, err := api.Database.GetUserByID(ctx, alog.AuditLog.ResourceID)
326326
if err != nil {
327327
if xerrors.Is(err, sql.ErrNoRows) {
328328
return true
@@ -331,7 +331,7 @@ func (api *API) auditLogIsResourceDeleted(ctx context.Context, alog database.Get
331331
}
332332
return user.Deleted
333333
case database.ResourceTypeWorkspace:
334-
workspace, err := api.Database.GetWorkspaceByID(ctx, alog.ResourceID)
334+
workspace, err := api.Database.GetWorkspaceByID(ctx, alog.AuditLog.ResourceID)
335335
if err != nil {
336336
if xerrors.Is(err, sql.ErrNoRows) {
337337
return true
@@ -340,7 +340,7 @@ func (api *API) auditLogIsResourceDeleted(ctx context.Context, alog database.Get
340340
}
341341
return workspace.Deleted
342342
case database.ResourceTypeWorkspaceBuild:
343-
workspaceBuild, err := api.Database.GetWorkspaceBuildByID(ctx, alog.ResourceID)
343+
workspaceBuild, err := api.Database.GetWorkspaceBuildByID(ctx, alog.AuditLog.ResourceID)
344344
if err != nil {
345345
if xerrors.Is(err, sql.ErrNoRows) {
346346
return true
@@ -357,15 +357,15 @@ func (api *API) auditLogIsResourceDeleted(ctx context.Context, alog database.Get
357357
}
358358
return workspace.Deleted
359359
case database.ResourceTypeOauth2ProviderApp:
360-
_, err := api.Database.GetOAuth2ProviderAppByID(ctx, alog.ResourceID)
360+
_, err := api.Database.GetOAuth2ProviderAppByID(ctx, alog.AuditLog.ResourceID)
361361
if xerrors.Is(err, sql.ErrNoRows) {
362362
return true
363363
} else if err != nil {
364364
api.Logger.Error(ctx, "unable to fetch oauth2 app", slog.Error(err))
365365
}
366366
return false
367367
case database.ResourceTypeOauth2ProviderAppSecret:
368-
_, err := api.Database.GetOAuth2ProviderAppSecretByID(ctx, alog.ResourceID)
368+
_, err := api.Database.GetOAuth2ProviderAppSecretByID(ctx, alog.AuditLog.ResourceID)
369369
if xerrors.Is(err, sql.ErrNoRows) {
370370
return true
371371
} else if err != nil {
@@ -378,17 +378,17 @@ func (api *API) auditLogIsResourceDeleted(ctx context.Context, alog database.Get
378378
}
379379

380380
func (api *API) auditLogResourceLink(ctx context.Context, alog database.GetAuditLogsOffsetRow, additionalFields audit.AdditionalFields) string {
381-
switch alog.ResourceType {
381+
switch alog.AuditLog.ResourceType {
382382
case database.ResourceTypeTemplate:
383383
return fmt.Sprintf("/templates/%s",
384-
alog.ResourceTarget)
384+
alog.AuditLog.ResourceTarget)
385385

386386
case database.ResourceTypeUser:
387387
return fmt.Sprintf("/users?filter=%s",
388-
alog.ResourceTarget)
388+
alog.AuditLog.ResourceTarget)
389389

390390
case database.ResourceTypeWorkspace:
391-
workspace, getWorkspaceErr := api.Database.GetWorkspaceByID(ctx, alog.ResourceID)
391+
workspace, getWorkspaceErr := api.Database.GetWorkspaceByID(ctx, alog.AuditLog.ResourceID)
392392
if getWorkspaceErr != nil {
393393
return ""
394394
}
@@ -397,13 +397,13 @@ func (api *API) auditLogResourceLink(ctx context.Context, alog database.GetAudit
397397
return ""
398398
}
399399
return fmt.Sprintf("/@%s/%s",
400-
workspaceOwner.Username, alog.ResourceTarget)
400+
workspaceOwner.Username, alog.AuditLog.ResourceTarget)
401401

402402
case database.ResourceTypeWorkspaceBuild:
403403
if len(additionalFields.WorkspaceName) == 0 || len(additionalFields.BuildNumber) == 0 {
404404
return ""
405405
}
406-
workspaceBuild, getWorkspaceBuildErr := api.Database.GetWorkspaceBuildByID(ctx, alog.ResourceID)
406+
workspaceBuild, getWorkspaceBuildErr := api.Database.GetWorkspaceBuildByID(ctx, alog.AuditLog.ResourceID)
407407
if getWorkspaceBuildErr != nil {
408408
return ""
409409
}
@@ -419,10 +419,10 @@ func (api *API) auditLogResourceLink(ctx context.Context, alog database.GetAudit
419419
workspaceOwner.Username, additionalFields.WorkspaceName, additionalFields.BuildNumber)
420420

421421
case database.ResourceTypeOauth2ProviderApp:
422-
return fmt.Sprintf("/deployment/oauth2-provider/apps/%s", alog.ResourceID)
422+
return fmt.Sprintf("/deployment/oauth2-provider/apps/%s", alog.AuditLog.ResourceID)
423423

424424
case database.ResourceTypeOauth2ProviderAppSecret:
425-
secret, err := api.Database.GetOAuth2ProviderAppSecretByID(ctx, alog.ResourceID)
425+
secret, err := api.Database.GetOAuth2ProviderAppSecretByID(ctx, alog.AuditLog.ResourceID)
426426
if err != nil {
427427
return ""
428428
}

‎coderd/audit_internal_test.go

Lines changed: 25 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -18,45 +18,55 @@ func TestAuditLogDescription(t *testing.T) {
1818
{
1919
name: "mainline",
2020
alog: database.GetAuditLogsOffsetRow{
21-
Action: database.AuditActionCreate,
22-
StatusCode: 200,
23-
ResourceType: database.ResourceTypeWorkspace,
21+
AuditLog: database.AuditLog{
22+
Action: database.AuditActionCreate,
23+
StatusCode: 200,
24+
ResourceType: database.ResourceTypeWorkspace,
25+
},
2426
},
2527
want: "{user} created workspace {target}",
2628
},
2729
{
2830
name: "unsuccessful",
2931
alog: database.GetAuditLogsOffsetRow{
30-
Action: database.AuditActionCreate,
31-
StatusCode: 400,
32-
ResourceType: database.ResourceTypeWorkspace,
32+
AuditLog: database.AuditLog{
33+
Action: database.AuditActionCreate,
34+
StatusCode: 400,
35+
ResourceType: database.ResourceTypeWorkspace,
36+
},
3337
},
3438
want: "{user} unsuccessfully attempted to create workspace {target}",
3539
},
3640
{
3741
name: "login",
3842
alog: database.GetAuditLogsOffsetRow{
39-
Action: database.AuditActionLogin,
40-
StatusCode: 200,
41-
ResourceType: database.ResourceTypeApiKey,
43+
AuditLog: database.AuditLog{
44+
Action: database.AuditActionLogin,
45+
StatusCode: 200,
46+
ResourceType: database.ResourceTypeApiKey,
47+
},
4248
},
4349
want: "{user} logged in",
4450
},
4551
{
4652
name: "unsuccessful_login",
4753
alog: database.GetAuditLogsOffsetRow{
48-
Action: database.AuditActionLogin,
49-
StatusCode: 401,
50-
ResourceType: database.ResourceTypeApiKey,
54+
AuditLog: database.AuditLog{
55+
Action: database.AuditActionLogin,
56+
StatusCode: 401,
57+
ResourceType: database.ResourceTypeApiKey,
58+
},
5159
},
5260
want: "{user} unsuccessfully attempted to login",
5361
},
5462
{
5563
name: "gitsshkey",
5664
alog: database.GetAuditLogsOffsetRow{
57-
Action: database.AuditActionDelete,
58-
StatusCode: 200,
59-
ResourceType: database.ResourceTypeGitSshKey,
65+
AuditLog: database.AuditLog{
66+
Action: database.AuditActionDelete,
67+
StatusCode: 200,
68+
ResourceType: database.ResourceTypeGitSshKey,
69+
},
6070
},
6171
want: "{user} deleted the git ssh key",
6272
},

‎coderd/audit_test.go

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import (
44
"context"
55
"encoding/json"
66
"fmt"
7-
"net/http"
87
"strconv"
98
"testing"
109
"time"
@@ -163,19 +162,18 @@ func TestAuditLogs(t *testing.T) {
163162
})
164163
require.NoError(t, err)
165164

166-
// Fetching audit logs without an organization selector should fail
167-
_, err = orgAdmin.AuditLogs(ctx, codersdk.AuditLogsRequest{
165+
// Fetching audit logs without an organization selector should only
166+
// return organization audit logs the org admin is an admin of.
167+
alogs, err := orgAdmin.AuditLogs(ctx, codersdk.AuditLogsRequest{
168168
Pagination: codersdk.Pagination{
169169
Limit: 5,
170170
},
171171
})
172-
var sdkError *codersdk.Error
173-
require.Error(t, err)
174-
require.ErrorAsf(t, err, &sdkError, "error should be of type *codersdk.Error")
175-
require.Equal(t, http.StatusForbidden, sdkError.StatusCode())
172+
require.NoError(t, err)
173+
require.Len(t, alogs.AuditLogs, 1)
176174

177175
// Using the organization selector allows the org admin to fetch audit logs
178-
alogs, err := orgAdmin.AuditLogs(ctx, codersdk.AuditLogsRequest{
176+
alogs, err = orgAdmin.AuditLogs(ctx, codersdk.AuditLogsRequest{
179177
SearchQuery: fmt.Sprintf("organization:%s", owner.OrganizationID.String()),
180178
Pagination: codersdk.Pagination{
181179
Limit: 5,

‎coderd/database/dbauthz/dbauthz.go

Lines changed: 8 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1248,22 +1248,12 @@ func (q *querier) GetApplicationName(ctx context.Context) (string, error) {
12481248
}
12491249

12501250
func (q *querier) GetAuditLogsOffset(ctx context.Context, arg database.GetAuditLogsOffsetParams) ([]database.GetAuditLogsOffsetRow, error) {
1251-
// To optimize the authz checks for audit logs, do not run an authorize
1252-
// check on each individual audit log row. In practice, audit logs are either
1253-
// fetched from a global or an organization scope.
1254-
// Applying a SQL filter would slow down the query for no benefit on how this query is
1255-
// actually used.
1256-
1257-
object := rbac.ResourceAuditLog
1258-
if arg.OrganizationID != uuid.Nil {
1259-
object = object.InOrg(arg.OrganizationID)
1260-
}
1261-
1262-
if err := q.authorizeContext(ctx, policy.ActionRead, object); err != nil {
1263-
return nil, err
1251+
prep, err := prepareSQLFilter(ctx, q.auth, policy.ActionRead, rbac.ResourceAuditLog.Type)
1252+
if err != nil {
1253+
return nil, xerrors.Errorf("(dev error) prepare sql filter: %w", err)
12641254
}
12651255

1266-
return q.db.GetAuditLogsOffset(ctx, arg)
1256+
return q.db.GetAuthorizedAuditLogsOffset(ctx, arg, prep)
12671257
}
12681258

12691259
func (q *querier) GetAuthorizationUserRoles(ctx context.Context, userID uuid.UUID) (database.GetAuthorizationUserRolesRow, error) {
@@ -3852,3 +3842,7 @@ func (q *querier) GetAuthorizedUsers(ctx context.Context, arg database.GetUsersP
38523842
// GetUsers is authenticated.
38533843
return q.GetUsers(ctx, arg)
38543844
}
3845+
3846+
func (q *querier) GetAuthorizedAuditLogsOffset(ctx context.Context, arg database.GetAuditLogsOffsetParams, _ rbac.PreparedAuthorized) ([]database.GetAuditLogsOffsetRow, error) {
3847+
return q.GetAuditLogsOffset(ctx, arg)
3848+
}

‎coderd/database/dbauthz/dbauthz_test.go

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -266,7 +266,14 @@ func (s *MethodTestSuite) TestAuditLogs() {
266266
_ = dbgen.AuditLog(s.T(), db, database.AuditLog{})
267267
check.Args(database.GetAuditLogsOffsetParams{
268268
LimitOpt: 10,
269-
}).Asserts(rbac.ResourceAuditLog, policy.ActionRead)
269+
}).Asserts()
270+
}))
271+
s.Run("GetAuthorizedAuditLogsOffset", s.Subtest(func(db database.Store, check *expects) {
272+
_ = dbgen.AuditLog(s.T(), db, database.AuditLog{})
273+
_ = dbgen.AuditLog(s.T(), db, database.AuditLog{})
274+
check.Args(database.GetAuditLogsOffsetParams{
275+
LimitOpt: 10,
276+
}, emptyPreparedAuthorized{}).Asserts()
270277
}))
271278
}
272279

0 commit comments

Comments
 (0)