Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
74 changes: 74 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,74 @@
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 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
63 changes: 61 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,47 @@ 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 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.
func (s *IssueService) validateRelations(ctx context.Context, projectID, workspaceID 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 err != nil || 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 err != nil || l == nil || l.ProjectID == nil || *l.ProjectID != projectID {
return ErrInvalidLabel
}
}
}
if parentID != nil {
parent, err := s.is.GetByID(ctx, *parentID)
if err != nil || parent == nil || parent.ProjectID != projectID {
return ErrInvalidParent
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
}
}
for _, id := range assigneeIDs {
ok, _ := s.ws.IsMember(ctx, workspaceID, id)
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 +518,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, stateID, labelIDs, assigneeIDs, parentID); err != nil {
return nil, err
}
issue := &model.Issue{
Name: name,
ProjectID: projectID,
Expand Down Expand Up @@ -549,6 +596,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, 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