diff --git a/server/portal/apps/projects/unit_test.py b/server/portal/apps/projects/unit_test.py index cdfb142dc..083ad4aaa 100644 --- a/server/portal/apps/projects/unit_test.py +++ b/server/portal/apps/projects/unit_test.py @@ -251,8 +251,8 @@ def create_shared_workspace_2_user( new_username = "new_user" mock_add_user_to_workspace(client, workspace_id, new_username, "writer") mock_service_account().files.setFacl.assert_called_with( - systemId="projects.system.name", - path="test.project-2", + systemId="test.project.test.project-2", + path="/", operation="ADD", recursionMethod="PHYSICAL", aclString=f"d:u:{new_username}:rwX,u:{new_username}:rwX", @@ -519,7 +519,7 @@ def test_add_member(mock_tapis_client, mock_owner, authenticated_user): "title": mock_system_result.notes.title, "description": getattr(mock_system_result.notes, "description", None), "created": mock_system_result.updated, - "projectId": "test.project-2", + "projectId": "test.project.test.project-2", "members": [ {"user": ws_o.get_project_user("username"), "access": "owner"}, ], @@ -548,8 +548,8 @@ def test_add_member(mock_tapis_client, mock_owner, authenticated_user): mock_service_account.assert_called() mock_service_account().files.setFacl.assert_called_with( - systemId="projects.system.name", - path="test.project-2", + systemId="test.project.test.project-2", + path="/", operation="ADD", recursionMethod="PHYSICAL", aclString=f"d:u:{new_username}:rwX,u:{new_username}:rwX", @@ -768,8 +768,8 @@ def test_get_workspace_role(mock_tapis_client, mock_owner, authenticated_user): new_username = "new_user" mock_add_user_to_workspace(client, workspace_id, new_username, "writer") mock_service_account().files.setFacl.assert_called_with( - systemId="projects.system.name", - path="test.project-2", + systemId="test.project.test.project-2", + path="/", operation="ADD", recursionMethod="PHYSICAL", aclString=f"d:u:{new_username}:rwX,u:{new_username}:rwX", @@ -801,8 +801,8 @@ def test_get_workspace_role(mock_tapis_client, mock_owner, authenticated_user): # Add user action mock_add_user_to_workspace(client, workspace_id, "GuestAccount", "reader") mock_service_account().files.setFacl.assert_called_with( - systemId="projects.system.name", - path="test.project-2", + systemId="test.project.test.project-2", + path="/", operation="ADD", recursionMethod="PHYSICAL", aclString="d:u:GuestAccount:rX,u:GuestAccount:rX", diff --git a/server/portal/apps/projects/views_unit_test.py b/server/portal/apps/projects/views_unit_test.py index bbfe03330..c47b38e41 100644 --- a/server/portal/apps/projects/views_unit_test.py +++ b/server/portal/apps/projects/views_unit_test.py @@ -161,9 +161,7 @@ def test_projects_search( def test_projects_search_result_not_in_tapis( - authenticated_user, client, - mock_tapis_client, mock_project_index, mock_project_search_indexer, project_list, @@ -219,6 +217,9 @@ def test_projects_post( def test_projects_post_setfacl_job( authenticated_user, client, mock_service_account, mock_tapis_client ): + + mock_rootDir = mock_service_account().systems.getSystem().rootDir + response = client.post( "/api/projects/", { @@ -227,7 +228,6 @@ def test_projects_post_setfacl_job( }, content_type="application/json", ) - assert response.status_code == 200 assert response.json() == { "status": 200, @@ -241,7 +241,7 @@ def test_projects_post_setfacl_job( ) mock_service_account().files.setFacl.assert_not_called() mock_service_account().jobs.submitJob.assert_called_with( - name='setfacl-project-test.project-2-username-add-writer', + name='setfacl-project-projects.system.name-username-add-writer', appId='setfacl-corral-wmaprtl', appVersion='0.0.1', description='Add/Remove ACLs on a directory', @@ -251,7 +251,7 @@ def test_projects_post_setfacl_job( 'schedulerOptions': [], 'envVariables': [ {'key': 'usernames', 'value': 'username'}, - {'key': 'directory', 'value': '/path/to/root/test.project-2'}, + {'key': 'directory', 'value': str(mock_rootDir)}, {'key': 'action', 'value': 'add'}, {'key': 'role', 'value': 'writer'}, ], @@ -373,7 +373,7 @@ def test_project_instance_patch( } -def test_project_change_role(authenticated_user, client, mock_project_mgr): +def test_project_change_role(client, mock_project_mgr, project_list): mock_project_mgr.change_project_role.return_value = MagicMock( metadata={"projectId": "PRJ-123"} ) @@ -395,7 +395,7 @@ def test_project_change_role(authenticated_user, client, mock_project_mgr): def test_project_change_system_role( - authenticated_user, client, mock_service_account, mock_tapis_client + client, mock_service_account, mock_tapis_client, project_list ): # USER translates to writer role patch_body = { @@ -409,8 +409,8 @@ def test_project_change_system_role( assert response.json() == {"status": 200, "response": "OK"} # System Id used in setFacl is project root system name mock_service_account().files.setFacl.assert_called_with( - systemId="projects.system.name", - path="PRJ-123", + systemId="test.project.PRJ-123", + path="/", operation="ADD", recursionMethod="PHYSICAL", aclString="d:u:test_user:rwX,u:test_user:rwX", @@ -431,8 +431,10 @@ def test_project_change_system_role( @override_settings(PORTAL_PROJECTS_USE_SET_FACL_JOB=True) def test_project_change_system_role_setfacl_job( - authenticated_user, client, mock_service_account, mock_tapis_client + client, mock_service_account, mock_tapis_client, project_list ): + mock_rootDir = mock_service_account().systems.getSystem().rootDir + # USER translates to writer role patch_body = { "action": "change_system_role", @@ -446,7 +448,7 @@ def test_project_change_system_role_setfacl_job( # System Id used in setFacl is project root system name mock_service_account().files.setFacl.assert_not_called() mock_service_account().jobs.submitJob.assert_called_with( - name='setfacl-project-PRJ-123-test_user-add-writer', + name='setfacl-project-test.project.PRJ-123-test_user-add-writer', appId='setfacl-corral-wmaprtl', appVersion='0.0.1', description='Add/Remove ACLs on a directory', @@ -456,7 +458,7 @@ def test_project_change_system_role_setfacl_job( 'schedulerOptions': [], 'envVariables': [ {'key': 'usernames', 'value': 'test_user'}, - {'key': 'directory', 'value': '/path/to/root/PRJ-123'}, + {'key': 'directory', 'value': str(mock_rootDir)}, {'key': 'action', 'value': 'add'}, {'key': 'role', 'value': 'writer'}, ], @@ -478,7 +480,7 @@ def test_project_change_system_role_setfacl_job( def test_members_view_add( - authenticated_user, client, mock_service_account, mock_tapis_client, project_list + authenticated_user, client, mock_tapis_client, project_list ): mock_tapis_client.systems.getSystem.return_value = project_list["tapis_response"][0] mock_tapis_client.systems.getShareInfo.return_value = TapisResult( @@ -525,9 +527,10 @@ def test_members_view_add( "title": project_list["api_response"][0]["title"], }, } - mock_service_account().files.setFacl.assert_called_with( - systemId="projects.system.name", - path="PRJ-123", + + mock_tapis_client.files.setFacl.assert_called_with( + systemId="test.project.PRJ-123", + path="/", operation="ADD", recursionMethod="PHYSICAL", aclString="d:u:test_user:rwX,u:test_user:rwX", @@ -599,7 +602,7 @@ def test_members_view_add_setfacl_job( } mock_service_account().files.setFacl.assert_not_called() mock_service_account().jobs.submitJob.assert_called_with( - name='setfacl-project-PRJ-123-test_user-add-writer', + name='setfacl-project-test.project.PRJ-123-test_user-add-writer', appId='setfacl-corral-wmaprtl', appVersion='0.0.1', description='Add/Remove ACLs on a directory', @@ -609,7 +612,7 @@ def test_members_view_add_setfacl_job( 'schedulerOptions': [], 'envVariables': [ {'key': 'usernames', 'value': 'test_user'}, - {'key': 'directory', 'value': '/path/to/root/PRJ-123'}, + {'key': 'directory', 'value': '/corral-repl/tacc/aci/CEP/projects/CEP-1018'}, {'key': 'action', 'value': 'add'}, {'key': 'role', 'value': 'writer'}, ], @@ -633,7 +636,7 @@ def test_members_view_add_setfacl_job( def test_members_view_remove( - authenticated_user, client, mock_service_account, mock_tapis_client, project_list + client, mock_service_account, mock_tapis_client, project_list ): mock_tapis_client.systems.getSystem.return_value = project_list["tapis_response"][0] patch_body = {"action": "remove_member", "username": "test_user"} @@ -661,8 +664,8 @@ def test_members_view_remove( }, } mock_service_account().files.setFacl.assert_called_with( - systemId="projects.system.name", - path="PRJ-123", + systemId="test.project.PRJ-123", + path="/", operation="REMOVE", recursionMethod="PHYSICAL", aclString="d:u:test_user,u:test_user", @@ -685,9 +688,10 @@ def test_members_view_remove( @override_settings(PORTAL_PROJECTS_USE_SET_FACL_JOB=True) def test_members_view_remove_setfacl_job( - authenticated_user, client, mock_service_account, mock_tapis_client, project_list + client, mock_service_account, mock_tapis_client, project_list ): mock_tapis_client.systems.getSystem.return_value = project_list["tapis_response"][0] + mock_rootDir = mock_service_account().systems.getSystem().rootDir patch_body = {"action": "remove_member", "username": "test_user"} response = client.patch("/api/projects/PRJ-123/members/", json.dumps(patch_body)) @@ -714,7 +718,7 @@ def test_members_view_remove_setfacl_job( } mock_service_account().files.setFacl.assert_not_called() mock_service_account().jobs.submitJob.assert_called_with( - name='setfacl-project-PRJ-123-test_user-remove-none', + name='setfacl-project-test.project.PRJ-123-test_user-remove-none', appId='setfacl-corral-wmaprtl', appVersion='0.0.1', description='Add/Remove ACLs on a directory', @@ -724,7 +728,7 @@ def test_members_view_remove_setfacl_job( 'schedulerOptions': [], 'envVariables': [ {'key': 'usernames', 'value': 'test_user'}, - {'key': 'directory', 'value': '/path/to/root/PRJ-123'}, + {'key': 'directory', 'value': str(mock_rootDir)}, {'key': 'action', 'value': 'remove'}, {'key': 'role', 'value': 'none'}, ], diff --git a/server/portal/apps/projects/workspace_operations/shared_workspace_operations.py b/server/portal/apps/projects/workspace_operations/shared_workspace_operations.py index 6a204d1ca..b5db4808d 100644 --- a/server/portal/apps/projects/workspace_operations/shared_workspace_operations.py +++ b/server/portal/apps/projects/workspace_operations/shared_workspace_operations.py @@ -60,8 +60,9 @@ def set_workspace_acls(client, system_id, path, username, operation, role): } if settings.PORTAL_PROJECTS_USE_SET_FACL_JOB: - logger.info(f"Using setfacl job to submit ACL change for project: {path}, username: {username}, operation: {operation}, role: {role}") - job_res = submit_workspace_acls_job(username, path, role, operation) + logger.info(f"""Using setfacl job to submit ACL change for project: {system_id}, + path: {path}, username: {username}, operation: {operation}, role: {role}""") + job_res = submit_workspace_acls_job(client, username, system_id, role, operation) logger.info(f"Submitted workspace ACL job {job_res.name} with UUID {job_res.uuid}") return @@ -73,7 +74,7 @@ def set_workspace_acls(client, system_id, path, username, operation, role): def submit_workspace_acls_job( - username, project_name, role, action=Literal["add", "remove"] + user_client, username, system_id, role, action=Literal["add", "remove"] ): """ Submit a job to set ACLs on a project for a specific user. This should be used if @@ -83,8 +84,10 @@ def submit_workspace_acls_job( client = service_account() portal_name = settings.PORTAL_NAMESPACE + prj = user_client.systems.getSystem(systemId=system_id) + job_body = { - "name": f"setfacl-project-{project_name}-{username}-{action}-{role}", + "name": f"setfacl-project-{system_id}-{username}-{action}-{role}"[:64], "appId": "setfacl-corral-wmaprtl", "appVersion": "0.0.1", "description": "Add/Remove ACLs on a directory", @@ -96,7 +99,7 @@ def submit_workspace_acls_job( {"key": "usernames", "value": username}, { "key": "directory", - "value": f"{settings.PORTAL_PROJECTS_ROOT_DIR}/{project_name}", + "value": f"{prj.rootDir}", }, {"key": "action", "value": action}, {"key": "role", "value": role}, @@ -199,11 +202,10 @@ def add_user_to_workspace(client: Tapis, """ Give a user POSIX and Tapis permissions on a workspace system. """ - service_client = service_account() system_id = f"{settings.PORTAL_PROJECTS_SYSTEM_PREFIX}.{workspace_id}" - set_workspace_acls(service_client, - settings.PORTAL_PROJECTS_ROOT_SYSTEM_NAME, - workspace_id, + set_workspace_acls(client, + system_id, + "/", username, "add", role) @@ -231,8 +233,8 @@ def change_user_role(client, workspace_id: str, username: str, new_role): service_client = service_account() system_id = f"{settings.PORTAL_PROJECTS_SYSTEM_PREFIX}.{workspace_id}" set_workspace_acls(service_client, - settings.PORTAL_PROJECTS_ROOT_SYSTEM_NAME, - workspace_id, + system_id, + "/", username, "add", new_role) @@ -247,8 +249,8 @@ def remove_user(client, workspace_id: str, username: str): service_client = service_account() system_id = f"{settings.PORTAL_PROJECTS_SYSTEM_PREFIX}.{workspace_id}" set_workspace_acls(service_client, - settings.PORTAL_PROJECTS_ROOT_SYSTEM_NAME, - workspace_id, + system_id, + "/", username, "remove", "none") @@ -271,8 +273,8 @@ def transfer_ownership(client, workspace_id: str, new_owner: str, old_owner: str service_client = service_account() system_id = f"{settings.PORTAL_PROJECTS_SYSTEM_PREFIX}.{workspace_id}" set_workspace_acls(service_client, - settings.PORTAL_PROJECTS_ROOT_SYSTEM_NAME, - workspace_id, + system_id, + "/", new_owner, "add", "writer") @@ -352,6 +354,10 @@ def get_project(client, workspace_id): access = 'edit' elif perms.permission == 'READ': access = 'read' + else: + logger.info(f"System shared to user without proper Tapis file permissions: {system_id}, username: {username}") + access = 'none' + users.append({"user": get_project_user(username), "access": access}) return { diff --git a/server/portal/settings/settings.py b/server/portal/settings/settings.py index d5ad0d855..1321974d1 100644 --- a/server/portal/settings/settings.py +++ b/server/portal/settings/settings.py @@ -601,7 +601,7 @@ PORTAL_ALLOCATION = getattr(settings_custom, '_PORTAL_ALLOCATION', '') -PORTAL_PROJECTS_USE_SET_FACL_JOB = getattr(settings_custom, '_PORTAL_PROJECTS_USE_SET_FACL_JOB', False) +PORTAL_PROJECTS_USE_SET_FACL_JOB = getattr(settings_custom, '_PORTAL_PROJECTS_USE_SET_FACL_JOB', True) """ SETTINGS: ELASTICSEARCH