diff --git a/controller/workitem.go b/controller/workitem.go index 644aaad804..38c6a7a0f9 100644 --- a/controller/workitem.go +++ b/controller/workitem.go @@ -71,23 +71,21 @@ func NewNotifyingWorkitemController(service *goa.Service, db application.DB, not config: config} } -// authorizeWorkitemTypeEditor returns true if the modifier is allowed to change -// workitem type else it returns false. -// Only space owner and workitem creator are allowed to change workitem type -func (c *WorkitemController) authorizeWorkitemTypeEditor(ctx context.Context, spaceID uuid.UUID, creatorID string, editorID string) (bool, error) { +// WorkitemCreatorOrSpaceOwner checks if the modifier is space owner or workitem creator +func (c *WorkitemController) WorkitemCreatorOrSpaceOwner(ctx context.Context, spaceID uuid.UUID, creatorID uuid.UUID, editorID uuid.UUID) error { // check if workitem editor is same as workitem creator if editorID == creatorID { - return true, nil + return nil } space, err := c.db.Spaces().Load(ctx, spaceID) if err != nil { - return false, errors.NewNotFoundError("space", spaceID.String()) + return errors.NewNotFoundError("space", spaceID.String()) } // check if workitem editor is same as space owner - if space != nil && editorID == space.OwnerID.String() { - return true, nil + if space != nil && editorID == space.OwnerID { + return nil } - return false, errors.NewUnauthorizedError("user is not allowed to change workitem type") + return errors.NewForbiddenError("user is not a workitem creator or space owner") } // Returns true if the user is the work item creator or space collaborator @@ -123,6 +121,14 @@ func (c *WorkitemController) Update(ctx *app.UpdateWorkitemContext) error { if creator == nil { return jsonapi.JSONErrorResponse(ctx, errors.NewInternalError(ctx, errs.New("work item doesn't have creator"))) } + creatorIDStr, ok := creator.(string) + if !ok { + return jsonapi.JSONErrorResponse(ctx, errs.Errorf("failed to convert user to string: %+v (%[1]T)", creator)) + } + creatorID, err := uuid.FromString(creatorIDStr) + if err != nil { + return jsonapi.JSONErrorResponse(ctx, err) + } authorized, err := authorizeWorkitemEditor(ctx, c.db, wi.SpaceID, creator.(string), currentUserIdentityID.String()) if err != nil { return jsonapi.JSONErrorResponse(ctx, err) @@ -134,13 +140,10 @@ func (c *WorkitemController) Update(ctx *app.UpdateWorkitemContext) error { if ctx.Payload.Data.Relationships != nil && ctx.Payload.Data.Relationships.BaseType != nil && ctx.Payload.Data.Relationships.BaseType.Data != nil && ctx.Payload.Data.Relationships.BaseType.Data.ID != wi.Type { - authorized, err := c.authorizeWorkitemTypeEditor(ctx, wi.SpaceID, creator.(string), currentUserIdentityID.String()) + err := c.WorkitemCreatorOrSpaceOwner(ctx, wi.SpaceID, creatorID, *currentUserIdentityID) if err != nil { return jsonapi.JSONErrorResponse(ctx, err) } - if !authorized { - return jsonapi.JSONErrorResponse(ctx, errors.NewForbiddenError("user is not authorized to change the workitemtype")) - } // Store new values of type and version newType := ctx.Payload.Data.Relationships.BaseType newVersion := ctx.Payload.Data.Attributes[workitem.SystemVersion] @@ -236,39 +239,52 @@ func (c *WorkitemController) Show(ctx *app.ShowWorkitemContext) error { // Delete does DELETE workitem func (c *WorkitemController) Delete(ctx *app.DeleteWorkitemContext) error { - // Temporarly disabled, See https://github.com/fabric8-services/fabric8-wit/issues/1036 - if true { - return ctx.MethodNotAllowed() - } currentUserIdentityID, err := login.ContextIdentity(ctx) if err != nil { return jsonapi.JSONErrorResponse(ctx, errors.NewUnauthorizedError(err.Error())) } + var wi *workitem.WorkItem err = application.Transactional(c.db, func(appl application.Application) error { wi, err = appl.WorkItems().LoadByID(ctx, ctx.WiID) if err != nil { - return errs.Wrap(err, fmt.Sprintf("Failed to load work item with id %v", ctx.WiID)) + return errs.Wrap(err, fmt.Sprintf("Fail to load work item with id %v", ctx.WiID)) } return nil }) if err != nil { return jsonapi.JSONErrorResponse(ctx, err) } - authorized, err := authz.Authorize(ctx, wi.SpaceID.String()) + + // Check if user is space owner or workitem creator. Only space owner or workitem creator are allowed to delete the workitem. + creator := wi.Fields[workitem.SystemCreator] + if creator == nil { + return jsonapi.JSONErrorResponse(ctx, errors.NewInternalError(ctx, errs.New("work item doesn't have creator"))) + } + creatorIDStr, ok := creator.(string) + if !ok { + return jsonapi.JSONErrorResponse(ctx, errs.Errorf("failed to convert user to string: %+v (%[1]T)", creator)) + } + creatorID, err := uuid.FromString(creatorIDStr) if err != nil { - return jsonapi.JSONErrorResponse(ctx, errors.NewUnauthorizedError(err.Error())) + return jsonapi.JSONErrorResponse(ctx, err) } - if !authorized { - return jsonapi.JSONErrorResponse(ctx, errors.NewForbiddenError("user is not authorized to access the space")) + err = c.WorkitemCreatorOrSpaceOwner(ctx, wi.SpaceID, creatorID, *currentUserIdentityID) + if err != nil { + forbidden, _ := errors.IsForbiddenError(err) + if forbidden { + return jsonapi.JSONErrorResponse(ctx, errors.NewForbiddenError("user is not authorized to delete the workitem")) + + } + return jsonapi.JSONErrorResponse(ctx, err) } err = application.Transactional(c.db, func(appl application.Application) error { - if err := appl.WorkItems().Delete(ctx, ctx.WiID, *currentUserIdentityID); err != nil { - return errs.Wrapf(err, "error deleting work item %s", ctx.WiID) - } if err := appl.WorkItemLinks().DeleteRelatedLinks(ctx, ctx.WiID, *currentUserIdentityID); err != nil { return errs.Wrapf(err, "failed to delete work item links related to work item %s", ctx.WiID) } + if err := appl.WorkItems().Delete(ctx, ctx.WiID, *currentUserIdentityID); err != nil { + return errs.Wrapf(err, "error deleting work item %s", ctx.WiID) + } return nil }) if err != nil { diff --git a/controller/workitem_blackbox_test.go b/controller/workitem_blackbox_test.go index 85bc31c1fe..409d98080c 100644 --- a/controller/workitem_blackbox_test.go +++ b/controller/workitem_blackbox_test.go @@ -2304,17 +2304,6 @@ func (s *WorkItem2Suite) TestWI2FailShowMissing() { test.ShowWorkitemNotFound(s.T(), s.svc.Context, s.svc, s.workitemCtrl, uuid.NewV4(), nil, nil) } -func (s *WorkItem2Suite) TestWI2FailOnDelete() { - c := minimumRequiredCreatePayload() - c.Data.Attributes[workitem.SystemTitle] = "Title" - c.Data.Attributes[workitem.SystemState] = workitem.SystemStateNew - c.Data.Relationships.BaseType = newRelationBaseType(workitem.SystemBug) - - _, createdWI := test.CreateWorkitemsCreated(s.T(), s.svc.Context, s.svc, s.workitemsCtrl, *c.Data.Relationships.Space.Data.ID, &c) - test.ShowWorkitemOK(s.T(), s.svc.Context, s.svc, s.workitemCtrl, *createdWI.Data.ID, nil, nil) - test.DeleteWorkitemMethodNotAllowed(s.T(), s.svc.Context, s.svc, s.workitemCtrl, *createdWI.Data.ID) -} - func (s *WorkItem2Suite) TestWI2CreateWithArea() { fxt := tf.NewTestFixture(s.T(), s.DB, tf.CreateWorkItemEnvironment(), @@ -3258,8 +3247,7 @@ func (s *WorkItemSuite) TestUpdateWorkitemForSpaceCollaborator() { // Not a space collaborator is not authorized to update test.UpdateWorkitemForbidden(s.T(), svcNotAuthorized.Context, svcNotAuthorized, workitemCtrlNotAuthorized, *wi.Data.ID, &payload2) // Not a space collaborator is not authorized to delete - // Temporarily disabled, See https://github.com/fabric8-services/fabric8-wit/issues/1036 - // test.DeleteWorkitemForbidden(s.T(), svcNotAuthrized.Context, svcNotAuthorized, workitemCtrlNotAuthorized, *wi.Data.ID) + test.DeleteWorkitemForbidden(s.T(), svcNotAuthorized.Context, svcNotAuthorized, workitemCtrlNotAuthorized, *wi.Data.ID) // Not a space collaborator is not authorized to reorder payload5 := minimumRequiredReorderPayload() var dataArray []*app.WorkItem // dataArray contains the workitem(s) that have to be reordered @@ -3390,3 +3378,43 @@ func (s *WorkItem2Suite) TestCreateAndUpdateWorkItemForEveryWIT() { }) } } + +// TestDeleteWorkitem tests the delete action +func (s *WorkItem2Suite) TestDeleteWorkitem() { + // Delete Workitem tests deletion of workitem + s.T().Run("Delete Workitem", func(t *testing.T) { + t.Run("ok", func(t *testing.T) { + fxt := tf.NewTestFixture(s.T(), s.DB, tf.WorkItems(1)) + s.svc = testsupport.ServiceAsUser("TestUpdateWI2-Service", *fxt.Identities[0]) + test.DeleteWorkitemOK(s.T(), s.svc.Context, s.svc, s.workitemCtrl, fxt.WorkItems[0].ID) + }) + t.Run("unauthorized", func(t *testing.T) { + fxt := tf.NewTestFixture(s.T(), s.DB, tf.WorkItems(1)) + svcNotAuthorized := goa.New("TestDeleteWI2-Service") + workitemCtrlNotAuthorized := NewWorkitemController(svcNotAuthorized, s.GormDB, s.Configuration) + test.DeleteWorkitemUnauthorized(s.T(), svcNotAuthorized.Context, svcNotAuthorized, workitemCtrlNotAuthorized, fxt.WorkItems[0].ID) + }) + t.Run("forbidden", func(t *testing.T) { + fxt := tf.NewTestFixture(s.T(), s.DB, tf.WorkItems(1), tf.Identities(2)) + s.svc = testsupport.ServiceAsUser("TestUpdateWI2-Service", *fxt.Identities[1]) + test.DeleteWorkitemForbidden(s.T(), s.svc.Context, s.svc, s.workitemCtrl, fxt.WorkItems[0].ID) + }) + t.Run("workitem not found", func(t *testing.T) { + test.DeleteWorkitemNotFound(s.T(), s.svc.Context, s.svc, s.workitemCtrl, uuid.NewV4()) + }) + }) + // Delete Workitem Links tests deletion of corresponding workitem links when a workitem is deleted + s.T().Run("Delete Workitem Links", func(t *testing.T) { + t.Run("ok", func(t *testing.T) { + fxt := tf.NewTestFixture(t, s.DB, + tf.CreateWorkItemEnvironment(), + tf.WorkItems(2, tf.SetWorkItemTitles("A", "B")), + tf.WorkItemLinkTypes(1), + tf.WorkItemLinksCustom(1, tf.BuildLinks(tf.LinkChain("A", "B")...)), + ) + s.svc = testsupport.ServiceAsUser("TestUpdateWI2-Service", *fxt.Identities[0]) + test.DeleteWorkitemOK(s.T(), s.svc.Context, s.svc, s.workitemCtrl, fxt.WorkItems[0].ID) + test.ShowWorkItemLinkNotFound(t, s.svc.Context, s.svc, s.linkCtrl, fxt.WorkItemLinks[0].ID, nil, nil) + }) + }) +} diff --git a/design/workitems.go b/design/workitems.go index 77c628f48b..2cf3f5bafa 100644 --- a/design/workitems.go +++ b/design/workitems.go @@ -152,9 +152,7 @@ var _ = a.Resource("workitem", func() { a.Params(func() { a.Param("wiID", d.UUID, "ID of the work item to delete") }) - a.Response(d.MethodNotAllowed) a.Response(d.OK) - a.Response(d.BadRequest, JSONAPIErrors) a.Response(d.InternalServerError, JSONAPIErrors) a.Response(d.NotFound, JSONAPIErrors) a.Response(d.Unauthorized, JSONAPIErrors) diff --git a/workitem/link/link_repository_blackbox_test.go b/workitem/link/link_repository_blackbox_test.go index 0c8e966814..a3a1c99637 100644 --- a/workitem/link/link_repository_blackbox_test.go +++ b/workitem/link/link_repository_blackbox_test.go @@ -892,3 +892,16 @@ func (s *linkRepoBlackBoxTest) TestDeleteLinkAndListChildren() { require.Len(t, childrenList, 0) }) } + +func (s *linkRepoBlackBoxTest) TestDeleteLink() { + s.T().Run("ok", func(t *testing.T) { + fxt := tf.NewTestFixture(t, s.DB, tf.WorkItemLinks(1)) + err := s.workitemLinkRepo.DeleteRelatedLinks(s.Ctx, fxt.WorkItems[0].ID, fxt.Identities[0].ID) + require.NoError(t, err) + + // check if link exists + err = s.workitemLinkRepo.CheckExists(s.Ctx, fxt.WorkItemLinks[0].ID) + require.Error(t, err) + require.IsType(t, errors.NotFoundError{}, err) + }) +} diff --git a/workitem/workitem_repository_blackbox_test.go b/workitem/workitem_repository_blackbox_test.go index 36bd2a8b18..50a62f3bfc 100644 --- a/workitem/workitem_repository_blackbox_test.go +++ b/workitem/workitem_repository_blackbox_test.go @@ -769,3 +769,18 @@ func (s *workItemRepoBlackBoxTest) TestList() { }) } + +func (s *workItemRepoBlackBoxTest) TestDeleteWorkitem() { + s.T().Run("ok", func(t *testing.T) { + fxt := tf.NewTestFixture(t, s.DB, + tf.WorkItems(1), + ) + err := s.repo.Delete(s.Ctx, fxt.WorkItems[0].ID, fxt.Identities[0].ID) + require.Nil(t, err) + + // check if workitem exists + err = s.repo.CheckExists(s.Ctx, fxt.WorkItems[0].ID) + require.Error(t, err) + require.IsType(t, errors.NotFoundError{}, errs.Cause(err)) + }) +}