From 94ba22562ad76bc660d711c830e574062d4846c1 Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Tue, 6 Mar 2018 11:57:34 +0000 Subject: [PATCH 1/4] Allow access_limited to be reset on update While investigating issue 450[1], I noticed that resetting/removing the access_limited status of an asset wasn't working as expected. This is because when we pass an empty `access_limited` array to gds-api-adapters/rest-client it's turned into an empty string in Asset Manager. This results in an `UnpermittedParameter` exception for the `access_limited` parameter in Asset Manager as it's expecting an Array but receives a String. The fix is to detect the empty string in the controller and convert it back into the empty array that's intended. I don't think this is particularly nice but I think it's good enough for now. Note that I haven't had to use `handle_empty_access_limited_param` in `WhitehallAssetsController#asset_params` as the problem only occurs when _updating_ an access-limited asset, and the `WhitehallAssetsController` is only responsible for _creating_ assets. I also considered using two fields to implement the access-limiting functionality: a boolean to control whether access-limiting was enabled in conjunction with an array of user UIDs who are authorised to view the asset. I decided against that approach because it's more work and because it means the implementation in Asset Manager diverges from the access-limiting functionality in the Content Store. [1]: https://github.com/alphagov/asset-manager/issues/450 --- app/controllers/assets_controller.rb | 3 ++- app/controllers/base_assets_controller.rb | 7 +++++++ spec/controllers/assets_controller_spec.rb | 11 +++++++++++ 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/app/controllers/assets_controller.rb b/app/controllers/assets_controller.rb index 8630474b..a92c0de1 100644 --- a/app/controllers/assets_controller.rb +++ b/app/controllers/assets_controller.rb @@ -38,8 +38,9 @@ def restrict_request_format end def asset_params + asset_params = handle_empty_access_limited_param(params) exclude_blank_redirect_url( - params + asset_params .require(:asset) .permit(:file, :draft, :redirect_url, :replacement_id, :parent_document_url, access_limited: []) ) diff --git a/app/controllers/base_assets_controller.rb b/app/controllers/base_assets_controller.rb index f2bd2dc2..a8322762 100644 --- a/app/controllers/base_assets_controller.rb +++ b/app/controllers/base_assets_controller.rb @@ -22,6 +22,13 @@ def exclude_blank_redirect_url(params) params.reject { |k, v| (k.to_sym == :redirect_url) && v.blank? } end + def handle_empty_access_limited_param(params) + if params.has_key?(:asset) && params[:asset].has_key?(:access_limited) && params[:asset][:access_limited].empty? + params[:asset][:access_limited] = [] + end + params + end + def cache_control AssetManager.cache_control end diff --git a/spec/controllers/assets_controller_spec.rb b/spec/controllers/assets_controller_spec.rb index 1b0904fb..494b5058 100644 --- a/spec/controllers/assets_controller_spec.rb +++ b/spec/controllers/assets_controller_spec.rb @@ -195,6 +195,17 @@ expect(assigns(:asset).access_limited).to eq(['user-id']) end + it 'resets access_limited to an empty array for an existing asset with an access_limited array' do + asset.update_attributes!(access_limited: ['user-uid']) + + # We have to use an empty string as that is what gds-api-adapters/rest-client + # will generate instead of an empty array + attributes = valid_attributes.merge(access_limited: '') + put :update, params: { id: asset.id, asset: attributes } + + expect(assigns(:asset).access_limited).to eq([]) + end + it 'stores redirect_url on existing asset' do redirect_url = 'https://example.com/path/file.ext' attributes = valid_attributes.merge(redirect_url: redirect_url) From e340058fa23fb783ad8fa548f2cbeb18d60f94e7 Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Wed, 7 Mar 2018 15:25:38 +0000 Subject: [PATCH 2/4] Avoid unnecessary variable in AssetsController#asset_params As suggested by @floehopper. --- app/controllers/assets_controller.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/controllers/assets_controller.rb b/app/controllers/assets_controller.rb index a92c0de1..a2e8c80f 100644 --- a/app/controllers/assets_controller.rb +++ b/app/controllers/assets_controller.rb @@ -38,9 +38,8 @@ def restrict_request_format end def asset_params - asset_params = handle_empty_access_limited_param(params) exclude_blank_redirect_url( - asset_params + handle_empty_access_limited_param(params) .require(:asset) .permit(:file, :draft, :redirect_url, :replacement_id, :parent_document_url, access_limited: []) ) From 56f23c48e457520b2ba6685ceb5983cc7f05fb3d Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Wed, 7 Mar 2018 15:27:09 +0000 Subject: [PATCH 3/4] Rename BaseAssetsController#exclude_blank_redirect_url As suggested by @floehopper. --- app/controllers/assets_controller.rb | 2 +- app/controllers/base_assets_controller.rb | 2 +- app/controllers/whitehall_assets_controller.rb | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/controllers/assets_controller.rb b/app/controllers/assets_controller.rb index a2e8c80f..461360d4 100644 --- a/app/controllers/assets_controller.rb +++ b/app/controllers/assets_controller.rb @@ -38,7 +38,7 @@ def restrict_request_format end def asset_params - exclude_blank_redirect_url( + normalize_redirect_url( handle_empty_access_limited_param(params) .require(:asset) .permit(:file, :draft, :redirect_url, :replacement_id, :parent_document_url, access_limited: []) diff --git a/app/controllers/base_assets_controller.rb b/app/controllers/base_assets_controller.rb index a8322762..94a63610 100644 --- a/app/controllers/base_assets_controller.rb +++ b/app/controllers/base_assets_controller.rb @@ -18,7 +18,7 @@ def create protected - def exclude_blank_redirect_url(params) + def normalize_redirect_url(params) params.reject { |k, v| (k.to_sym == :redirect_url) && v.blank? } end diff --git a/app/controllers/whitehall_assets_controller.rb b/app/controllers/whitehall_assets_controller.rb index 5d119d19..1580bbbf 100644 --- a/app/controllers/whitehall_assets_controller.rb +++ b/app/controllers/whitehall_assets_controller.rb @@ -10,7 +10,7 @@ def create private def asset_params - exclude_blank_redirect_url( + normalize_redirect_url( params .require(:asset) .permit( From f2c34a9574a6546a3f22d934fa02eb660795eee5 Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Wed, 7 Mar 2018 15:27:57 +0000 Subject: [PATCH 4/4] Rename BaseAssetsController#handle_empty_access_limited_param As suggested by @floehopper. --- app/controllers/assets_controller.rb | 2 +- app/controllers/base_assets_controller.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/assets_controller.rb b/app/controllers/assets_controller.rb index 461360d4..a8814fc4 100644 --- a/app/controllers/assets_controller.rb +++ b/app/controllers/assets_controller.rb @@ -39,7 +39,7 @@ def restrict_request_format def asset_params normalize_redirect_url( - handle_empty_access_limited_param(params) + normalize_access_limited(params) .require(:asset) .permit(:file, :draft, :redirect_url, :replacement_id, :parent_document_url, access_limited: []) ) diff --git a/app/controllers/base_assets_controller.rb b/app/controllers/base_assets_controller.rb index 94a63610..700055c4 100644 --- a/app/controllers/base_assets_controller.rb +++ b/app/controllers/base_assets_controller.rb @@ -22,7 +22,7 @@ def normalize_redirect_url(params) params.reject { |k, v| (k.to_sym == :redirect_url) && v.blank? } end - def handle_empty_access_limited_param(params) + def normalize_access_limited(params) if params.has_key?(:asset) && params[:asset].has_key?(:access_limited) && params[:asset][:access_limited].empty? params[:asset][:access_limited] = [] end