From e1cbb23e6daf67a69e1b1b850d9a21d7f99f40d4 Mon Sep 17 00:00:00 2001 From: maxsibilla Date: Fri, 5 Apr 2024 12:25:43 -0400 Subject: [PATCH 1/6] Create new method to check if user has the correct privileges to update an entity --- docker/docker-compose.yml | 2 +- src/app.py | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/docker/docker-compose.yml b/docker/docker-compose.yml index f0df66ad..f20edf01 100644 --- a/docker/docker-compose.yml +++ b/docker/docker-compose.yml @@ -37,4 +37,4 @@ services: networks: # This is the network created by gateway to enable communicaton between multiple docker-compose projects sennet_docker_network: - external: true + external: false diff --git a/src/app.py b/src/app.py index d313ce2a..10de0fc7 100644 --- a/src/app.py +++ b/src/app.py @@ -1,6 +1,8 @@ import ast import collections from datetime import datetime +from typing import List + from flask import Flask, g, jsonify, request from neo4j.exceptions import TransactionError import os @@ -1237,6 +1239,9 @@ def update_entity(id): # Get target entity and return as a dict if exists entity_dict = query_target_entity(id, user_token) + # Check that the user has the correct access to modify this entity + validate_user_update_privilege(entity_dict, user_token) + # Normalize user provided entity_type normalized_entity_type = schema_manager.normalize_entity_type(entity_dict['entity_type']) @@ -4772,6 +4777,35 @@ def access_level_prefix_dir(dir_name): return hm_file_helper.ensureTrailingSlashURL(hm_file_helper.ensureBeginningSlashURL(dir_name)) +""" +Check if a user has valid access to update a given entity + +Parameters +---------- +entity : str + The entity that is attempting to be updated +user_token : str + The token passed in via the request header that will be used to authenticate + +""" + + +def validate_user_update_privilege(entity, user_token): + # A user has update privileges if they are a data admin or are in the same group that registered the entity + is_admin = auth_helper_instance.has_data_admin_privs(user_token) + if isinstance(is_admin, Response): + is_admin = False + + user_write_groups: List[dict] = auth_helper_instance.get_user_write_groups(user_token) + if isinstance(user_write_groups, Response): + abort_forbidden(f"Entity with ID: {entity['sennet_id']} is not editable without presenting a token.") + + user_group_uuids = [d['uuid'] for d in user_write_groups] + if entity['group_uuid'] not in user_group_uuids and is_admin is False: + abort_forbidden(f"User does not have write privileges for entity with ID: {entity['sennet_id']}. " + f"Reach out to the help desk to request access to this group.") + + """ Formats error into dict From f36f3ab29bc8966419a9dc2dc4f96873744c0867 Mon Sep 17 00:00:00 2001 From: maxsibilla Date: Fri, 5 Apr 2024 12:26:27 -0400 Subject: [PATCH 2/6] Reverting docker change --- docker/docker-compose.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docker/docker-compose.yml b/docker/docker-compose.yml index f20edf01..f0df66ad 100644 --- a/docker/docker-compose.yml +++ b/docker/docker-compose.yml @@ -37,4 +37,4 @@ services: networks: # This is the network created by gateway to enable communicaton between multiple docker-compose projects sennet_docker_network: - external: false + external: true From 5d2248d664460c77d59fffcdebb4da9cdb02a32e Mon Sep 17 00:00:00 2001 From: Tyler Madonna Date: Mon, 8 Apr 2024 11:56:21 -0400 Subject: [PATCH 3/6] Fixing tests by mocking AuthHelper --- test/data/update_entity_success_dataset.json | 8 ++++---- test/data/update_entity_success_sample.json | 8 ++++---- test/data/update_entity_success_source.json | 8 ++++---- test/test_app.py | 19 ++++++++++++++++++- 4 files changed, 30 insertions(+), 13 deletions(-) diff --git a/test/data/update_entity_success_dataset.json b/test/data/update_entity_success_dataset.json index 3ba1a9b2..76a1a3c8 100644 --- a/test/data/update_entity_success_dataset.json +++ b/test/data/update_entity_success_dataset.json @@ -22,7 +22,7 @@ "description": "Unit test for dataset", "entity_type": "Dataset", "group_name": "University of Pittsburgh TMC", - "group_uuid": "28db7a2b-ed8a-11ec-8b0a-9fe9b51132b1", + "group_uuid": "3108cb5a-3b0c-4c6d-a944-ce7271ebe325", "lab_dataset_id": "Dataset Unit Test", "last_modified_timestamp": 1683833568803, "last_modified_user_displayname": "Test User", @@ -68,7 +68,7 @@ "created_by_user_displayname": "Test User", "description": "Unit test for dataset", "dataset_info": "lab notes", - "group_uuid": "28db7a2b-ed8a-11ec-8b0a-9fe9b51132b1", + "group_uuid": "3108cb5a-3b0c-4c6d-a944-ce7271ebe325", "created_timestamp": 1681831855041, "created_by_user_sub": "cd17bfa7-24fd-49ca-82ec-2d456ba53730", "uuid": "6a7be8e95c62c74545a29666111899d9", @@ -139,7 +139,7 @@ "created_by_user_displayname": "Test User", "description": "Unit test for dataset", "dataset_info": "lab notes", - "group_uuid": "28db7a2b-ed8a-11ec-8b0a-9fe9b51132b1", + "group_uuid": "3108cb5a-3b0c-4c6d-a944-ce7271ebe325", "created_timestamp": 1681831855041, "created_by_user_sub": "9e5b670f-228d-433c-bb86-a3228d5ca49d", "uuid": "6a7be8e95c62c74545a29666111899d9", @@ -167,7 +167,7 @@ "sennet_id": "SNT554.XLGX.327", "last_modified_timestamp": 1683833568803, "created_by_user_displayname": "Test User", - "group_uuid": "28db7a2b-ed8a-11ec-8b0a-9fe9b51132b1", + "group_uuid": "3108cb5a-3b0c-4c6d-a944-ce7271ebe325", "created_timestamp": 1681831855041, "created_by_user_sub": "9e5b670f-228d-433c-bb86-a3228d5ca49d", "uuid": "6a7be8e95c62c74545a29666111899d9", diff --git a/test/data/update_entity_success_sample.json b/test/data/update_entity_success_sample.json index 109f4c49..50984690 100644 --- a/test/data/update_entity_success_sample.json +++ b/test/data/update_entity_success_sample.json @@ -17,7 +17,7 @@ "description": "Sample lab notes", "entity_type": "Sample", "group_name": "Massachusetts General Hospital TDA", - "group_uuid": "39a276b3-ee73-11ec-87fd-31892bd489e1", + "group_uuid": "3108cb5a-3b0c-4c6d-a944-ce7271ebe325", "lab_tissue_sample_id": "Sample unit test", "last_modified_timestamp": 1683828921963, "last_modified_user_displayname": "Test User", @@ -85,7 +85,7 @@ "created_by_user_displayname": "Test User", "description": "The ancestor is a Source", "sample_category": "Organ", - "group_uuid": "39a276b3-ee73-11ec-87fd-31892bd489e1", + "group_uuid": "3108cb5a-3b0c-4c6d-a944-ce7271ebe325", "created_timestamp": 1681826894432, "created_by_user_sub": "9e5b670f-228d-433c-bb86-a3228d5ca49d", "uuid": "f8946c392b6bd14595ff5a6b2fdc8497", @@ -160,7 +160,7 @@ "created_by_user_displayname": "Test User", "description": "Sample lab notes", "sample_category": "Organ", - "group_uuid": "39a276b3-ee73-11ec-87fd-31892bd489e1", + "group_uuid": "3108cb5a-3b0c-4c6d-a944-ce7271ebe325", "created_timestamp": 1681837878549, "created_by_user_sub": "9e5b670f-228d-433c-bb86-a3228d5ca49d", "uuid": "f8946c392b6bd14595ff5a6b2fdc8497", @@ -192,7 +192,7 @@ "sennet_id": "SNT439.NHLL.499", "last_modified_timestamp": 1683828921963, "created_by_user_displayname": "Test User", - "group_uuid": "39a276b3-ee73-11ec-87fd-31892bd489e1", + "group_uuid": "3108cb5a-3b0c-4c6d-a944-ce7271ebe325", "created_timestamp": 1681837878549, "created_by_user_sub": "9e5b670f-228d-433c-bb86-a3228d5ca49d", "uuid": "f8946c392b6bd14595ff5a6b2fdc8497", diff --git a/test/data/update_entity_success_source.json b/test/data/update_entity_success_source.json index ac9e9112..f03e03c3 100644 --- a/test/data/update_entity_success_source.json +++ b/test/data/update_entity_success_source.json @@ -15,7 +15,7 @@ "description": "Unit test lab notes", "entity_type": "Source", "group_name": "CODCC Testing Group", - "group_uuid": "57192604-18e0-11ed-b79b-972795fc9504", + "group_uuid": "3108cb5a-3b0c-4c6d-a944-ce7271ebe325", "lab_source_id": "Unit test", "last_modified_timestamp": 1683815673885, "last_modified_user_displayname": "Test User", @@ -70,7 +70,7 @@ "created_by_user_displayname": "Test User", "description": "This is a mouse source.", "source_type": "Mouse", - "group_uuid": "57192604-18e0-11ed-b79b-972795fc9504", + "group_uuid": "3108cb5a-3b0c-4c6d-a944-ce7271ebe325", "created_timestamp": 1681826894432, "created_by_user_sub": "9e5b670f-228d-433c-bb86-a3228d5ca49d", "uuid": "8af152b82ea653a8e5189267a7e6f82a", @@ -123,7 +123,7 @@ "created_by_user_displayname": "Test User", "description": "Unit test lab notes", "source_type": "Human Organoid", - "group_uuid": "57192604-18e0-11ed-b79b-972795fc9504", + "group_uuid": "3108cb5a-3b0c-4c6d-a944-ce7271ebe325", "created_timestamp": 1681826894432, "created_by_user_sub": "9e5b670f-228d-433c-bb86-a3228d5ca49d", "uuid": "8af152b82ea653a8e5189267a7e6f82a", @@ -157,7 +157,7 @@ "sennet_id": "SNT986.NQMB.577", "last_modified_timestamp": 1683815673885, "created_by_user_displayname": "Test User", - "group_uuid": "57192604-18e0-11ed-b79b-972795fc9504", + "group_uuid": "3108cb5a-3b0c-4c6d-a944-ce7271ebe325", "created_timestamp": 1681826894432, "created_by_user_sub": "9e5b670f-228d-433c-bb86-a3228d5ca49d", "uuid": "8af152b82ea653a8e5189267a7e6f82a", diff --git a/test/test_app.py b/test/test_app.py index 9a9d2c24..03e279a0 100644 --- a/test/test_app.py +++ b/test/test_app.py @@ -4,7 +4,7 @@ import json import os import random -from unittest.mock import patch +from unittest.mock import MagicMock, patch from flask import Response import pytest @@ -28,6 +28,23 @@ def ontology_mock(): with (patch('atlas_consortia_commons.ubkg.ubkg_sdk.UbkgSDK', new=test_utils.MockOntology)): yield +@pytest.fixture(scope="session", autouse=True) +def auth_helper_mock(): + auth_mock = MagicMock() + auth_mock.getAuthorizationTokens.return_value = "test_token" + auth_mock.has_data_admin_privs.return_value = False + auth_mock.get_user_write_groups.return_value = [{ + "uuid": "3108cb5a-3b0c-4c6d-a944-ce7271ebe325" + }] + + # auth_helper_instance gets created (from 'import app') before fixture is called + app_module.auth_helper_instance = auth_mock + with ( + patch("hubmap_commons.hm_auth.AuthHelper.create", return_value=auth_mock), + patch("hubmap_commons.hm_auth.AuthHelper.instance", return_value=auth_mock), + ): + yield + ### Index def test_index(app): From d6e74eb4e97366164ed551108bb939701ec1f2cd Mon Sep 17 00:00:00 2001 From: maxsibilla Date: Mon, 8 Apr 2024 12:53:05 -0400 Subject: [PATCH 4/6] Updating response messages. --- src/app.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/app.py b/src/app.py index 10de0fc7..db9a13b4 100644 --- a/src/app.py +++ b/src/app.py @@ -4798,12 +4798,12 @@ def validate_user_update_privilege(entity, user_token): user_write_groups: List[dict] = auth_helper_instance.get_user_write_groups(user_token) if isinstance(user_write_groups, Response): - abort_forbidden(f"Entity with ID: {entity['sennet_id']} is not editable without presenting a token.") + return user_write_groups user_group_uuids = [d['uuid'] for d in user_write_groups] if entity['group_uuid'] not in user_group_uuids and is_admin is False: - abort_forbidden(f"User does not have write privileges for entity with ID: {entity['sennet_id']}. " - f"Reach out to the help desk to request access to this group.") + abort_forbidden(f"User does not have write privileges for this entity. " + f"Reach out to the help desk to request access to group: {entity['group_uuid']}.") """ From 0f3e20e4e2cb67c8276b5933ef72318ff6786dba Mon Sep 17 00:00:00 2001 From: maxsibilla Date: Mon, 8 Apr 2024 13:07:14 -0400 Subject: [PATCH 5/6] Aborting in scenario where a response is returned from auth_helper_instance --- src/app.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app.py b/src/app.py index db9a13b4..8ba0140c 100644 --- a/src/app.py +++ b/src/app.py @@ -4798,7 +4798,7 @@ def validate_user_update_privilege(entity, user_token): user_write_groups: List[dict] = auth_helper_instance.get_user_write_groups(user_token) if isinstance(user_write_groups, Response): - return user_write_groups + abort(user_write_groups) user_group_uuids = [d['uuid'] for d in user_write_groups] if entity['group_uuid'] not in user_group_uuids and is_admin is False: From c0228039da77d6437a1f536cd7a860f54e7bf362 Mon Sep 17 00:00:00 2001 From: maxsibilla Date: Mon, 8 Apr 2024 13:58:23 -0400 Subject: [PATCH 6/6] Aborting in instance where is_admin is a response --- src/app.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/app.py b/src/app.py index 8ba0140c..efc5af35 100644 --- a/src/app.py +++ b/src/app.py @@ -3,7 +3,7 @@ from datetime import datetime from typing import List -from flask import Flask, g, jsonify, request +from flask import Flask, g from neo4j.exceptions import TransactionError import os import re @@ -44,7 +44,6 @@ # Atlas Consortia commons from atlas_consortia_commons.ubkg import initialize_ubkg from atlas_consortia_commons.rest import * -from atlas_consortia_commons.rest import abort_bad_req, abort_not_found from atlas_consortia_commons.string import equals from atlas_consortia_commons.ubkg.ubkg_sdk import init_ontology from lib.ontology import Ontology @@ -4794,7 +4793,7 @@ def validate_user_update_privilege(entity, user_token): # A user has update privileges if they are a data admin or are in the same group that registered the entity is_admin = auth_helper_instance.has_data_admin_privs(user_token) if isinstance(is_admin, Response): - is_admin = False + abort(is_admin) user_write_groups: List[dict] = auth_helper_instance.get_user_write_groups(user_token) if isinstance(user_write_groups, Response):