Skip to content

Commit e10c545

Browse files
EmyrkEmyrk
committed
PR comments
1 parent f45156c commit e10c545

File tree

2 files changed

+27
-10
lines changed

2 files changed

+27
-10
lines changed

‎coderd/database/querier_test.go

Lines changed: 26 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -789,6 +789,9 @@ func TestAuthorizedAuditLogs(t *testing.T) {
789789
}))
790790
}
791791

792+
// This map is a simple way to insert a given number of organizations
793+
// and audit logs for each organization.
794+
// map[orgID][]AuditLogID
792795
orgAuditLogs := map[uuid.UUID][]uuid.UUID{
793796
uuid.New(): {uuid.New(), uuid.New()},
794797
uuid.New(): {uuid.New(), uuid.New()},
@@ -828,46 +831,55 @@ func TestAuthorizedAuditLogs(t *testing.T) {
828831
t.Run("NoAccess", func(t *testing.T) {
829832
t.Parallel()
830833

831-
siteAuditorCtx := dbauthz.As(ctx, rbac.Subject{
834+
// Given: A user who is a member of 0 organizations
835+
memberCtx := dbauthz.As(ctx, rbac.Subject{
832836
FriendlyName: "member",
833837
ID: uuid.NewString(),
834838
Roles: rbac.Roles{memberRole},
835839
Scope: rbac.ScopeAll,
836840
})
837841

838-
logs, err := db.GetAuditLogsOffset(siteAuditorCtx, database.GetAuditLogsOffsetParams{})
842+
// When: The user queries for audit logs
843+
logs, err := db.GetAuditLogsOffset(memberCtx, database.GetAuditLogsOffsetParams{})
839844
require.NoError(t, err)
845+
// Then: No logs returned
840846
require.Len(t, logs, 0, "no logs should be returned")
841847
})
842848

843849
t.Run("SiteWideAuditor", func(t *testing.T) {
844850
t.Parallel()
845851

852+
// Given: A site wide auditor
846853
siteAuditorCtx := dbauthz.As(ctx, rbac.Subject{
847854
FriendlyName: "owner",
848855
ID: uuid.NewString(),
849856
Roles: rbac.Roles{auditorRole},
850857
Scope: rbac.ScopeAll,
851858
})
852859

860+
// When: the auditor queries for audit logs
853861
logs, err := db.GetAuditLogsOffset(siteAuditorCtx, database.GetAuditLogsOffsetParams{})
854862
require.NoError(t, err)
863+
// Then: All logs are returned
855864
require.ElementsMatch(t, auditOnlyIDs(allLogs), auditOnlyIDs(logs))
856865
})
857866

858867
t.Run("SingleOrgAuditor", func(t *testing.T) {
859868
t.Parallel()
860869

861870
orgID := orgIDs[0]
862-
siteAuditorCtx := dbauthz.As(ctx, rbac.Subject{
871+
// Given: An organization scoped auditor
872+
orgAuditCtx := dbauthz.As(ctx, rbac.Subject{
863873
FriendlyName: "org-auditor",
864874
ID: uuid.NewString(),
865875
Roles: rbac.Roles{orgAuditorRoles(t, orgID)},
866876
Scope: rbac.ScopeAll,
867877
})
868878

869-
logs, err := db.GetAuditLogsOffset(siteAuditorCtx, database.GetAuditLogsOffsetParams{})
879+
// When: The auditor queries for audit logs
880+
logs, err := db.GetAuditLogsOffset(orgAuditCtx, database.GetAuditLogsOffsetParams{})
870881
require.NoError(t, err)
882+
// Then: Only the logs for the organization are returned
871883
require.ElementsMatch(t, orgAuditLogs[orgID], auditOnlyIDs(logs))
872884
})
873885

@@ -876,30 +888,36 @@ func TestAuthorizedAuditLogs(t *testing.T) {
876888

877889
first := orgIDs[0]
878890
second := orgIDs[1]
879-
siteAuditorCtx := dbauthz.As(ctx, rbac.Subject{
891+
// Given: A user who is an auditor for two organizations
892+
multiOrgAuditCtx := dbauthz.As(ctx, rbac.Subject{
880893
FriendlyName: "org-auditor",
881894
ID: uuid.NewString(),
882895
Roles: rbac.Roles{orgAuditorRoles(t, first), orgAuditorRoles(t, second)},
883896
Scope: rbac.ScopeAll,
884897
})
885898

886-
logs, err := db.GetAuditLogsOffset(siteAuditorCtx, database.GetAuditLogsOffsetParams{})
899+
// When: The user queries for audit logs
900+
logs, err := db.GetAuditLogsOffset(multiOrgAuditCtx, database.GetAuditLogsOffsetParams{})
887901
require.NoError(t, err)
902+
// Then: All logs for both organizations are returned
888903
require.ElementsMatch(t, append(orgAuditLogs[first], orgAuditLogs[second]...), auditOnlyIDs(logs))
889904
})
890905

891906
t.Run("ErroneousOrg", func(t *testing.T) {
892907
t.Parallel()
893908

894-
siteAuditorCtx := dbauthz.As(ctx, rbac.Subject{
909+
// Given: A user who is an auditor for an organization that has 0 logs
910+
userCtx := dbauthz.As(ctx, rbac.Subject{
895911
FriendlyName: "org-auditor",
896912
ID: uuid.NewString(),
897913
Roles: rbac.Roles{orgAuditorRoles(t, uuid.New())},
898914
Scope: rbac.ScopeAll,
899915
})
900916

901-
logs, err := db.GetAuditLogsOffset(siteAuditorCtx, database.GetAuditLogsOffsetParams{})
917+
// When: The user queries for audit logs
918+
logs, err := db.GetAuditLogsOffset(userCtx, database.GetAuditLogsOffsetParams{})
902919
require.NoError(t, err)
920+
// Then: No logs are returned
903921
require.Len(t, logs, 0, "no logs should be returned")
904922
})
905923
}

‎coderd/rbac/regosql/configs.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,11 +40,10 @@ func AuditLogConverter() *sqltypes.VariableConverter {
4040
matcher := sqltypes.NewVariableConverter().RegisterMatcher(
4141
resourceIDMatcher(),
4242
sqltypes.StringVarMatcher("COALESCE(audit_logs.organization_id :: text, '')", []string{"input", "object", "org_owner"}),
43-
// Templates have no user owner, only owner by an organization.
43+
// Aduit logs have no user owner, only owner by an organization.
4444
sqltypes.AlwaysFalse(userOwnerMatcher()),
4545
)
4646
matcher.RegisterMatcher(
47-
// No ACLs on the user type
4847
sqltypes.AlwaysFalse(groupACLMatcher(matcher)),
4948
sqltypes.AlwaysFalse(userACLMatcher(matcher)),
5049
)

0 commit comments

Comments
 (0)