Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
floehopper opened this issue Jan 30, 2018 · 11 comments
Closed

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

floehopper opened this issue Jan 30, 2018 · 11 comments
Assignees
@chrisroos
Milestone
Serve Whitehall a...

Comments

@floehopper
Copy link
Contributor

e.g. limited to groups of users in an organisation
@chrisroos chrisroos mentioned this issue Jan 31, 2018
Closed
14 tasks
@chrisroos chrisroos self-assigned this Feb 2, 2018
@chrisroos
Copy link
Contributor

I'm going to start working on this.

@chrisroos
Copy link
Contributor

chrisroos commented Feb 14, 2018 •
edited
Loading

I'm working on the changes necessary to Asset Manager in WIP: Allow draft asset access to be restricted to specific users #471.

I'm working on the changes necessary to Whitehall in WIP: Send access_limited data to Asset Manager #3767.

@chrisroos chrisroos mentioned this issue Feb 27, 2018
Closed
@chrisroos
Copy link
Contributor

The changes in #471 and alphagov/whitehall#3767 have been deployed to production.

@floehopper realised that we also need to handle attachments created by the BulkUploadsController in Whitehall. I had hoped that the BulkUploadsController might not be used any longer but there are (a small number of) requests for it within the last 14 days so I think we do need to handle these attachments.

@chrisroos
Copy link
Contributor

I made the changes to the BulkUploadsController in Whitehall in alphagov/whitehall#3822 which has also been merged to master.

I plan to test the functionality in integration to ensure that it's working as expected.

@floehopper
Copy link
Contributor Author

@chrisroos Does this work for OfficialStatistics & NationalStatistics publication types which are access-limited by default?

@chrisroos
Copy link
Contributor

Good question, @floehopper. I'll investigate.

@chrisroos
Copy link
Contributor

@chrisroos Does this work for OfficialStatistics & NationalStatistics publication types which are access-limited by default?

I've checked and believe that this will just work because setting Publication#publication_type to something that's access limited by default will set the access_limited attribute of that publication to true:

> Publication.new.access_limited?
=> nil
> Publication.new(publication_type: PublicationType::OfficialStatistics).access_limited?
=> true
> Publication.new(publication_type: PublicationType::PolicyPaper).access_limited?
=> false

@floehopper
Copy link
Contributor Author

👍

@chrisroos
Copy link
Contributor

I've just tested this in integration and think I've found a problem. The access_limited array of UIDs doesn't appear to be reset to an empty array when an access-limited edition in Whitehall is updated so that it's no longer access-limited. I'm going to investigate further.

chrisroos added a commit that referenced this issue Mar 6, 2018
@chrisroos
Allow access_limited to be reset on update
aadc1af 
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. The fix is to handle 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.

[1]: #450
@chrisroos chrisroos mentioned this issue Mar 6, 2018
Merged
@chrisroos
Copy link
Contributor

I've opened #510 to fix the problem that occurs when we try to reset the access-limited state of an asset.

chrisroos added a commit that referenced this issue Mar 7, 2018
@chrisroos
Allow access_limited to be reset on update
9c88be7 
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.

[1]: #450
chrisroos added a commit that referenced this issue Mar 7, 2018
@chrisroos
Allow access_limited to be reset on update
94ba225 
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
@chrisroos
Copy link
Contributor

PR #510 has been merged and deployed to integration and I've now confirmed that this functionality is working as expected.

I created an access-limited consultation in Whitehall integration and added two attachments: one directly and one via the bulk upload functionality. I observed that both attachments were marked as access-limited in Asset Manager. I then removed the access-limiting from the consultation and observed that both attachments were no longer marked as access-limited in Asset Manager.

I'm happy that this is working as expected so I'm closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@chrisroos chrisroos

Labels
None yet
Projects
None yet
Milestone
Serve Whitehall attachment assets fro...
Development

No branches or pull requests
2 participants
@floehopper @chrisroos