WIP: org-scope for frobulators

Signed-off-by: Danny Kopping <danny@coder.com>
coder · johnstcn · Jul 30, 2024 · Jul 30, 2024 · Jul 31, 2024 · Aug 1, 2024
commit cf9df67f5e244f55780160e51c358b6b16a6380c
diff --git a/coderd/coderd.go b/coderd/coderd.go
@@ -909,6 +909,17 @@ func New(options *Options) *API {
 						})
 					})
 				})
+				r.Route("/frobulators", func(r chi.Router) {
+					r.Use(apiKeyMiddleware)
+					r.Route("/{user}", func(r chi.Router) {
+						r.Use(
+							httpmw.ExtractUserParam(options.Database),
+						)
+						r.Get("/", api.listFrobulators)
+						r.Post("/", api.createFrobulator)
+						r.Delete("/{id}", api.deleteFrobulator)
+					})
+				})
 			})
 		})
 		r.Route("/templates", func(r chi.Router) {
@@ -1257,17 +1268,6 @@ func New(options *Options) *API {
 			})
 			r.Get("/dispatch-methods", api.notificationDispatchMethods)
 		})
-		r.Route("/frobulators", func(r chi.Router) {
-			r.Use(apiKeyMiddleware)
-			r.Get("/", api.listAllFrobulators)
-			r.Route("/{user}", func(r chi.Router) {
-				r.Use(
-					httpmw.ExtractUserParam(options.Database),
-				)
-				r.Get("/", api.listUserFrobulators)
-				r.Post("/", api.createFrobulator)
-			})
-		})
 	})
 
 	if options.SwaggerEndpoint {

diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go
@@ -1059,6 +1059,13 @@ func (q *querier) DeleteExternalAuthLink(ctx context.Context, arg database.Delet
 	}, q.db.DeleteExternalAuthLink)(ctx, arg)
 }
 
+func (q *querier) DeleteFrobulator(ctx context.Context, args database.DeleteFrobulatorParams) error {
+	if err := q.authorizeContext(ctx, policy.ActionDelete, rbac.ResourceFrobulator.WithID(args.ID).WithOwner(args.UserID.String()).InOrg(args.OrgID)); err != nil {
+		return err
+	}
+	return q.db.DeleteFrobulator(ctx, args)
+}
+
 func (q *querier) DeleteGitSSHKey(ctx context.Context, userID uuid.UUID) error {
 	return fetchAndExec(q.log, q.auth, policy.ActionUpdatePersonal, q.db.GetGitSSHKey, q.db.DeleteGitSSHKey)(ctx, userID)
 }
@@ -1298,13 +1305,6 @@ func (q *querier) GetActiveWorkspaceBuildsByTemplateID(ctx context.Context, temp
 	return q.db.GetActiveWorkspaceBuildsByTemplateID(ctx, templateID)
 }
 
-func (q *querier) GetAllFrobulators(ctx context.Context) ([]database.Frobulator, error) {
-	if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceFrobulator); err != nil {
-		return nil, err
-	}
-	return q.db.GetAllFrobulators(ctx)
-}
-
 func (q *querier) GetAllTailnetAgents(ctx context.Context) ([]database.TailnetAgent, error) {
 	if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceTailnetCoordinator); err != nil {
 		return []database.TailnetAgent{}, err
@@ -1464,6 +1464,13 @@ func (q *querier) GetFileTemplates(ctx context.Context, fileID uuid.UUID) ([]dat
 	return q.db.GetFileTemplates(ctx, fileID)
 }
 
+func (q *querier) GetFrobulators(ctx context.Context, arg database.GetFrobulatorsParams) ([]database.Frobulator, error) {
+	if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceFrobulator.WithOwner(arg.UserID.String()).InOrg(arg.OrgID)); err != nil {
+		return nil, err
+	}
+	return q.db.GetFrobulators(ctx, arg)
+}
+
 func (q *querier) GetGitSSHKey(ctx context.Context, userID uuid.UUID) (database.GitSSHKey, error) {
 	return fetchWithAction(q.log, q.auth, policy.ActionReadPersonal, q.db.GetGitSSHKey)(ctx, userID)
 }
@@ -2172,19 +2179,6 @@ func (q *querier) GetUserCount(ctx context.Context) (int64, error) {
 	return q.db.GetUserCount(ctx)
 }
 
-func (q *querier) GetUserFrobulators(ctx context.Context, userID uuid.UUID) ([]database.Frobulator, error) {
-	return fetchWithPostFilter(q.auth, policy.ActionRead, q.db.GetUserFrobulators)(ctx, userID)
-	// Alternatively: just check if you can read *a* Frobulator owned by your ID.
-	// This is technically incorrect, as if Frobulators later become org-scoped, this will no longer be correct!
-	// But it's **much, much faster** .
-	/*
-		if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceFrobulator.WithOwner(userID.String())); err != nil {
-			return nil, err
-		}
-		return q.db.GetUserFrobulators(ctx, userID)
-	*/
-}
-
 func (q *querier) GetUserLatencyInsights(ctx context.Context, arg database.GetUserLatencyInsightsParams) ([]database.GetUserLatencyInsightsRow, error) {
 	// Used by insights endpoints. Need to check both for auditors and for regular users with template acl perms.
 	if err := q.authorizeContext(ctx, policy.ActionViewInsights, rbac.ResourceTemplate); err != nil {
@@ -2705,9 +2699,9 @@ func (q *querier) InsertFile(ctx context.Context, arg database.InsertFileParams)
 	return insert(q.log, q.auth, rbac.ResourceFile.WithOwner(arg.CreatedBy.String()), q.db.InsertFile)(ctx, arg)
 }
 
-func (q *querier) InsertFrobulator(ctx context.Context, arg database.InsertFrobulatorParams) error {
-	if err := q.authorizeContext(ctx, policy.ActionCreate, rbac.ResourceFrobulator.WithOwner(arg.UserID.String())); err != nil {
-		return err
+func (q *querier) InsertFrobulator(ctx context.Context, arg database.InsertFrobulatorParams) (database.Frobulator, error) {
+	if err := q.authorizeContext(ctx, policy.ActionCreate, rbac.ResourceFrobulator.WithOwner(arg.UserID.String()).InOrg(arg.OrgID)); err != nil {
+		return database.Frobulator{}, err
 	}
 
 	return q.db.InsertFrobulator(ctx, arg)

diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go
@@ -2751,18 +2751,40 @@ func (s *MethodTestSuite) TestNotifications() {
 }
 
 func (s *MethodTestSuite) TestFrobulators() {
-	s.Run("GetAllFrobulators", s.Subtest(func(db database.Store, check *expects) {
-		check.Args().Asserts(rbac.ResourceFrobulator, policy.ActionRead)
-	}))
-	s.Run("GetUserFrobulators", s.Subtest(func(db database.Store, check *expects) {
+	s.Run("GetFrobulators", s.Subtest(func(db database.Store, check *expects) {
 		user := dbgen.User(s.T(), db, database.User{})
-		check.Args(user.ID).Asserts(rbac.ResourceFrobulator.WithOwner(user.ID.String()), policy.ActionRead)
+		org := dbgen.Organization(s.T(), db, database.Organization{})
+		check.Args(database.GetFrobulatorsParams{
+			UserID: user.ID,
+			OrgID:  org.ID,
+		}).Asserts(
+			rbac.ResourceFrobulator.
+				WithOwner(user.ID.String()).
+				InOrg(org.ID),
+			policy.ActionRead,
+		)
 	}))
 	s.Run("InsertFrobulator", s.Subtest(func(db database.Store, check *expects) {
 		user := dbgen.User(s.T(), db, database.User{})
+		org := dbgen.Organization(s.T(), db, database.Organization{})
 		check.Args(database.InsertFrobulatorParams{
 			UserID: user.ID,
-		}).Asserts(rbac.ResourceFrobulator.WithOwner(user.ID.String()), policy.ActionCreate)
+			OrgID:  org.ID,
+		}).Asserts(rbac.ResourceFrobulator.WithOwner(user.ID.String()).InOrg(org.ID), policy.ActionCreate)
+	}))
+	s.Run("DeleteFrobulator", s.Subtest(func(db database.Store, check *expects) {
+		user := dbgen.User(s.T(), db, database.User{})
+		org := dbgen.Organization(s.T(), db, database.Organization{})
+		frob := dbgen.Frobulator(s.T(), db, database.Frobulator{
+			UserID:      user.ID,
+			OrgID:       org.ID,
+			ModelNumber: "warhead-1",
+		})
+		check.Args(database.DeleteFrobulatorParams{
+			ID:     frob.ID,
+			UserID: frob.UserID,
+			OrgID:  frob.OrgID,
+		}).Asserts(rbac.ResourceFrobulator.WithOwner(frob.UserID.String()).InOrg(frob.OrgID).WithID(frob.ID), policy.ActionDelete)
 	}))
 }
 

diff --git a/coderd/database/dbgen/dbgen.go b/coderd/database/dbgen/dbgen.go
@@ -892,6 +892,16 @@ func CustomRole(t testing.TB, db database.Store, seed database.CustomRole) datab
 	return role
 }
 
+func Frobulator(t testing.TB, db database.Store, orig database.Frobulator) database.Frobulator {
+	frob, err := db.InsertFrobulator(genCtx, database.InsertFrobulatorParams{
+		UserID:      orig.UserID,
+		OrgID:       orig.OrgID,
+		ModelNumber: orig.ModelNumber,
+	})
+	require.NoError(t, err, "insert frobulator")
+	return frob
+}
+
 func must[V any](v V, err error) V {
 	if err != nil {
 		panic(err)

diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go
@@ -1485,6 +1485,20 @@ func (q *FakeQuerier) DeleteExternalAuthLink(_ context.Context, arg database.Del
 	return sql.ErrNoRows
 }
 
+func (q *FakeQuerier) DeleteFrobulator(_ context.Context, args database.DeleteFrobulatorParams) error {
+	q.mutex.Lock()
+	defer q.mutex.Unlock()
+
+	for i, frob := range q.frobulators {
+		if frob.ID == args.ID && frob.UserID == args.UserID && frob.OrgID == args.OrgID {
+			q.frobulators = append(q.frobulators[:i], q.frobulators[i+1:]...)
+			return nil
+		}
+	}
+
+	return sql.ErrNoRows
+}
+
 func (q *FakeQuerier) DeleteGitSSHKey(_ context.Context, userID uuid.UUID) error {
 	q.mutex.Lock()
 	defer q.mutex.Unlock()
@@ -2130,13 +2144,6 @@ func (q *FakeQuerier) GetActiveWorkspaceBuildsByTemplateID(ctx context.Context,
 	return filteredBuilds, nil
 }
 
-func (q *FakeQuerier) GetAllFrobulators(_ context.Context) ([]database.Frobulator, error) {
-	q.mutex.RLock()
-	defer q.mutex.RUnlock()
-
-	return q.frobulators, nil
-}
-
 func (*FakeQuerier) GetAllTailnetAgents(_ context.Context) ([]database.TailnetAgent, error) {
 	return nil, ErrUnimplemented
 }
@@ -2508,6 +2515,27 @@ func (q *FakeQuerier) GetFileTemplates(_ context.Context, id uuid.UUID) ([]datab
 	return rows, nil
 }
 
+func (q *FakeQuerier) GetFrobulators(_ context.Context, arg database.GetFrobulatorsParams) ([]database.Frobulator, error) {
+	err := validateDatabaseType(arg)
+	if err != nil {
+		return nil, err
+	}
+
+	q.mutex.RLock()
+	defer q.mutex.RUnlock()
+
+	out := make([]database.Frobulator, 0, len(q.frobulators))
+	for _, frob := range q.frobulators {
+		if frob.UserID != arg.UserID || frob.OrgID != arg.OrgID {
+			continue
+		}
+
+		out = append(out, frob)
+	}
+
+	return out, nil
+}
+
 func (q *FakeQuerier) GetGitSSHKey(_ context.Context, userID uuid.UUID) (database.GitSSHKey, error) {
 	q.mutex.RLock()
 	defer q.mutex.RUnlock()
@@ -4865,22 +4893,6 @@ func (q *FakeQuerier) GetUserCount(_ context.Context) (int64, error) {
 	return existing, nil
 }
 
-func (q *FakeQuerier) GetUserFrobulators(_ context.Context, userID uuid.UUID) ([]database.Frobulator, error) {
-	q.mutex.RLock()
-	defer q.mutex.RUnlock()
-
-	out := make([]database.Frobulator, 0, len(q.frobulators))
-	for _, frob := range q.frobulators {
-		if frob.UserID != userID {
-			continue
-		}
-
-		out = append(out, frob)
-	}
-
-	return out, nil
-}
-
 func (q *FakeQuerier) GetUserLatencyInsights(_ context.Context, arg database.GetUserLatencyInsightsParams) ([]database.GetUserLatencyInsightsRow, error) {
 	err := validateDatabaseType(arg)
 	if err != nil {
@@ -6307,23 +6319,25 @@ func (q *FakeQuerier) InsertFile(_ context.Context, arg database.InsertFileParam
 	return file, nil
 }
 
-func (q *FakeQuerier) InsertFrobulator(_ context.Context, arg database.InsertFrobulatorParams) error {
+func (q *FakeQuerier) InsertFrobulator(_ context.Context, arg database.InsertFrobulatorParams) (database.Frobulator, error) {
 	err := validateDatabaseType(arg)
 	if err != nil {
-		return err
+		return database.Frobulator{}, err
 	}
 
 	q.mutex.Lock()
 	defer q.mutex.Unlock()
 
 	// nolint:gosimple // This is fine as it is.
-	q.frobulators = append(q.frobulators, database.Frobulator{
-		ID:          arg.ID,
+	frob := database.Frobulator{
+		ID:          uuid.New(),
 		UserID:      arg.UserID,
+		OrgID:       arg.OrgID,
 		ModelNumber: arg.ModelNumber,
-	})
+	}
+	q.frobulators = append(q.frobulators, frob)
 
-	return nil
+	return frob, nil
 }
 
 func (q *FakeQuerier) InsertGitSSHKey(_ context.Context, arg database.InsertGitSSHKeyParams) (database.GitSSHKey, error) {

diff --git a/coderd/database/dbmetrics/dbmetrics.go b/coderd/database/dbmetrics/dbmetrics.go