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.

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

Upload switch to allow sharing samples with 3rd party services #801

Merged
merged 17 commits into from
May 11, 2023

Conversation

Repumba
Copy link
Contributor

@Repumba Repumba commented Apr 20, 2023

Your checklist for this pull request

  • I've read the contributing guideline.
  • I've tested my changes by building and running the project, and testing changed functionality (if applicable)
  • I've added automated tests for my change (if applicable, optional)
  • I've updated documentation to reflect my change (if applicable)

What is the current behaviour?

What is the new behaviour?

Make it possible to disallow sharing samples with 3rd parties
Decision is made during upload. If you have modify_3rd_party_sharing capability you can change it in front-end

Front end:
image
New box visible only for users with modify_3rd_party_sharing capability

image
Checkbox is visible for every user

Test plan

I've added automatic tests for changing share_3rd_party attribute and for re-uploading samples with different values for share_3rd_party

Closing issues

closes #740

@Repumba Repumba changed the title migration and initial changes Upload switch to allow sharing samples with 3rd party services Apr 20, 2023
@Repumba Repumba marked this pull request as ready for review April 27, 2023 09:45
Copy link
Member

@psrok1 psrok1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK in general, added minor suggestions.

Another things that came to my mind and could be added in next PRs:

  • feature is mostly useful for mwdb.cert.pl instance. It would be nice to have option in configuration ENABLE_3RD_PARTY_SHARING_CONSENT that will hide that switch from UI if set to False (which is default) as it can be confusing for self-hosted instance users. MWDB Core still doesn't send anything to 3rd parties on its own.
  • Switch position could be prettier (e.g. on side of the label) but I haven't managed to do that quickly.

@@ -135,3 +135,17 @@ MWDB-Core 2.3.0 includes automatic migration spawned on ``mwdb-core configure``
- enable ``karton_reanalyze`` for all groups having ``karton_manage`` capability before.

Built-in integration emulates the original ``karton`` attribute behavior and still exposes and accepts the values provided that way.

Sharing with third parties
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think better place for that section is https://mwdb.readthedocs.io/en/latest/user-guide/9-Sharing-objects.html with making karton a link to https://mwdb.readthedocs.io/en/latest/karton-guide.html (for someone who doesn't know what is Karton)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

selected_object.share_3rd_party = True
db.session.commit()

return "OK"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HTTP 200 without content (as in other endpoints) might be better than non-JSON "OK", because this will crash response.json()

Suggested change
return "OK"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -547,6 +555,11 @@ function removeKartonAnalysisFromObject(id, analysis_id) {
return axios.delete(`/object/${id}/karton/${analysis_id}`);
}

function enableSharing3rdParty(identifier) {
console.log(`/object/${identifier}/share_3rd_party`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
console.log(`/object/${identifier}/share_3rd_party`);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -33,6 +33,7 @@ def send_file_to_karton(file) -> str:
"attributes": file.get_attributes(
as_dict=True, check_permissions=False
),
"share_3rd_party": file.share_3rd_party,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add it as another header ("3rd_party": file.share_3rd_party) instead of payload so we don't route objects with share_3rd_party=False to 3rd-party consumers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sidenote: that's another usecase for flags mentioned in CERT-Polska/karton#167

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@psrok1 psrok1 merged commit 2803e99 into master May 11, 2023
@psrok1 psrok1 deleted the feature/3rd-party-sharing branch May 11, 2023 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upload switch: Disallow to share sample with 3rd party services.
2 participants