Allow access_limited to be reset on update #510

chrisroos · 2018-03-06T12:02:42Z

While investigating issue #450, 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.

floehopper

I've added some comment which are mostly/all just suggestions for improvements. I'm happy for you to address them as you see fit. Marking as approved.

floehopper · 2018-03-07T10:37:20Z

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)


I think it would be useful to explain why you have to operate on the params before the calls to require and permit.

I've updated the commit note to explain why I need to modify the params before calling require and permit.

floehopper · 2018-03-07T10:45:51Z

app/controllers/assets_controller.rb

    exclude_blank_redirect_url(
-      params
+      asset_params
        .require(:asset)
        .permit(:file, :draft, :redirect_url, :replacement_id, :parent_document_url, access_limited: [])
    )


A possible alternative approach which I think looks a bit cleaner:

def asset_params exclude_blank_redirect_url( handle_empty_access_limited_param(params) .require(:asset) .permit(:file, :draft, :redirect_url, :replacement_id, :parent_document_url, access_limited: []) ) ) end

I've changed this in a new commit in this branch.

floehopper · 2018-03-07T10:46:49Z

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)


Minor: I think the _param suffix is unnecessary, given params are passed in as an argument, and inconsistent with exclude_blank_redirect_url. I'd prefer the method name to be handle_empty_access_limited.

Minor: Possibly rename both these methods as follows:

exclude_blank_redirect_url -> normalize_redirect_url

handle_empty_access_limited_param -> normalize_access_limited

I've renamed the two methods as you've suggested - in two new commits in this branch.

floehopper · 2018-03-07T10:50:58Z

app/controllers/base_assets_controller.rb

+    end
+    params
+  end
+


This seems quite "ugly", but I can't see a way to simplify it. I think it would be useful to mention other solutions you considered, e.g. having two fields: Asset#access_limit? & Asset#authorized_users, and ideally capturing these ideas in a GitHub issue.

I've updated the commit note in the initial commit to explain the alternative approach of having two fields.

I've captured the idea of using two fields in #511.

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]: #450

@floehopper

As suggested by @floehopper.

@floehopper

As suggested by @floehopper.

@floehopper

As suggested by @floehopper.

chrisroos · 2018-03-07T15:51:29Z

Thanks for reviewing, @floehopper. I've addressed your feedback, rebased on master, force pushed and merged!

chrisroos requested review from floehopper and chrislo March 6, 2018 12:02

chrisroos mentioned this pull request Mar 6, 2018

Implement access-limiting behaviour for Whitehall attachments in Asset Manager #450

Closed

floehopper self-assigned this Mar 6, 2018

floehopper approved these changes Mar 7, 2018

View reviewed changes

chrisroos force-pushed the allow-access-limited-to-be-reset-on-update branch from aadc1af to ee64cb6 Compare March 7, 2018 15:34

chrisroos added 4 commits March 7, 2018 15:38

Avoid unnecessary variable in AssetsController#asset_params

e340058

As suggested by @floehopper.

Rename BaseAssetsController#exclude_blank_redirect_url

56f23c4

As suggested by @floehopper.

Rename BaseAssetsController#handle_empty_access_limited_param

f2c34a9

As suggested by @floehopper.

chrisroos force-pushed the allow-access-limited-to-be-reset-on-update branch from ee64cb6 to f2c34a9 Compare March 7, 2018 15:40

chrisroos merged commit 8ea0c5b into master Mar 7, 2018

chrisroos deleted the allow-access-limited-to-be-reset-on-update branch March 7, 2018 15:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow access_limited to be reset on update #510

Allow access_limited to be reset on update #510

chrisroos commented Mar 6, 2018 •

edited

Loading

floehopper left a comment

floehopper Mar 7, 2018

chrisroos Mar 7, 2018

floehopper Mar 7, 2018

chrisroos Mar 7, 2018

floehopper Mar 7, 2018

floehopper Mar 7, 2018

chrisroos Mar 7, 2018

floehopper Mar 7, 2018

chrisroos Mar 7, 2018

chrisroos commented Mar 7, 2018

Allow access_limited to be reset on update #510

Allow access_limited to be reset on update #510

Conversation

chrisroos commented Mar 6, 2018 • edited Loading

floehopper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisroos commented Mar 7, 2018

chrisroos commented Mar 6, 2018 •

edited

Loading