From d24db31e4267a87f2dfb64e17ec72b6be2c79cbc Mon Sep 17 00:00:00 2001 From: Eleanor Deal Date: Fri, 18 May 2018 12:22:05 +0100 Subject: [PATCH 01/34] Update specs to have clearer summaries and descriptions for dev site --- swagger.yaml | 73 +++++++++++++++++++++------------------------------- 1 file changed, 29 insertions(+), 44 deletions(-) diff --git a/swagger.yaml b/swagger.yaml index fd42aadf..70cff20e 100644 --- a/swagger.yaml +++ b/swagger.yaml @@ -1,8 +1,11 @@ swagger: "2.0" info: - description: "An API used to query information about datasets published by the ONS." + description: "Used to find information about data published by the ONS. + `Datasets` are published in unique `versions`, which are categorized by `edition`. + Data in each version is broken down by `dimensions`, and a unique combination + of dimension `options` in a version can be used to retrieve `observation` level data." version: "1.0.0" - title: "Dataset API" + title: "Find data" license: name: "Open Government Licence v3.0" url: "http://www.nationalarchives.gov.uk/doc/open-government-licence/version/3/" @@ -158,27 +161,6 @@ securityDefinitions: in: header type: apiKey paths: - /search/datasets: - get: - tags: - - "Public" - summary: "Search for datasets" - description: "Perform a search on all published datasets by the ONS" - parameters: - - $ref: '#/parameters/limit' - - $ref: '#/parameters/offset' - - $ref: '#/parameters/query' - produces: - - "application/json" - responses: - 200: - description: "A json list containing datasets which have been published" - schema: - $ref: '#/definitions/Datasets' - 400: - description: "Body of the post request is invalid" - 500: - description: "Failed to process the request due to an internal error" /datasets: get: tags: @@ -278,8 +260,8 @@ paths: get: tags: - "Public" - summary: "Get a list of all editions from a dataset" - description: "Get a list of editions from a type of dataset" + summary: "Get a list of editions of a dataset" + description: "Get a list of editions of a type of dataset" parameters: - $ref: '#/parameters/id' - $ref: '#/parameters/limit' @@ -319,7 +301,7 @@ paths: get: tags: - "Public" - summary: "Get a list of all versions from an edition of a dataset" + summary: "Get a list of versions of an edition" description: "Get a list of all versions for an edition of a dataset" parameters: - $ref: '#/parameters/edition' @@ -344,7 +326,7 @@ paths: put: tags: - "Private user" - summary: "Update a version for an edition of a dataset" + summary: "Update a version" description: "Update a version for an edition of a dataset, if the state is changed to associated or published, the parent documents(dataset and edition resources) will also be updated. A version can only be updated if the state is not published" parameters: - $ref: '#/parameters/id' @@ -372,8 +354,8 @@ paths: get: tags: - "Public" - summary: "Get a specific version and edition of a dataset" - description: "Get a specific version and edition of a dataset" + summary: "Get a version" + description: "Get a specific version of an edition of a dataset" parameters: - $ref: '#/parameters/edition' - $ref: '#/parameters/id' @@ -396,7 +378,7 @@ paths: get: tags: - "Public" - summary: "Get all dimensions from a dataset" + summary: "Get a list of dimensions from a dataset" description: "Get all dimensions which are used in the dataset" parameters: - $ref: '#/parameters/edition' @@ -423,7 +405,7 @@ paths: get: tags: - "Public" - summary: "Get all options from a dimension" + summary: "Get a list of options from a dimension" description: "Get a list of all options which appear in this dimension and dataset" parameters: - $ref: '#/parameters/dimension' @@ -452,7 +434,7 @@ paths: get: tags: - "Public" - summary: "Get metadata relevant to a version" + summary: "Get metadata for a version" description: "Get all metadata relevant to a version" parameters: - $ref: '#/parameters/edition' @@ -476,8 +458,11 @@ paths: get: tags: - "Public" - summary: "Get a specific observation from version" - description: "Get observation from a version of the dataset by providing a single dimension option for each dimension" + summary: "Get specific observations" + description: "Get observations from a version of the dataset. By providing + a single option for each dimension, a single observation will be returned. + A wildcard (*) can be provided for one dimension, to retrieve a list of + observations." parameters: - $ref: '#/parameters/edition' - $ref: '#/parameters/id' @@ -507,7 +492,7 @@ paths: get: tags: - "Private user" - summary: "Get a list of instances" + summary: "Get instances" description: "Get a list of instances which has been paged" parameters: - $ref: '#/parameters/limit' @@ -557,7 +542,7 @@ paths: get: tags: - "Private user" - summary: "Get Information about an instance" + summary: "Get an instance" description: "Get the current state of an instance, this includes all events which have happened." parameters: - $ref: '#/parameters/instance_id' @@ -579,7 +564,7 @@ paths: put: tags: - "Private" - summary: "Update an instances properties" + summary: "Update an instance" description: "Update an instance by providing an unique id and a set of properties to over write" parameters: - $ref: '#/parameters/instance_id' @@ -605,7 +590,7 @@ paths: get: tags: - "Private user" - summary: "Get all dimensions from an instance" + summary: "Get a list of dimensions for an instance" description: "Get all dimensions from an instance" parameters: - $ref: '#/parameters/instance_id' @@ -633,7 +618,7 @@ paths: post: tags: - "Private" - summary: "Create a new dimension" + summary: "Create a dimension" description: "Create a new dimension which is related to an instance" parameters: - $ref: '#/parameters/instance_id' @@ -653,7 +638,7 @@ paths: put: tags: - "Private user" - summary: "Update dimension description and/or label" + summary: "Update dimension" description: "Update the label and/or description of a dimension within an instance, by providing dimension name and properties to over write" parameters: - $ref: '#/parameters/instance_id' @@ -678,7 +663,7 @@ paths: get: tags: - "Private user" - summary: "Get all unique options from a dimension" + summary: "Get a list of options for a dimension" description: "Get all unique options from a dimension" parameters: - $ref: '#/parameters/instance_id' @@ -740,7 +725,7 @@ paths: put: tags: - "Private" - summary: "Increment the observation inserted count" + summary: "Increment the inserted observation count" description: "This will add to the number already store in the api" parameters: - $ref: '#/parameters/instance_id' @@ -762,7 +747,7 @@ paths: put: tags: - "Private" - summary: "Update the state of an import task in an instance" + summary: "Update import tasks for an instance" description: "The instance import process involves multiple tasks. This endpoint updates the state of an import task." parameters: - $ref: '#/parameters/instance_id' @@ -784,7 +769,7 @@ paths: put: tags: - "Private" - summary: "Add an option to a dimension within the instance" + summary: "Add an option to an instance dimension" description: | Add a option to a dimension which has been found in the V4 data parameters: From 659893ae275d81045e3e35e56a647feac9a0ec5a Mon Sep 17 00:00:00 2001 From: Eleanor Deal Date: Fri, 18 May 2018 15:15:28 +0100 Subject: [PATCH 02/34] Add x-label for more user friendly spec name --- swagger.yaml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/swagger.yaml b/swagger.yaml index 70cff20e..8780becb 100644 --- a/swagger.yaml +++ b/swagger.yaml @@ -5,7 +5,8 @@ info: Data in each version is broken down by `dimensions`, and a unique combination of dimension `options` in a version can be used to retrieve `observation` level data." version: "1.0.0" - title: "Find data" + title: "Dataset API" + x-label: "Find data" license: name: "Open Government Licence v3.0" url: "http://www.nationalarchives.gov.uk/doc/open-government-licence/version/3/" From d5412369db42d9e36858da2d0887a0d671d4cfdf Mon Sep 17 00:00:00 2001 From: Dave Llewellyn <439837@gmail.com> Date: Mon, 21 May 2018 08:18:28 +0100 Subject: [PATCH 03/34] added auditing to delete dataset --- api/api.go | 25 ++++++++++++++------ api/dataset.go | 57 +++++++++++++++++++++++++++++++-------------- apierrors/errors.go | 34 ++++++++++++++------------- 3 files changed, 76 insertions(+), 40 deletions(-) diff --git a/api/api.go b/api/api.go index 1ea53c52..e49705bf 100644 --- a/api/api.go +++ b/api/api.go @@ -38,12 +38,13 @@ const ( dimensionOptionDocType = "dimension-option" // audit actions - getDatasetsAction = "getDatasets" - getDatasetAction = "getDataset" - getEditionsAction = "getEditions" - getEditionAction = "getEdition" - getVersionsAction = "getVersions" - getVersionAction = "getVersion" + getDatasetsAction = "getDatasets" + getDatasetAction = "getDataset" + getEditionsAction = "getEditions" + getEditionAction = "getEdition" + getVersionsAction = "getVersions" + getVersionAction = "getVersion" + deleteDatasetAction = "deleteDataset" // audit results actionAttempted = "attempted" @@ -183,11 +184,21 @@ func handleErrorType(docType string, err error, w http.ResponseWriter) { switch docType { default: - if err == errs.ErrDatasetNotFound || err == errs.ErrEditionNotFound || err == errs.ErrVersionNotFound || err == errs.ErrDimensionNodeNotFound || err == errs.ErrInstanceNotFound { + if err == errs.ErrEditionNotFound || err == errs.ErrVersionNotFound || err == errs.ErrDimensionNodeNotFound || err == errs.ErrInstanceNotFound { http.Error(w, err.Error(), http.StatusNotFound) } else { http.Error(w, err.Error(), http.StatusInternalServerError) } + case "dataset": + if err == errs.ErrDatasetNotFound { + http.Error(w, err.Error(), http.StatusNotFound) + } else if err == errs.ErrDeleteDatasetNotFound { + http.Error(w, err.Error(), http.StatusNoContent) + } else if err == errs.ErrDeletePublishedDatasetForbidden { + http.Error(w, err.Error(), http.StatusForbidden) + } else { + http.Error(w, err.Error(), http.StatusInternalServerError) + } case "edition": if err == errs.ErrDatasetNotFound { http.Error(w, err.Error(), http.StatusNotFound) diff --git a/api/dataset.go b/api/dataset.go index 2d501916..4e49d7f7 100644 --- a/api/dataset.go +++ b/api/dataset.go @@ -285,34 +285,57 @@ func (api *DatasetAPI) publishDataset(currentDataset *models.DatasetUpdate, vers func (api *DatasetAPI) deleteDataset(w http.ResponseWriter, r *http.Request) { vars := mux.Vars(r) datasetID := vars["id"] + logData := log.Data{"dataset_id": datasetID} + auditParams := common.Params{"dataset_id": datasetID} - currentDataset, err := api.dataStore.Backend.GetDataset(datasetID) - if err == errs.ErrDatasetNotFound { - log.Debug("cannot delete dataset, it does not exist", log.Data{"dataset_id": datasetID}) - w.WriteHeader(http.StatusNoContent) // idempotent + if err := api.auditor.Record(r.Context(), deleteDatasetAction, actionAttempted, auditParams); err != nil { + handleAuditingFailure(w, err, logData) return } + + // attempt to delete the dataset. + err := func() error { + currentDataset, err := api.dataStore.Backend.GetDataset(datasetID) + + if err == errs.ErrDatasetNotFound { + log.Debug("cannot delete dataset, it does not exist", logData) + return errs.ErrDeleteDatasetNotFound + } + + if err != nil { + log.ErrorC("failed to run query for existing dataset", err, logData) + return err + } + + if currentDataset.Current != nil && currentDataset.Current.State == models.PublishedState { + log.ErrorC("unable to delete a published dataset", errs.ErrDeletePublishedDatasetForbidden, logData) + return errs.ErrDeletePublishedDatasetForbidden + } + + if err := api.dataStore.Backend.DeleteDataset(datasetID); err != nil { + log.ErrorC("failed to delete dataset", err, logData) + return err + } + log.Debug("dataset deleted successfully", logData) + return nil + }() + + // audit the outcome if err != nil { - log.ErrorC("failed to run query for existing dataset", err, log.Data{"dataset_id": datasetID}) + if err := api.auditor.Record(r.Context(), deleteDatasetAction, actionUnsuccessful, auditParams); err != nil { + handleAuditingFailure(w, err, logData) + return + } handleErrorType(datasetDocType, err, w) return } - if currentDataset.Current != nil && currentDataset.Current.State == models.PublishedState { - err = fmt.Errorf("forbidden - a published dataset cannot be deleted") - log.ErrorC("unable to delete a published dataset", err, log.Data{"dataset_id": datasetID}) - http.Error(w, err.Error(), http.StatusForbidden) - return - } - - if err := api.dataStore.Backend.DeleteDataset(datasetID); err != nil { - log.ErrorC("failed to delete dataset", err, log.Data{"dataset_id": datasetID}) - handleErrorType(datasetDocType, err, w) + if err := api.auditor.Record(r.Context(), deleteDatasetAction, actionSuccessful, auditParams); err != nil { + handleAuditingFailure(w, err, logData) return } - w.WriteHeader(http.StatusNoContent) - log.Debug("delete dataset", log.Data{"dataset_id": datasetID}) + log.Debug("delete dataset", logData) } func mapResults(results []models.DatasetUpdate) []*models.Dataset { diff --git a/apierrors/errors.go b/apierrors/errors.go index ea5acf56..c4a57b8b 100644 --- a/apierrors/errors.go +++ b/apierrors/errors.go @@ -4,20 +4,22 @@ import "errors" // Error messages for Dataset API var ( - ErrDatasetNotFound = errors.New("Dataset not found") - ErrEditionNotFound = errors.New("Edition not found") - ErrVersionNotFound = errors.New("Version not found") - ErrDimensionNodeNotFound = errors.New("Dimension node not found") - ErrDimensionNotFound = errors.New("Dimension not found") - ErrDimensionsNotFound = errors.New("Dimensions not found") - ErrInstanceNotFound = errors.New("Instance not found") - ErrUnauthorised = errors.New("Unauthorised access to API") - ErrNoAuthHeader = errors.New("No authentication header provided") - ErrResourceState = errors.New("Incorrect resource state") - ErrVersionMissingState = errors.New("Missing state from version") - ErrInternalServer = errors.New("internal error") - ErrObservationsNotFound = errors.New("No observations found") - ErrIndexOutOfRange = errors.New("index out of range") - ErrMissingVersionHeaders = errors.New("missing headers from version doc") - ErrTooManyWildcards = errors.New("only one wildcard (*) is allowed as a value in selected query parameters") + ErrDatasetNotFound = errors.New("Dataset not found") + ErrDeletePublishedDatasetForbidden = errors.New("Published dataset cannot be deleted") // attempting to delete a published dataset + ErrDeleteDatasetNotFound = errors.New("Dataset not found") // attempting to delete a dataset that does not exist + ErrEditionNotFound = errors.New("Edition not found") + ErrVersionNotFound = errors.New("Version not found") + ErrDimensionNodeNotFound = errors.New("Dimension node not found") + ErrDimensionNotFound = errors.New("Dimension not found") + ErrDimensionsNotFound = errors.New("Dimensions not found") + ErrInstanceNotFound = errors.New("Instance not found") + ErrUnauthorised = errors.New("Unauthorised access to API") + ErrNoAuthHeader = errors.New("No authentication header provided") + ErrResourceState = errors.New("Incorrect resource state") + ErrVersionMissingState = errors.New("Missing state from version") + ErrInternalServer = errors.New("internal error") + ErrObservationsNotFound = errors.New("No observations found") + ErrIndexOutOfRange = errors.New("index out of range") + ErrMissingVersionHeaders = errors.New("missing headers from version doc") + ErrTooManyWildcards = errors.New("only one wildcard (*) is allowed as a value in selected query parameters") ) From 007d2d79e62ce72993730455b5a7b8a7c79c54dd Mon Sep 17 00:00:00 2001 From: Dave Llewellyn <439837@gmail.com> Date: Mon, 21 May 2018 08:21:28 +0100 Subject: [PATCH 04/34] moved new errors to resolve merge conflict --- apierrors/errors.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apierrors/errors.go b/apierrors/errors.go index c4a57b8b..ad106723 100644 --- a/apierrors/errors.go +++ b/apierrors/errors.go @@ -5,8 +5,6 @@ import "errors" // Error messages for Dataset API var ( ErrDatasetNotFound = errors.New("Dataset not found") - ErrDeletePublishedDatasetForbidden = errors.New("Published dataset cannot be deleted") // attempting to delete a published dataset - ErrDeleteDatasetNotFound = errors.New("Dataset not found") // attempting to delete a dataset that does not exist ErrEditionNotFound = errors.New("Edition not found") ErrVersionNotFound = errors.New("Version not found") ErrDimensionNodeNotFound = errors.New("Dimension node not found") @@ -22,4 +20,6 @@ var ( ErrIndexOutOfRange = errors.New("index out of range") ErrMissingVersionHeaders = errors.New("missing headers from version doc") ErrTooManyWildcards = errors.New("only one wildcard (*) is allowed as a value in selected query parameters") + ErrDeletePublishedDatasetForbidden = errors.New("Published dataset cannot be deleted") + ErrDeleteDatasetNotFound = errors.New("Dataset not found") ) From 31e032e3b2c1d888a4a2bfd811447e9af8da347e Mon Sep 17 00:00:00 2001 From: Eleanor Deal Date: Mon, 21 May 2018 10:30:31 +0100 Subject: [PATCH 05/34] Remove x-label and make title more descriptive --- swagger.yaml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/swagger.yaml b/swagger.yaml index 8780becb..6ee7e744 100644 --- a/swagger.yaml +++ b/swagger.yaml @@ -5,8 +5,7 @@ info: Data in each version is broken down by `dimensions`, and a unique combination of dimension `options` in a version can be used to retrieve `observation` level data." version: "1.0.0" - title: "Dataset API" - x-label: "Find data" + title: "Explore our data" license: name: "Open Government Licence v3.0" url: "http://www.nationalarchives.gov.uk/doc/open-government-licence/version/3/" From 13340edfbdb1aaac2817243cea8c7767cf251d33 Mon Sep 17 00:00:00 2001 From: Dave Llewellyn <439837@gmail.com> Date: Mon, 21 May 2018 12:17:22 +0100 Subject: [PATCH 06/34] added test to cover delete dataset auditing successful --- api/dataset_test.go | 58 ++++++++++++++++++++++++++++++++++++++------- apierrors/errors.go | 4 ++-- 2 files changed, 51 insertions(+), 11 deletions(-) diff --git a/api/dataset_test.go b/api/dataset_test.go index a72a9392..74888230 100644 --- a/api/dataset_test.go +++ b/api/dataset_test.go @@ -725,10 +725,18 @@ func TestDeleteDatasetReturnsSuccessfully(t *testing.T) { }, } - api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, getMockAuditor(), genericMockedObservationStore) + auditorMock := getMockAuditor() + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditorMock, genericMockedObservationStore) api.router.ServeHTTP(w, r) + + calls := auditorMock.RecordCalls() + ap := common.Params{"dataset_id": "123"} + So(w.Code, ShouldEqual, http.StatusNoContent) + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], deleteDatasetAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], deleteDatasetAction, actionSuccessful, ap) So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) So(len(mockedDataStore.DeleteDatasetCalls()), ShouldEqual, 1) }) @@ -750,12 +758,20 @@ func TestDeleteDatasetReturnsError(t *testing.T) { }, } - api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, getMockAuditor(), genericMockedObservationStore) + auditorMock := getMockAuditor() + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditorMock, genericMockedObservationStore) api.router.ServeHTTP(w, r) + So(w.Code, ShouldEqual, http.StatusForbidden) So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) So(len(mockedDataStore.DeleteDatasetCalls()), ShouldEqual, 0) + + calls := auditorMock.RecordCalls() + ap := common.Params{"dataset_id": "123"} + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], deleteDatasetAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], deleteDatasetAction, actionUnsuccessful, ap) }) Convey("When the api cannot connect to datastore return an internal server error", t, func() { @@ -773,7 +789,8 @@ func TestDeleteDatasetReturnsError(t *testing.T) { }, } - api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, getMockAuditor(), genericMockedObservationStore) + auditorMock := getMockAuditor() + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditorMock, genericMockedObservationStore) api.router.ServeHTTP(w, r) So(w.Code, ShouldEqual, http.StatusInternalServerError) @@ -781,6 +798,13 @@ func TestDeleteDatasetReturnsError(t *testing.T) { So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) So(len(mockedDataStore.DeleteDatasetCalls()), ShouldEqual, 1) + + calls := auditorMock.RecordCalls() + ap := common.Params{"dataset_id": "123"} + + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], deleteDatasetAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], deleteDatasetAction, actionUnsuccessful, ap) }) Convey("When the dataset document cannot be found return status not found ", t, func() { @@ -798,13 +822,21 @@ func TestDeleteDatasetReturnsError(t *testing.T) { }, } - api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, getMockAuditor(), genericMockedObservationStore) + auditorMock := getMockAuditor() + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditorMock, genericMockedObservationStore) api.router.ServeHTTP(w, r) - So(w.Code, ShouldEqual, http.StatusNoContent) + So(w.Code, ShouldEqual, http.StatusNoContent) So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) So(len(mockedDataStore.UpdateDatasetCalls()), ShouldEqual, 0) + + calls := auditorMock.RecordCalls() + ap := common.Params{"dataset_id": "123"} + + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], deleteDatasetAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], deleteDatasetAction, actionUnsuccessful, ap) }) Convey("When the dataset document cannot be queried return status 500 ", t, func() { @@ -822,13 +854,20 @@ func TestDeleteDatasetReturnsError(t *testing.T) { }, } - api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, getMockAuditor(), genericMockedObservationStore) + auditorMock := getMockAuditor() + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditorMock, genericMockedObservationStore) api.router.ServeHTTP(w, r) - So(w.Code, ShouldEqual, http.StatusInternalServerError) + So(w.Code, ShouldEqual, http.StatusInternalServerError) So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) So(len(mockedDataStore.UpdateDatasetCalls()), ShouldEqual, 0) + + calls := auditorMock.RecordCalls() + ap := common.Params{"dataset_id": "123"} + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], deleteDatasetAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], deleteDatasetAction, actionUnsuccessful, ap) }) Convey("When the request is not authorised to delete the dataset return status not found", t, func() { @@ -839,15 +878,16 @@ func TestDeleteDatasetReturnsError(t *testing.T) { w := httptest.NewRecorder() mockedDataStore := &storetest.StorerMock{} - api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, getMockAuditor(), genericMockedObservationStore) + auditorMock := getMockAuditor() + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditorMock, genericMockedObservationStore) api.router.ServeHTTP(w, r) So(w.Code, ShouldEqual, http.StatusUnauthorized) So(w.Body.String(), ShouldResemble, "unauthenticated request\n") - So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 0) So(len(mockedDataStore.DeleteDatasetCalls()), ShouldEqual, 0) + So(len(auditorMock.RecordCalls()), ShouldEqual, 0) }) } diff --git a/apierrors/errors.go b/apierrors/errors.go index fcb3c613..62a96a31 100644 --- a/apierrors/errors.go +++ b/apierrors/errors.go @@ -20,6 +20,6 @@ var ( ErrIndexOutOfRange = errors.New("index out of range") ErrMissingVersionHeadersOrDimensions = errors.New("missing headers or dimensions or both from version doc") ErrTooManyWildcards = errors.New("only one wildcard (*) is allowed as a value in selected query parameters") - ErrDeletePublishedDatasetForbidden = errors.New("Published dataset cannot be deleted") - ErrDeleteDatasetNotFound = errors.New("Dataset not found") + ErrDeletePublishedDatasetForbidden = errors.New("a published dataset cannot be deleted") + ErrDeleteDatasetNotFound = errors.New("dataset not found") ) From b6b4b84a2bb5b6c3a877be6e2e88633a7b82fc73 Mon Sep 17 00:00:00 2001 From: Dave Llewellyn <439837@gmail.com> Date: Tue, 22 May 2018 08:15:53 +0100 Subject: [PATCH 07/34] added tests for delete dataset to audit errors --- api/dataset_test.go | 107 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 107 insertions(+) diff --git a/api/dataset_test.go b/api/dataset_test.go index 74888230..d8c6d883 100644 --- a/api/dataset_test.go +++ b/api/dataset_test.go @@ -891,6 +891,113 @@ func TestDeleteDatasetReturnsError(t *testing.T) { }) } +func TestDeleteDatasetAuditErrors(t *testing.T) { + t.Parallel() + + Convey("When auditing an attempt to delete a dataset errors then a 500 status is returned", t, func() { + r, err := createRequestWithAuth("DELETE", "http://localhost:22000/datasets/123", nil) + So(err, ShouldBeNil) + + w := httptest.NewRecorder() + mockedDataStore := &storetest.StorerMock{} + + auditorMock := &audit.AuditorServiceMock{ + RecordFunc: func(ctx context.Context, action string, result string, params common.Params) error { + return errors.New("audit error") + }, + } + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditorMock, genericMockedObservationStore) + + api.router.ServeHTTP(w, r) + + So(w.Code, ShouldEqual, http.StatusInternalServerError) + So(w.Body.String(), ShouldEqual, internalServerErr) + + calls := auditorMock.RecordCalls() + ap := common.Params{"dataset_id": "123"} + So(len(calls), ShouldEqual, 1) + verifyAuditRecordCalls(calls[0], deleteDatasetAction, actionAttempted, ap) + + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 0) + So(len(mockedDataStore.DeleteDatasetCalls()), ShouldEqual, 0) + }) + + Convey("When auditing delete dataset unsuccessful errors then a 500 status is returned", t, func() { + r, err := createRequestWithAuth("DELETE", "http://localhost:22000/datasets/123", nil) + So(err, ShouldBeNil) + + w := httptest.NewRecorder() + mockedDataStore := &storetest.StorerMock{ + GetDatasetFunc: func(string) (*models.DatasetUpdate, error) { + return nil, errs.ErrDatasetNotFound + }, + } + + auditorMock := &audit.AuditorServiceMock{ + RecordFunc: func(ctx context.Context, action string, result string, params common.Params) error { + if deleteDatasetAction == action && result == actionUnsuccessful { + return errors.New("audit error") + } + return nil + }, + } + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditorMock, genericMockedObservationStore) + + api.router.ServeHTTP(w, r) + + So(w.Code, ShouldEqual, http.StatusInternalServerError) + So(w.Body.String(), ShouldEqual, internalServerErr) + + calls := auditorMock.RecordCalls() + ap := common.Params{"dataset_id": "123"} + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], deleteDatasetAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], deleteDatasetAction, actionUnsuccessful, ap) + + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) + So(len(mockedDataStore.DeleteDatasetCalls()), ShouldEqual, 0) + }) + + Convey("When auditing delete dataset unsuccessful errors then a 500 status is returned", t, func() { + r, err := createRequestWithAuth("DELETE", "http://localhost:22000/datasets/123", nil) + So(err, ShouldBeNil) + + w := httptest.NewRecorder() + mockedDataStore := &storetest.StorerMock{ + GetDatasetFunc: func(string) (*models.DatasetUpdate, error) { + return &models.DatasetUpdate{Next: &models.Dataset{State: models.CreatedState}}, nil + }, + DeleteDatasetFunc: func(string) error { + return nil + }, + } + + auditorMock := &audit.AuditorServiceMock{ + RecordFunc: func(ctx context.Context, action string, result string, params common.Params) error { + if deleteDatasetAction == action && result == actionSuccessful { + return errors.New("audit error") + } + return nil + }, + } + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditorMock, genericMockedObservationStore) + + api.router.ServeHTTP(w, r) + + So(w.Code, ShouldEqual, http.StatusInternalServerError) + So(w.Body.String(), ShouldEqual, internalServerErr) + + calls := auditorMock.RecordCalls() + ap := common.Params{"dataset_id": "123"} + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], deleteDatasetAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], deleteDatasetAction, actionSuccessful, ap) + + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) + So(len(mockedDataStore.DeleteDatasetCalls()), ShouldEqual, 1) + }) +} + func getMockAuditor() *audit.AuditorServiceMock { return &audit.AuditorServiceMock{ RecordFunc: func(ctx context.Context, action string, result string, params common.Params) error { From 7c1952b6182ea46819ea8366aca8eb87e77f9ccd Mon Sep 17 00:00:00 2001 From: Dave Llewellyn <439837@gmail.com> Date: Tue, 22 May 2018 08:20:37 +0100 Subject: [PATCH 08/34] renamed convey statements to read more fluently --- api/dataset_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/api/dataset_test.go b/api/dataset_test.go index d8c6d883..6d011043 100644 --- a/api/dataset_test.go +++ b/api/dataset_test.go @@ -894,7 +894,7 @@ func TestDeleteDatasetReturnsError(t *testing.T) { func TestDeleteDatasetAuditErrors(t *testing.T) { t.Parallel() - Convey("When auditing an attempt to delete a dataset errors then a 500 status is returned", t, func() { + Convey("When auditing delete a dataset attempted returns an errors then a 500 status is returned", t, func() { r, err := createRequestWithAuth("DELETE", "http://localhost:22000/datasets/123", nil) So(err, ShouldBeNil) @@ -922,7 +922,7 @@ func TestDeleteDatasetAuditErrors(t *testing.T) { So(len(mockedDataStore.DeleteDatasetCalls()), ShouldEqual, 0) }) - Convey("When auditing delete dataset unsuccessful errors then a 500 status is returned", t, func() { + Convey("When auditing delete dataset unsuccessful returns an errors then a 500 status is returned", t, func() { r, err := createRequestWithAuth("DELETE", "http://localhost:22000/datasets/123", nil) So(err, ShouldBeNil) @@ -958,7 +958,7 @@ func TestDeleteDatasetAuditErrors(t *testing.T) { So(len(mockedDataStore.DeleteDatasetCalls()), ShouldEqual, 0) }) - Convey("When auditing delete dataset unsuccessful errors then a 500 status is returned", t, func() { + Convey("When auditing delete dataset successful returns an errors then a 500 status is returned", t, func() { r, err := createRequestWithAuth("DELETE", "http://localhost:22000/datasets/123", nil) So(err, ShouldBeNil) From 06c9476a56646ae548e6e7d4f1754d3956283a53 Mon Sep 17 00:00:00 2001 From: Dave Llewellyn <439837@gmail.com> Date: Wed, 23 May 2018 08:57:57 +0100 Subject: [PATCH 09/34] refactored audit error logging and updated tests to test all delete dataset audit failure cases --- api/api.go | 57 ++++++++++- api/dataset.go | 17 ++-- api/dataset_test.go | 224 +++++++++++++++++++++++++++++++++----------- 3 files changed, 230 insertions(+), 68 deletions(-) diff --git a/api/api.go b/api/api.go index 232cbfc8..3e4e7614 100644 --- a/api/api.go +++ b/api/api.go @@ -4,7 +4,6 @@ import ( "bytes" "context" "encoding/json" - "errors" "io/ioutil" "net/http" "time" @@ -18,12 +17,14 @@ import ( "github.com/ONSdigital/dp-dataset-api/url" "github.com/ONSdigital/go-ns/audit" "github.com/ONSdigital/go-ns/common" + "github.com/ONSdigital/go-ns/handlers/requestID" "github.com/ONSdigital/go-ns/healthcheck" "github.com/ONSdigital/go-ns/identity" "github.com/ONSdigital/go-ns/log" "github.com/ONSdigital/go-ns/server" "github.com/gorilla/mux" "github.com/justinas/alice" + "github.com/pkg/errors" ) var httpServer *server.Server @@ -51,7 +52,10 @@ const ( actionSuccessful = "successful" actionUnsuccessful = "unsuccessful" - auditError = "error while attempting to record audit event, failing request" + auditError = "error while attempting to record audit event, failing request" + actionAttemptedAuditErr = "failed to audit action attempted event, returning internal server error" + auditActionUnsuccessfulErr = "failed to audit action unsuccessful event" + auditActionSuccessfulErr = "failed to audit action successful event" ) // PublishCheck Checks if an version has been published @@ -356,6 +360,55 @@ func handleAuditingFailure(w http.ResponseWriter, err error, logData log.Data) { http.Error(w, "internal server error", http.StatusInternalServerError) } +func actionAttemptedAuditFailure(ctx context.Context, w http.ResponseWriter, auditedAction string, err error, logData log.Data) { + logData["auditAction"] = auditedAction + logData["result"] = actionAttempted + + if user := common.User(ctx); user != "" { + logData["user"] = user + } + + wrappedErr := errors.WithMessage(err, actionAttemptedAuditErr) + if reqID := requestID.Get(ctx); reqID != "" { + log.ErrorC(reqID, wrappedErr, logData) + } else { + log.Error(wrappedErr, logData) + } + http.Error(w, "internal server error", http.StatusInternalServerError) +} + +func auditActionUnsuccessfulFailure(ctx context.Context, auditedAction string, err error, logData log.Data) { + logData["action"] = auditedAction + logData["result"] = actionUnsuccessful + + if user := common.User(ctx); user != "" { + logData["user"] = user + } + wrappedErr := errors.WithMessage(err, auditActionUnsuccessfulErr) + if reqID := requestID.Get(ctx); reqID != "" { + log.ErrorC(reqID, wrappedErr, logData) + } else { + log.Error(wrappedErr, logData) + } + +} + +func auditActionSuccessfulFailure(ctx context.Context, auditedAction string, err error, logData log.Data) { + logData["auditAction"] = auditedAction + logData["auditResult"] = actionSuccessful + + if user := common.User(ctx); user != "" { + logData["user"] = user + } + + wrappedErr := errors.WithMessage(err, auditActionSuccessfulErr) + if reqID := requestID.Get(ctx); reqID != "" { + log.ErrorC(reqID, wrappedErr, logData) + } else { + log.Error(wrappedErr, logData) + } +} + // Close represents the graceful shutting down of the http server func Close(ctx context.Context) error { if err := httpServer.Shutdown(ctx); err != nil { diff --git a/api/dataset.go b/api/dataset.go index 4e49d7f7..c0a787ee 100644 --- a/api/dataset.go +++ b/api/dataset.go @@ -283,13 +283,14 @@ func (api *DatasetAPI) publishDataset(currentDataset *models.DatasetUpdate, vers } func (api *DatasetAPI) deleteDataset(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() vars := mux.Vars(r) datasetID := vars["id"] logData := log.Data{"dataset_id": datasetID} auditParams := common.Params{"dataset_id": datasetID} - if err := api.auditor.Record(r.Context(), deleteDatasetAction, actionAttempted, auditParams); err != nil { - handleAuditingFailure(w, err, logData) + if err := api.auditor.Record(ctx, deleteDatasetAction, actionAttempted, auditParams); err != nil { + actionAttemptedAuditFailure(ctx, w, deleteDatasetAction, err, logData) return } @@ -320,19 +321,17 @@ func (api *DatasetAPI) deleteDataset(w http.ResponseWriter, r *http.Request) { return nil }() - // audit the outcome if err != nil { - if err := api.auditor.Record(r.Context(), deleteDatasetAction, actionUnsuccessful, auditParams); err != nil { - handleAuditingFailure(w, err, logData) - return + if err := api.auditor.Record(ctx, deleteDatasetAction, actionUnsuccessful, auditParams); err != nil { + auditActionUnsuccessfulFailure(ctx, deleteDatasetAction, err, logData) } handleErrorType(datasetDocType, err, w) return } - if err := api.auditor.Record(r.Context(), deleteDatasetAction, actionSuccessful, auditParams); err != nil { - handleAuditingFailure(w, err, logData) - return + if err := api.auditor.Record(ctx, deleteDatasetAction, actionSuccessful, auditParams); err != nil { + auditActionSuccessfulFailure(ctx, deleteDatasetAction, err, logData) + // fall through and return the origin status code as the action has been carried out at this point. } w.WriteHeader(http.StatusNoContent) log.Debug("delete dataset", logData) diff --git a/api/dataset_test.go b/api/dataset_test.go index 6d011043..0805eeea 100644 --- a/api/dataset_test.go +++ b/api/dataset_test.go @@ -891,49 +891,44 @@ func TestDeleteDatasetReturnsError(t *testing.T) { }) } -func TestDeleteDatasetAuditErrors(t *testing.T) { +func TestDeleteDatasetAuditActionAttemptedError(t *testing.T) { t.Parallel() - - Convey("When auditing delete a dataset attempted returns an errors then a 500 status is returned", t, func() { - r, err := createRequestWithAuth("DELETE", "http://localhost:22000/datasets/123", nil) - So(err, ShouldBeNil) - - w := httptest.NewRecorder() - mockedDataStore := &storetest.StorerMock{} - + Convey("given audit action attempted returns an error", t, func() { auditorMock := &audit.AuditorServiceMock{ RecordFunc: func(ctx context.Context, action string, result string, params common.Params) error { return errors.New("audit error") }, } - api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditorMock, genericMockedObservationStore) - api.router.ServeHTTP(w, r) + Convey("when delete dataset is called", func() { + r, err := createRequestWithAuth("DELETE", "http://localhost:22000/datasets/123", nil) + So(err, ShouldBeNil) - So(w.Code, ShouldEqual, http.StatusInternalServerError) - So(w.Body.String(), ShouldEqual, internalServerErr) + w := httptest.NewRecorder() + mockedDataStore := &storetest.StorerMock{} + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditorMock, genericMockedObservationStore) - calls := auditorMock.RecordCalls() - ap := common.Params{"dataset_id": "123"} - So(len(calls), ShouldEqual, 1) - verifyAuditRecordCalls(calls[0], deleteDatasetAction, actionAttempted, ap) + api.router.ServeHTTP(w, r) - So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 0) - So(len(mockedDataStore.DeleteDatasetCalls()), ShouldEqual, 0) - }) + Convey("then a 500 status is returned", func() { + So(w.Code, ShouldEqual, http.StatusInternalServerError) + So(w.Body.String(), ShouldEqual, internalServerErr) - Convey("When auditing delete dataset unsuccessful returns an errors then a 500 status is returned", t, func() { - r, err := createRequestWithAuth("DELETE", "http://localhost:22000/datasets/123", nil) - So(err, ShouldBeNil) + calls := auditorMock.RecordCalls() + ap := common.Params{"dataset_id": "123"} + So(len(calls), ShouldEqual, 1) + verifyAuditRecordCalls(calls[0], deleteDatasetAction, actionAttempted, ap) - w := httptest.NewRecorder() - mockedDataStore := &storetest.StorerMock{ - GetDatasetFunc: func(string) (*models.DatasetUpdate, error) { - return nil, errs.ErrDatasetNotFound - }, - } + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 0) + So(len(mockedDataStore.DeleteDatasetCalls()), ShouldEqual, 0) + }) + }) + }) +} - auditorMock := &audit.AuditorServiceMock{ +func TestDeleteDatasetAuditActionUnsuccessfulError(t *testing.T) { + getAuditor := func() *audit.AuditorServiceMock { + return &audit.AuditorServiceMock{ RecordFunc: func(ctx context.Context, action string, result string, params common.Params) error { if deleteDatasetAction == action && result == actionUnsuccessful { return errors.New("audit error") @@ -941,24 +936,137 @@ func TestDeleteDatasetAuditErrors(t *testing.T) { return nil }, } - api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditorMock, genericMockedObservationStore) + } - api.router.ServeHTTP(w, r) + Convey("given auditing action unsuccessful returns an errors", t, func() { + auditorMock := getAuditor() - So(w.Code, ShouldEqual, http.StatusInternalServerError) - So(w.Body.String(), ShouldEqual, internalServerErr) + Convey("when attempting to delete a dataset that does not exist", func() { - calls := auditorMock.RecordCalls() - ap := common.Params{"dataset_id": "123"} - So(len(calls), ShouldEqual, 2) - verifyAuditRecordCalls(calls[0], deleteDatasetAction, actionAttempted, ap) - verifyAuditRecordCalls(calls[1], deleteDatasetAction, actionUnsuccessful, ap) + r, err := createRequestWithAuth("DELETE", "http://localhost:22000/datasets/123", nil) + So(err, ShouldBeNil) - So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) - So(len(mockedDataStore.DeleteDatasetCalls()), ShouldEqual, 0) + w := httptest.NewRecorder() + mockedDataStore := &storetest.StorerMock{ + GetDatasetFunc: func(string) (*models.DatasetUpdate, error) { + return nil, errs.ErrDatasetNotFound + }, + } + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditorMock, genericMockedObservationStore) + + api.router.ServeHTTP(w, r) + + Convey("then a 204 status is returned", func() { + So(w.Code, ShouldEqual, http.StatusNoContent) + + calls := auditorMock.RecordCalls() + ap := common.Params{"dataset_id": "123"} + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], deleteDatasetAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], deleteDatasetAction, actionUnsuccessful, ap) + + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) + So(len(mockedDataStore.DeleteDatasetCalls()), ShouldEqual, 0) + }) + }) + + Convey("when dataStore.Backend.GetDataset returns an error", func() { + auditorMock = getAuditor() + + r, err := createRequestWithAuth("DELETE", "http://localhost:22000/datasets/123", nil) + So(err, ShouldBeNil) + + w := httptest.NewRecorder() + mockedDataStore := &storetest.StorerMock{ + GetDatasetFunc: func(string) (*models.DatasetUpdate, error) { + return nil, errors.New("dataStore.Backend.GetDataset error") + }, + } + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditorMock, genericMockedObservationStore) + + api.router.ServeHTTP(w, r) + + Convey("then a 500 status is returned", func() { + So(w.Code, ShouldEqual, http.StatusInternalServerError) + + calls := auditorMock.RecordCalls() + ap := common.Params{"dataset_id": "123"} + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], deleteDatasetAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], deleteDatasetAction, actionUnsuccessful, ap) + + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) + So(len(mockedDataStore.DeleteDatasetCalls()), ShouldEqual, 0) + }) + }) + + Convey("when attempting to delete a published dataset", func() { + auditorMock = getAuditor() + + r, err := createRequestWithAuth("DELETE", "http://localhost:22000/datasets/123", nil) + So(err, ShouldBeNil) + + w := httptest.NewRecorder() + mockedDataStore := &storetest.StorerMock{ + GetDatasetFunc: func(string) (*models.DatasetUpdate, error) { + return &models.DatasetUpdate{Current: &models.Dataset{State: models.PublishedState}}, nil + }, + } + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditorMock, genericMockedObservationStore) + + api.router.ServeHTTP(w, r) + + Convey("then a 403 status is returned", func() { + So(w.Code, ShouldEqual, http.StatusForbidden) + + calls := auditorMock.RecordCalls() + ap := common.Params{"dataset_id": "123"} + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], deleteDatasetAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], deleteDatasetAction, actionUnsuccessful, ap) + + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) + So(len(mockedDataStore.DeleteDatasetCalls()), ShouldEqual, 0) + }) + }) + + Convey("when dataStore.Backend.DeleteDataset returns an error", func() { + auditorMock = getAuditor() + + r, err := createRequestWithAuth("DELETE", "http://localhost:22000/datasets/123", nil) + So(err, ShouldBeNil) + + w := httptest.NewRecorder() + mockedDataStore := &storetest.StorerMock{ + GetDatasetFunc: func(string) (*models.DatasetUpdate, error) { + return &models.DatasetUpdate{Next: &models.Dataset{State: models.CompletedState}}, nil + }, + DeleteDatasetFunc: func(ID string) error { + return errors.New("DeleteDatasetFunc error") + }, + } + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditorMock, genericMockedObservationStore) + + api.router.ServeHTTP(w, r) + + Convey("then a 500 status is returned", func() { + So(w.Code, ShouldEqual, http.StatusInternalServerError) + + calls := auditorMock.RecordCalls() + ap := common.Params{"dataset_id": "123"} + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], deleteDatasetAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], deleteDatasetAction, actionUnsuccessful, ap) + + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) + So(len(mockedDataStore.DeleteDatasetCalls()), ShouldEqual, 1) + }) + }) }) +} - Convey("When auditing delete dataset successful returns an errors then a 500 status is returned", t, func() { +func TestDeleteDatasetAuditActionSuccessfulError(t *testing.T) { + Convey("given audit action successful returns an error", t, func() { r, err := createRequestWithAuth("DELETE", "http://localhost:22000/datasets/123", nil) So(err, ShouldBeNil) @@ -980,21 +1088,23 @@ func TestDeleteDatasetAuditErrors(t *testing.T) { return nil }, } - api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditorMock, genericMockedObservationStore) - - api.router.ServeHTTP(w, r) - - So(w.Code, ShouldEqual, http.StatusInternalServerError) - So(w.Body.String(), ShouldEqual, internalServerErr) - - calls := auditorMock.RecordCalls() - ap := common.Params{"dataset_id": "123"} - So(len(calls), ShouldEqual, 2) - verifyAuditRecordCalls(calls[0], deleteDatasetAction, actionAttempted, ap) - verifyAuditRecordCalls(calls[1], deleteDatasetAction, actionSuccessful, ap) - - So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) - So(len(mockedDataStore.DeleteDatasetCalls()), ShouldEqual, 1) + Convey("when delete dataset is called", func() { + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditorMock, genericMockedObservationStore) + api.router.ServeHTTP(w, r) + + Convey("then a 204 status is returned", func() { + So(w.Code, ShouldEqual, http.StatusNoContent) + + calls := auditorMock.RecordCalls() + ap := common.Params{"dataset_id": "123"} + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], deleteDatasetAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], deleteDatasetAction, actionSuccessful, ap) + + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) + So(len(mockedDataStore.DeleteDatasetCalls()), ShouldEqual, 1) + }) + }) }) } From 59f61712f4cad76804a173540534feeb27d3aa3d Mon Sep 17 00:00:00 2001 From: Dave Llewellyn <439837@gmail.com> Date: Wed, 23 May 2018 12:55:00 +0100 Subject: [PATCH 10/34] refactor audit errorhandling and refactored getdatasets to use better approach to auditing --- api/api.go | 45 ++++------------------ api/dataset.go | 91 ++++++++++++++++++++++++--------------------- api/dataset_test.go | 9 ++--- apierrors/errors.go | 1 + 4 files changed, 60 insertions(+), 86 deletions(-) diff --git a/api/api.go b/api/api.go index 3e4e7614..473b1198 100644 --- a/api/api.go +++ b/api/api.go @@ -52,10 +52,8 @@ const ( actionSuccessful = "successful" actionUnsuccessful = "unsuccessful" - auditError = "error while attempting to record audit event, failing request" - actionAttemptedAuditErr = "failed to audit action attempted event, returning internal server error" - auditActionUnsuccessfulErr = "failed to audit action unsuccessful event" - auditActionSuccessfulErr = "failed to audit action successful event" + auditError = "error while attempting to record audit event, failing request" + auditActionErr = "failed to audit action" ) // PublishCheck Checks if an version has been published @@ -360,48 +358,19 @@ func handleAuditingFailure(w http.ResponseWriter, err error, logData log.Data) { http.Error(w, "internal server error", http.StatusInternalServerError) } -func actionAttemptedAuditFailure(ctx context.Context, w http.ResponseWriter, auditedAction string, err error, logData log.Data) { - logData["auditAction"] = auditedAction - logData["result"] = actionAttempted - - if user := common.User(ctx); user != "" { - logData["user"] = user - } - - wrappedErr := errors.WithMessage(err, actionAttemptedAuditErr) - if reqID := requestID.Get(ctx); reqID != "" { - log.ErrorC(reqID, wrappedErr, logData) - } else { - log.Error(wrappedErr, logData) - } - http.Error(w, "internal server error", http.StatusInternalServerError) -} - -func auditActionUnsuccessfulFailure(ctx context.Context, auditedAction string, err error, logData log.Data) { - logData["action"] = auditedAction - logData["result"] = actionUnsuccessful - - if user := common.User(ctx); user != "" { - logData["user"] = user - } - wrappedErr := errors.WithMessage(err, auditActionUnsuccessfulErr) - if reqID := requestID.Get(ctx); reqID != "" { - log.ErrorC(reqID, wrappedErr, logData) - } else { - log.Error(wrappedErr, logData) +func auditActionFailure(ctx context.Context, auditedAction string, auditedResult string, err error, logData log.Data) { + if logData == nil { + logData = log.Data{} } -} - -func auditActionSuccessfulFailure(ctx context.Context, auditedAction string, err error, logData log.Data) { logData["auditAction"] = auditedAction - logData["auditResult"] = actionSuccessful + logData["auditResult"] = auditedResult if user := common.User(ctx); user != "" { logData["user"] = user } - wrappedErr := errors.WithMessage(err, auditActionSuccessfulErr) + wrappedErr := errors.WithMessage(err, auditActionErr) if reqID := requestID.Get(ctx); reqID != "" { log.ErrorC(reqID, wrappedErr, logData) } else { diff --git a/api/dataset.go b/api/dataset.go index c0a787ee..96b56d57 100644 --- a/api/dataset.go +++ b/api/dataset.go @@ -14,53 +14,57 @@ import ( ) func (api *DatasetAPI) getDatasets(w http.ResponseWriter, r *http.Request) { - if err := api.auditor.Record(r.Context(), getDatasetsAction, actionAttempted, nil); err != nil { - handleAuditingFailure(w, err, nil) - return - } - - results, err := api.dataStore.Backend.GetDatasets() - if err != nil { - log.Error(err, nil) - if auditErr := api.auditor.Record(r.Context(), getDatasetsAction, actionUnsuccessful, nil); auditErr != nil { - handleAuditingFailure(w, auditErr, nil) - return - } - handleErrorType(datasetDocType, err, w) + ctx := r.Context() + if err := api.auditor.Record(ctx, getDatasetsAction, actionAttempted, nil); err != nil { + auditActionFailure(ctx, getDatasetsAction, actionAttempted, err, nil) + handleErrorType(datasetDocType, errs.ErrAuditActionAttemptedFailure, w) return } - authorised, logData := api.authenticate(r, log.Data{}) - - var b []byte - if authorised { - - // User has valid authentication to get raw dataset document - datasets := &models.DatasetUpdateResults{} - datasets.Items = results - b, err = json.Marshal(datasets) + b, err := func() ([]byte, error) { + results, err := api.dataStore.Backend.GetDatasets() if err != nil { - log.ErrorC("failed to marshal dataset resource into bytes", err, nil) - handleErrorType(datasetDocType, err, w) - return + log.Error(err, nil) + return nil, err } - } else { - - // User is not authenticated and hence has only access to current sub document - datasets := &models.DatasetResults{} - datasets.Items = mapResults(results) + authorised, logData := api.authenticate(r, log.Data{}) + + var b []byte + if authorised { + + // User has valid authentication to get raw dataset document + datasets := &models.DatasetUpdateResults{} + datasets.Items = results + b, err = json.Marshal(datasets) + if err != nil { + log.ErrorC("failed to marshal dataset resource into bytes", err, logData) + return nil, err + } + } else { + + // User is not authenticated and hence has only access to current sub document + datasets := &models.DatasetResults{} + datasets.Items = mapResults(results) + + b, err = json.Marshal(datasets) + if err != nil { + log.ErrorC("failed to marshal dataset resource into bytes", err, logData) + return nil, err + } + } + return b, err + }() - b, err = json.Marshal(datasets) - if err != nil { - log.ErrorC("failed to marshal dataset resource into bytes", err, nil) - handleErrorType(datasetDocType, err, w) - return + if err != nil { + if auditErr := api.auditor.Record(ctx, getDatasetsAction, actionUnsuccessful, nil); auditErr != nil { + auditActionFailure(ctx, getDatasetsAction, actionUnsuccessful, auditErr, nil) } + handleErrorType(datasetDocType, err, w) + return } - if auditErr := api.auditor.Record(r.Context(), getDatasetsAction, actionSuccessful, nil); auditErr != nil { - handleAuditingFailure(w, auditErr, logData) - return + if auditErr := api.auditor.Record(ctx, getDatasetsAction, actionSuccessful, nil); auditErr != nil { + auditActionFailure(ctx, getDatasetsAction, actionSuccessful, auditErr, nil) } setJSONContentType(w) @@ -69,7 +73,7 @@ func (api *DatasetAPI) getDatasets(w http.ResponseWriter, r *http.Request) { log.Error(err, nil) http.Error(w, err.Error(), http.StatusInternalServerError) } - log.Debug("get all datasets", logData) + log.Debug("get all datasets", nil) } func (api *DatasetAPI) getDataset(w http.ResponseWriter, r *http.Request) { @@ -290,7 +294,8 @@ func (api *DatasetAPI) deleteDataset(w http.ResponseWriter, r *http.Request) { auditParams := common.Params{"dataset_id": datasetID} if err := api.auditor.Record(ctx, deleteDatasetAction, actionAttempted, auditParams); err != nil { - actionAttemptedAuditFailure(ctx, w, deleteDatasetAction, err, logData) + auditActionFailure(ctx, deleteDatasetAction, actionAttempted, err, logData) + handleErrorType(datasetDocType, err, w) return } @@ -322,15 +327,15 @@ func (api *DatasetAPI) deleteDataset(w http.ResponseWriter, r *http.Request) { }() if err != nil { - if err := api.auditor.Record(ctx, deleteDatasetAction, actionUnsuccessful, auditParams); err != nil { - auditActionUnsuccessfulFailure(ctx, deleteDatasetAction, err, logData) + if auditErr := api.auditor.Record(ctx, deleteDatasetAction, actionUnsuccessful, auditParams); auditErr != nil { + auditActionFailure(ctx, deleteDatasetAction, actionUnsuccessful, auditErr, logData) } handleErrorType(datasetDocType, err, w) return } if err := api.auditor.Record(ctx, deleteDatasetAction, actionSuccessful, auditParams); err != nil { - auditActionSuccessfulFailure(ctx, deleteDatasetAction, err, logData) + auditActionFailure(ctx, deleteDatasetAction, actionSuccessful, err, logData) // fall through and return the origin status code as the action has been carried out at this point. } w.WriteHeader(http.StatusNoContent) diff --git a/api/dataset_test.go b/api/dataset_test.go index 0805eeea..5e3c0f87 100644 --- a/api/dataset_test.go +++ b/api/dataset_test.go @@ -7,6 +7,7 @@ import ( "io" "net/http" "net/http/httptest" + "strings" "testing" "time" @@ -152,7 +153,7 @@ func TestGetDatasetsReturnsErrorIfAuditAttemptFails(t *testing.T) { api.router.ServeHTTP(w, r) So(w.Code, ShouldEqual, http.StatusInternalServerError) - So(w.Body.String(), ShouldEqual, internalServerErr) + So(strings.TrimSpace(w.Body.String()), ShouldEqual, errInternal.Error()) recCalls := auditMock.RecordCalls() So(len(recCalls), ShouldEqual, 2) @@ -191,7 +192,7 @@ func TestGetDatasetsReturnsError(t *testing.T) { func TestGetDatasetsAuditActionSuccessfulError(t *testing.T) { t.Parallel() - Convey("when a successful request to get dataset fails to audit action successful then a 500 response is returned", t, func() { + Convey("when a successful request to get dataset fails to audit action successful then a 200 response is returned", t, func() { r := httptest.NewRequest("GET", "http://localhost:22000/datasets", nil) w := httptest.NewRecorder() mockedDataStore := &storetest.StorerMock{ @@ -217,8 +218,7 @@ func TestGetDatasetsAuditActionSuccessfulError(t *testing.T) { verifyAuditRecordCalls(recCalls[1], getDatasetsAction, actionSuccessful, nil) So(len(mockedDataStore.GetDatasetsCalls()), ShouldEqual, 1) - So(w.Code, ShouldEqual, http.StatusInternalServerError) - So(w.Body.String(), ShouldEqual, internalServerErr) + So(w.Code, ShouldEqual, http.StatusOK) }) } @@ -912,7 +912,6 @@ func TestDeleteDatasetAuditActionAttemptedError(t *testing.T) { Convey("then a 500 status is returned", func() { So(w.Code, ShouldEqual, http.StatusInternalServerError) - So(w.Body.String(), ShouldEqual, internalServerErr) calls := auditorMock.RecordCalls() ap := common.Params{"dataset_id": "123"} diff --git a/apierrors/errors.go b/apierrors/errors.go index 62a96a31..e14998fa 100644 --- a/apierrors/errors.go +++ b/apierrors/errors.go @@ -22,4 +22,5 @@ var ( ErrTooManyWildcards = errors.New("only one wildcard (*) is allowed as a value in selected query parameters") ErrDeletePublishedDatasetForbidden = errors.New("a published dataset cannot be deleted") ErrDeleteDatasetNotFound = errors.New("dataset not found") + ErrAuditActionAttemptedFailure = errors.New("internal server error") ) From f1f541d3ab29d7b7bd1d6e553baabe50b0f34f82 Mon Sep 17 00:00:00 2001 From: Dave Llewellyn <439837@gmail.com> Date: Wed, 23 May 2018 16:41:19 +0100 Subject: [PATCH 11/34] implemented new auditing pattern in getdataset endpoint --- api/api.go | 5 +- api/dataset.go | 86 ++++++++++++++++++--------------- api/dataset_test.go | 113 ++++++++++++++++++++++++-------------------- 3 files changed, 112 insertions(+), 92 deletions(-) diff --git a/api/api.go b/api/api.go index 473b1198..d59a4cc1 100644 --- a/api/api.go +++ b/api/api.go @@ -370,11 +370,10 @@ func auditActionFailure(ctx context.Context, auditedAction string, auditedResult logData["user"] = user } - wrappedErr := errors.WithMessage(err, auditActionErr) if reqID := requestID.Get(ctx); reqID != "" { - log.ErrorC(reqID, wrappedErr, logData) + log.ErrorC(reqID, errors.WithMessage(err, auditActionErr), logData) } else { - log.Error(wrappedErr, logData) + log.Error(errors.WithMessage(err, auditActionErr), logData) } } diff --git a/api/dataset.go b/api/dataset.go index 96b56d57..759d44f4 100644 --- a/api/dataset.go +++ b/api/dataset.go @@ -9,8 +9,10 @@ import ( errs "github.com/ONSdigital/dp-dataset-api/apierrors" "github.com/ONSdigital/dp-dataset-api/models" "github.com/ONSdigital/go-ns/common" + "github.com/ONSdigital/go-ns/handlers/requestID" "github.com/ONSdigital/go-ns/log" "github.com/gorilla/mux" + "github.com/pkg/errors" ) func (api *DatasetAPI) getDatasets(w http.ResponseWriter, r *http.Request) { @@ -77,61 +79,67 @@ func (api *DatasetAPI) getDatasets(w http.ResponseWriter, r *http.Request) { } func (api *DatasetAPI) getDataset(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() vars := mux.Vars(r) id := vars["id"] logData := log.Data{"dataset_id": id} auditParams := common.Params{"dataset_id": id} - if auditErr := api.auditor.Record(r.Context(), getDatasetAction, actionAttempted, auditParams); auditErr != nil { - handleAuditingFailure(w, auditErr, logData) + if auditErr := api.auditor.Record(ctx, getDatasetAction, actionAttempted, auditParams); auditErr != nil { + auditActionFailure(ctx, getDatasetAction, actionAttempted, auditErr, logData) + handleErrorType(datasetDocType, errs.ErrInternalServer, w) return } - dataset, err := api.dataStore.Backend.GetDataset(id) - if err != nil { - log.Error(err, logData) - if auditErr := api.auditor.Record(r.Context(), getDatasetAction, actionUnsuccessful, auditParams); auditErr != nil { - handleAuditingFailure(w, auditErr, logData) - return + b, err := func() ([]byte, error) { + dataset, err := api.dataStore.Backend.GetDataset(id) + if err != nil { + log.ErrorC(requestID.Get(ctx), errors.WithMessage(err, "getDataset dataStore.Backend.GetDataset returned an error"), logData) + return nil, err } - handleErrorType(datasetDocType, err, w) - return - } - authorised, logData := api.authenticate(r, logData) + authorised, logData := api.authenticate(r, logData) - var b []byte - if !authorised { - // User is not authenticated and hence has only access to current sub document - if dataset.Current == nil { - log.Debug("published dataset not found", nil) - handleErrorType(datasetDocType, errs.ErrDatasetNotFound, w) - return - } + var b []byte + if !authorised { + // User is not authenticated and hence has only access to current sub document + if dataset.Current == nil { + log.Debug("published dataset not found", logData) + return nil, errs.ErrDatasetNotFound + } - dataset.Current.ID = dataset.ID - b, err = json.Marshal(dataset.Current) - if err != nil { - log.ErrorC("failed to marshal dataset current sub document resource into bytes", err, logData) - http.Error(w, err.Error(), http.StatusInternalServerError) - return - } - } else { - // User has valid authentication to get raw dataset document - if dataset == nil { - log.Debug("published or unpublished dataset not found", logData) - handleErrorType(datasetDocType, errs.ErrDatasetNotFound, w) + dataset.Current.ID = dataset.ID + b, err = json.Marshal(dataset.Current) + if err != nil { + log.ErrorC(requestID.Get(ctx), errors.WithMessage(err, "getDataset failed to marshal dataset current sub document resource into bytes"), logData) + return nil, err + } + } else { + // User has valid authentication to get raw dataset document + if dataset == nil { + log.Debug("published or unpublished dataset not found", logData) + return nil, errs.ErrDatasetNotFound + } + b, err = json.Marshal(dataset) + if err != nil { + log.ErrorC(requestID.Get(ctx), errors.WithMessage(err, "getDataset failed to marshal dataset current sub document resource into bytes"), logData) + return nil, err + } } - b, err = json.Marshal(dataset) - if err != nil { - log.ErrorC("failed to marshal dataset current sub document resource into bytes", err, logData) - http.Error(w, err.Error(), http.StatusInternalServerError) - return + return b, nil + }() + + if err != nil { + if auditErr := api.auditor.Record(ctx, getDatasetAction, actionUnsuccessful, auditParams); auditErr != nil { + auditActionFailure(ctx, getDatasetAction, actionUnsuccessful, auditErr, logData) } + handleErrorType(datasetDocType, err, w) + return } - if auditErr := api.auditor.Record(r.Context(), getDatasetAction, actionSuccessful, auditParams); auditErr != nil { - handleAuditingFailure(w, auditErr, logData) + if auditErr := api.auditor.Record(ctx, getDatasetAction, actionSuccessful, auditParams); auditErr != nil { + auditActionFailure(ctx, getDatasetAction, actionSuccessful, auditErr, logData) + handleErrorType(datasetDocType, errs.ErrInternalServer, w) return } diff --git a/api/dataset_test.go b/api/dataset_test.go index 5e3c0f87..c5df516d 100644 --- a/api/dataset_test.go +++ b/api/dataset_test.go @@ -339,32 +339,34 @@ func TestGetDatasetReturnsError(t *testing.T) { } func TestGetDatasetAuditingErrors(t *testing.T) { - Convey("when auditing get dataset attempt action returns an error then a 500 response is returned", t, func() { - r := httptest.NewRequest("GET", "http://localhost:22000/datasets/123-456", nil) - w := httptest.NewRecorder() - + Convey("given auditing attempted action returns an error", t, func() { auditMock := getMockAuditor() auditMock.RecordFunc = func(ctx context.Context, action string, result string, params common.Params) error { return errors.New("auditing error") } - mockDatastore := &storetest.StorerMock{} - api := GetAPIWithMockedDatastore(mockDatastore, &mocks.DownloadsGeneratorMock{}, auditMock, genericMockedObservationStore) - api.router.ServeHTTP(w, r) + Convey("when get dataset is called", func() { + r := httptest.NewRequest("GET", "http://localhost:22000/datasets/123-456", nil) + w := httptest.NewRecorder() - recCalls := auditMock.RecordCalls() - So(len(recCalls), ShouldEqual, 1) - verifyAuditRecordCalls(recCalls[0], getDatasetAction, actionAttempted, auditParams) + mockDatastore := &storetest.StorerMock{} + api := GetAPIWithMockedDatastore(mockDatastore, &mocks.DownloadsGeneratorMock{}, auditMock, genericMockedObservationStore) - So(w.Code, ShouldEqual, http.StatusInternalServerError) - So(w.Body.String(), ShouldEqual, internalServerErr) - So(len(mockDatastore.GetDatasetCalls()), ShouldEqual, 0) - }) + api.router.ServeHTTP(w, r) - Convey("when auditing get dataset successful action returns an error then a 500 response is returned", t, func() { - r := httptest.NewRequest("GET", "http://localhost:22000/datasets/123-456", nil) - w := httptest.NewRecorder() + Convey("then a 500 status is returned", func() { + recCalls := auditMock.RecordCalls() + So(len(recCalls), ShouldEqual, 1) + verifyAuditRecordCalls(recCalls[0], getDatasetAction, actionAttempted, auditParams) + So(w.Code, ShouldEqual, http.StatusInternalServerError) + So(strings.TrimSpace(w.Body.String()), ShouldEqual, errs.ErrInternalServer.Error()) + So(len(mockDatastore.GetDatasetCalls()), ShouldEqual, 0) + }) + }) + }) + + Convey("given audit action successful returns an error", t, func() { auditMock := getMockAuditor() auditMock.RecordFunc = func(ctx context.Context, action string, result string, params common.Params) error { if action == getDatasetAction && result == actionSuccessful { @@ -373,35 +375,33 @@ func TestGetDatasetAuditingErrors(t *testing.T) { return nil } - mockDatastore := &storetest.StorerMock{ - GetDatasetFunc: func(id string) (*models.DatasetUpdate, error) { - return &models.DatasetUpdate{ID: "123", Current: &models.Dataset{ID: "123"}}, nil - }, - } - api := GetAPIWithMockedDatastore(mockDatastore, &mocks.DownloadsGeneratorMock{}, auditMock, genericMockedObservationStore) - - api.router.ServeHTTP(w, r) + Convey("when get dataset is called", func() { + r := httptest.NewRequest("GET", "http://localhost:22000/datasets/123-456", nil) + w := httptest.NewRecorder() - recCalls := auditMock.RecordCalls() - So(len(recCalls), ShouldEqual, 2) - verifyAuditRecordCalls(recCalls[0], getDatasetAction, actionAttempted, auditParams) - verifyAuditRecordCalls(recCalls[1], getDatasetAction, actionSuccessful, auditParams) + mockDatastore := &storetest.StorerMock{ + GetDatasetFunc: func(id string) (*models.DatasetUpdate, error) { + return &models.DatasetUpdate{ID: "123", Current: &models.Dataset{ID: "123"}}, nil + }, + } + api := GetAPIWithMockedDatastore(mockDatastore, &mocks.DownloadsGeneratorMock{}, auditMock, genericMockedObservationStore) - So(len(mockDatastore.GetDatasetCalls()), ShouldEqual, 1) - So(w.Code, ShouldEqual, http.StatusInternalServerError) - So(w.Body.String(), ShouldEqual, internalServerErr) - }) + api.router.ServeHTTP(w, r) - Convey("when get dataset errors and auditing action unsuccessful errors then a 500 response is returned", t, func() { - r := httptest.NewRequest("GET", "http://localhost:22000/datasets/123-456", nil) - w := httptest.NewRecorder() + Convey("then a 500 status is returned", func() { + recCalls := auditMock.RecordCalls() + So(len(recCalls), ShouldEqual, 2) + verifyAuditRecordCalls(recCalls[0], getDatasetAction, actionAttempted, auditParams) + verifyAuditRecordCalls(recCalls[1], getDatasetAction, actionSuccessful, auditParams) - mockDatastore := &storetest.StorerMock{ - GetDatasetFunc: func(id string) (*models.DatasetUpdate, error) { - return nil, errors.New("get dataset error") - }, - } + So(len(mockDatastore.GetDatasetCalls()), ShouldEqual, 1) + So(w.Code, ShouldEqual, http.StatusInternalServerError) + So(strings.TrimSpace(w.Body.String()), ShouldEqual, errs.ErrInternalServer.Error()) + }) + }) + }) + Convey("given auditing action unsuccessful returns an error", t, func() { auditMock := getMockAuditor() auditMock.RecordFunc = func(ctx context.Context, action string, result string, params common.Params) error { if action == getDatasetAction && result == actionUnsuccessful { @@ -409,19 +409,32 @@ func TestGetDatasetAuditingErrors(t *testing.T) { } return nil } - api := GetAPIWithMockedDatastore(mockDatastore, &mocks.DownloadsGeneratorMock{}, auditMock, genericMockedObservationStore) - api.router.ServeHTTP(w, r) + Convey("when get dataset is called", func() { + r := httptest.NewRequest("GET", "http://localhost:22000/datasets/123-456", nil) + w := httptest.NewRecorder() - So(w.Code, ShouldEqual, http.StatusInternalServerError) - So(w.Body.String(), ShouldEqual, internalServerErr) + mockDatastore := &storetest.StorerMock{ + GetDatasetFunc: func(id string) (*models.DatasetUpdate, error) { + return nil, errors.New("get dataset error") + }, + } - recCalls := auditMock.RecordCalls() - So(len(recCalls), ShouldEqual, 2) - verifyAuditRecordCalls(recCalls[0], getDatasetAction, actionAttempted, auditParams) - verifyAuditRecordCalls(recCalls[1], getDatasetAction, actionUnsuccessful, auditParams) + api := GetAPIWithMockedDatastore(mockDatastore, &mocks.DownloadsGeneratorMock{}, auditMock, genericMockedObservationStore) + api.router.ServeHTTP(w, r) + + Convey("then a 500 status is returned", func() { + So(w.Code, ShouldEqual, http.StatusInternalServerError) + So(strings.TrimSpace(w.Body.String()), ShouldEqual, "get dataset error") + + recCalls := auditMock.RecordCalls() + So(len(recCalls), ShouldEqual, 2) + verifyAuditRecordCalls(recCalls[0], getDatasetAction, actionAttempted, auditParams) + verifyAuditRecordCalls(recCalls[1], getDatasetAction, actionUnsuccessful, auditParams) - So(len(mockDatastore.GetDatasetCalls()), ShouldEqual, 1) + So(len(mockDatastore.GetDatasetCalls()), ShouldEqual, 1) + }) + }) }) } From 6401598874b4302a538a929ff873f34fdd55aef9 Mon Sep 17 00:00:00 2001 From: Eleanor Deal Date: Thu, 24 May 2018 10:22:42 +0100 Subject: [PATCH 12/34] Remove offset and limit from specs where not valid parameters --- swagger.yaml | 28 ---------------------------- 1 file changed, 28 deletions(-) diff --git a/swagger.yaml b/swagger.yaml index 6ee7e744..10ce6f92 100644 --- a/swagger.yaml +++ b/swagger.yaml @@ -72,11 +72,6 @@ parameters: required: true schema: $ref: '#/definitions/Instance' - limit: - name: limit - description: "The number of resources to be returned" - in: query - type: integer new_dataset: name: dataset description: "A new dataset" @@ -104,11 +99,6 @@ parameters: in: path required: true type: string - offset: - name: offset - description: "The first row of resources to retrieve, starting at 0. Use this parameter as a pagination mechanism along with the limit parameter" - in: query - type: integer newInstance: name: instance description: "An instance related to an import job" @@ -116,11 +106,6 @@ parameters: required: true schema: $ref: '#/definitions/NewInstance' - query: - name: q - description: "The query text to search datasets with" - in: query - type: string state: name: "state" description: "A comma separated list of state values to filter on (e.g. ‘completed,edition-confirmed’)" @@ -167,9 +152,6 @@ paths: - "Public" summary: "Get a list of datasets" description: "Returns a list of all datasets provided by the ONS that can be filtered using the filter API" - parameters: - - $ref: '#/parameters/limit' - - $ref: '#/parameters/offset' produces: - "application/json" responses: @@ -264,8 +246,6 @@ paths: description: "Get a list of editions of a type of dataset" parameters: - $ref: '#/parameters/id' - - $ref: '#/parameters/limit' - - $ref: '#/parameters/offset' responses: 200: description: "A json list containing all editions for a dataset" @@ -306,8 +286,6 @@ paths: parameters: - $ref: '#/parameters/edition' - $ref: '#/parameters/id' - - $ref: '#/parameters/limit' - - $ref: '#/parameters/offset' responses: 200: description: "A json list containing all versions for a set type of dataset and edition" @@ -384,8 +362,6 @@ paths: - $ref: '#/parameters/edition' - $ref: '#/parameters/id' - $ref: '#/parameters/version' - - $ref: '#/parameters/limit' - - $ref: '#/parameters/offset' responses: 200: description: "A json list of dimensions" @@ -412,8 +388,6 @@ paths: - $ref: '#/parameters/edition' - $ref: '#/parameters/id' - $ref: '#/parameters/version' - - $ref: '#/parameters/limit' - - $ref: '#/parameters/offset' responses: 200: description: "Json object containing all options for a dimension" @@ -495,8 +469,6 @@ paths: summary: "Get instances" description: "Get a list of instances which has been paged" parameters: - - $ref: '#/parameters/limit' - - $ref: '#/parameters/offset' - $ref: '#/parameters/state' produces: - "application/json" From f153c20efd75769f422db808ab456a7561980dd4 Mon Sep 17 00:00:00 2001 From: mattrout92 Date: Thu, 24 May 2018 11:19:13 +0100 Subject: [PATCH 13/34] Use correct versions link href on dimensions endpoint --- api/dimensions.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/api/dimensions.go b/api/dimensions.go index 122c07d8..5d319999 100644 --- a/api/dimensions.go +++ b/api/dimensions.go @@ -91,7 +91,8 @@ func (api *DatasetAPI) createListOfDimensions(versionDoc *models.Version, dimens dimension.Links.CodeList = opt.Links.CodeList dimension.Links.Options = models.LinkObject{ID: opt.Name, HRef: fmt.Sprintf("%s/datasets/%s/editions/%s/versions/%s/dimensions/%s/options", api.host, versionDoc.Links.Dataset.ID, versionDoc.Edition, versionDoc.Links.Version.ID, opt.Name)} - dimension.Links.Version = *versionDoc.Links.Self + dimension.Links.Version = models.LinkObject{HRef: fmt.Sprintf("%s/datasets/%s/editions/%s/versions/%s", + api.host, versionDoc.Links.Dataset.ID, versionDoc.Edition, versionDoc.Links.Version.ID)} // Add description to dimension from hash map dimension.Description = dimensionDescriptions[dimension.Name] @@ -151,6 +152,11 @@ func (api *DatasetAPI) getDimensionOptions(w http.ResponseWriter, r *http.Reques return } + for i := range results.Items { + results.Items[i].Links.Version.HRef = fmt.Sprintf("%s/datasets/%s/editions/%s/versions/%s", + api.host, datasetID, editionID, versionID) + } + b, err := json.Marshal(results) if err != nil { log.ErrorC("failed to marshal list of dimension option resources into bytes", err, logData) From fa52f4b84114c881e0ebdb7f3c3bb948bcd18c60 Mon Sep 17 00:00:00 2001 From: Dave Llewellyn <439837@gmail.com> Date: Thu, 24 May 2018 11:29:50 +0100 Subject: [PATCH 14/34] addressed PR comments and added better logging to provide more context to errors --- api/api.go | 33 +++++++++++++++++++++++++++------ api/dataset.go | 11 ++++++----- 2 files changed, 33 insertions(+), 11 deletions(-) diff --git a/api/api.go b/api/api.go index 473b1198..6fa195fd 100644 --- a/api/api.go +++ b/api/api.go @@ -366,16 +366,37 @@ func auditActionFailure(ctx context.Context, auditedAction string, auditedResult logData["auditAction"] = auditedAction logData["auditResult"] = auditedResult + logError(ctx, errors.WithMessage(err, auditActionErr), logData) +} + +func logError(ctx context.Context, err error, data log.Data) { + if data == nil { + data = log.Data{} + } + reqID := requestID.Get(ctx) + if user := common.User(ctx); user != "" { + data["user"] = user + } + + if caller := common.Caller(ctx); caller != "" { + data["caller"] = caller + } + log.ErrorC(reqID, err, data) +} + +func logInfo(ctx context.Context, message string, data log.Data) { + if data == nil { + data = log.Data{} + } + reqID := requestID.Get(ctx) if user := common.User(ctx); user != "" { - logData["user"] = user + data["user"] = user } - wrappedErr := errors.WithMessage(err, auditActionErr) - if reqID := requestID.Get(ctx); reqID != "" { - log.ErrorC(reqID, wrappedErr, logData) - } else { - log.Error(wrappedErr, logData) + if caller := common.Caller(ctx); caller != "" { + data["caller"] = caller } + log.InfoC(reqID, message, data) } // Close represents the graceful shutting down of the http server diff --git a/api/dataset.go b/api/dataset.go index 96b56d57..f1da116f 100644 --- a/api/dataset.go +++ b/api/dataset.go @@ -11,6 +11,7 @@ import ( "github.com/ONSdigital/go-ns/common" "github.com/ONSdigital/go-ns/log" "github.com/gorilla/mux" + "github.com/pkg/errors" ) func (api *DatasetAPI) getDatasets(w http.ResponseWriter, r *http.Request) { @@ -24,7 +25,7 @@ func (api *DatasetAPI) getDatasets(w http.ResponseWriter, r *http.Request) { b, err := func() ([]byte, error) { results, err := api.dataStore.Backend.GetDatasets() if err != nil { - log.Error(err, nil) + logError(ctx, errors.WithMessage(err, "api endpoint getDatasets datastore.GetDatasets returned an error"), nil) return nil, err } authorised, logData := api.authenticate(r, log.Data{}) @@ -37,7 +38,7 @@ func (api *DatasetAPI) getDatasets(w http.ResponseWriter, r *http.Request) { datasets.Items = results b, err = json.Marshal(datasets) if err != nil { - log.ErrorC("failed to marshal dataset resource into bytes", err, logData) + logError(ctx, errors.WithMessage(err, "api endpoint getDatasets failed to marshal dataset resource into bytes"), logData) return nil, err } } else { @@ -48,7 +49,7 @@ func (api *DatasetAPI) getDatasets(w http.ResponseWriter, r *http.Request) { b, err = json.Marshal(datasets) if err != nil { - log.ErrorC("failed to marshal dataset resource into bytes", err, logData) + logError(ctx, errors.WithMessage(err, "api endpoint getDatasets failed to marshal dataset resource into bytes"), logData) return nil, err } } @@ -70,10 +71,10 @@ func (api *DatasetAPI) getDatasets(w http.ResponseWriter, r *http.Request) { setJSONContentType(w) _, err = w.Write(b) if err != nil { - log.Error(err, nil) + logError(ctx, errors.WithMessage(err, "api endpoint getDatasets error writing response body"), nil) http.Error(w, err.Error(), http.StatusInternalServerError) } - log.Debug("get all datasets", nil) + logInfo(ctx, "api endpoint getDatasets request successful", nil) } func (api *DatasetAPI) getDataset(w http.ResponseWriter, r *http.Request) { From b3aabfc2d3a91568d77aea080f5ecad174c76b55 Mon Sep 17 00:00:00 2001 From: Dave Llewellyn <439837@gmail.com> Date: Thu, 24 May 2018 16:00:48 +0100 Subject: [PATCH 15/34] added audit to addDataset endpoint and fixed unit test --- api/api.go | 5 +- api/dataset.go | 124 +++++++++++++++++++++++++------------------- api/dataset_test.go | 78 ++++++++++++++++++++++++++++ apierrors/errors.go | 2 + 4 files changed, 155 insertions(+), 54 deletions(-) diff --git a/api/api.go b/api/api.go index 6edd647d..6782737e 100644 --- a/api/api.go +++ b/api/api.go @@ -46,6 +46,7 @@ const ( getVersionsAction = "getVersions" getVersionAction = "getVersion" deleteDatasetAction = "deleteDataset" + addDatasetAction = "addDataset" // audit results actionAttempted = "attempted" @@ -196,8 +197,10 @@ func handleErrorType(docType string, err error, w http.ResponseWriter) { http.Error(w, err.Error(), http.StatusNotFound) } else if err == errs.ErrDeleteDatasetNotFound { http.Error(w, err.Error(), http.StatusNoContent) - } else if err == errs.ErrDeletePublishedDatasetForbidden { + } else if err == errs.ErrDeletePublishedDatasetForbidden || err == errs.ErrAddDatasetAlreadyExists { http.Error(w, err.Error(), http.StatusForbidden) + } else if err == errs.ErrAddDatasetBadRequest { + http.Error(w, err.Error(), http.StatusBadRequest) } else { http.Error(w, err.Error(), http.StatusInternalServerError) } diff --git a/api/dataset.go b/api/dataset.go index a1cfc496..2b8f9e53 100644 --- a/api/dataset.go +++ b/api/dataset.go @@ -9,7 +9,6 @@ import ( errs "github.com/ONSdigital/dp-dataset-api/apierrors" "github.com/ONSdigital/dp-dataset-api/models" "github.com/ONSdigital/go-ns/common" - "github.com/ONSdigital/go-ns/handlers/requestID" "github.com/ONSdigital/go-ns/log" "github.com/gorilla/mux" "github.com/pkg/errors" @@ -94,7 +93,7 @@ func (api *DatasetAPI) getDataset(w http.ResponseWriter, r *http.Request) { b, err := func() ([]byte, error) { dataset, err := api.dataStore.Backend.GetDataset(id) if err != nil { - log.ErrorC(requestID.Get(ctx), errors.WithMessage(err, "getDataset dataStore.Backend.GetDataset returned an error"), logData) + logError(ctx, errors.WithMessage(err, "getDataset endpoint: dataStore.Backend.GetDataset returned an error"), logData) return nil, err } @@ -104,25 +103,25 @@ func (api *DatasetAPI) getDataset(w http.ResponseWriter, r *http.Request) { if !authorised { // User is not authenticated and hence has only access to current sub document if dataset.Current == nil { - log.Debug("published dataset not found", logData) + logInfo(ctx, "getDataste endpoint: published dataset not found", logData) return nil, errs.ErrDatasetNotFound } dataset.Current.ID = dataset.ID b, err = json.Marshal(dataset.Current) if err != nil { - log.ErrorC(requestID.Get(ctx), errors.WithMessage(err, "getDataset failed to marshal dataset current sub document resource into bytes"), logData) + logError(ctx, errors.WithMessage(err, "getDataset endpoint: failed to marshal dataset current sub document resource into bytes"), logData) return nil, err } } else { // User has valid authentication to get raw dataset document if dataset == nil { - log.Debug("published or unpublished dataset not found", logData) + logInfo(ctx, "getDataset endpoint: published or unpublished dataset not found", logData) return nil, errs.ErrDatasetNotFound } b, err = json.Marshal(dataset) if err != nil { - log.ErrorC(requestID.Get(ctx), errors.WithMessage(err, "getDataset failed to marshal dataset current sub document resource into bytes"), logData) + logError(ctx, errors.WithMessage(err, "getDataset endpoint: failed to marshal dataset current sub document resource into bytes"), logData) return nil, err } } @@ -146,83 +145,102 @@ func (api *DatasetAPI) getDataset(w http.ResponseWriter, r *http.Request) { setJSONContentType(w) _, err = w.Write(b) if err != nil { - log.Error(err, logData) + logError(ctx, errors.WithMessage(err, "getDataset endpoint: error writing bytes to response"), logData) http.Error(w, err.Error(), http.StatusInternalServerError) } - log.Debug("get dataset", logData) + logInfo(ctx, "getDataset endpoint: request successful", logData) } func (api *DatasetAPI) addDataset(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() vars := mux.Vars(r) datasetID := vars["id"] - // TODO Could just do an insert, if dataset already existed we would get a duplicate key error - // instead of reading then writing doc - _, err := api.dataStore.Backend.GetDataset(datasetID) - if err != nil { - if err != errs.ErrDatasetNotFound { - log.ErrorC("failed to check if dataset exists", err, log.Data{"dataset_id": datasetID}) - handleErrorType(datasetDocType, err, w) - return - } - } else { - err = fmt.Errorf("forbidden - dataset already exists") - log.ErrorC("unable to create a dataset that already exists", err, log.Data{"dataset_id": datasetID}) - http.Error(w, err.Error(), http.StatusForbidden) - return - } + logData := log.Data{"dataset_id": datasetID} + auditParams := common.Params{"dataset_id": datasetID} - dataset, err := models.CreateDataset(r.Body) - if err != nil { - log.ErrorC("failed to model dataset resource based on request", err, log.Data{"dataset_id": datasetID}) - http.Error(w, err.Error(), http.StatusBadRequest) + if err := api.auditor.Record(ctx, addDatasetAction, actionAttempted, auditParams); err != nil { + auditActionFailure(ctx, addDatasetAction, actionAttempted, err, logData) + handleErrorType(datasetDocType, errs.ErrInternalServer, w) return } - defer r.Body.Close() - dataset.State = models.CreatedState - dataset.ID = datasetID + // TODO Could just do an insert, if dataset already existed we would get a duplicate key error instead of reading then writing doc + b, err := func() ([]byte, error) { + _, err := api.dataStore.Backend.GetDataset(datasetID) + if err != nil { + if err != errs.ErrDatasetNotFound { + logError(ctx, errors.WithMessage(err, "addDataset endpoint: error checking if dataset exists"), logData) + return nil, err + } + } else { + logError(ctx, errors.WithMessage(errs.ErrAddDatasetAlreadyExists, "addDataset endpoint: unable to create a dataset that already exists"), logData) + return nil, errs.ErrAddDatasetAlreadyExists + } - if dataset.Links == nil { - dataset.Links = &models.DatasetLinks{} - } + defer r.Body.Close() + dataset, err := models.CreateDataset(r.Body) + if err != nil { + logError(ctx, errors.WithMessage(err, "addDataset endpoint: failed to model dataset resource based on request"), logData) + return nil, errs.ErrAddDatasetBadRequest + } - dataset.Links.Editions = &models.LinkObject{ - HRef: fmt.Sprintf("%s/datasets/%s/editions", api.host, datasetID), - } + dataset.State = models.CreatedState + dataset.ID = datasetID - dataset.Links.Self = &models.LinkObject{ - HRef: fmt.Sprintf("%s/datasets/%s", api.host, datasetID), - } + if dataset.Links == nil { + dataset.Links = &models.DatasetLinks{} + } - dataset.LastUpdated = time.Now() + dataset.Links.Editions = &models.LinkObject{ + HRef: fmt.Sprintf("%s/datasets/%s/editions", api.host, datasetID), + } - datasetDoc := &models.DatasetUpdate{ - ID: datasetID, - Next: dataset, - } + dataset.Links.Self = &models.LinkObject{ + HRef: fmt.Sprintf("%s/datasets/%s", api.host, datasetID), + } + + dataset.LastUpdated = time.Now() + + datasetDoc := &models.DatasetUpdate{ + ID: datasetID, + Next: dataset, + } - if err = api.dataStore.Backend.UpsertDataset(datasetID, datasetDoc); err != nil { - log.ErrorC("failed to insert dataset resource to datastore", err, log.Data{"new_dataset": datasetID}) + if err = api.dataStore.Backend.UpsertDataset(datasetID, datasetDoc); err != nil { + logData["new_dataset"] = datasetID + logError(ctx, errors.WithMessage(err, "addDataset endpoint: failed to insert dataset resource to datastore"), logData) + return nil, err + } + + b, err := json.Marshal(datasetDoc) + if err != nil { + logError(ctx, errors.WithMessage(err, "addDataset endpoint: failed to marshal dataset resource into bytes"), logData) + return nil, err + } + return b, nil + }() + + if err != nil { + if auditErr := api.auditor.Record(ctx, addDatasetAction, actionUnsuccessful, auditParams); auditErr != nil { + auditActionFailure(ctx, addDatasetAction, actionUnsuccessful, auditErr, logData) + } handleErrorType(datasetDocType, err, w) return } - b, err := json.Marshal(datasetDoc) - if err != nil { - log.ErrorC("failed to marshal dataset resource into bytes", err, log.Data{"new_dataset": datasetID}) - w.WriteHeader(http.StatusInternalServerError) - return + if auditErr := api.auditor.Record(ctx, addDatasetAction, actionSuccessful, auditParams); auditErr != nil { + auditActionFailure(ctx, addDatasetAction, actionUnsuccessful, auditErr, logData) } setJSONContentType(w) w.WriteHeader(http.StatusCreated) _, err = w.Write(b) if err != nil { - log.Error(err, log.Data{"dataset_id": datasetID}) + logError(ctx, errors.WithMessage(err, "addDataset endpoint: error writing bytes to response"), logData) http.Error(w, err.Error(), http.StatusInternalServerError) } - log.Debug("upsert dataset", log.Data{"dataset_id": datasetID}) + logInfo(ctx, "addDataset endpoint: request completed successfully", logData) } func (api *DatasetAPI) putDataset(w http.ResponseWriter, r *http.Request) { diff --git a/api/dataset_test.go b/api/dataset_test.go index c5df516d..9702c512 100644 --- a/api/dataset_test.go +++ b/api/dataset_test.go @@ -575,6 +575,76 @@ func TestPostDatasetReturnsError(t *testing.T) { }) } +func TestPostDatasetAuditErrors(t *testing.T) { + ap := common.Params{"dataset_id": "123"} + + Convey("given audit action attempted returns an error", t, func() { + auditor := getMockAuditorFunc(func(a string, r string) error { + if a == addDatasetAction && r == actionAttempted { + return errors.New("auditing error") + } + return nil + }) + + Convey("when add dataset is called", func() { + r, err := createRequestWithAuth("POST", "http://localhost:22000/datasets/123", bytes.NewBufferString("{")) + So(err, ShouldBeNil) + + w := httptest.NewRecorder() + mockedDataStore := &storetest.StorerMock{} + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditor, genericMockedObservationStore) + api.router.ServeHTTP(w, r) + + Convey("then a 500 status is returned", func() { + So(w.Code, ShouldEqual, http.StatusInternalServerError) + So(strings.TrimSpace(w.Body.String()), ShouldEqual, errs.ErrInternalServer.Error()) + + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 0) + So(len(mockedDataStore.UpsertDatasetCalls()), ShouldEqual, 0) + + calls := auditor.RecordCalls() + So(len(calls), ShouldEqual, 1) + verifyAuditRecordCalls(calls[0], addDatasetAction, actionAttempted, ap) + }) + }) + }) + + Convey("given audit action unsuccessful returns an error", t, func() { + auditor := getMockAuditorFunc(func(a string, r string) error { + if a == addDatasetAction && r == actionUnsuccessful { + return errors.New("auditing error") + } + return nil + }) + + Convey("when datastore getdataset returns an error", func() { + r, err := createRequestWithAuth("POST", "http://localhost:22000/datasets/123", bytes.NewBufferString("{")) + So(err, ShouldBeNil) + w := httptest.NewRecorder() + + mockedDataStore := &storetest.StorerMock{ + GetDatasetFunc: func(string) (*models.DatasetUpdate, error) { + return nil, errors.New("get dataset error") + }, + } + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditor, genericMockedObservationStore) + api.router.ServeHTTP(w, r) + + Convey("then a 500 status is returned", func() { + So(w.Code, ShouldEqual, http.StatusInternalServerError) + So(strings.TrimSpace(w.Body.String()), ShouldEqual, "get dataset error") + + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) + So(len(mockedDataStore.UpsertDatasetCalls()), ShouldEqual, 0) + + calls := auditor.RecordCalls() + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], addDatasetAction, actionAttempted, ap) + }) + }) + }) +} + func TestPutDatasetReturnsSuccessfully(t *testing.T) { t.Parallel() Convey("A successful request to put dataset returns 200 OK response", t, func() { @@ -1127,3 +1197,11 @@ func getMockAuditor() *audit.AuditorServiceMock { }, } } + +func getMockAuditorFunc(f func(action string, result string) error) *audit.AuditorServiceMock { + return &audit.AuditorServiceMock{ + RecordFunc: func(ctx context.Context, action string, result string, params common.Params) error { + return f(action, result) + }, + } +} diff --git a/apierrors/errors.go b/apierrors/errors.go index e14998fa..8778f782 100644 --- a/apierrors/errors.go +++ b/apierrors/errors.go @@ -5,6 +5,8 @@ import "errors" // Error messages for Dataset API var ( ErrDatasetNotFound = errors.New("Dataset not found") + ErrAddDatasetAlreadyExists = errors.New("forbidden - dataset already exists") + ErrAddDatasetBadRequest = errors.New("Failed to parse json body") ErrEditionNotFound = errors.New("Edition not found") ErrVersionNotFound = errors.New("Version not found") ErrDimensionNodeNotFound = errors.New("Dimension node not found") From 3dbfd45047762c59cc50ab30786acfa57704ebc0 Mon Sep 17 00:00:00 2001 From: Dave Llewellyn <439837@gmail.com> Date: Thu, 24 May 2018 16:41:57 +0100 Subject: [PATCH 16/34] added audit action unsuccess tests for dataset exist and upsert error --- api/dataset_test.go | 57 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/api/dataset_test.go b/api/dataset_test.go index 9702c512..56c4ce80 100644 --- a/api/dataset_test.go +++ b/api/dataset_test.go @@ -642,6 +642,63 @@ func TestPostDatasetAuditErrors(t *testing.T) { verifyAuditRecordCalls(calls[0], addDatasetAction, actionAttempted, ap) }) }) + + Convey("when datastore getdataset returns an existing dataset", func() { + r, err := createRequestWithAuth("POST", "http://localhost:22000/datasets/123", bytes.NewBufferString("{")) + So(err, ShouldBeNil) + w := httptest.NewRecorder() + + mockedDataStore := &storetest.StorerMock{ + GetDatasetFunc: func(string) (*models.DatasetUpdate, error) { + return &models.DatasetUpdate{}, nil + }, + } + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditor, genericMockedObservationStore) + api.router.ServeHTTP(w, r) + + Convey("then a 403 status is returned", func() { + So(w.Code, ShouldEqual, http.StatusForbidden) + So(strings.TrimSpace(w.Body.String()), ShouldEqual, errs.ErrAddDatasetAlreadyExists.Error()) + + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) + So(len(mockedDataStore.UpsertDatasetCalls()), ShouldEqual, 0) + + calls := auditor.RecordCalls() + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], addDatasetAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], addDatasetAction, actionUnsuccessful, ap) + }) + }) + + Convey("when datastore upsertDataset returns error", func() { + r, err := createRequestWithAuth("POST", "http://localhost:22000/datasets/123", bytes.NewBufferString(datasetPayload)) + So(err, ShouldBeNil) + w := httptest.NewRecorder() + + mockedDataStore := &storetest.StorerMock{ + GetDatasetFunc: func(string) (*models.DatasetUpdate, error) { + return nil, errs.ErrDatasetNotFound + }, + UpsertDatasetFunc: func(ID string, datasetDoc *models.DatasetUpdate) error { + return errors.New("upsert datset error") + }, + } + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditor, genericMockedObservationStore) + api.router.ServeHTTP(w, r) + + Convey("then a 500 status is returned", func() { + So(w.Code, ShouldEqual, http.StatusInternalServerError) + So(strings.TrimSpace(w.Body.String()), ShouldEqual, "upsert datset error") + + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) + So(len(mockedDataStore.UpsertDatasetCalls()), ShouldEqual, 1) + + calls := auditor.RecordCalls() + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], addDatasetAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], addDatasetAction, actionUnsuccessful, ap) + }) + }) }) } From b873f73b35a09c382b264a248843485d92ee7a73 Mon Sep 17 00:00:00 2001 From: Dave Llewellyn <439837@gmail.com> Date: Thu, 24 May 2018 16:50:49 +0100 Subject: [PATCH 17/34] added audit action successful erro test for add dataset --- api/dataset_test.go | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/api/dataset_test.go b/api/dataset_test.go index 56c4ce80..7356945f 100644 --- a/api/dataset_test.go +++ b/api/dataset_test.go @@ -700,6 +700,43 @@ func TestPostDatasetAuditErrors(t *testing.T) { }) }) }) + + Convey("given audit action successful returns an error", t, func() { + auditor := getMockAuditorFunc(func(a string, r string) error { + if a == addDatasetAction && r == actionSuccessful { + return errors.New("auditing error") + } + return nil + }) + + Convey("when add dataset is successful", func() { + r, err := createRequestWithAuth("POST", "http://localhost:22000/datasets/123", bytes.NewBufferString(datasetPayload)) + So(err, ShouldBeNil) + w := httptest.NewRecorder() + + mockedDataStore := &storetest.StorerMock{ + GetDatasetFunc: func(string) (*models.DatasetUpdate, error) { + return nil, errs.ErrDatasetNotFound + }, + UpsertDatasetFunc: func(ID string, datasetDoc *models.DatasetUpdate) error { + return nil + }, + } + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditor, genericMockedObservationStore) + api.router.ServeHTTP(w, r) + + Convey("then a 201 status is returned", func() { + So(w.Code, ShouldEqual, http.StatusCreated) + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) + So(len(mockedDataStore.UpsertDatasetCalls()), ShouldEqual, 1) + + calls := auditor.RecordCalls() + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], addDatasetAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], addDatasetAction, actionSuccessful, ap) + }) + }) + }) } func TestPutDatasetReturnsSuccessfully(t *testing.T) { From 4f6ebdc91348d4d5675d4c6f6e33270331c78efc Mon Sep 17 00:00:00 2001 From: Dave Llewellyn <439837@gmail.com> Date: Fri, 25 May 2018 10:20:18 +0100 Subject: [PATCH 18/34] initial refactoring of get metadata endpoint to make auditing easier --- api/metadata.go | 111 ++++++++++++++++++++++++++------------------ apierrors/errors.go | 4 ++ 2 files changed, 71 insertions(+), 44 deletions(-) diff --git a/api/metadata.go b/api/metadata.go index dc9d06cb..bedb1d06 100644 --- a/api/metadata.go +++ b/api/metadata.go @@ -8,70 +8,74 @@ import ( "github.com/ONSdigital/dp-dataset-api/models" "github.com/ONSdigital/go-ns/log" "github.com/gorilla/mux" + "github.com/pkg/errors" ) func (api *DatasetAPI) getMetadata(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() vars := mux.Vars(r) datasetID := vars["id"] edition := vars["edition"] version := vars["version"] logData := log.Data{"dataset_id": datasetID, "edition": edition, "version": version} - // get dataset document - datasetDoc, err := api.dataStore.Backend.GetDataset(datasetID) - if err != nil { - log.Error(err, logData) - handleErrorType(versionDocType, err, w) - return - } + b, err := func() ([]byte, error) { + datasetDoc, err := api.dataStore.Backend.GetDataset(datasetID) + if err != nil { + logError(ctx, errors.WithMessage(err, "getMetadata endpoint: get datastore.getDataset returned an error"), logData) + return nil, err + } - authorised, logData := api.authenticate(r, logData) + authorised, logData := api.authenticate(r, logData) + var state string - var state string + // if request is authenticated then access resources of state other than published + if !authorised { + // Check for current sub document + if datasetDoc.Current == nil || datasetDoc.Current.State != models.PublishedState { + logData["dataset"] = datasetDoc.Current + logError(ctx, errors.New("getMetadata endpoint: caller not is authorised and dataset but currently unpublished"), logData) + return nil, errs.ErrDatasetNotFound + } - // if request is authenticated then access resources of state other than published - if !authorised { - // Check for current sub document - if datasetDoc.Current == nil || datasetDoc.Current.State != models.PublishedState { - log.ErrorC("found dataset but currently unpublished", errs.ErrDatasetNotFound, log.Data{"dataset_id": datasetID, "edition": edition, "version": version, "dataset": datasetDoc.Current}) - http.Error(w, errs.ErrDatasetNotFound.Error(), http.StatusNotFound) - return + state = datasetDoc.Current.State } - state = datasetDoc.Current.State - } + if err = api.dataStore.Backend.CheckEditionExists(datasetID, edition, state); err != nil { + logError(ctx, errors.WithMessage(err, "getMetadata endpoint: failed to find edition for dataset"), logData) + return nil, err + } - if err = api.dataStore.Backend.CheckEditionExists(datasetID, edition, state); err != nil { - log.ErrorC("failed to find edition for dataset", err, logData) - handleErrorType(versionDocType, err, w) - return - } + versionDoc, err := api.dataStore.Backend.GetVersion(datasetID, edition, version, state) + if err != nil { + logError(ctx, errors.WithMessage(err, "getMetadata endpoint: failed to find version for dataset edition"), logData) + return nil, errs.ErrMetadataVersionNotFound + } - versionDoc, err := api.dataStore.Backend.GetVersion(datasetID, edition, version, state) - if err != nil { - log.ErrorC("failed to find version for dataset edition", err, log.Data{"dataset_id": datasetID, "edition": edition, "version": version}) - handleErrorType(versionDocType, err, w) - return - } + if err = models.CheckState("version", versionDoc.State); err != nil { + logData["state"] = versionDoc.State + logError(ctx, errors.WithMessage(err, "getMetadata endpoint: unpublished version has an invalid state"), logData) + return nil, err + } - if err = models.CheckState("version", versionDoc.State); err != nil { - log.ErrorC("unpublished version has an invalid state", err, log.Data{"state": versionDoc.State}) - http.Error(w, err.Error(), http.StatusInternalServerError) - return - } + var metaDataDoc *models.Metadata + // combine version and dataset metadata + if state != models.PublishedState { + metaDataDoc = models.CreateMetaDataDoc(datasetDoc.Next, versionDoc, api.urlBuilder) + } else { + metaDataDoc = models.CreateMetaDataDoc(datasetDoc.Current, versionDoc, api.urlBuilder) + } - var metaDataDoc *models.Metadata - // combine version and dataset metadata - if state != models.PublishedState { - metaDataDoc = models.CreateMetaDataDoc(datasetDoc.Next, versionDoc, api.urlBuilder) - } else { - metaDataDoc = models.CreateMetaDataDoc(datasetDoc.Current, versionDoc, api.urlBuilder) - } + b, err := json.Marshal(metaDataDoc) + if err != nil { + logError(ctx, errors.WithMessage(err, "getMetadata endpoint: failed to marshal metadata resource into bytes"), logData) + return nil, err + } + return b, err + }() - b, err := json.Marshal(metaDataDoc) if err != nil { - log.ErrorC("failed to marshal metadata resource into bytes", err, logData) - http.Error(w, err.Error(), http.StatusInternalServerError) + handleMetadataErr(w, err) return } @@ -84,3 +88,22 @@ func (api *DatasetAPI) getMetadata(w http.ResponseWriter, r *http.Request) { log.Debug("get metadata relevant to version", logData) } + +func handleMetadataErr(w http.ResponseWriter, err error) { + var responseStatus int + + switch { + case err == errs.ErrEditionNotFound: + responseStatus = http.StatusNotFound + case err == errs.ErrMetadataNoCurrentPublished: + responseStatus = http.StatusNotFound + case err == errs.ErrMetadataVersionNotFound: + responseStatus = http.StatusNotFound + case err == errs.ErrDatasetNotFound: + responseStatus = http.StatusNotFound + default: + responseStatus = http.StatusInternalServerError + } + + http.Error(w, err.Error(), responseStatus) +} diff --git a/apierrors/errors.go b/apierrors/errors.go index e14998fa..73071175 100644 --- a/apierrors/errors.go +++ b/apierrors/errors.go @@ -23,4 +23,8 @@ var ( ErrDeletePublishedDatasetForbidden = errors.New("a published dataset cannot be deleted") ErrDeleteDatasetNotFound = errors.New("dataset not found") ErrAuditActionAttemptedFailure = errors.New("internal server error") + + // metadata endpoint errors + ErrMetadataNoCurrentPublished = errors.New("bob") + ErrMetadataVersionNotFound = errors.New("Version not found") ) From 195529af8c5410caeea6a52ee78d3a194cc0a431 Mon Sep 17 00:00:00 2001 From: Dave Llewellyn <439837@gmail.com> Date: Fri, 25 May 2018 10:39:31 +0100 Subject: [PATCH 19/34] added audit calls to getMetadata endpoint and updated happy path tests to verify audit calls --- api/api.go | 1 + api/metadata.go | 22 +++++++++++++++++++--- api/metadata_test.go | 19 +++++++++++++++++-- 3 files changed, 37 insertions(+), 5 deletions(-) diff --git a/api/api.go b/api/api.go index 6782737e..e5890005 100644 --- a/api/api.go +++ b/api/api.go @@ -47,6 +47,7 @@ const ( getVersionAction = "getVersion" deleteDatasetAction = "deleteDataset" addDatasetAction = "addDataset" + getMetadataAction = "getMetadata" // audit results actionAttempted = "attempted" diff --git a/api/metadata.go b/api/metadata.go index bedb1d06..7bfeb137 100644 --- a/api/metadata.go +++ b/api/metadata.go @@ -6,6 +6,7 @@ import ( errs "github.com/ONSdigital/dp-dataset-api/apierrors" "github.com/ONSdigital/dp-dataset-api/models" + "github.com/ONSdigital/go-ns/common" "github.com/ONSdigital/go-ns/log" "github.com/gorilla/mux" "github.com/pkg/errors" @@ -18,6 +19,13 @@ func (api *DatasetAPI) getMetadata(w http.ResponseWriter, r *http.Request) { edition := vars["edition"] version := vars["version"] logData := log.Data{"dataset_id": datasetID, "edition": edition, "version": version} + auditParams := common.Params{"dataset_id": datasetID, "edition": edition, "version": version} + + if auditErr := api.auditor.Record(ctx, getMetadataAction, actionAttempted, auditParams); auditErr != nil { + auditActionFailure(ctx, getMetadataAction, actionAttempted, auditErr, logData) + handleMetadataErr(w, auditErr) + return + } b, err := func() ([]byte, error) { datasetDoc, err := api.dataStore.Backend.GetDataset(datasetID) @@ -75,18 +83,26 @@ func (api *DatasetAPI) getMetadata(w http.ResponseWriter, r *http.Request) { }() if err != nil { + if auditErr := api.auditor.Record(ctx, getMetadataAction, actionUnsuccessful, auditParams); auditErr != nil { + auditActionFailure(ctx, getMetadataAction, actionUnsuccessful, auditErr, logData) + } handleMetadataErr(w, err) return } + if auditErr := api.auditor.Record(ctx, getMetadataAction, actionSuccessful, auditParams); auditErr != nil { + auditActionFailure(ctx, getMetadataAction, actionSuccessful, auditErr, logData) + handleMetadataErr(w, auditErr) + return + } + setJSONContentType(w) _, err = w.Write(b) if err != nil { - log.Error(err, logData) + logError(ctx, errors.WithMessage(err, "getMetadata endpoint: failed to write bytes to response"), logData) http.Error(w, err.Error(), http.StatusInternalServerError) } - - log.Debug("get metadata relevant to version", logData) + logInfo(ctx, "getMetadata endpoint: get metadata request successful", logData) } func handleMetadataErr(w http.ResponseWriter, err error) { diff --git a/api/metadata_test.go b/api/metadata_test.go index c99ef428..2e008dd1 100644 --- a/api/metadata_test.go +++ b/api/metadata_test.go @@ -12,6 +12,7 @@ import ( "github.com/ONSdigital/dp-dataset-api/mocks" "github.com/ONSdigital/dp-dataset-api/models" "github.com/ONSdigital/dp-dataset-api/store/datastoretest" + "github.com/ONSdigital/go-ns/common" . "github.com/smartystreets/goconvey/convey" ) @@ -37,7 +38,8 @@ func TestGetMetadataReturnsOk(t *testing.T) { }, } - api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, getMockAuditor(), genericMockedObservationStore) + auditor := getMockAuditor() + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditor, genericMockedObservationStore) api.router.ServeHTTP(w, r) So(w.Code, ShouldEqual, http.StatusOK) @@ -45,6 +47,12 @@ func TestGetMetadataReturnsOk(t *testing.T) { So(len(mockedDataStore.CheckEditionExistsCalls()), ShouldEqual, 1) So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 1) + calls := auditor.RecordCalls() + ap := common.Params{"dataset_id": "123", "edition": "2017", "version": "1"} + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], getMetadataAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], getMetadataAction, actionSuccessful, ap) + bytes, err := ioutil.ReadAll(w.Body) if err != nil { os.Exit(1) @@ -93,7 +101,8 @@ func TestGetMetadataReturnsOk(t *testing.T) { }, } - api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, getMockAuditor(), genericMockedObservationStore) + auditor := getMockAuditor() + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditor, genericMockedObservationStore) api.router.ServeHTTP(w, r) So(w.Code, ShouldEqual, http.StatusOK) @@ -101,6 +110,12 @@ func TestGetMetadataReturnsOk(t *testing.T) { So(len(mockedDataStore.CheckEditionExistsCalls()), ShouldEqual, 1) So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 1) + calls := auditor.RecordCalls() + ap := common.Params{"dataset_id": "123", "edition": "2017", "version": "1"} + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], getMetadataAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], getMetadataAction, actionSuccessful, ap) + bytes, err := ioutil.ReadAll(w.Body) if err != nil { os.Exit(1) From bc57bf9faa7f2fb35abf46dbff223d585f1fc0c2 Mon Sep 17 00:00:00 2001 From: Dave Llewellyn <439837@gmail.com> Date: Fri, 25 May 2018 10:49:57 +0100 Subject: [PATCH 20/34] added test for audit attempted action error --- api/metadata_test.go | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/api/metadata_test.go b/api/metadata_test.go index 2e008dd1..634d8303 100644 --- a/api/metadata_test.go +++ b/api/metadata_test.go @@ -13,6 +13,7 @@ import ( "github.com/ONSdigital/dp-dataset-api/models" "github.com/ONSdigital/dp-dataset-api/store/datastoretest" "github.com/ONSdigital/go-ns/common" + "github.com/pkg/errors" . "github.com/smartystreets/goconvey/convey" ) @@ -300,6 +301,41 @@ func TestGetMetadataReturnsError(t *testing.T) { }) } +func TestGetMetadataAuditingErrors(t *testing.T) { + ap := common.Params{"dataset_id": "123", "edition": "2017", "version": "1"} + + Convey("given auditing action attempted returns an error", t, func() { + auditor := getMockAuditorFunc(func(a string, r string) error { + if a == getMetadataAction && r == actionAttempted { + return errors.New("audit error") + } + return nil + }) + + Convey("when get metadata is called", func() { + r := httptest.NewRequest("GET", "http://localhost:22000/datasets/123/editions/2017/versions/1/metadata", nil) + w := httptest.NewRecorder() + + mockedDataStore := &storetest.StorerMock{} + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditor, genericMockedObservationStore) + + api.router.ServeHTTP(w, r) + + Convey("then a 500 status is returned", func() { + So(w.Code, ShouldEqual, http.StatusInternalServerError) + + calls := auditor.RecordCalls() + So(len(calls), ShouldEqual, 1) + verifyAuditRecordCalls(calls[0], getMetadataAction, actionAttempted, ap) + + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 0) + So(len(mockedDataStore.CheckEditionExistsCalls()), ShouldEqual, 0) + So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 0) + }) + }) + }) +} + // createDatasetDoc returns a datasetUpdate doc containing minimal fields but // there is a clear difference between the current and next sub documents func createDatasetDoc() *models.DatasetUpdate { From 10c29a92d45891de44bb02a7e9567b10dd7d21c8 Mon Sep 17 00:00:00 2001 From: Dave Llewellyn <439837@gmail.com> Date: Fri, 25 May 2018 10:54:23 +0100 Subject: [PATCH 21/34] added auiting unsuccessful test for dataset not found err --- api/metadata_test.go | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/api/metadata_test.go b/api/metadata_test.go index 634d8303..1225685b 100644 --- a/api/metadata_test.go +++ b/api/metadata_test.go @@ -334,6 +334,42 @@ func TestGetMetadataAuditingErrors(t *testing.T) { }) }) }) + + Convey("given auditing action unsuccessful returns an error", t, func() { + auditor := getMockAuditorFunc(func(a string, r string) error { + if a == getMetadataAction && r == actionUnsuccessful { + return errors.New("audit error") + } + return nil + }) + + Convey("when datastore getDataset returns dataset not found error", func() { + r := httptest.NewRequest("GET", "http://localhost:22000/datasets/123/editions/2017/versions/1/metadata", nil) + w := httptest.NewRecorder() + + mockedDataStore := &storetest.StorerMock{ + GetDatasetFunc: func(ID string) (*models.DatasetUpdate, error) { + return nil, errs.ErrDatasetNotFound + }, + } + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditor, genericMockedObservationStore) + + api.router.ServeHTTP(w, r) + + Convey("then a 404 status is returned", func() { + So(w.Code, ShouldEqual, http.StatusNotFound) + + calls := auditor.RecordCalls() + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], getMetadataAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], getMetadataAction, actionUnsuccessful, ap) + + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) + So(len(mockedDataStore.CheckEditionExistsCalls()), ShouldEqual, 0) + So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 0) + }) + }) + }) } // createDatasetDoc returns a datasetUpdate doc containing minimal fields but From 0c86aa678ec906038ef58c99c8f0ce9b73021e2e Mon Sep 17 00:00:00 2001 From: Dave Llewellyn <439837@gmail.com> Date: Fri, 25 May 2018 11:05:30 +0100 Subject: [PATCH 22/34] added audit unsuccessful tests to cover dataset current empty, edition not found and version not found --- api/metadata_test.go | 90 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 90 insertions(+) diff --git a/api/metadata_test.go b/api/metadata_test.go index 1225685b..fd2b6649 100644 --- a/api/metadata_test.go +++ b/api/metadata_test.go @@ -369,6 +369,96 @@ func TestGetMetadataAuditingErrors(t *testing.T) { So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 0) }) }) + + Convey("when dataset.current is empty", func() { + r := httptest.NewRequest("GET", "http://localhost:22000/datasets/123/editions/2017/versions/1/metadata", nil) + w := httptest.NewRecorder() + + mockedDataStore := &storetest.StorerMock{ + GetDatasetFunc: func(ID string) (*models.DatasetUpdate, error) { + return &models.DatasetUpdate{}, nil + }, + } + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditor, genericMockedObservationStore) + + api.router.ServeHTTP(w, r) + + Convey("then a 404 status is returned", func() { + So(w.Code, ShouldEqual, http.StatusNotFound) + + calls := auditor.RecordCalls() + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], getMetadataAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], getMetadataAction, actionUnsuccessful, ap) + + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) + So(len(mockedDataStore.CheckEditionExistsCalls()), ShouldEqual, 0) + So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 0) + }) + }) + + Convey("when dataset edition does not exist", func() { + r := httptest.NewRequest("GET", "http://localhost:22000/datasets/123/editions/2017/versions/1/metadata", nil) + w := httptest.NewRecorder() + + mockedDataStore := &storetest.StorerMock{ + GetDatasetFunc: func(ID string) (*models.DatasetUpdate, error) { + return createDatasetDoc(), nil + }, + CheckEditionExistsFunc: func(ID string, editionID string, state string) error { + return errs.ErrEditionNotFound + }, + } + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditor, genericMockedObservationStore) + + api.router.ServeHTTP(w, r) + + Convey("then a 404 status is returned", func() { + So(w.Code, ShouldEqual, http.StatusNotFound) + + calls := auditor.RecordCalls() + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], getMetadataAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], getMetadataAction, actionUnsuccessful, ap) + + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) + So(len(mockedDataStore.CheckEditionExistsCalls()), ShouldEqual, 1) + So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 0) + }) + }) + + Convey("when dataset version does not exist", func() { + r := httptest.NewRequest("GET", "http://localhost:22000/datasets/123/editions/2017/versions/1/metadata", nil) + w := httptest.NewRecorder() + + mockedDataStore := &storetest.StorerMock{ + GetDatasetFunc: func(ID string) (*models.DatasetUpdate, error) { + return createDatasetDoc(), nil + }, + CheckEditionExistsFunc: func(ID string, editionID string, state string) error { + return nil + }, + GetVersionFunc: func(datasetID string, editionID string, version string, state string) (*models.Version, error) { + return nil, errs.ErrVersionNotFound + }, + } + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditor, genericMockedObservationStore) + + api.router.ServeHTTP(w, r) + + Convey("then a 404 status is returned", func() { + So(w.Code, ShouldEqual, http.StatusNotFound) + + calls := auditor.RecordCalls() + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], getMetadataAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], getMetadataAction, actionUnsuccessful, ap) + + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) + So(len(mockedDataStore.CheckEditionExistsCalls()), ShouldEqual, 1) + So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 1) + }) + }) }) } From 7ecf7b701a0101c486baa387d48ff7c67ed820ce Mon Sep 17 00:00:00 2001 From: Dave Llewellyn <439837@gmail.com> Date: Fri, 25 May 2018 11:40:48 +0100 Subject: [PATCH 23/34] added audit test for action unsuccessful version not published --- api/metadata_test.go | 37 ++++++++++++++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/api/metadata_test.go b/api/metadata_test.go index fd2b6649..da17ac35 100644 --- a/api/metadata_test.go +++ b/api/metadata_test.go @@ -446,7 +446,7 @@ func TestGetMetadataAuditingErrors(t *testing.T) { api.router.ServeHTTP(w, r) - Convey("then a 404 status is returned", func() { + Convey("then a 500 status is returned", func() { So(w.Code, ShouldEqual, http.StatusNotFound) calls := auditor.RecordCalls() @@ -459,6 +459,41 @@ func TestGetMetadataAuditingErrors(t *testing.T) { So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 1) }) }) + + Convey("when version not published", func() { + r := httptest.NewRequest("GET", "http://localhost:22000/datasets/123/editions/2017/versions/1/metadata", nil) + w := httptest.NewRecorder() + versionDoc := createVersionDoc() + versionDoc.State = "not published" + + mockedDataStore := &storetest.StorerMock{ + GetDatasetFunc: func(ID string) (*models.DatasetUpdate, error) { + return createDatasetDoc(), nil + }, + CheckEditionExistsFunc: func(ID string, editionID string, state string) error { + return nil + }, + GetVersionFunc: func(datasetID string, editionID string, version string, state string) (*models.Version, error) { + return versionDoc, nil + }, + } + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditor, genericMockedObservationStore) + + api.router.ServeHTTP(w, r) + + Convey("then a 500 status is returned", func() { + So(w.Code, ShouldEqual, http.StatusInternalServerError) + + calls := auditor.RecordCalls() + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], getMetadataAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], getMetadataAction, actionUnsuccessful, ap) + + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) + So(len(mockedDataStore.CheckEditionExistsCalls()), ShouldEqual, 1) + So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 1) + }) + }) }) } From d69f1239e641eac3a62e3223786337913367872a Mon Sep 17 00:00:00 2001 From: Dave Llewellyn <439837@gmail.com> Date: Fri, 25 May 2018 11:48:49 +0100 Subject: [PATCH 24/34] added audit action successful error test case --- api/metadata.go | 2 -- api/metadata_test.go | 44 ++++++++++++++++++++++++++++++++++++++++++++ apierrors/errors.go | 3 +-- 3 files changed, 45 insertions(+), 4 deletions(-) diff --git a/api/metadata.go b/api/metadata.go index 7bfeb137..f3b1f1fb 100644 --- a/api/metadata.go +++ b/api/metadata.go @@ -111,8 +111,6 @@ func handleMetadataErr(w http.ResponseWriter, err error) { switch { case err == errs.ErrEditionNotFound: responseStatus = http.StatusNotFound - case err == errs.ErrMetadataNoCurrentPublished: - responseStatus = http.StatusNotFound case err == errs.ErrMetadataVersionNotFound: responseStatus = http.StatusNotFound case err == errs.ErrDatasetNotFound: diff --git a/api/metadata_test.go b/api/metadata_test.go index da17ac35..dccac445 100644 --- a/api/metadata_test.go +++ b/api/metadata_test.go @@ -495,6 +495,50 @@ func TestGetMetadataAuditingErrors(t *testing.T) { }) }) }) + + Convey("given auditing action successful returns an error", t, func() { + auditor := getMockAuditorFunc(func(a string, r string) error { + if a == getMetadataAction && r == actionSuccessful { + return errors.New("audit error") + } + return nil + }) + + Convey("when get metadata is called", func() { + r := httptest.NewRequest("GET", "http://localhost:22000/datasets/123/editions/2017/versions/1/metadata", nil) + w := httptest.NewRecorder() + + mockedDataStore := &storetest.StorerMock{ + GetDatasetFunc: func(ID string) (*models.DatasetUpdate, error) { + return createDatasetDoc(), nil + }, + CheckEditionExistsFunc: func(ID string, editionID string, state string) error { + return nil + }, + GetVersionFunc: func(datasetID string, editionID string, version string, state string) (*models.Version, error) { + return createVersionDoc(), nil + }, + } + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditor, genericMockedObservationStore) + + api.router.ServeHTTP(w, r) + + Convey("then a 500 status is returned", func() { + So(w.Code, ShouldEqual, http.StatusInternalServerError) + + calls := auditor.RecordCalls() + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], getMetadataAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], getMetadataAction, actionSuccessful, ap) + + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) + So(len(mockedDataStore.CheckEditionExistsCalls()), ShouldEqual, 1) + So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 1) + }) + }) + + }) + } // createDatasetDoc returns a datasetUpdate doc containing minimal fields but diff --git a/apierrors/errors.go b/apierrors/errors.go index b8be72f7..821a2d16 100644 --- a/apierrors/errors.go +++ b/apierrors/errors.go @@ -27,6 +27,5 @@ var ( ErrAuditActionAttemptedFailure = errors.New("internal server error") // metadata endpoint errors - ErrMetadataNoCurrentPublished = errors.New("bob") - ErrMetadataVersionNotFound = errors.New("Version not found") + ErrMetadataVersionNotFound = errors.New("Version not found") ) From ad5cb3718b14baecadd630a0f7e8d13a7c833253 Mon Sep 17 00:00:00 2001 From: Dave Llewellyn <439837@gmail.com> Date: Fri, 25 May 2018 15:08:56 +0100 Subject: [PATCH 25/34] refactored get dimensions endpoint pattern ready for auditing --- api/dimensions.go | 96 ++++++++++++++++++++++++++++++----------------- 1 file changed, 62 insertions(+), 34 deletions(-) diff --git a/api/dimensions.go b/api/dimensions.go index 5d319999..1417deae 100644 --- a/api/dimensions.go +++ b/api/dimensions.go @@ -1,73 +1,81 @@ package api import ( + "context" "encoding/json" "fmt" "net/http" + errs "github.com/ONSdigital/dp-dataset-api/apierrors" "github.com/ONSdigital/dp-dataset-api/models" "github.com/ONSdigital/go-ns/log" "github.com/gorilla/mux" + "github.com/pkg/errors" "gopkg.in/mgo.v2/bson" ) func (api *DatasetAPI) getDimensions(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() vars := mux.Vars(r) datasetID := vars["id"] edition := vars["edition"] version := vars["version"] logData := log.Data{"dataset_id": datasetID, "edition": edition, "version": version} - authorised, logData := api.authenticate(r, logData) + b, err := func() ([]byte, error) { + authorised, logData := api.authenticate(r, logData) - var state string - if !authorised { - state = models.PublishedState - } + var state string + if !authorised { + state = models.PublishedState + } - versionDoc, err := api.dataStore.Backend.GetVersion(datasetID, edition, version, state) - if err != nil { - log.ErrorC("failed to get version", err, logData) - handleErrorType(dimensionDocType, err, w) - return - } + versionDoc, err := api.dataStore.Backend.GetVersion(datasetID, edition, version, state) + if err != nil { + logError(ctx, errors.WithMessage(err, "getDimensions endpoint: datastore.getversion returned an error"), logData) + return nil, err + } - if err = models.CheckState("version", versionDoc.State); err != nil { - log.ErrorC("unpublished version has an invalid state", err, log.Data{"state": versionDoc.State}) - http.Error(w, err.Error(), http.StatusInternalServerError) - return - } + if err = models.CheckState("version", versionDoc.State); err != nil { + logData["state"] = versionDoc.State + logError(ctx, errors.WithMessage(err, "getDimensions endpoint: unpublished version has an invalid state"), logData) + return nil, err + } - dimensions, err := api.dataStore.Backend.GetDimensions(datasetID, versionDoc.ID) - if err != nil { - log.ErrorC("failed to get version dimensions", err, logData) - handleErrorType(dimensionDocType, err, w) - return - } + dimensions, err := api.dataStore.Backend.GetDimensions(datasetID, versionDoc.ID) + if err != nil { + logError(ctx, errors.WithMessage(err, "getDimensions endpoint: failed to get version dimensions"), logData) + return nil, err + } - results, err := api.createListOfDimensions(versionDoc, dimensions) - if err != nil { - log.ErrorC("failed to convert bson to dimension", err, logData) - http.Error(w, err.Error(), http.StatusInternalServerError) - } + results, err := api.createListOfDimensions(versionDoc, dimensions) + if err != nil { + logError(ctx, errors.WithMessage(err, "getDimensions endpoint: failed to convert bson to dimension"), logData) + return nil, err + } - listOfDimensions := &models.DatasetDimensionResults{Items: results} + listOfDimensions := &models.DatasetDimensionResults{Items: results} + + b, err := json.Marshal(listOfDimensions) + if err != nil { + logError(ctx, errors.WithMessage(err, "getDimensions endpoint: failed to marshal list of dimension resources into bytes"), logData) + return nil, err + } + return b, nil + }() - b, err := json.Marshal(listOfDimensions) if err != nil { - log.ErrorC("failed to marshal list of dimension resources into bytes", err, logData) - http.Error(w, err.Error(), http.StatusInternalServerError) + handleDimensionsErr(ctx, w, err) return } setJSONContentType(w) _, err = w.Write(b) if err != nil { - log.Error(err, logData) + logError(ctx, errors.WithMessage(err, "getDimensions endpoint: error writing bytes to response"), logData) http.Error(w, err.Error(), http.StatusInternalServerError) } - - log.Debug("get dimensions", log.Data{"dataset_id": datasetID, "edition": edition, "version": version}) + logInfo(ctx, "getDimensions endpoint: request successful", logData) } func (api *DatasetAPI) createListOfDimensions(versionDoc *models.Version, dimensions []bson.M) ([]models.Dimension, error) { @@ -173,3 +181,23 @@ func (api *DatasetAPI) getDimensionOptions(w http.ResponseWriter, r *http.Reques log.Debug("get dimension options", logData) } + +func handleDimensionsErr(ctx context.Context, w http.ResponseWriter, err error) { + var responseStatus int + + switch { + case err == errs.ErrDatasetNotFound: + responseStatus = http.StatusNotFound + case err == errs.ErrEditionNotFound: + responseStatus = http.StatusNotFound + case err == errs.ErrVersionNotFound: + responseStatus = http.StatusNotFound + case err == errs.ErrDimensionsNotFound: + responseStatus = http.StatusNotFound + default: + responseStatus = http.StatusInternalServerError + } + + logError(ctx, errors.WithMessage(err, "request unsuccessful"), log.Data{"responseStatus": responseStatus}) + http.Error(w, err.Error(), responseStatus) +} From cfd4fb00f250b813d5eff7d89edecf5af3aa7244 Mon Sep 17 00:00:00 2001 From: Dave Llewellyn <439837@gmail.com> Date: Fri, 25 May 2018 15:15:47 +0100 Subject: [PATCH 26/34] added happy path audit to get dimensions and updated happy path test case --- api/api.go | 1 + api/dimensions.go | 14 ++++++++++++++ api/dimensions_test.go | 14 +++++++++++++- 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/api/api.go b/api/api.go index 6782737e..f0f7f06a 100644 --- a/api/api.go +++ b/api/api.go @@ -47,6 +47,7 @@ const ( getVersionAction = "getVersion" deleteDatasetAction = "deleteDataset" addDatasetAction = "addDataset" + getDimensionsAction = "getDimensions" // audit results actionAttempted = "attempted" diff --git a/api/dimensions.go b/api/dimensions.go index 1417deae..d171ca53 100644 --- a/api/dimensions.go +++ b/api/dimensions.go @@ -8,6 +8,7 @@ import ( errs "github.com/ONSdigital/dp-dataset-api/apierrors" "github.com/ONSdigital/dp-dataset-api/models" + "github.com/ONSdigital/go-ns/common" "github.com/ONSdigital/go-ns/log" "github.com/gorilla/mux" "github.com/pkg/errors" @@ -21,6 +22,13 @@ func (api *DatasetAPI) getDimensions(w http.ResponseWriter, r *http.Request) { edition := vars["edition"] version := vars["version"] logData := log.Data{"dataset_id": datasetID, "edition": edition, "version": version} + auditParams := common.Params{"dataset_id": datasetID, "edition": edition, "version": version} + + if err := api.auditor.Record(ctx, getDimensionsAction, actionAttempted, auditParams); err != nil { + auditActionFailure(ctx, getDimensionsAction, actionAttempted, err, logData) + handleDimensionsErr(ctx, w, err) + return + } b, err := func() ([]byte, error) { authorised, logData := api.authenticate(r, logData) @@ -69,6 +77,12 @@ func (api *DatasetAPI) getDimensions(w http.ResponseWriter, r *http.Request) { return } + if auditErr := api.auditor.Record(ctx, getDimensionsAction, actionSuccessful, auditParams); auditErr != nil { + auditActionFailure(ctx, getDimensionsAction, actionAttempted, auditErr, logData) + handleDimensionsErr(ctx, w, auditErr) + return + } + setJSONContentType(w) _, err = w.Write(b) if err != nil { diff --git a/api/dimensions_test.go b/api/dimensions_test.go index 22ae5e3f..9063a0d5 100644 --- a/api/dimensions_test.go +++ b/api/dimensions_test.go @@ -9,6 +9,7 @@ import ( "github.com/ONSdigital/dp-dataset-api/mocks" "github.com/ONSdigital/dp-dataset-api/models" "github.com/ONSdigital/dp-dataset-api/store/datastoretest" + "github.com/ONSdigital/go-ns/common" . "github.com/smartystreets/goconvey/convey" "gopkg.in/mgo.v2/bson" ) @@ -27,12 +28,23 @@ func TestGetDimensionsReturnsOk(t *testing.T) { }, } - api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, getMockAuditor(), genericMockedObservationStore) + auditor := getMockAuditor() + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditor, genericMockedObservationStore) api.router.ServeHTTP(w, r) So(w.Code, ShouldEqual, http.StatusOK) So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 1) So(len(mockedDataStore.GetDimensionsCalls()), ShouldEqual, 1) + + calls := auditor.RecordCalls() + ap := common.Params{ + "dataset_id": "123", + "edition": "2017", + "version": "1", + } + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], getDimensionsAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], getDimensionsAction, actionSuccessful, ap) }) } From f819ef1257af86130258d1c1babc9f64b0be4458 Mon Sep 17 00:00:00 2001 From: Dave Llewellyn <439837@gmail.com> Date: Fri, 25 May 2018 15:21:20 +0100 Subject: [PATCH 27/34] updated get dimensions error test cases to verify audit calls --- api/dimensions.go | 3 +++ api/dimensions_test.go | 39 +++++++++++++++++++++++++++++++++++---- 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/api/dimensions.go b/api/dimensions.go index d171ca53..965d0ca1 100644 --- a/api/dimensions.go +++ b/api/dimensions.go @@ -73,6 +73,9 @@ func (api *DatasetAPI) getDimensions(w http.ResponseWriter, r *http.Request) { }() if err != nil { + if auditErr := api.auditor.Record(ctx, getDimensionsAction, actionUnsuccessful, auditParams); auditErr != nil { + auditActionFailure(ctx, getDimensionsAction, actionAttempted, auditErr, logData) + } handleDimensionsErr(ctx, w, err) return } diff --git a/api/dimensions_test.go b/api/dimensions_test.go index 9063a0d5..ad0a297f 100644 --- a/api/dimensions_test.go +++ b/api/dimensions_test.go @@ -49,6 +49,12 @@ func TestGetDimensionsReturnsOk(t *testing.T) { } func TestGetDimensionsReturnsErrors(t *testing.T) { + ap := common.Params{ + "dataset_id": "123", + "edition": "2017", + "version": "1", + } + t.Parallel() Convey("When the api cannot connect to datastore to get dimension resource return an internal server error", t, func() { r := httptest.NewRequest("GET", "http://localhost:22000/datasets/123/editions/2017/versions/1/dimensions", nil) @@ -59,7 +65,8 @@ func TestGetDimensionsReturnsErrors(t *testing.T) { }, } - api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, getMockAuditor(), genericMockedObservationStore) + auditor := getMockAuditor() + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditor, genericMockedObservationStore) api.router.ServeHTTP(w, r) So(w.Code, ShouldEqual, http.StatusInternalServerError) @@ -67,6 +74,11 @@ func TestGetDimensionsReturnsErrors(t *testing.T) { So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 1) So(len(mockedDataStore.GetDimensionsCalls()), ShouldEqual, 0) + + calls := auditor.RecordCalls() + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], getDimensionsAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], getDimensionsAction, actionUnsuccessful, ap) }) Convey("When the request contain an invalid version return not found", t, func() { @@ -78,7 +90,8 @@ func TestGetDimensionsReturnsErrors(t *testing.T) { }, } - api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, getMockAuditor(), genericMockedObservationStore) + auditor := getMockAuditor() + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditor, genericMockedObservationStore) api.router.ServeHTTP(w, r) So(w.Code, ShouldEqual, http.StatusNotFound) @@ -86,6 +99,12 @@ func TestGetDimensionsReturnsErrors(t *testing.T) { So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 1) So(len(mockedDataStore.GetDimensionsCalls()), ShouldEqual, 0) + + calls := auditor.RecordCalls() + + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], getDimensionsAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], getDimensionsAction, actionUnsuccessful, ap) }) Convey("When there are no dimensions then return not found error", t, func() { @@ -100,7 +119,8 @@ func TestGetDimensionsReturnsErrors(t *testing.T) { }, } - api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, getMockAuditor(), genericMockedObservationStore) + auditor := getMockAuditor() + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditor, genericMockedObservationStore) api.router.ServeHTTP(w, r) So(w.Code, ShouldEqual, http.StatusNotFound) @@ -108,6 +128,11 @@ func TestGetDimensionsReturnsErrors(t *testing.T) { So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 1) So(len(mockedDataStore.GetDimensionsCalls()), ShouldEqual, 1) + + calls := auditor.RecordCalls() + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], getDimensionsAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], getDimensionsAction, actionUnsuccessful, ap) }) Convey("When the version has an invalid state return internal server error", t, func() { @@ -119,7 +144,8 @@ func TestGetDimensionsReturnsErrors(t *testing.T) { }, } - api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, getMockAuditor(), genericMockedObservationStore) + auditor := getMockAuditor() + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditor, genericMockedObservationStore) api.router.ServeHTTP(w, r) So(w.Code, ShouldEqual, http.StatusInternalServerError) @@ -127,6 +153,11 @@ func TestGetDimensionsReturnsErrors(t *testing.T) { So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 1) So(len(mockedDataStore.GetDimensionsCalls()), ShouldEqual, 0) + + calls := auditor.RecordCalls() + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], getDimensionsAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], getDimensionsAction, actionUnsuccessful, ap) }) } From fb9638e084ba01fb34777b722676dfe65b7f74c0 Mon Sep 17 00:00:00 2001 From: Dave Llewellyn <439837@gmail.com> Date: Fri, 25 May 2018 15:43:50 +0100 Subject: [PATCH 28/34] added test to cover audit unsuccessful cases --- api/dataset_test.go | 11 ++++ api/dimensions_test.go | 138 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 149 insertions(+) diff --git a/api/dataset_test.go b/api/dataset_test.go index 7356945f..877ec56e 100644 --- a/api/dataset_test.go +++ b/api/dataset_test.go @@ -1292,6 +1292,17 @@ func getMockAuditor() *audit.AuditorServiceMock { } } +func createAuditor(a string, r string) *audit.AuditorServiceMock { + return &audit.AuditorServiceMock{ + RecordFunc: func(ctx context.Context, action string, result string, params common.Params) error { + if action == a && result == r { + return errors.New("auditing error") + } + return nil + }, + } +} + func getMockAuditorFunc(f func(action string, result string) error) *audit.AuditorServiceMock { return &audit.AuditorServiceMock{ RecordFunc: func(ctx context.Context, action string, result string, params common.Params) error { diff --git a/api/dimensions_test.go b/api/dimensions_test.go index ad0a297f..0afc5d26 100644 --- a/api/dimensions_test.go +++ b/api/dimensions_test.go @@ -161,6 +161,144 @@ func TestGetDimensionsReturnsErrors(t *testing.T) { }) } +func TestGetDimensionsAuditingErrors(t *testing.T) { + t.Parallel() + ap := common.Params{"dataset_id": "123", "edition": "2017", "version": "1"} + + Convey("given audit action attempted returns an error", t, func() { + auditor := createAuditor(getDimensionsAction, actionAttempted) + + Convey("when get dimensions is called", func() { + r := httptest.NewRequest("GET", "http://localhost:22000/datasets/123/editions/2017/versions/1/dimensions", nil) + w := httptest.NewRecorder() + mockedDataStore := &storetest.StorerMock{} + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditor, genericMockedObservationStore) + + api.router.ServeHTTP(w, r) + + Convey("then a 500 status is returned", func() { + So(w.Code, ShouldEqual, http.StatusInternalServerError) + So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 0) + So(len(mockedDataStore.GetDimensionsCalls()), ShouldEqual, 0) + + calls := auditor.RecordCalls() + So(len(calls), ShouldEqual, 1) + verifyAuditRecordCalls(calls[0], getDimensionsAction, actionAttempted, ap) + }) + }) + }) + + Convey("given audit action successful returns an error", t, func() { + auditor := createAuditor(getDimensionsAction, actionSuccessful) + + Convey("when get dimensions is called", func() { + r := httptest.NewRequest("GET", "http://localhost:22000/datasets/123/editions/2017/versions/1/dimensions", nil) + w := httptest.NewRecorder() + mockedDataStore := &storetest.StorerMock{ + GetVersionFunc: func(datasetID, edition, version, state string) (*models.Version, error) { + return &models.Version{State: models.AssociatedState}, nil + }, + GetDimensionsFunc: func(datasetID, versionID string) ([]bson.M, error) { + return []bson.M{}, nil + }, + } + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditor, genericMockedObservationStore) + + api.router.ServeHTTP(w, r) + + Convey("then a 500 status is returned", func() { + So(w.Code, ShouldEqual, http.StatusInternalServerError) + So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 1) + So(len(mockedDataStore.GetDimensionsCalls()), ShouldEqual, 1) + + calls := auditor.RecordCalls() + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], getDimensionsAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], getDimensionsAction, actionSuccessful, ap) + }) + }) + }) + + Convey("given audit action unsuccessful returns an error", t, func() { + auditor := createAuditor(getDimensionsAction, actionUnsuccessful) + + Convey("when datastore.getVersion returns an error", func() { + r := httptest.NewRequest("GET", "http://localhost:22000/datasets/123/editions/2017/versions/1/dimensions", nil) + w := httptest.NewRecorder() + mockedDataStore := &storetest.StorerMock{ + GetVersionFunc: func(datasetID, edition, version, state string) (*models.Version, error) { + return nil, errs.ErrVersionNotFound + }, + } + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditor, genericMockedObservationStore) + + api.router.ServeHTTP(w, r) + + Convey("then a 500 status is returned", func() { + So(w.Code, ShouldEqual, http.StatusNotFound) + So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 1) + So(len(mockedDataStore.GetDimensionsCalls()), ShouldEqual, 0) + + calls := auditor.RecordCalls() + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], getDimensionsAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], getDimensionsAction, actionUnsuccessful, ap) + }) + }) + + Convey("when the version in not in a valid state", func() { + r := httptest.NewRequest("GET", "http://localhost:22000/datasets/123/editions/2017/versions/1/dimensions", nil) + w := httptest.NewRecorder() + mockedDataStore := &storetest.StorerMock{ + GetVersionFunc: func(datasetID, edition, version, state string) (*models.Version, error) { + return &models.Version{State: "BROKEN"}, nil + }, + } + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditor, genericMockedObservationStore) + + api.router.ServeHTTP(w, r) + + Convey("then a 500 status is returned", func() { + So(w.Code, ShouldEqual, http.StatusInternalServerError) + So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 1) + So(len(mockedDataStore.GetDimensionsCalls()), ShouldEqual, 0) + + calls := auditor.RecordCalls() + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], getDimensionsAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], getDimensionsAction, actionUnsuccessful, ap) + }) + }) + + Convey("when datastore.getDataset returns an error", func() { + r := httptest.NewRequest("GET", "http://localhost:22000/datasets/123/editions/2017/versions/1/dimensions", nil) + w := httptest.NewRecorder() + mockedDataStore := &storetest.StorerMock{ + GetVersionFunc: func(datasetID, edition, version, state string) (*models.Version, error) { + return &models.Version{State: models.AssociatedState}, nil + }, + GetDimensionsFunc: func(datasetID string, versionID string) ([]bson.M, error) { + return nil, errs.ErrDimensionsNotFound + }, + } + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditor, genericMockedObservationStore) + + api.router.ServeHTTP(w, r) + + Convey("then a 500 status is returned", func() { + So(w.Code, ShouldEqual, http.StatusNotFound) + So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 1) + So(len(mockedDataStore.GetDimensionsCalls()), ShouldEqual, 1) + + calls := auditor.RecordCalls() + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], getDimensionsAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], getDimensionsAction, actionUnsuccessful, ap) + }) + }) + }) +} + func TestGetDimensionOptionsReturnsOk(t *testing.T) { t.Parallel() Convey("When a valid dimension is provided then a list of options can be returned successfully", t, func() { From 95a08d1c83eb96cabcbf67774f67a7bbec0b0136 Mon Sep 17 00:00:00 2001 From: Dave Llewellyn <439837@gmail.com> Date: Tue, 29 May 2018 07:49:16 +0100 Subject: [PATCH 29/34] addressed review comments --- api/dimensions.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/dimensions.go b/api/dimensions.go index 965d0ca1..6a0b3383 100644 --- a/api/dimensions.go +++ b/api/dimensions.go @@ -74,14 +74,14 @@ func (api *DatasetAPI) getDimensions(w http.ResponseWriter, r *http.Request) { if err != nil { if auditErr := api.auditor.Record(ctx, getDimensionsAction, actionUnsuccessful, auditParams); auditErr != nil { - auditActionFailure(ctx, getDimensionsAction, actionAttempted, auditErr, logData) + auditActionFailure(ctx, getDimensionsAction, actionUnsuccessful, auditErr, logData) } handleDimensionsErr(ctx, w, err) return } if auditErr := api.auditor.Record(ctx, getDimensionsAction, actionSuccessful, auditParams); auditErr != nil { - auditActionFailure(ctx, getDimensionsAction, actionAttempted, auditErr, logData) + auditActionFailure(ctx, getDimensionsAction, actionSuccessful, auditErr, logData) handleDimensionsErr(ctx, w, auditErr) return } From 15e20a6e05bb3c081f16978ec6493db9abc4168b Mon Sep 17 00:00:00 2001 From: Dave Llewellyn <439837@gmail.com> Date: Tue, 29 May 2018 09:10:33 +0100 Subject: [PATCH 30/34] refactor putDataset endpoint business logic to make way for the Audit Train! Whoop Whoop! --- api/api.go | 2 +- api/dataset.go | 57 +++++++++++++++++++++++++-------------------- api/dataset_test.go | 1 + apierrors/errors.go | 2 +- 4 files changed, 35 insertions(+), 27 deletions(-) diff --git a/api/api.go b/api/api.go index 4c81ad0e..96d6c4b5 100644 --- a/api/api.go +++ b/api/api.go @@ -201,7 +201,7 @@ func handleErrorType(docType string, err error, w http.ResponseWriter) { http.Error(w, err.Error(), http.StatusNoContent) } else if err == errs.ErrDeletePublishedDatasetForbidden || err == errs.ErrAddDatasetAlreadyExists { http.Error(w, err.Error(), http.StatusForbidden) - } else if err == errs.ErrAddDatasetBadRequest { + } else if err == errs.ErrAddUpdateDatasetBadRequest { http.Error(w, err.Error(), http.StatusBadRequest) } else { http.Error(w, err.Error(), http.StatusInternalServerError) diff --git a/api/dataset.go b/api/dataset.go index 2b8f9e53..05b01dde 100644 --- a/api/dataset.go +++ b/api/dataset.go @@ -182,7 +182,7 @@ func (api *DatasetAPI) addDataset(w http.ResponseWriter, r *http.Request) { dataset, err := models.CreateDataset(r.Body) if err != nil { logError(ctx, errors.WithMessage(err, "addDataset endpoint: failed to model dataset resource based on request"), logData) - return nil, errs.ErrAddDatasetBadRequest + return nil, errs.ErrAddUpdateDatasetBadRequest } dataset.State = models.CreatedState @@ -244,41 +244,48 @@ func (api *DatasetAPI) addDataset(w http.ResponseWriter, r *http.Request) { } func (api *DatasetAPI) putDataset(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() vars := mux.Vars(r) datasetID := vars["id"] + data := log.Data{"dataset_id": datasetID} - dataset, err := models.CreateDataset(r.Body) - if err != nil { - log.ErrorC("failed to model dataset resource based on request", err, log.Data{"dataset_id": datasetID}) - http.Error(w, err.Error(), http.StatusBadRequest) - return - } - defer r.Body.Close() + err := func() error { + defer r.Body.Close() - currentDataset, err := api.dataStore.Backend.GetDataset(datasetID) - if err != nil { - log.ErrorC("failed to find dataset", err, log.Data{"dataset_id": datasetID}) - handleErrorType(datasetDocType, err, w) - return - } + dataset, err := models.CreateDataset(r.Body) + if err != nil { + logError(ctx, errors.WithMessage(err, "putDataset endpoint: failed to model dataset resource based on request"), data) + return errs.ErrAddUpdateDatasetBadRequest + } - if dataset.State == models.PublishedState { - if err := api.publishDataset(currentDataset, nil); err != nil { - log.ErrorC("failed to update dataset document to published", err, log.Data{"dataset_id": datasetID}) - handleErrorType(versionDocType, err, w) - return + currentDataset, err := api.dataStore.Backend.GetDataset(datasetID) + if err != nil { + logError(ctx, errors.WithMessage(err, "putDataset endpoint: datastore.getDataset returned an error"), data) + return err } - } else { - if err := api.dataStore.Backend.UpdateDataset(datasetID, dataset, currentDataset.Next.State); err != nil { - log.ErrorC("failed to update dataset resource", err, log.Data{"dataset_id": datasetID}) - handleErrorType(datasetDocType, err, w) - return + + if dataset.State == models.PublishedState { + if err := api.publishDataset(currentDataset, nil); err != nil { + logError(ctx, errors.WithMessage(err, "putDataset endpoint: failed to update dataset document to published"), data) + return err + } + } else { + if err := api.dataStore.Backend.UpdateDataset(datasetID, dataset, currentDataset.Next.State); err != nil { + logError(ctx, errors.WithMessage(err, "putDataset endpoint: failed to update dataset resource"), data) + return err + } } + return nil + }() + + if err != nil { + handleErrorType(datasetDocType, err, w) + return } setJSONContentType(w) w.WriteHeader(http.StatusOK) - log.Debug("update dataset", log.Data{"dataset_id": datasetID}) + logInfo(ctx, "putDataset endpoint: request successful", data) } func (api *DatasetAPI) publishDataset(currentDataset *models.DatasetUpdate, version *models.Version) error { diff --git a/api/dataset_test.go b/api/dataset_test.go index 877ec56e..5a90461e 100644 --- a/api/dataset_test.go +++ b/api/dataset_test.go @@ -793,6 +793,7 @@ func TestPutDatasetReturnsError(t *testing.T) { api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, getMockAuditor(), genericMockedObservationStore) api.router.ServeHTTP(w, r) + So(w.Code, ShouldEqual, http.StatusBadRequest) So(w.Body.String(), ShouldResemble, "Failed to parse json body\n") diff --git a/apierrors/errors.go b/apierrors/errors.go index 821a2d16..8a85b532 100644 --- a/apierrors/errors.go +++ b/apierrors/errors.go @@ -6,7 +6,7 @@ import "errors" var ( ErrDatasetNotFound = errors.New("Dataset not found") ErrAddDatasetAlreadyExists = errors.New("forbidden - dataset already exists") - ErrAddDatasetBadRequest = errors.New("Failed to parse json body") + ErrAddUpdateDatasetBadRequest = errors.New("Failed to parse json body") ErrEditionNotFound = errors.New("Edition not found") ErrVersionNotFound = errors.New("Version not found") ErrDimensionNodeNotFound = errors.New("Dimension node not found") From ff3f0ccf73ee5c0d14d6014a3aa7d77c5b9be49c Mon Sep 17 00:00:00 2001 From: Dave Llewellyn <439837@gmail.com> Date: Tue, 29 May 2018 09:21:58 +0100 Subject: [PATCH 31/34] plumbed in calls to auditor and updated existing tests to assert audit calls --- api/api.go | 5 +++-- api/dataset.go | 6 ++++++ api/dataset_test.go | 17 +++++++++++++++-- 3 files changed, 24 insertions(+), 4 deletions(-) diff --git a/api/api.go b/api/api.go index 96d6c4b5..4807338a 100644 --- a/api/api.go +++ b/api/api.go @@ -41,12 +41,13 @@ const ( // audit actions getDatasetsAction = "getDatasets" getDatasetAction = "getDataset" + deleteDatasetAction = "deleteDataset" + addDatasetAction = "addDataset" + putDatasetAction = "putDataset" getEditionsAction = "getEditions" getEditionAction = "getEdition" getVersionsAction = "getVersions" getVersionAction = "getVersion" - deleteDatasetAction = "deleteDataset" - addDatasetAction = "addDataset" getDimensionsAction = "getDimensions" getMetadataAction = "getMetadata" diff --git a/api/dataset.go b/api/dataset.go index 05b01dde..67544b40 100644 --- a/api/dataset.go +++ b/api/dataset.go @@ -248,6 +248,9 @@ func (api *DatasetAPI) putDataset(w http.ResponseWriter, r *http.Request) { vars := mux.Vars(r) datasetID := vars["id"] data := log.Data{"dataset_id": datasetID} + auditParams := common.Params{"dataset_id": datasetID} + + api.auditor.Record(ctx, putDatasetAction, actionAttempted, auditParams) err := func() error { defer r.Body.Close() @@ -279,10 +282,13 @@ func (api *DatasetAPI) putDataset(w http.ResponseWriter, r *http.Request) { }() if err != nil { + api.auditor.Record(ctx, putDatasetAction, actionUnsuccessful, auditParams) handleErrorType(datasetDocType, err, w) return } + api.auditor.Record(ctx, putDatasetAction, actionSuccessful, auditParams) + setJSONContentType(w) w.WriteHeader(http.StatusOK) logInfo(ctx, "putDataset endpoint: request successful", data) diff --git a/api/dataset_test.go b/api/dataset_test.go index 5a90461e..ab53ba48 100644 --- a/api/dataset_test.go +++ b/api/dataset_test.go @@ -762,12 +762,19 @@ func TestPutDatasetReturnsSuccessfully(t *testing.T) { } mockedDataStore.UpdateDataset("123", dataset, models.CreatedState) - api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, getMockAuditor(), genericMockedObservationStore) + auditor := createAuditor("", "") + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditor, genericMockedObservationStore) api.router.ServeHTTP(w, r) + So(w.Code, ShouldEqual, http.StatusOK) So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) So(len(mockedDataStore.UpdateDatasetCalls()), ShouldEqual, 2) + + calls := auditor.RecordCalls() + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], putDatasetAction, actionAttempted, common.Params{"dataset_id": "123"}) + verifyAuditRecordCalls(calls[1], putDatasetAction, actionSuccessful, common.Params{"dataset_id": "123"}) }) } @@ -790,7 +797,8 @@ func TestPutDatasetReturnsError(t *testing.T) { }, } - api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, getMockAuditor(), genericMockedObservationStore) + auditor := createAuditor("", "") + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditor, genericMockedObservationStore) api.router.ServeHTTP(w, r) @@ -799,6 +807,11 @@ func TestPutDatasetReturnsError(t *testing.T) { So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 0) So(len(mockedDataStore.UpdateVersionCalls()), ShouldEqual, 0) + + calls := auditor.RecordCalls() + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], putDatasetAction, actionAttempted, common.Params{"dataset_id": "123"}) + verifyAuditRecordCalls(calls[1], putDatasetAction, actionUnsuccessful, common.Params{"dataset_id": "123"}) }) Convey("When the api cannot connect to datastore return an internal server error", t, func() { From 23484fed0c3b3a367d1c40472e512a4b7367fd20 Mon Sep 17 00:00:00 2001 From: Dave Llewellyn <439837@gmail.com> Date: Tue, 29 May 2018 09:26:20 +0100 Subject: [PATCH 32/34] updated more existing tests to verify audit calls --- api/dataset_test.go | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/api/dataset_test.go b/api/dataset_test.go index ab53ba48..e93d2efd 100644 --- a/api/dataset_test.go +++ b/api/dataset_test.go @@ -836,7 +836,8 @@ func TestPutDatasetReturnsError(t *testing.T) { } mockedDataStore.UpdateDataset("123", dataset, models.CreatedState) - api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, getMockAuditor(), genericMockedObservationStore) + auditor := createAuditor("", "") + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditor, genericMockedObservationStore) api.router.ServeHTTP(w, r) So(w.Code, ShouldEqual, http.StatusInternalServerError) @@ -844,6 +845,11 @@ func TestPutDatasetReturnsError(t *testing.T) { So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) So(len(mockedDataStore.UpdateDatasetCalls()), ShouldEqual, 2) + + calls := auditor.RecordCalls() + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], putDatasetAction, actionAttempted, common.Params{"dataset_id": "123"}) + verifyAuditRecordCalls(calls[1], putDatasetAction, actionUnsuccessful, common.Params{"dataset_id": "123"}) }) Convey("When the dataset document cannot be found return status not found ", t, func() { @@ -863,7 +869,8 @@ func TestPutDatasetReturnsError(t *testing.T) { }, } - api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, getMockAuditor(), genericMockedObservationStore) + auditor := createAuditor("", "") + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditor, genericMockedObservationStore) api.router.ServeHTTP(w, r) So(w.Code, ShouldEqual, http.StatusNotFound) @@ -871,6 +878,11 @@ func TestPutDatasetReturnsError(t *testing.T) { So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) So(len(mockedDataStore.UpdateDatasetCalls()), ShouldEqual, 0) + + calls := auditor.RecordCalls() + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], putDatasetAction, actionAttempted, common.Params{"dataset_id": "123"}) + verifyAuditRecordCalls(calls[1], putDatasetAction, actionUnsuccessful, common.Params{"dataset_id": "123"}) }) Convey("When the request is not authorised to update dataset return status not found", t, func() { @@ -889,7 +901,8 @@ func TestPutDatasetReturnsError(t *testing.T) { }, } - api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, getMockAuditor(), genericMockedObservationStore) + auditor := createAuditor("", "") + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditor, genericMockedObservationStore) api.router.ServeHTTP(w, r) So(w.Code, ShouldEqual, http.StatusUnauthorized) @@ -897,6 +910,7 @@ func TestPutDatasetReturnsError(t *testing.T) { So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 0) So(len(mockedDataStore.UpdateDatasetCalls()), ShouldEqual, 0) + So(len(auditor.RecordCalls()), ShouldEqual, 0) }) } From c7c4d3d2e1e305d0fbbca9c7e62d1b8c7ea6d742 Mon Sep 17 00:00:00 2001 From: Dave Llewellyn <439837@gmail.com> Date: Tue, 29 May 2018 10:09:56 +0100 Subject: [PATCH 33/34] added new test cases to conver audit unsuccessful cases --- api/dataset.go | 14 +++- api/dataset_test.go | 195 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 206 insertions(+), 3 deletions(-) diff --git a/api/dataset.go b/api/dataset.go index 67544b40..90f052e9 100644 --- a/api/dataset.go +++ b/api/dataset.go @@ -250,7 +250,11 @@ func (api *DatasetAPI) putDataset(w http.ResponseWriter, r *http.Request) { data := log.Data{"dataset_id": datasetID} auditParams := common.Params{"dataset_id": datasetID} - api.auditor.Record(ctx, putDatasetAction, actionAttempted, auditParams) + if err := api.auditor.Record(ctx, putDatasetAction, actionAttempted, auditParams); err != nil { + auditActionFailure(ctx, putDatasetAction, actionAttempted, err, data) + handleErrorType(datasetDocType, err, w) + return + } err := func() error { defer r.Body.Close() @@ -282,12 +286,16 @@ func (api *DatasetAPI) putDataset(w http.ResponseWriter, r *http.Request) { }() if err != nil { - api.auditor.Record(ctx, putDatasetAction, actionUnsuccessful, auditParams) + if err := api.auditor.Record(ctx, putDatasetAction, actionUnsuccessful, auditParams); err != nil { + auditActionFailure(ctx, putDatasetAction, actionUnsuccessful, err, data) + } handleErrorType(datasetDocType, err, w) return } - api.auditor.Record(ctx, putDatasetAction, actionSuccessful, auditParams) + if err := api.auditor.Record(ctx, putDatasetAction, actionSuccessful, auditParams); err != nil { + auditActionFailure(ctx, putDatasetAction, actionSuccessful, err, data) + } setJSONContentType(w) w.WriteHeader(http.StatusOK) diff --git a/api/dataset_test.go b/api/dataset_test.go index e93d2efd..6aa7f2ff 100644 --- a/api/dataset_test.go +++ b/api/dataset_test.go @@ -3,6 +3,7 @@ package api import ( "bytes" "context" + "encoding/json" "errors" "io" "net/http" @@ -914,6 +915,200 @@ func TestPutDatasetReturnsError(t *testing.T) { }) } +func TestPutDatasetAuditErrors(t *testing.T) { + ap := common.Params{"dataset_id": "123"} + + t.Parallel() + Convey("given audit action attempted returns an error", t, func() { + auditor := createAuditor(putDatasetAction, actionAttempted) + + Convey("when put dataset is called", func() { + r, err := createRequestWithAuth("PUT", "http://localhost:22000/datasets/123", bytes.NewBufferString(datasetPayload)) + So(err, ShouldBeNil) + + w := httptest.NewRecorder() + mockedDataStore := &storetest.StorerMock{ + GetDatasetFunc: func(string) (*models.DatasetUpdate, error) { + return &models.DatasetUpdate{Next: &models.Dataset{}}, nil + }, + UpdateDatasetFunc: func(string, *models.Dataset, string) error { + return nil + }, + } + + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditor, genericMockedObservationStore) + + api.router.ServeHTTP(w, r) + + Convey("then a 500 status is returned", func() { + So(w.Code, ShouldEqual, http.StatusInternalServerError) + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 0) + So(len(mockedDataStore.UpdateDatasetCalls()), ShouldEqual, 0) + + calls := auditor.RecordCalls() + So(len(calls), ShouldEqual, 1) + verifyAuditRecordCalls(calls[0], putDatasetAction, actionAttempted, ap) + }) + }) + }) + + Convey("given audit action successful returns an error", t, func() { + auditor := createAuditor(putDatasetAction, actionSuccessful) + + Convey("when a put dataset request is successful", func() { + r, err := createRequestWithAuth("PUT", "http://localhost:22000/datasets/123", bytes.NewBufferString(datasetPayload)) + So(err, ShouldBeNil) + + w := httptest.NewRecorder() + mockedDataStore := &storetest.StorerMock{ + GetDatasetFunc: func(string) (*models.DatasetUpdate, error) { + return &models.DatasetUpdate{Next: &models.Dataset{}}, nil + }, + UpdateDatasetFunc: func(string, *models.Dataset, string) error { + return nil + }, + } + + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditor, genericMockedObservationStore) + api.router.ServeHTTP(w, r) + + Convey("then a 200 status is returned", func() { + So(w.Code, ShouldEqual, http.StatusOK) + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) + So(len(mockedDataStore.UpdateDatasetCalls()), ShouldEqual, 1) + + calls := auditor.RecordCalls() + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], putDatasetAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], putDatasetAction, actionSuccessful, ap) + }) + }) + }) + + Convey("given audit action unsuccessful returns an error", t, func() { + auditor := createAuditor(putDatasetAction, actionUnsuccessful) + + Convey("when a put dataset request contains an invalid dataset body", func() { + r, err := createRequestWithAuth("PUT", "http://localhost:22000/datasets/123", bytes.NewBufferString("`zxcvbnm,./")) + So(err, ShouldBeNil) + + w := httptest.NewRecorder() + mockedDataStore := &storetest.StorerMock{ + GetDatasetFunc: func(string) (*models.DatasetUpdate, error) { + return &models.DatasetUpdate{Next: &models.Dataset{}}, nil + }, + UpdateDatasetFunc: func(string, *models.Dataset, string) error { + return nil + }, + } + + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditor, genericMockedObservationStore) + api.router.ServeHTTP(w, r) + + Convey("then a 400 status is returned", func() { + So(w.Code, ShouldEqual, http.StatusBadRequest) + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 0) + So(len(mockedDataStore.UpdateDatasetCalls()), ShouldEqual, 0) + + calls := auditor.RecordCalls() + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], putDatasetAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], putDatasetAction, actionUnsuccessful, ap) + }) + }) + + Convey("when datastore.getDataset returns an error", func() { + r, err := createRequestWithAuth("PUT", "http://localhost:22000/datasets/123", bytes.NewBufferString(datasetPayload)) + So(err, ShouldBeNil) + + w := httptest.NewRecorder() + mockedDataStore := &storetest.StorerMock{ + GetDatasetFunc: func(string) (*models.DatasetUpdate, error) { + return nil, errs.ErrDatasetNotFound + }, + } + + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditor, genericMockedObservationStore) + api.router.ServeHTTP(w, r) + + Convey("then a 400 status is returned", func() { + So(w.Code, ShouldEqual, http.StatusNotFound) + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) + So(len(mockedDataStore.UpdateDatasetCalls()), ShouldEqual, 0) + + calls := auditor.RecordCalls() + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], putDatasetAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], putDatasetAction, actionUnsuccessful, ap) + }) + }) + + Convey("when the requested dataset has a published state", func() { + + var publishedDataset models.Dataset + json.Unmarshal([]byte(datasetPayload), &publishedDataset) + publishedDataset.State = models.PublishedState + b, _ := json.Marshal(publishedDataset) + + r, err := createRequestWithAuth("PUT", "http://localhost:22000/datasets/123", bytes.NewBuffer(b)) + So(err, ShouldBeNil) + + w := httptest.NewRecorder() + mockedDataStore := &storetest.StorerMock{ + GetDatasetFunc: func(string) (*models.DatasetUpdate, error) { + return &models.DatasetUpdate{Next: &models.Dataset{}}, nil + }, + UpsertDatasetFunc: func(ID string, datasetDoc *models.DatasetUpdate) error { + return errors.New("upsertDataset error") + }, + } + + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditor, genericMockedObservationStore) + api.router.ServeHTTP(w, r) + + Convey("then a 400 status is returned", func() { + So(w.Code, ShouldEqual, http.StatusInternalServerError) + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) + So(len(mockedDataStore.UpsertDatasetCalls()), ShouldEqual, 1) + + calls := auditor.RecordCalls() + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], putDatasetAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], putDatasetAction, actionUnsuccessful, ap) + }) + }) + + Convey("when datastore.UpdateDataset returns an error", func() { + r, err := createRequestWithAuth("PUT", "http://localhost:22000/datasets/123", bytes.NewBufferString(datasetPayload)) + So(err, ShouldBeNil) + + w := httptest.NewRecorder() + mockedDataStore := &storetest.StorerMock{ + GetDatasetFunc: func(string) (*models.DatasetUpdate, error) { + return &models.DatasetUpdate{Next: &models.Dataset{}}, nil + }, + UpdateDatasetFunc: func(ID string, dataset *models.Dataset, currentState string) error { + return errors.New("update dataset error") + }, + } + + api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}, auditor, genericMockedObservationStore) + api.router.ServeHTTP(w, r) + + Convey("then a 400 status is returned", func() { + So(w.Code, ShouldEqual, http.StatusInternalServerError) + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) + So(len(mockedDataStore.UpdateDatasetCalls()), ShouldEqual, 1) + + calls := auditor.RecordCalls() + So(len(calls), ShouldEqual, 2) + verifyAuditRecordCalls(calls[0], putDatasetAction, actionAttempted, ap) + verifyAuditRecordCalls(calls[1], putDatasetAction, actionUnsuccessful, ap) + }) + }) + }) +} + func TestDeleteDatasetReturnsSuccessfully(t *testing.T) { t.Parallel() Convey("A successful request to delete dataset returns 200 OK response", t, func() { From 55e93e482c0d3c65ce04a9e80a0d6954fe5cace8 Mon Sep 17 00:00:00 2001 From: Dave Llewellyn <439837@gmail.com> Date: Tue, 29 May 2018 10:35:02 +0100 Subject: [PATCH 34/34] refactored dataset endpoints to use separate func for resolving error response status codes --- api/dataset.go | 52 ++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 40 insertions(+), 12 deletions(-) diff --git a/api/dataset.go b/api/dataset.go index 90f052e9..c9ea158a 100644 --- a/api/dataset.go +++ b/api/dataset.go @@ -1,6 +1,7 @@ package api import ( + "context" "encoding/json" "fmt" "net/http" @@ -18,7 +19,7 @@ func (api *DatasetAPI) getDatasets(w http.ResponseWriter, r *http.Request) { ctx := r.Context() if err := api.auditor.Record(ctx, getDatasetsAction, actionAttempted, nil); err != nil { auditActionFailure(ctx, getDatasetsAction, actionAttempted, err, nil) - handleErrorType(datasetDocType, errs.ErrAuditActionAttemptedFailure, w) + handleDatasetAPIErr(ctx, errs.ErrAuditActionAttemptedFailure, w, nil) return } @@ -60,7 +61,7 @@ func (api *DatasetAPI) getDatasets(w http.ResponseWriter, r *http.Request) { if auditErr := api.auditor.Record(ctx, getDatasetsAction, actionUnsuccessful, nil); auditErr != nil { auditActionFailure(ctx, getDatasetsAction, actionUnsuccessful, auditErr, nil) } - handleErrorType(datasetDocType, err, w) + handleDatasetAPIErr(ctx, err, w, nil) return } @@ -86,7 +87,7 @@ func (api *DatasetAPI) getDataset(w http.ResponseWriter, r *http.Request) { if auditErr := api.auditor.Record(ctx, getDatasetAction, actionAttempted, auditParams); auditErr != nil { auditActionFailure(ctx, getDatasetAction, actionAttempted, auditErr, logData) - handleErrorType(datasetDocType, errs.ErrInternalServer, w) + handleDatasetAPIErr(ctx, errs.ErrInternalServer, w, logData) return } @@ -132,13 +133,13 @@ func (api *DatasetAPI) getDataset(w http.ResponseWriter, r *http.Request) { if auditErr := api.auditor.Record(ctx, getDatasetAction, actionUnsuccessful, auditParams); auditErr != nil { auditActionFailure(ctx, getDatasetAction, actionUnsuccessful, auditErr, logData) } - handleErrorType(datasetDocType, err, w) + handleDatasetAPIErr(ctx, err, w, logData) return } if auditErr := api.auditor.Record(ctx, getDatasetAction, actionSuccessful, auditParams); auditErr != nil { auditActionFailure(ctx, getDatasetAction, actionSuccessful, auditErr, logData) - handleErrorType(datasetDocType, errs.ErrInternalServer, w) + handleDatasetAPIErr(ctx, errs.ErrInternalServer, w, logData) return } @@ -161,7 +162,7 @@ func (api *DatasetAPI) addDataset(w http.ResponseWriter, r *http.Request) { if err := api.auditor.Record(ctx, addDatasetAction, actionAttempted, auditParams); err != nil { auditActionFailure(ctx, addDatasetAction, actionAttempted, err, logData) - handleErrorType(datasetDocType, errs.ErrInternalServer, w) + handleDatasetAPIErr(ctx, errs.ErrInternalServer, w, logData) return } @@ -225,12 +226,12 @@ func (api *DatasetAPI) addDataset(w http.ResponseWriter, r *http.Request) { if auditErr := api.auditor.Record(ctx, addDatasetAction, actionUnsuccessful, auditParams); auditErr != nil { auditActionFailure(ctx, addDatasetAction, actionUnsuccessful, auditErr, logData) } - handleErrorType(datasetDocType, err, w) + handleDatasetAPIErr(ctx, err, w, logData) return } if auditErr := api.auditor.Record(ctx, addDatasetAction, actionSuccessful, auditParams); auditErr != nil { - auditActionFailure(ctx, addDatasetAction, actionUnsuccessful, auditErr, logData) + auditActionFailure(ctx, addDatasetAction, actionSuccessful, auditErr, logData) } setJSONContentType(w) @@ -252,7 +253,7 @@ func (api *DatasetAPI) putDataset(w http.ResponseWriter, r *http.Request) { if err := api.auditor.Record(ctx, putDatasetAction, actionAttempted, auditParams); err != nil { auditActionFailure(ctx, putDatasetAction, actionAttempted, err, data) - handleErrorType(datasetDocType, err, w) + handleDatasetAPIErr(ctx, err, w, data) return } @@ -289,7 +290,8 @@ func (api *DatasetAPI) putDataset(w http.ResponseWriter, r *http.Request) { if err := api.auditor.Record(ctx, putDatasetAction, actionUnsuccessful, auditParams); err != nil { auditActionFailure(ctx, putDatasetAction, actionUnsuccessful, err, data) } - handleErrorType(datasetDocType, err, w) + + handleDatasetAPIErr(ctx, err, w, data) return } @@ -342,7 +344,7 @@ func (api *DatasetAPI) deleteDataset(w http.ResponseWriter, r *http.Request) { if err := api.auditor.Record(ctx, deleteDatasetAction, actionAttempted, auditParams); err != nil { auditActionFailure(ctx, deleteDatasetAction, actionAttempted, err, logData) - handleErrorType(datasetDocType, err, w) + handleDatasetAPIErr(ctx, err, w, logData) return } @@ -377,7 +379,7 @@ func (api *DatasetAPI) deleteDataset(w http.ResponseWriter, r *http.Request) { if auditErr := api.auditor.Record(ctx, deleteDatasetAction, actionUnsuccessful, auditParams); auditErr != nil { auditActionFailure(ctx, deleteDatasetAction, actionUnsuccessful, auditErr, logData) } - handleErrorType(datasetDocType, err, w) + handleDatasetAPIErr(ctx, err, w, logData) return } @@ -401,3 +403,29 @@ func mapResults(results []models.DatasetUpdate) []*models.Dataset { } return items } + +func handleDatasetAPIErr(ctx context.Context, err error, w http.ResponseWriter, data log.Data) { + if data == nil { + data = log.Data{} + } + + var status int + switch { + case err == errs.ErrDeletePublishedDatasetForbidden: + status = http.StatusForbidden + case err == errs.ErrAddDatasetAlreadyExists: + status = http.StatusForbidden + case err == errs.ErrDatasetNotFound: + status = http.StatusNotFound + case err == errs.ErrDeleteDatasetNotFound: + status = http.StatusNoContent + case err == errs.ErrAddUpdateDatasetBadRequest: + status = http.StatusBadRequest + default: + status = http.StatusInternalServerError + } + + data["responseStatus"] = status + logError(ctx, errors.WithMessage(err, "request unsuccessful"), data) + http.Error(w, err.Error(), status) +}