task/WP-682: Fix Community Project Folders on Core portals (#1041)

* To update ACLs on shared systems outside of the root project's directory (community data), the target of updating ACLs needs to be the system itself rather than the parent root system.

* In the case where a user is added to a project but doesn’t have file permissions assigned, this function would fail which blocks the portal from getting the project at all.

* Resolving issues with root directory hardcoding and refactoring function to use the system terminology instead of project terminology

* minor refactoring

* Changed ACL job to submit with the user's client, so that we know the system will have the credentials needed for the job. Truncated the job name to fit with Tapis JSON requirements. And changed the client used to add a user to a shared system so that we know the system will have the credentials needed to make the change.

* Adding path back into logger statement.

* Fixed unit tests to work with code changes.

* Fixed linting errors

* Minor cleanup, setting the set_facl_job to run by default

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",

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'},
             ],

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 {

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