Add unit testS

coder · Emyrk · Aug 1, 2024 · Jul 31, 2024 · Jul 31, 2024 · Jul 31, 2024
commit dee0ca851cb4f167288e8079d348765ba8c5d1c8
diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go
@@ -1248,27 +1248,11 @@ func (q *querier) GetApplicationName(ctx context.Context) (string, error) {
 }
 
 func (q *querier) GetAuditLogsOffset(ctx context.Context, arg database.GetAuditLogsOffsetParams) ([]database.GetAuditLogsOffsetRow, error) {
-	//// 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.
-	//
-	//object := rbac.ResourceAuditLog
-	//if arg.OrganizationID != uuid.Nil {
-	//	object = object.InOrg(arg.OrganizationID)
-	//}
-	//
-	//if err := q.authorizeContext(ctx, policy.ActionRead, object); err != nil {
-	//	return nil, err
-	//}
-
 	prep, err := prepareSQLFilter(ctx, q.auth, policy.ActionRead, rbac.ResourceAuditLog.Type)
 	if err != nil {
 		return nil, xerrors.Errorf("(dev error) prepare sql filter: %w", err)
 	}
 
-
 	return q.db.GetAuthorizedAuditLogsOffset(ctx, arg, prep)
 }
 

diff --git a/coderd/database/dbgen/dbgen.go b/coderd/database/dbgen/dbgen.go
@@ -40,10 +40,11 @@ var genCtx = dbauthz.As(context.Background(), rbac.Subject{
 
 func AuditLog(t testing.TB, db database.Store, seed database.AuditLog) database.AuditLog {
 	log, err := db.InsertAuditLog(genCtx, database.InsertAuditLogParams{
-		ID:             takeFirst(seed.ID, uuid.New()),
-		Time:           takeFirst(seed.Time, dbtime.Now()),
-		UserID:         takeFirst(seed.UserID, uuid.New()),
-		OrganizationID: takeFirst(seed.OrganizationID, uuid.New()),
+		ID:     takeFirst(seed.ID, uuid.New()),
+		Time:   takeFirst(seed.Time, dbtime.Now()),
+		UserID: takeFirst(seed.UserID, uuid.New()),
+		// Default to the nil uuid. So by default audit logs are not org scoped.
+		OrganizationID: takeFirst(seed.OrganizationID),
 		Ip: pqtype.Inet{
 			IPNet: takeFirstIP(seed.Ip.IPNet, net.IPNet{}),
 			Valid: takeFirst(seed.Ip.Valid, false),

diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go
@@ -12,13 +12,19 @@
 	"time"
 
 	"github.com/google/uuid"
+	"github.com/prometheus/client_golang/prometheus"
 	"github.com/stretchr/testify/require"
 
+	"cdr.dev/slog/sloggers/slogtest"
+	"github.com/coder/coder/v2/coderd/coderdtest"
 	"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/database/dbgen"
+	"github.com/coder/coder/v2/coderd/database/dbtestutil"
 	"github.com/coder/coder/v2/coderd/database/dbtime"
 	"github.com/coder/coder/v2/coderd/database/migrations"
+	"github.com/coder/coder/v2/coderd/rbac"
 	"github.com/coder/coder/v2/testutil"
 )
 
@@ -767,6 +773,143 @@
 	}
 }
 
+func TestAuthorizedAuditLogs(t *testing.T) {
+	t.Parallel()
+
+	var allLogs []database.AuditLog
+	db, _ := dbtestutil.NewDB(t)
+	authz := rbac.NewAuthorizer(prometheus.NewRegistry())
+	db = dbauthz.New(db, authz, slogtest.Make(t, &slogtest.Options{}), coderdtest.AccessControlStorePointer())
+
+	siteWideIDs := []uuid.UUID{uuid.New(), uuid.New()}
+	for _, id := range siteWideIDs {
+		allLogs = append(allLogs, dbgen.AuditLog(t, db, database.AuditLog{
+			ID:             id,
+			OrganizationID: uuid.Nil,
+		}))
+
+	}
+
+	orgAuditLogs := map[uuid.UUID][]uuid.UUID{
+		uuid.New(): {uuid.New(), uuid.New()},
+		uuid.New(): {uuid.New(), uuid.New()},
+	}
+	orgIDs := make([]uuid.UUID, 0, len(orgAuditLogs))
+	for orgID := range orgAuditLogs {
+		orgIDs = append(orgIDs, orgID)
+	}
+	for orgID, ids := range orgAuditLogs {
+		dbgen.Organization(t, db, database.Organization{
+			ID: orgID,
+		})
+		for _, id := range ids {
+			allLogs = append(allLogs, dbgen.AuditLog(t, db, database.AuditLog{
+				ID:             id,
+				OrganizationID: orgID,
+			}))
+		}
+	}
+
+	// Now fetch all the logs
+	ctx := testutil.Context(t, testutil.WaitLong)
+	auditorRole, err := rbac.RoleByName(rbac.RoleAuditor())
+	require.NoError(t, err)
+
+	memberRole, err := rbac.RoleByName(rbac.RoleMember())
+	require.NoError(t, err)
+
+	orgAuditorRoles := func(t *testing.T, orgID uuid.UUID) rbac.Role {
+		t.Helper()
+
+		role, err := rbac.RoleByName(rbac.ScopedRoleOrgAuditor(orgID))
+		require.NoError(t, err)
+		return role
+	}
+
+	t.Run("NoAccess", func(t *testing.T) {
+		siteAuditorCtx := dbauthz.As(ctx, rbac.Subject{
+			FriendlyName: "member",
+			ID:           uuid.NewString(),
+			Roles:        rbac.Roles{memberRole},
+			Scope:        rbac.ScopeAll,
+		})
+
+		logs, err := db.GetAuditLogsOffset(siteAuditorCtx, database.GetAuditLogsOffsetParams{})
+		require.NoError(t, err)
+		require.Len(t, logs, 0, "no logs should be returned")
+	})
+
+	t.Run("SiteWideAuditor", func(t *testing.T) {
+		siteAuditorCtx := dbauthz.As(ctx, rbac.Subject{
+			FriendlyName: "owner",
+			ID:           uuid.NewString(),
+			Roles:        rbac.Roles{auditorRole},
+			Scope:        rbac.ScopeAll,
+		})
+
+		logs, err := db.GetAuditLogsOffset(siteAuditorCtx, database.GetAuditLogsOffsetParams{})
+		require.NoError(t, err)
+		require.ElementsMatch(t, auditOnlyIDs(allLogs), auditOnlyIDs(logs))
+	})
+
+	t.Run("SingleOrgAuditor", func(t *testing.T) {
+		orgID := orgIDs[0]
+		siteAuditorCtx := dbauthz.As(ctx, rbac.Subject{
+			FriendlyName: "org-auditor",
+			ID:           uuid.NewString(),
+			Roles:        rbac.Roles{orgAuditorRoles(t, orgID)},
+			Scope:        rbac.ScopeAll,
+		})
+
+		logs, err := db.GetAuditLogsOffset(siteAuditorCtx, database.GetAuditLogsOffsetParams{})
+		require.NoError(t, err)
+		require.ElementsMatch(t, orgAuditLogs[orgID], auditOnlyIDs(logs))
+	})
+
+	t.Run("TwoOrgAuditors", func(t *testing.T) {
+		first := orgIDs[0]
+		second := orgIDs[1]
+		siteAuditorCtx := dbauthz.As(ctx, rbac.Subject{
+			FriendlyName: "org-auditor",
+			ID:           uuid.NewString(),
+			Roles:        rbac.Roles{orgAuditorRoles(t, first), orgAuditorRoles(t, second)},
+			Scope:        rbac.ScopeAll,
+		})
+
+		logs, err := db.GetAuditLogsOffset(siteAuditorCtx, database.GetAuditLogsOffsetParams{})
+		require.NoError(t, err)
+		require.ElementsMatch(t, append(orgAuditLogs[first], orgAuditLogs[second]...), auditOnlyIDs(logs))
+	})
+
+	t.Run("ErroneousOrg", func(t *testing.T) {
+		siteAuditorCtx := dbauthz.As(ctx, rbac.Subject{
+			FriendlyName: "org-auditor",
+			ID:           uuid.NewString(),
+			Roles:        rbac.Roles{orgAuditorRoles(t, uuid.New())},
+			Scope:        rbac.ScopeAll,
+		})
+
+		logs, err := db.GetAuditLogsOffset(siteAuditorCtx, database.GetAuditLogsOffsetParams{})
+		require.NoError(t, err)
+		require.Len(t, logs, 0, "no logs should be returned")
+	})
+}
+
+func auditOnlyIDs[T database.AuditLog | database.GetAuditLogsOffsetRow](logs []T) []uuid.UUID {
+	ids := make([]uuid.UUID, 0, len(logs))
+	for _, log := range logs {
+		switch log := any(log).(type) {
+		case database.AuditLog:
+			ids = append(ids, log.ID)
+		case database.GetAuditLogsOffsetRow:
+			ids = append(ids, log.AuditLog.ID)
+		default:
+			panic("unreachable")
+		}
+	}
+	return ids
+}
+
 type tvArgs struct {
 	Status database.ProvisionerJobStatus
 	// CreateWorkspace is true if we should create a workspace for the template version

@@ -39,7 +39,7 @@ func TemplateConverter() *sqltypes.VariableConverter {
 func AuditLogConverter() *sqltypes.VariableConverter {
 	matcher := sqltypes.NewVariableConverter().RegisterMatcher(
 		resourceIDMatcher(),
-		organizationOwnerMatcher(),
+		sqltypes.StringVarMatcher("COALESCE(audit_logs.organization_id :: text, '')", []string{"input", "object", "org_owner"}),
 		// Templates have no user owner, only owner by an organization.
 		sqltypes.AlwaysFalse(userOwnerMatcher()),
 	)