Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions apps/api/internal/handler/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,14 @@ type IssueHandler struct {
Issue *service.IssueService
}

// invalidRelationError reports whether err is a rejected related-id error that
// should surface as a 400 (bad state/label/parent/assignee for the scope).
func invalidRelationError(err error) bool {
return err == service.ErrInvalidState || err == service.ErrInvalidLabel ||
err == service.ErrInvalidParent || err == service.ErrInvalidAssignee ||
err == service.ErrInvalidPriority
}

func issueID(c *gin.Context) (uuid.UUID, bool) {
idStr := c.Param("pk")
if idStr == "" {
Expand Down Expand Up @@ -197,6 +205,10 @@ func (h *IssueHandler) Create(c *gin.Context) {
c.JSON(http.StatusNotFound, gin.H{"error": "Not found"})
return
}
if invalidRelationError(err) {
c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()})
return
}
c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to create issue"})
return
}
Expand Down Expand Up @@ -305,6 +317,10 @@ func (h *IssueHandler) Update(c *gin.Context) {
c.JSON(http.StatusNotFound, gin.H{"error": "Issue not found"})
return
}
if invalidRelationError(err) {
c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()})
return
}
c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to update issue"})
return
}
Expand Down
83 changes: 83 additions & 0 deletions apps/api/internal/handler/issue_relation_validation_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
package handler_test

import (
"net/http"
"testing"

"github.com/Devlaner/devlane/api/internal/testutil"
"github.com/stretchr/testify/require"
)

func TestIssue_CreateRejectsForeignRelations(t *testing.T) {
ts := testutil.NewTestServer(t)
w := testutil.SeedWorld(t, ts.DB)

// A second project in the same workspace, and one in another workspace.
otherProject := testutil.CreateProject(t, ts.DB, w.Workspace.ID, w.User.ID)
foreignState := testutil.CreateState(t, ts.DB, otherProject.ID, w.Workspace.ID)
foreignLabel := testutil.CreateLabel(t, ts.DB, otherProject.ID, w.Workspace.ID)
foreignParent := testutil.CreateIssue(t, ts.DB, otherProject.ID, w.Workspace.ID, w.User.ID)
nonMember := testutil.CreateUser(t, ts.DB)

base := "/api/workspaces/" + w.Workspace.Slug + "/projects/" + w.Project.ID.String() + "/issues/"

require.Equal(t, http.StatusBadRequest,
ts.POST(base, map[string]any{"name": "x", "state_id": foreignState.ID.String()}, w.Session).Code,
"a state from another project must be rejected")
require.Equal(t, http.StatusBadRequest,
ts.POST(base, map[string]any{"name": "x", "label_ids": []string{foreignLabel.ID.String()}}, w.Session).Code,
"a label from another project must be rejected")
require.Equal(t, http.StatusBadRequest,
ts.POST(base, map[string]any{"name": "x", "parent_id": foreignParent.ID.String()}, w.Session).Code,
"a parent from another project must be rejected")
require.Equal(t, http.StatusBadRequest,
ts.POST(base, map[string]any{"name": "x", "assignee_ids": []string{nonMember.ID.String()}}, w.Session).Code,
"an assignee who isn't a workspace member must be rejected")

// A well-scoped create still works.
okState := testutil.CreateState(t, ts.DB, w.Project.ID, w.Workspace.ID)
okLabel := testutil.CreateLabel(t, ts.DB, w.Project.ID, w.Workspace.ID)
okParent := testutil.CreateIssue(t, ts.DB, w.Project.ID, w.Workspace.ID, w.User.ID)
rr := ts.POST(base, map[string]any{
"name": "valid",
"state_id": okState.ID.String(),
"label_ids": []string{okLabel.ID.String()},
"assignee_ids": []string{w.User.ID.String()},
"parent_id": okParent.ID.String(),
}, w.Session)
require.Equal(t, http.StatusCreated, rr.Code, "body=%s", rr.Body.String())
}

func TestIssue_UpdateRejectsForeignRelations(t *testing.T) {
ts := testutil.NewTestServer(t)
w := testutil.SeedWorld(t, ts.DB)
otherProject := testutil.CreateProject(t, ts.DB, w.Workspace.ID, w.User.ID)
foreignState := testutil.CreateState(t, ts.DB, otherProject.ID, w.Workspace.ID)
foreignLabel := testutil.CreateLabel(t, ts.DB, otherProject.ID, w.Workspace.ID)
nonMember := testutil.CreateUser(t, ts.DB)

issue := testutil.CreateIssue(t, ts.DB, w.Project.ID, w.Workspace.ID, w.User.ID)
base := "/api/workspaces/" + w.Workspace.Slug + "/projects/" + w.Project.ID.String() +
"/issues/" + issue.ID.String() + "/"

require.Equal(t, http.StatusBadRequest,
ts.PATCH(base, map[string]any{"state_id": foreignState.ID.String()}, w.Session).Code)
require.Equal(t, http.StatusBadRequest,
ts.PATCH(base, map[string]any{"label_ids": []string{foreignLabel.ID.String()}}, w.Session).Code)
require.Equal(t, http.StatusBadRequest,
ts.PATCH(base, map[string]any{"assignee_ids": []string{nonMember.ID.String()}}, w.Session).Code)

// A parent from another project is rejected.
foreignParent := testutil.CreateIssue(t, ts.DB, otherProject.ID, w.Workspace.ID, w.User.ID)
require.Equal(t, http.StatusBadRequest,
ts.PATCH(base, map[string]any{"parent_id": foreignParent.ID.String()}, w.Session).Code)

// An issue can't be made its own parent.
require.Equal(t, http.StatusBadRequest,
ts.PATCH(base, map[string]any{"parent_id": issue.ID.String()}, w.Session).Code)

// A well-scoped update still works.
okState := testutil.CreateState(t, ts.DB, w.Project.ID, w.Workspace.ID)
require.Equal(t, http.StatusOK,
ts.PATCH(base, map[string]any{"state_id": okState.ID.String()}, w.Session).Code)
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
1 change: 1 addition & 0 deletions apps/api/internal/router/router.go
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ func New(cfg Config) *gin.Engine {
issueReactionStore := store.NewIssueReactionStore(cfg.DB)
issueSvc.SetReactionStore(issueReactionStore)
issueSvc.SetStateStore(stateStore)
issueSvc.SetLabelStore(labelStore)
commentReactionStore := store.NewCommentReactionStore(cfg.DB)
commentSvc := service.NewCommentService(commentStore, issueStore, projectStore, workspaceStore)
commentSvc.SetReactionStore(commentReactionStore)
Expand Down
90 changes: 88 additions & 2 deletions apps/api/internal/service/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@ var (
ErrEpicHasChildren = errors.New("epic has child work items")
// ErrMoveSameProject is returned when a move targets the issue's current project.
ErrMoveSameProject = errors.New("issue already in target project")
// ErrInvalidLabel / ErrInvalidParent / ErrInvalidAssignee are returned when a
// related id supplied on create/update doesn't belong to the allowed scope.
ErrInvalidLabel = errors.New("invalid label for project")
ErrInvalidParent = errors.New("invalid parent for project")
ErrInvalidAssignee = errors.New("assignee is not a workspace member")
)

// validPriorities is the accepted set of work-item priority values.
Expand All @@ -41,7 +46,8 @@ type IssueService struct {
notify *NotificationService // optional — may be nil
subs *store.IssueSubscriberStore // optional — auto-subscribe assignees/mentions
reactions *store.IssueReactionStore // optional — per-issue emoji reactions
states *store.StateStore // optional — validates state ownership on bulk update
states *store.StateStore // optional — validates state ownership
labels *store.LabelStore // optional — validates label ownership
}

func NewIssueService(is *store.IssueStore, ps *store.ProjectStore, ws *store.WorkspaceStore) *IssueService {
Expand All @@ -63,9 +69,74 @@ func (s *IssueService) SetSubscriberStore(subs *store.IssueSubscriberStore) { s.
// SetReactionStore wires per-issue emoji reactions support. Optional.
func (s *IssueService) SetReactionStore(r *store.IssueReactionStore) { s.reactions = r }

// SetStateStore wires state-ownership validation for bulk updates. Optional.
// SetStateStore wires state-ownership validation. Optional.
func (s *IssueService) SetStateStore(st *store.StateStore) { s.states = st }

// SetLabelStore wires label-ownership validation. Optional.
func (s *IssueService) SetLabelStore(l *store.LabelStore) { s.labels = l }

// validateRelations rejects related ids that fall outside the allowed scope:
// state and labels must belong to the same project, a parent must be another
// issue in the same project (and never the issue itself — pass selfID on
// update; uuid.Nil on create), and assignees must be members of the workspace.
// Only the provided (non-nil / non-empty) fields are checked. Ownership stores
// are optional; when one isn't wired the corresponding check is skipped. A
// genuine "not found" maps to the invalid-* sentinel, but any other datastore
// error is returned so query failures surface as 5xx, not 400.
func (s *IssueService) validateRelations(ctx context.Context, projectID, workspaceID, selfID uuid.UUID, stateID *uuid.UUID, labelIDs []uuid.UUID, assigneeIDs []uuid.UUID, parentID *uuid.UUID) error {
if stateID != nil && s.states != nil {
st, err := s.states.GetByID(ctx, *stateID)
if errors.Is(err, gorm.ErrRecordNotFound) {
return ErrInvalidState
}
if err != nil {
return err
}
if st == nil || st.ProjectID != projectID {
return ErrInvalidState
}
}
if len(labelIDs) > 0 && s.labels != nil {
for _, id := range labelIDs {
l, err := s.labels.GetByID(ctx, id)
if errors.Is(err, gorm.ErrRecordNotFound) {
return ErrInvalidLabel
}
if err != nil {
return err
}
if l == nil || l.ProjectID == nil || *l.ProjectID != projectID {
return ErrInvalidLabel
}
}
}
if parentID != nil {
if *parentID == selfID {
return ErrInvalidParent // an issue can't be its own parent
}
parent, err := s.is.GetByID(ctx, *parentID)
if errors.Is(err, gorm.ErrRecordNotFound) {
return ErrInvalidParent
}
if err != nil {
return err
}
if parent == nil || parent.ProjectID != projectID {
return ErrInvalidParent
}
}
for _, id := range assigneeIDs {
ok, err := s.ws.IsMember(ctx, workspaceID, id)
if err != nil {
return err
}
if !ok {
return ErrInvalidAssignee
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
}
return nil
}

// autoSubscribe is a fire-and-forget helper used by the assignee and mention
// hooks. Errors are logged-and-ignored — the user's primary action must not
// fail because of a subscription bookkeeping issue.
Expand Down Expand Up @@ -474,6 +545,9 @@ func (s *IssueService) Create(ctx context.Context, workspaceSlug string, project
return nil, err
}
wrk, _ := s.ws.GetBySlug(ctx, workspaceSlug)
if err := s.validateRelations(ctx, projectID, wrk.ID, uuid.Nil, stateID, labelIDs, assigneeIDs, parentID); err != nil {
return nil, err
}
issue := &model.Issue{
Name: name,
ProjectID: projectID,
Expand Down Expand Up @@ -549,6 +623,18 @@ func (s *IssueService) Update(ctx context.Context, workspaceSlug string, project
return nil, err
}

// Reject related ids outside the allowed scope before touching anything.
var wantAssignees, wantLabels []uuid.UUID
if assigneeIDs != nil {
wantAssignees = *assigneeIDs
}
if labelIDs != nil {
wantLabels = *labelIDs
}
if err := s.validateRelations(ctx, issue.ProjectID, issue.WorkspaceID, issue.ID, stateID, wantLabels, wantAssignees, parentID); err != nil {
return nil, err
}

// Snapshot values before mutation so we can diff them for the activity log.
prevName := issue.Name
prevPriority := issue.Priority
Expand Down
Loading