Issue 1199 add sharing permisions to sample types #1953

kdp-cloud · 2024-07-10T12:16:36Z

Users are now able to set policies with permissions.
The current rules for viewing, editing and deleting should still remain the same.
There is also a upgrade job that adds policies to the existing sample types.

…o project member

Add extra constraints to being ISA-JSON compliant like must have a template id and must only be linked to ISA-JSON compliant investigations.

Since investigations can be associated through studies and assay. Better write a custom related_investigations method.

stuzart

I think the main problem is the mixing of explicit permissions then being over-ruled by the old implicit permissions

I've not checked the tests as these may be affected by changes

stuzart · 2024-09-11T09:28:28Z

app/controllers/sample_types_controller.rb

@@ -186,21 +215,22 @@ def find_sample_type
    @sample_type = scope.find(params[:id])
  end

-  #intercepts the standard 'find_and_authorize_requested_item' for additional special check for a referring_sample_id
+  # intercepts the standard 'find_and_authorize_requested_item' for additional special check for a referring_sample_id
  def authorize_requested_sample_type


I feel this method is redundant now. If sharing permissions have been set explicitly I don't think we should be revealing hidden sample types based on linked sample any more.

stuzart · 2024-09-11T09:37:32Z

app/models/sample_type.rb

-
+  def can_download?(user = User.current_user)
+    can_view?(user)
+  end
  def can_view?(user = User.current_user, referring_sample = nil, view_in_single_page = false)


this should left to the sharing permissions set now, rather than over-ruling them. I would confusing to users if they think they've set it to hidden but then find it is visible. Could also lead to mistrust that other items, such as data, they've set as private is magically being revealed without their knowledge.

I believe this was discussed in the samples working group and decided to leave it to the user to set sensible permissions

stuzart · 2024-09-11T09:39:59Z

app/models/sample_type.rb

  end

+  def can_edit?(user = User.current_user)


again, it should down to the sharing permissions set rather than over-ruling to make more permissive

stuzart · 2024-09-11T09:43:44Z

lib/seek/acts_as_asset/content_blobs.rb

@@ -5,7 +5,7 @@ module ContentBlobs
      module ClassMethods
        extend ActiveSupport::Concern
        included do
-          after_destroy :mark_deleted_content_blobs
+          after_destroy :mark_deleted_content_blobs, unless: -> { self.class == SampleType }


why not for sample types ?

stuzart · 2024-09-11T10:15:34Z

lib/tasks/seek_upgrades.rake

+          st.update_column(:policy_id, st.assays.first.policy_id) if st.assays.any?
+          st.update_column(:policy_id, st.studies.first.policy_id) if st.studies.any?
+        else
+          project = st.projects.first


what about multiple projects ?

stuzart · 2024-09-11T10:16:33Z

lib/tasks/seek_upgrades.rake

+        else
+          project = st.projects.first
+          if project.use_default_policy
+            st.update_column(:policy_id, project.default_policy_id)


this will create a reference to the default policy, which will then also be changed if the ST sharing permissions are changed. I'm also not sure it's right

stuzart · 2024-09-11T10:17:47Z

lib/tasks/seek_upgrades.rake

+            policy = Policy.new
+            policy.name = 'default policy'
+            policy.access_type = Policy::NO_ACCESS
+            policy.permissions << Permission.create(contributor_type: Permission::PERSON, contributor: st.contributor,


contributor will already have manage permissions by default

stuzart · 2024-09-11T10:18:50Z

lib/tasks/seek_upgrades.rake

+            policy.access_type = Policy::NO_ACCESS
+            policy.permissions << Permission.create(contributor_type: Permission::PERSON, contributor: st.contributor,
+                                                    access_type: Policy::MANAGING)
+            project.people.each do |person|


should just add the permission for the project, not each person.

stuzart · 2024-09-11T10:21:09Z

lib/tasks/seek_upgrades.rake

@@ -235,6 +236,40 @@ namespace :seek do
    end
  end

+  task(add_policies_to_existing_sample_types: [:environment]) do


I think generally this needs a rethink and currently doesn't reflect the original permissions.

Would need checking more closely, but generally:

make visible if linked to public samples, otherwise hidden

add visible permissions for each project

give manage rights to the project administrators

kdp-cloud added the enhancement label Jul 10, 2024

kdp-cloud added this to the 1.16.0 milestone Jul 10, 2024

kdp-cloud self-assigned this Jul 10, 2024

kdp-cloud linked an issue Jul 10, 2024 that may be closed by this pull request

Add sharing permissions to Sample Type #1199

Open

kdp-cloud marked this pull request as ready for review July 10, 2024 19:09

kdp-cloud added 23 commits July 24, 2024 14:53

Add migration for sample_type_auth_lookup table

5793a5b

Typo

53adc21

Apply migration to schema

08424e2

Implementation of acts_as_asset

45c905b

Write upgrade task

089d33b

Add missing routes

c91dd6c

Keep Rubocop happy

edfb8f6

Include the assets methods and routes

869abfa

Add manage page

f025c5b

Make sure contributors don't get a double permission when they're als…

8641699

…o project member

Can't access manage page of ISA-JSON compliant sample types

2bb4864

Make sure user can't manage_update ISA-JSON compliant sample types

080a4c8

Change is_isa_json_compliant

dcd8e57

Add extra constraints to being ISA-JSON compliant like must have a template id and must only be linked to ISA-JSON compliant investigations.

Remove comment out test

ff53254

Convert into before_action

cadf689

Add controller test for accessing the manage page

4a26951

Change ISA-JSON compliant factories

2a824fc

Add collections as resources

0777ba8

Add has_content_blobs concern

a364e38

Remove custom auth methods

bc3d9e7

Fix controller tests

7a80dbc

custom related investigations instead of association through studies

0aca7dd

Since investigations can be associated through studies and assay. Better write a custom related_investigations method.

Skip action if is SampleType

2763620

kdp-cloud added 12 commits July 24, 2024 14:55

Fix sample type tests

ef7fe5f

Add policy and discussion_links to schema

14103b5

Fix some more tests

b21a9a2

Add permissions partial

be30864

Update sharing policies and tags

5838a2b

Add manage sample type button to show page

4dd1be2

Write controller test for managing sample types

8e93be0

Reuse attributes

7c56386

Test for isa_study sample type permissions

01385ea

Simplify isa assays controller test

f4a912c

Test for isa_assay sample type permissions

60605a4

Typo

2a90c9b

kdp-cloud force-pushed the issue_1199_add_sharing_permisions_to_sample_types branch from 9c27d04 to 2a90c9b Compare July 24, 2024 13:01

Update policies before linking to sample types

c116e8c

stuzart self-requested a review September 11, 2024 09:12

stuzart requested changes Sep 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 1199 add sharing permisions to sample types #1953

Issue 1199 add sharing permisions to sample types #1953

kdp-cloud commented Jul 10, 2024

stuzart left a comment

stuzart Sep 11, 2024

stuzart Sep 11, 2024

stuzart Sep 11, 2024

stuzart Sep 11, 2024

stuzart Sep 11, 2024

stuzart Sep 11, 2024

stuzart Sep 11, 2024

stuzart Sep 11, 2024

stuzart Sep 11, 2024

Issue 1199 add sharing permisions to sample types #1953

Are you sure you want to change the base?

Issue 1199 add sharing permisions to sample types #1953

Conversation

kdp-cloud commented Jul 10, 2024

stuzart 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