check role membership

sherlock/db/migrations/000062_environment_v3.up.sql

@@ -9,7 +9,8 @@ alter table environments
 
 alter table environments
     add constraint lifecycle_valid
-        check ((lifecycle = 'template' and template_environment_id is null) or
+        check ((lifecycle = 'template' and
+                template_environment_id is null) or
                (lifecycle = 'dynamic' and
                 template_environment_id is not null and
                 base is not null and base != '' and

sherlock/db/migrations/000092_required_role.down.sql

@@ -0,0 +1,37 @@
+alter table clusters
+    alter column requires_suitability set not null;
+
+alter table environments
+    drop constraint if exists fk_environments_required_role;
+
+alter table environments
+    drop column if exists required_role_id;
+
+alter table environments
+    add constraint lifecycle_valid_temp
+        check ((lifecycle = 'template' and
+                template_environment_id is null) or
+               (lifecycle = 'dynamic' and
+                template_environment_id is not null and
+                base is not null and base != '' and
+                default_cluster_id is not null and
+                requires_suitability is not null) or
+               (lifecycle = 'static' and
+                base is not null and base != '' and
+                default_cluster_id is not null and
+                requires_suitability is not null));
+
+alter table environments
+    validate constraint lifecycle_valid_temp;
+
+alter table environments
+    drop constraint lifecycle_valid;
+
+alter table environments
+    rename constraint lifecycle_valid_temp to lifecycle_valid;
+
+alter table clusters
+    drop constraint if exists fk_clusters_required_role;
+
+alter table clusters
+    drop column if exists required_role_id;

sherlock/db/migrations/000092_required_role.up.sql

@@ -0,0 +1,43 @@
+alter table clusters
+    add column required_role_id bigint;
+
+alter table clusters
+    add constraint fk_clusters_required_role
+        foreign key (required_role_id) references roles;
+
+-- Now formally allow nulls in requires_suitability column to match with the semantics of required_role_id
+alter table clusters
+    alter column requires_suitability drop not null;
+
+alter table environments
+    add column required_role_id bigint;
+
+alter table environments
+    add constraint fk_environments_required_role
+        foreign key (required_role_id) references roles;
+
+-- The not null requirement on requires_suitability here is lifecycle-dependent, see 000062_environment_v3.up.sql,
+-- so we need to drop the not null constraint here to match the semantics of required_role_id.
+-- We do this by adding a new constraint as not valid, so that it commits instantly, and then validate it.
+-- This requires much, much less locking than a normal add constraint.
+-- https://www.postgresql.org/docs/current/sql-altertable.html#SQL-ALTERTABLE-NOTES
+alter table environments
+    add constraint lifecycle_valid_temp
+        check ((lifecycle = 'template' and
+                template_environment_id is null) or
+               (lifecycle = 'dynamic' and
+                template_environment_id is not null and
+                base is not null and base != '' and
+                default_cluster_id is not null) or
+               (lifecycle = 'static' and
+                base is not null and base != '' and
+                default_cluster_id is not null)) not valid;
+
+alter table environments
+    validate constraint lifecycle_valid_temp;
+
+alter table environments
+    drop constraint lifecycle_valid;
+
+alter table environments
+    rename constraint lifecycle_valid_temp to lifecycle_valid;

sherlock/internal/api/sherlock/clusters_v3.go

@@ -1,13 +1,16 @@
 package sherlock
 
 import (
+	"fmt"
 	"github.com/broadinstitute/sherlock/go-shared/pkg/utils"
 	"github.com/broadinstitute/sherlock/sherlock/internal/models"
+	"gorm.io/gorm"
 )
 
 type ClusterV3 struct {
 	CommonFields
-	CiIdentifier *CiIdentifierV3 `json:"ciIdentifier,omitempty" form:"-"`
+	CiIdentifier     *CiIdentifierV3 `json:"ciIdentifier,omitempty" form:"-"`
+	RequiredRoleInfo *RoleV3         `json:"requiredRoleInfo,omitempty" form:"-"`
 	ClusterV3Create
 }
 
@@ -23,12 +26,13 @@ type ClusterV3Create struct {
 type ClusterV3Edit struct {
 	Base                *string `json:"base"  form:"base"`      // Required when creating
 	Address             *string `json:"address" form:"address"` // Required when creating
-	RequiresSuitability *bool   `json:"requiresSuitability" form:"requiresSuitability" default:"false"`
+	RequiresSuitability *bool   `json:"requiresSuitability" form:"requiresSuitability"`
+	RequiredRole        *string `json:"requiredRole" form:"requiredRole"` // If present, requires membership in the given role for mutations
 	HelmfileRef         *string `json:"helmfileRef" form:"helmfileRef" default:"HEAD"`
 }
 
-func (c ClusterV3) toModel() models.Cluster {
-	return models.Cluster{
+func (c ClusterV3) toModel(db *gorm.DB) (models.Cluster, error) {
+	ret := models.Cluster{
 		Model:               c.toGormModel(),
 		Name:                c.Name,
 		Provider:            c.Provider,
@@ -40,24 +44,34 @@ func (c ClusterV3) toModel() models.Cluster {
 		RequiresSuitability: c.RequiresSuitability,
 		HelmfileRef:         c.HelmfileRef,
 	}
+	if c.RequiredRole != nil && *c.RequiredRole != "" {
+		requiredRoleModel, err := roleModelFromSelector(*c.RequiredRole)
+		if err != nil {
+			return models.Cluster{}, err
+		}
+		var requiredRole models.Role
+		if err = db.Where(requiredRoleModel).Select("id").First(&requiredRole).Error; err != nil {
+			return models.Cluster{}, fmt.Errorf("required role '%s' not found: %w", *c.RequiredRole, err)
+		} else {
+			ret.RequiredRoleID = &requiredRole.ID
+		}
+	}
+	return ret, nil
 }
 
-func (c ClusterV3Create) toModel() models.Cluster {
-	return ClusterV3{ClusterV3Create: c}.toModel()
+func (c ClusterV3Create) toModel(db *gorm.DB) (models.Cluster, error) {
+	return ClusterV3{ClusterV3Create: c}.toModel(db)
 }
 
-func (c ClusterV3Edit) toModel() models.Cluster {
-	return ClusterV3Create{ClusterV3Edit: c}.toModel()
+func (c ClusterV3Edit) toModel(db *gorm.DB) (models.Cluster, error) {
+	return ClusterV3Create{ClusterV3Edit: c}.toModel(db)
 }
 
 func clusterFromModel(model models.Cluster) ClusterV3 {
-	var ciIdentifier *CiIdentifierV3
-	if model.CiIdentifier != nil {
-		ciIdentifier = utils.PointerTo(ciIdentifierFromModel(*model.CiIdentifier))
-	}
-	return ClusterV3{
-		CommonFields: commonFieldsFromGormModel(model.Model),
-		CiIdentifier: ciIdentifier,
+	ret := ClusterV3{
+		CommonFields:     commonFieldsFromGormModel(model.Model),
+		CiIdentifier:     utils.NilOrCall(ciIdentifierFromModel, model.CiIdentifier),
+		RequiredRoleInfo: utils.NilOrCall(roleFromModel, model.RequiredRole),
 		ClusterV3Create: ClusterV3Create{
 			Name:              model.Name,
 			Provider:          model.Provider,
@@ -72,4 +86,10 @@ func clusterFromModel(model models.Cluster) ClusterV3 {
 			},
 		},
 	}
+	if model.RequiredRole != nil && model.RequiredRole.Name != nil {
+		ret.RequiredRole = model.RequiredRole.Name
+	} else if model.RequiredRoleID != nil {
+		ret.RequiredRole = utils.PointerTo(utils.UintToString(*model.RequiredRoleID))
+	}
+	return ret
 }

sherlock/internal/api/sherlock/clusters_v3_create.go

@@ -39,7 +39,11 @@ func clustersV3Create(ctx *gin.Context) {
 		return
 	}
 
-	toCreate := body.toModel()
+	toCreate, err := body.toModel(db)
+	if err != nil {
+		errors.AbortRequest(ctx, err)
+		return
+	}
 	if err = db.Create(&toCreate).Error; err != nil {
 		errors.AbortRequest(ctx, err)
 		return

sherlock/internal/api/sherlock/clusters_v3_create_test.go

@@ -44,9 +44,6 @@ func (s *handlerSuite) TestClustersV3Create_defaults() {
 	s.Equal(http.StatusCreated, code)
 	s.Equal("cluster-name", got.Name)
 	s.Equal("us-central1-a", got.Location)
-	if s.NotNil(got.RequiresSuitability) {
-		s.False(*got.RequiresSuitability)
-	}
 	if s.NotNil(got.HelmfileRef) {
 		s.Equal("HEAD", *got.HelmfileRef)
 	}

sherlock/internal/api/sherlock/clusters_v3_edit.go

@@ -38,7 +38,11 @@ func clustersV3Edit(ctx *gin.Context) {
 		return
 	}
 
-	edits := body.toModel()
+	edits, err := body.toModel(db)
+	if err != nil {
+		errors.AbortRequest(ctx, err)
+		return
+	}
 
 	var toEdit models.Cluster
 	if err = db.Preload(clause.Associations).Where(&query).First(&toEdit).Error; err != nil {

sherlock/internal/api/sherlock/clusters_v3_list.go

@@ -33,7 +33,11 @@ func clustersV3List(ctx *gin.Context) {
 		errors.AbortRequest(ctx, err)
 		return
 	}
-	modelFilter := filter.toModel()
+	modelFilter, err := filter.toModel(db)
+	if err != nil {
+		errors.AbortRequest(ctx, err)
+		return
+	}
 
 	limit, err := utils.ParseInt(ctx.DefaultQuery("limit", "0"))
 	if err != nil {

sherlock/internal/api/sherlock/environments_v3.go

@@ -20,6 +20,7 @@ type EnvironmentV3 struct {
 	DefaultClusterInfo       *ClusterV3              `json:"defaultClusterInfo,omitempty" form:"-"`
 	PagerdutyIntegrationInfo *PagerdutyIntegrationV3 `json:"pagerdutyIntegrationInfo,omitempty" form:"-"`
 	OwnerInfo                *UserV3                 `json:"ownerInfo,omitempty" form:"-"`
+	RequiredRoleInfo         *RoleV3                 `json:"requiredRoleInfo,omitempty" form:"-"`
 	EnvironmentV3Create
 }
 
@@ -38,7 +39,8 @@ type EnvironmentV3Create struct {
 type EnvironmentV3Edit struct {
 	DefaultCluster              *string    `json:"defaultCluster" form:"defaultCluster"`
 	Owner                       *string    `json:"owner" form:"owner"` // When creating, will default to you
-	RequiresSuitability         *bool      `json:"requiresSuitability" form:"requiresSuitability" default:"false"`
+	RequiresSuitability         *bool      `json:"requiresSuitability" form:"requiresSuitability"`
+	RequiredRole                *string    `json:"requiredRole" form:"requiredRole"` // If present, requires membership in the given role for mutations
 	BaseDomain                  *string    `json:"baseDomain" form:"baseDomain" default:"bee.envs-terra.bio"`
 	NamePrefixesDomain          *bool      `json:"namePrefixesDomain" form:"namePrefixesDomain" default:"true"`
 	HelmfileRef                 *string    `json:"helmfileRef" form:"helmfileRef" default:"HEAD"`
@@ -140,6 +142,18 @@ func (e EnvironmentV3) toModel(db *gorm.DB) (models.Environment, error) {
 			ret.OwnerID = &owner.ID
 		}
 	}
+	if e.RequiredRole != nil && *e.RequiredRole != "" {
+		requiredRoleModel, err := roleModelFromSelector(*e.RequiredRole)
+		if err != nil {
+			return models.Environment{}, err
+		}
+		var requiredRole models.Role
+		if err = db.Where(&requiredRoleModel).Select("id").First(&requiredRole).Error; err != nil {
+			return models.Environment{}, fmt.Errorf("required role '%s' not found: %w", *e.RequiredRole, err)
+		} else {
+			ret.RequiredRoleID = &requiredRole.ID
+		}
+	}
 	return ret, nil
 }
 
@@ -161,6 +175,7 @@ func environmentFromModel(model models.Environment) EnvironmentV3 {
 		DefaultClusterInfo:       utils.NilOrCall(clusterFromModel, model.DefaultCluster),
 		PagerdutyIntegrationInfo: utils.NilOrCall(pagerdutyIntegrationFromModel, model.PagerdutyIntegration),
 		OwnerInfo:                utils.NilOrCall(userFromModel, model.Owner),
+		RequiredRoleInfo:         utils.NilOrCall(roleFromModel, model.RequiredRole),
 		EnvironmentV3Create: EnvironmentV3Create{
 			Base:                      model.Base,
 			AutoPopulateChartReleases: model.AutoPopulateChartReleases,
@@ -210,5 +225,10 @@ func environmentFromModel(model models.Environment) EnvironmentV3 {
 		log.Error().Uint("environment", model.ID).Err(err).Msg("failed to parse offline schedule end time")
 		ret.OfflineScheduleEndTime = nil
 	}
+	if model.RequiredRole != nil && model.RequiredRole.Name != nil {
+		ret.RequiredRole = model.RequiredRole.Name
+	} else if model.RequiredRoleID != nil {
+		ret.RequiredRole = utils.PointerTo(utils.UintToString(*model.RequiredRoleID))
+	}
 	return ret
 }

sherlock/internal/api/sherlock/environments_v3_test.go

@@ -158,6 +158,7 @@ func (s *handlerSuite) TestEnvironmentV3_toModel() {
 						DefaultCluster:              utils.PointerTo(defaultCluster.Name),
 						Owner:                       utils.PointerTo(owner.Email),
 						RequiresSuitability:         utils.PointerTo(true),
+						RequiredRole:                s.TestData.Role_TerraSuitableEngineer().Name,
 						BaseDomain:                  utils.PointerTo("base-domain"),
 						NamePrefixesDomain:          utils.PointerTo(true),
 						HelmfileRef:                 utils.PointerTo("HEAD"),
@@ -193,6 +194,7 @@ func (s *handlerSuite) TestEnvironmentV3_toModel() {
 				DefaultClusterID:          &defaultCluster.ID,
 				OwnerID:                   &owner.ID,
 				RequiresSuitability:       utils.PointerTo(true),
+				RequiredRoleID:            utils.PointerTo(s.TestData.Role_TerraSuitableEngineer().ID),
 				BaseDomain:                utils.PointerTo("base-domain"),
 				NamePrefixesDomain:        utils.PointerTo(true),
 				HelmfileRef:               utils.PointerTo("HEAD"),
@@ -345,6 +347,8 @@ func Test_environmentFromModel(t *testing.T) {
 				Owner:                     &models.User{Model: gorm.Model{ID: 5}, Email: "[email protected]"},
 				OwnerID:                   utils.PointerTo[uint](5),
 				RequiresSuitability:       utils.PointerTo(true),
+				RequiredRole:              &models.Role{Model: gorm.Model{ID: 999}, RoleFields: models.RoleFields{Name: utils.PointerTo("role-name")}},
+				RequiredRoleID:            utils.PointerTo[uint](999),
 				BaseDomain:                utils.PointerTo("base-domain"),
 				NamePrefixesDomain:        utils.PointerTo(true),
 				HelmfileRef:               utils.PointerTo("HEAD"),
@@ -376,6 +380,7 @@ func Test_environmentFromModel(t *testing.T) {
 				PagerdutyIntegrationInfo: &PagerdutyIntegrationV3{CommonFields: CommonFields{ID: 6}, PagerdutyID: "blah"},
 				OwnerInfo: &UserV3{CommonFields: CommonFields{ID: 5}, Email: "[email protected]", Suitable: utils.PointerTo(false),
 					SuitabilityDescription: utils.PointerTo("no matching suitability record found or loaded; assuming unsuitable")},
+				RequiredRoleInfo: &RoleV3{CommonFields: CommonFields{ID: 999}, RoleV3Edit: RoleV3Edit{Name: utils.PointerTo("role-name")}},
 				EnvironmentV3Create: EnvironmentV3Create{
 					Base:                      "base",
 					AutoPopulateChartReleases: utils.PointerTo(true),
@@ -389,6 +394,7 @@ func Test_environmentFromModel(t *testing.T) {
 						DefaultCluster:              utils.PointerTo("name-4"),
 						Owner:                       utils.PointerTo("[email protected]"),
 						RequiresSuitability:         utils.PointerTo(true),
+						RequiredRole:                utils.PointerTo("role-name"),
 						BaseDomain:                  utils.PointerTo("base-domain"),
 						NamePrefixesDomain:          utils.PointerTo(true),
 						HelmfileRef:                 utils.PointerTo("HEAD"),
@@ -420,6 +426,19 @@ func Test_environmentFromModel(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "role ID case",
+			args: args{model: models.Environment{
+				RequiredRoleID: utils.PointerTo[uint](999),
+			}},
+			want: EnvironmentV3{
+				EnvironmentV3Create: EnvironmentV3Create{
+					EnvironmentV3Edit: EnvironmentV3Edit{
+						RequiredRole: utils.PointerTo("999"),
+					},
+				},
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

sherlock/internal/models/cluster.go

@@ -18,6 +18,8 @@ type Cluster struct {
 	Base                *string
 	Address             *string
 	RequiresSuitability *bool
+	RequiredRole        *Role
+	RequiredRoleID      *uint
 	HelmfileRef         *string
 }
 
@@ -34,6 +36,9 @@ func (c *Cluster) errorIfForbidden(tx *gorm.DB) error {
 	if err != nil {
 		return err
 	}
+	if err = user.ErrIfNotActiveInRole(tx, c.RequiredRoleID); err != nil {
+		return err
+	}
 	if c.RequiresSuitability == nil || *c.RequiresSuitability {
 		if err = user.ErrIfNotSuitable(); err != nil {
 			return fmt.Errorf("(%s) suitability required: %w", errors.Forbidden, err)

sherlock/internal/models/cluster_test.go

@@ -140,21 +140,6 @@ func (s *modelSuite) TestClusterLocationValidationSqlMissing() {
 	s.ErrorContains(err, "location")
 }
 
-func (s *modelSuite) TestClusterRequiresSuitabilityValidationSqlMissing() {
-	s.SetSuitableTestUserForDB() // To actually prompt the SQL error we have to be suitable to get past the permissions
-	err := s.DB.Create(&Cluster{
-		Name:              "some-name",
-		Provider:          "google",
-		GoogleProject:     "some-project",
-		AzureSubscription: "some-subscription",
-		Location:          "some-location",
-		Base:              utils.PointerTo("some-base"),
-		Address:           utils.PointerTo("0.0.0.0"),
-		HelmfileRef:       utils.PointerTo("HEAD"),
-	}).Error
-	s.ErrorContains(err, "requires_suitability")
-}
-
 func (s *modelSuite) TestClusterHelmfileRefValidationSqlMissing() {
 	s.SetNonSuitableTestUserForDB()
 	err := s.DB.Create(&Cluster{