Fix label value define permissions (#2596)

* Simplify if test and keep check- prefix
* Fix label value define permissions

app/api/controller/repo/label_value_define.go

@@ -18,7 +18,6 @@ import (
 	"context"
 	"fmt"
 
-	apiauth "github.com/harness/gitness/app/api/auth"
 	"github.com/harness/gitness/app/auth"
 	"github.com/harness/gitness/types"
 	"github.com/harness/gitness/types/enum"
@@ -32,7 +31,7 @@ func (c *Controller) DefineLabelValue(
 	key string,
 	in *types.DefineValueInput,
 ) (*types.LabelValue, error) {
-	repo, err := GetRepo(ctx, c.repoStore, repoRef, []enum.RepoState{enum.RepoStateActive})
+	repo, err := c.getRepoCheckAccess(ctx, session, repoRef, enum.PermissionRepoEdit)
 	if err != nil {
 		return nil, fmt.Errorf("failed to acquire access to repo: %w", err)
 	}
@@ -46,16 +45,6 @@ func (c *Controller) DefineLabelValue(
 		return nil, fmt.Errorf("failed to find repo label: %w", err)
 	}
 
-	permission := enum.PermissionRepoEdit
-	if label.Type == enum.LabelTypeDynamic {
-		permission = enum.PermissionRepoReview
-	}
-
-	if err = apiauth.CheckRepo(
-		ctx, c.authorizer, session, repo, permission); err != nil {
-		return nil, fmt.Errorf("access check failed: %w", err)
-	}
-
 	value, err := c.labelSvc.DefineValue(ctx, session.Principal.ID, label.ID, in)
 	if err != nil {
 		return nil, fmt.Errorf("failed to create repo label value: %w", err)

app/api/controller/space/label_value_define.go

@@ -31,7 +31,6 @@ func (c *Controller) DefineLabelValue(
 	key string,
 	in *types.DefineValueInput,
 ) (*types.LabelValue, error) {
-	// TODO: permission check should be based on static vs dynamic label
 	space, err := c.getSpaceCheckAuth(ctx, session, spaceRef, enum.PermissionRepoEdit)
 	if err != nil {
 		return nil, fmt.Errorf("failed to acquire access to space: %w", err)

app/services/label/label.go

@@ -96,7 +96,7 @@ func (s *Service) Save(
 
 	err = s.tx.WithTx(ctx, func(ctx context.Context) error {
 		label, err = s.labelStore.FindByID(ctx, in.Label.ID)
-		if err != nil {
+		if err != nil { //nolint:nestif
 			if !errors.Is(err, store.ErrResourceNotFound) {
 				return err
 			}
@@ -105,6 +105,10 @@ func (s *Service) Save(
 				return err
 			}
 		} else {
+			if err := checkLabelInScope(spaceID, repoID, label); err != nil {
+				return err
+			}
+
 			label, err = s.update(ctx, principalID, label, &types.UpdateLabelInput{
 				Key:         &in.Label.Key,
 				Type:        &in.Label.Type,
@@ -338,3 +342,15 @@ func applyChanges(principalID int64, label *types.Label, in *types.UpdateLabelIn
 
 	return label, hasChanges
 }
+
+func checkLabelInScope(
+	spaceID, repoID *int64,
+	label *types.Label,
+) error {
+	if (repoID != nil && (label.RepoID == nil || *label.RepoID != *repoID)) ||
+		(spaceID != nil && (label.SpaceID == nil || *label.SpaceID != *spaceID)) {
+		return errors.InvalidArgument("label is not defined in requested scope")
+	}
+
+	return nil
+}

app/services/label/label_pullreq.go

@@ -52,13 +52,9 @@ func (s *Service) AssignToPullReq(
 		return nil, fmt.Errorf("failed to find label by id: %w", err)
 	}
 
-	if err := s.checkLabelIsInSpace(ctx, repoParentID, label); err != nil {
+	if err := s.checkPullreqLabelInScope(ctx, repoParentID, repoID, label); err != nil {
 		return nil, err
 	}
-	if label.RepoID != nil && *label.RepoID != repoID {
-		return nil,
-			errors.InvalidArgument("label %d is not defined in current repo", label.ID)
-	}
 
 	oldPullreqLabel, err := s.pullReqLabelAssignmentStore.FindByLabelID(ctx, pullreqID, label.ID)
 	if err != nil && !errors.Is(err, store.ErrResourceNotFound) {
@@ -149,23 +145,6 @@ func (s *Service) AssignToPullReq(
 	}, nil
 }
 
-func (s *Service) checkLabelIsInSpace(
-	ctx context.Context,
-	repoParentID int64,
-	label *types.Label,
-) error {
-	if label.SpaceID != nil {
-		spaceIDs, err := s.spaceStore.GetAncestorIDs(ctx, repoParentID)
-		if err != nil {
-			return fmt.Errorf("failed to get parent space ids: %w", err)
-		}
-		if ok := slices.Contains(spaceIDs, *label.SpaceID); !ok {
-			return errors.NotFound("label %d is not defined in current space tree path", label.ID)
-		}
-	}
-	return nil
-}
-
 func (s *Service) getOrDefineValue(
 	ctx context.Context,
 	principalID int64,
@@ -208,7 +187,7 @@ func (s *Service) UnassignFromPullReq(
 		return nil, nil, fmt.Errorf("failed to find label by id: %w", err)
 	}
 
-	if err := s.checkLabelIsInSpace(ctx, repoParentID, label); err != nil {
+	if err := s.checkPullreqLabelInScope(ctx, repoParentID, repoID, label); err != nil {
 		return nil, nil, err
 	}
 
@@ -217,20 +196,6 @@ func (s *Service) UnassignFromPullReq(
 		return nil, nil, fmt.Errorf("failed to find label value: %w", err)
 	}
 
-	if label.RepoID != nil && *label.RepoID != repoID {
-		return nil, nil, errors.InvalidArgument(
-			"label %d is not defined in current repo", label.ID)
-	} else if label.SpaceID != nil {
-		spaceIDs, err := s.spaceStore.GetAncestorIDs(ctx, repoParentID)
-		if err != nil {
-			return nil, nil, fmt.Errorf("failed to get parent space ids: %w", err)
-		}
-		if ok := slices.Contains(spaceIDs, *label.SpaceID); !ok {
-			return nil, nil, errors.NotFound(
-				"label %d is not defined in current space tree path", label.ID)
-		}
-	}
-
 	return label, value, s.pullReqLabelAssignmentStore.Unassign(ctx, pullreqID, labelID)
 }
 
@@ -408,3 +373,25 @@ func newPullReqLabel(
 		UpdatedBy: principalID,
 	}
 }
+
+func (s *Service) checkPullreqLabelInScope(
+	ctx context.Context,
+	repoParentID, repoID int64,
+	label *types.Label,
+) error {
+	if label.RepoID != nil && *label.RepoID != repoID {
+		return errors.InvalidArgument("label %d is not defined in current repo", label.ID)
+	}
+
+	if label.SpaceID != nil {
+		spaceIDs, err := s.spaceStore.GetAncestorIDs(ctx, repoParentID)
+		if err != nil {
+			return fmt.Errorf("failed to get parent space ids: %w", err)
+		}
+		if ok := slices.Contains(spaceIDs, *label.SpaceID); !ok {
+			return errors.InvalidArgument("label %d is not defined in current space tree path", label.ID)
+		}
+	}
+
+	return nil
+}