diff --git a/api/bundle_update_test.go b/api/bundle_update_test.go index 71f5ddeb..976caa07 100644 --- a/api/bundle_update_test.go +++ b/api/bundle_update_test.go @@ -47,7 +47,7 @@ func TestPutBundle_Success(t *testing.T) { CreatedBy: &models.User{Email: "creator@example.com"}, UpdatedAt: &now, ManagedBy: models.ManagedByDataAdmin, - PreviewTeams: &[]models.PreviewTeam{{ID: "team-1"}}, + PreviewTeams: &[]models.PreviewTeam{{ID: team1}}, } updateRequest := &models.Bundle{ @@ -55,7 +55,7 @@ func TestPutBundle_Success(t *testing.T) { BundleType: models.BundleTypeManual, State: models.BundleStateDraft, ManagedBy: models.ManagedByDataAdmin, - PreviewTeams: &[]models.PreviewTeam{{ID: "team-1"}}, + PreviewTeams: &[]models.PreviewTeam{{ID: team1}}, } updateRequestJSON, err := json.Marshal(updateRequest) @@ -145,7 +145,7 @@ func TestPutBundle_Success(t *testing.T) { Convey("When putBundle is called with new preview teams and an existing preview team", func() { bundleRequest := updateRequest - bundleRequest.PreviewTeams = &[]models.PreviewTeam{{ID: "team-1"}, {ID: "team-2"}, {ID: "team-3"}} + bundleRequest.PreviewTeams = &[]models.PreviewTeam{{ID: team1}, {ID: "team-2"}, {ID: "team-3"}} bundleRequestJSON, err := json.Marshal(bundleRequest) So(err, ShouldBeNil) @@ -178,6 +178,137 @@ func TestPutBundle_Success(t *testing.T) { }) } +func TestPutBundle_BackfillsPolicyConditionsForNewPreviewTeams(t *testing.T) { + t.Parallel() + + Convey("Given a valid PUT request that adds a new preview team to a bundle with existing content", t, func() { + now := time.Now().UTC() + existingBundle := &models.Bundle{ + ID: bundle1, + Title: "Original Title", + BundleType: models.BundleTypeManual, + ETag: "original-etag", + State: models.BundleStateDraft, + CreatedAt: &now, + CreatedBy: &models.User{Email: "publisher1@example.com"}, + UpdatedAt: &now, + ManagedBy: models.ManagedByDataAdmin, + PreviewTeams: &[]models.PreviewTeam{{ID: team1}}, + } + + updateRequest := &models.Bundle{ + Title: title1, + BundleType: models.BundleTypeManual, + State: models.BundleStateDraft, + ManagedBy: models.ManagedByDataAdmin, + PreviewTeams: &[]models.PreviewTeam{{ID: team1}, {ID: "team-2"}}, + } + + updateRequestJSON, err := json.Marshal(updateRequest) + So(err, ShouldBeNil) + + mockedDatastore := &storetest.StorerMock{ + GetBundleFunc: func(ctx context.Context, id string) (*models.Bundle, error) { + if id == bundle1 { + return existingBundle, nil + } + return nil, apierrors.ErrBundleNotFound + }, + CheckBundleExistsByTitleUpdateFunc: func(ctx context.Context, title, excludeID string) (bool, error) { + if title == title1 && excludeID == bundle1 { + return false, nil + } + return false, nil + }, + UpdateBundleFunc: func(ctx context.Context, bundleID string, bundle *models.Bundle) (*models.Bundle, error) { + if bundleID == bundle1 { + bundle.ID = bundleID + return bundle, nil + } + return nil, errors.New("failed to update bundle") + }, + UpdateBundleETagFunc: func(ctx context.Context, bundleID, email string) (*models.Bundle, error) { + if bundleID == bundle1 { + updatedBundle := *existingBundle + updatedBundle.Title = title1 + updatedBundle.ETag = newEtag + updatedBundle.PreviewTeams = &[]models.PreviewTeam{{ID: team1}, {ID: "team-2"}} + return &updatedBundle, nil + } + return nil, errors.New("failed to update ETag") + }, + CreateEventFunc: func(ctx context.Context, event *models.Event) error { + if event.Action == models.ActionUpdate && event.Resource == urlString { + return nil + } + return errors.New("failed to create event") + }, + GetContentItemsByBundleIDFunc: func(ctx context.Context, bundleID string) ([]*models.ContentItem, error) { + return []*models.ContentItem{ + { + ID: "content-1", + Metadata: models.Metadata{ + DatasetID: "staticDataset1", + EditionID: "2025", + }, + }, + }, nil + }, + } + + team2PolicyLookupCount := 0 + mockPermissionsClient := &permissionsAPISDKMock.ClienterMock{ + GetPolicyFunc: func(ctx context.Context, id string, headers permissionsAPISDK.Headers) (*permissionsAPIModels.Policy, error) { + if id == team1 { + return &permissionsAPIModels.Policy{ID: team1}, nil + } + + if id == "team-2" { + team2PolicyLookupCount++ + if team2PolicyLookupCount == 1 { + return nil, errors.New("404 policy not found") + } + return &permissionsAPIModels.Policy{ID: "team-2"}, nil + } + + return nil, errors.New("404 policy not found") + }, + PostPolicyWithIDFunc: func(ctx context.Context, id string, policy permissionsAPIModels.PolicyInfo, headers permissionsAPISDK.Headers) (*permissionsAPIModels.Policy, error) { + return nil, nil + }, + PutPolicyFunc: func(ctx context.Context, id string, policy permissionsAPIModels.Policy, headers permissionsAPISDK.Headers) error { + So(id, ShouldEqual, "team-2") + So(policy.Condition.Values, ShouldContain, "staticDataset1") + So(policy.Condition.Values, ShouldContain, "staticDataset1/2025") + So(len(policy.Condition.Values), ShouldEqual, 2) + return nil + }, + } + + bundleAPI := GetBundleAPIWithMocks(store.Datastore{Backend: mockedDatastore}, &datasetAPISDKMock.ClienterMock{}, mockPermissionsClient, false) + + Convey("When putBundle is called", func() { + r := httptest.NewRequest("PUT", "/bundles/bundle-1", bytes.NewReader(updateRequestJSON)) + r = mux.SetURLVars(r, map[string]string{"bundle-id": bundle1}) + r.Header.Set("If-Match", "original-etag") + r.Header.Set("Authorization", "Bearer test-auth-token") + w := httptest.NewRecorder() + + bundleAPI.putBundle(w, r) + + Convey("Then it should return 200 OK", func() { + So(w.Code, ShouldEqual, http.StatusOK) + }) + + Convey("And create a policy for the new team and backfill existing bundle content conditions", func() { + So(len(mockPermissionsClient.GetPolicyCalls()), ShouldEqual, 3) + So(len(mockPermissionsClient.PostPolicyWithIDCalls()), ShouldEqual, 1) + So(len(mockPermissionsClient.PutPolicyCalls()), ShouldEqual, 1) + }) + }) + }) +} + func TestPutBundleNoPreviewTeam_Success(t *testing.T) { t.Parallel() @@ -442,7 +573,7 @@ func TestPutBundle_BundleNotFound_Failure(t *testing.T) { BundleType: models.BundleTypeManual, State: models.BundleStateDraft, ManagedBy: models.ManagedByDataAdmin, - PreviewTeams: &[]models.PreviewTeam{{ID: "team-1"}}, + PreviewTeams: &[]models.PreviewTeam{{ID: team1}}, } updateRequestJSON, err := json.Marshal(updateRequest) @@ -496,7 +627,7 @@ func TestPutBundle_AuthenticationFailure(t *testing.T) { BundleType: models.BundleTypeManual, State: models.BundleStateDraft, ManagedBy: models.ManagedByDataAdmin, - PreviewTeams: &[]models.PreviewTeam{{ID: "team-1"}}, + PreviewTeams: &[]models.PreviewTeam{{ID: team1}}, } updateRequestJSON, err := json.Marshal(updateRequest) @@ -622,7 +753,7 @@ func TestPutBundle_CreateBundlePolicies_Failure(t *testing.T) { BundleType: models.BundleTypeManual, State: models.BundleStateDraft, ManagedBy: models.ManagedByDataAdmin, - PreviewTeams: &[]models.PreviewTeam{{ID: "team-1"}}, + PreviewTeams: &[]models.PreviewTeam{{ID: team1}}, } updateRequestJSON, err := json.Marshal(updateRequest) diff --git a/application/policies.go b/application/policies.go index 56426778..33e0ef41 100644 --- a/application/policies.go +++ b/application/policies.go @@ -3,6 +3,7 @@ package application import ( "context" "net/http" + "sort" "strconv" "strings" @@ -311,13 +312,34 @@ func (s *StateMachineBundleAPI) AddPolicyConditionsForAddedPreviewTeams(ctx cont } for value := range valuesToAdd { - if !existingValues[value] { - policy.Condition.Values = append(policy.Condition.Values, value) + existingValues[value] = true + } + + updatedValues := make([]string, 0, len(existingValues)) + for value := range existingValues { + updatedValues = append(updatedValues, value) + } + sort.Strings(updatedValues) + + currentValues := append([]string{}, policy.Condition.Values...) + sort.Strings(currentValues) + + valuesChanged := len(currentValues) != len(updatedValues) + if !valuesChanged { + for i := range updatedValues { + if updatedValues[i] != currentValues[i] { + valuesChanged = true + break + } } } - err = s.PermissionsAPIClient.PutPolicy(ctx, team.ID, *policy, permissionsAPISDK.Headers{Authorization: authToken}) - if err != nil { + if !valuesChanged { + continue + } + + policy.Condition.Values = updatedValues + if err := s.PermissionsAPIClient.PutPolicy(ctx, team.ID, *policy, permissionsAPISDK.Headers{Authorization: authToken}); err != nil { return err } } diff --git a/application/policies_test.go b/application/policies_test.go index 6df0fc8e..5c311542 100644 --- a/application/policies_test.go +++ b/application/policies_test.go @@ -748,7 +748,7 @@ func TestAddPolicyConditionsForAddedPreviewTeams(t *testing.T) { Convey("Then no duplicate values are added", func() { So(err, ShouldBeNil) - So(len(mockPermissionsAPIClient.PutPolicyCalls()), ShouldEqual, 1) + So(len(mockPermissionsAPIClient.PutPolicyCalls()), ShouldEqual, 0) }) }) diff --git a/features/bundle_update.feature b/features/bundle_update.feature index 4f7ea47c..493577ca 100644 --- a/features/bundle_update.feature +++ b/features/bundle_update.feature @@ -170,6 +170,64 @@ Feature: Update a bundle - PUT /bundles/{id} ] """ + Scenario: PUT /bundles/{id} backfills policy condition values when preview team is added after content + Given I have these content items: + """ + [ + { + "id": "content-1", + "bundle_id": "bundle-1", + "content_type": "DATASET", + "metadata": { + "dataset_id": "static-test-dataset", + "edition_id": "time-series", + "version_id": 1 + }, + "links": { + "edit": "/datasets/static-test-dataset/editions/time-series/versions/1", + "preview": "/datasets/static-test-dataset/editions/time-series/versions/1" + } + } + ] + """ + And I am an admin user + And I set the header "If-Match" to "etag-bundle-1" + When I PUT "/bundles/bundle-1" + """ + { + "bundle_type": "MANUAL", + "preview_teams": [ + { + "id": "team-preview-1" + } + ], + "state": "DRAFT", + "title": "Updated Bundle Title", + "managed_by": "DATA-ADMIN" + } + """ + Then the HTTP status code should be "200" + And the following policies should exist: + """ + [ + { + "id": "team-preview-1", + "role": "datasets-previewer", + "entities": [ + "groups/team-preview-1" + ], + "condition": { + "attribute": "dataset_edition", + "operator": "StringEquals", + "values": [ + "static-test-dataset", + "static-test-dataset/time-series" + ] + } + } + ] + """ + Scenario: PUT /bundles/{id} with state transition from DRAFT to IN_REVIEW Given I am an admin user And I set the header "If-Match" to "etag-bundle-1"