From 6ac95f23353c1d92a24b70d76d1d62c22d771960 Mon Sep 17 00:00:00 2001 From: Sriharsha Chintalapani Date: Tue, 19 Mar 2024 23:43:20 -0700 Subject: [PATCH] Check assignee permissions before updating the entity description/tags (#15582) * Check assignee permissions before updating the entity description/tags * make assignee field enabled even if entity already has owner * minor fix --------- Co-authored-by: Ashish Gupta --- .../service/jdbi3/FeedRepository.java | 29 ++- .../service/resources/feeds/FeedResource.java | 4 +- .../resources/ui/cypress/common/TaskUtils.ts | 7 +- .../ui/cypress/e2e/Flow/Task.spec.ts | 185 ++++++++++++++++-- .../Entity/Task/TaskTab/TaskTab.component.tsx | 9 +- .../RequestDescriptionPage.tsx | 1 - .../RequestTagPage/RequestTagPage.tsx | 1 - .../UpdateDescriptionPage.tsx | 1 - .../TasksPage/UpdateTagPage/UpdateTagPage.tsx | 1 - 9 files changed, 205 insertions(+), 33 deletions(-) diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/FeedRepository.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/FeedRepository.java index d03ddcc4b24..813dfcdc313 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/FeedRepository.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/FeedRepository.java @@ -64,6 +64,7 @@ import org.openmetadata.schema.type.ChangeEvent; import org.openmetadata.schema.type.EntityReference; import org.openmetadata.schema.type.EventType; import org.openmetadata.schema.type.Include; +import org.openmetadata.schema.type.MetadataOperation; import org.openmetadata.schema.type.Post; import org.openmetadata.schema.type.Reaction; import org.openmetadata.schema.type.Relationship; @@ -85,6 +86,9 @@ import org.openmetadata.service.resources.feeds.FeedUtil; import org.openmetadata.service.resources.feeds.MessageParser; import org.openmetadata.service.resources.feeds.MessageParser.EntityLink; import org.openmetadata.service.security.AuthorizationException; +import org.openmetadata.service.security.Authorizer; +import org.openmetadata.service.security.policyevaluator.OperationContext; +import org.openmetadata.service.security.policyevaluator.ResourceContext; import org.openmetadata.service.util.EntityUtil; import org.openmetadata.service.util.FullyQualifiedName; import org.openmetadata.service.util.JsonUtils; @@ -786,11 +790,12 @@ public class FeedRepository { } public void checkPermissionsForResolveTask( - Thread thread, boolean closeTask, SecurityContext securityContext) { + Authorizer authorizer, Thread thread, boolean closeTask, SecurityContext securityContext) { String userName = securityContext.getUserPrincipal().getName(); User user = Entity.getEntityByName(USER, userName, TEAMS_FIELD, NON_DELETED); EntityLink about = EntityLink.parse(thread.getAbout()); EntityReference aboutRef = EntityUtil.validateEntityLink(about); + ThreadContext threadContext = getThreadContext(thread); if (Boolean.TRUE.equals(user.getIsAdmin())) { return; // Allow admin resolve/close task } @@ -800,9 +805,25 @@ public class FeedRepository { // Allow if user created the task to close task (and not resolve task) EntityReference owner = Entity.getOwner(aboutRef); List assignees = thread.getTask().getAssignees(); - if (assignees.stream().anyMatch(assignee -> assignee.getName().equals(userName)) - || owner.getName().equals(userName) - || closeTask && thread.getCreatedBy().equals(userName)) { + if (owner.getName().equals(userName) || closeTask && thread.getCreatedBy().equals(userName)) { + return; + } + + // Allow if user is an assignee of the task and if the assignee has permissions to update the + // entity + if (assignees.stream().anyMatch(assignee -> assignee.getName().equals(userName))) { + // If entity does not exist, this is a create operation, else update operation + ResourceContext resourceContext = + new ResourceContext<>(aboutRef.getType(), aboutRef.getId(), null); + if (EntityUtil.isDescriptionTask(threadContext.getTaskWorkflow().getTaskType())) { + OperationContext operationContext = + new OperationContext(aboutRef.getType(), MetadataOperation.EDIT_DESCRIPTION); + authorizer.authorize(securityContext, operationContext, resourceContext); + } else if (EntityUtil.isTagTask(threadContext.getTaskWorkflow().getTaskType())) { + OperationContext operationContext = + new OperationContext(aboutRef.getType(), MetadataOperation.EDIT_TAGS); + authorizer.authorize(securityContext, operationContext, resourceContext); + } return; } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/resources/feeds/FeedResource.java b/openmetadata-service/src/main/java/org/openmetadata/service/resources/feeds/FeedResource.java index 555c6793564..976799e1d0f 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/resources/feeds/FeedResource.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/resources/feeds/FeedResource.java @@ -297,7 +297,7 @@ public class FeedResource { String id, @Valid ResolveTask resolveTask) { Thread task = dao.getTask(Integer.parseInt(id)); - dao.checkPermissionsForResolveTask(task, false, securityContext); + dao.checkPermissionsForResolveTask(authorizer, task, false, securityContext); return dao.resolveTask(uriInfo, task, securityContext.getUserPrincipal().getName(), resolveTask) .toResponse(); } @@ -326,7 +326,7 @@ public class FeedResource { String id, @Valid CloseTask closeTask) { Thread task = dao.getTask(Integer.parseInt(id)); - dao.checkPermissionsForResolveTask(task, true, securityContext); + dao.checkPermissionsForResolveTask(authorizer, task, true, securityContext); return dao.closeTask(uriInfo, task, securityContext.getUserPrincipal().getName(), closeTask) .toResponse(); } diff --git a/openmetadata-ui/src/main/resources/ui/cypress/common/TaskUtils.ts b/openmetadata-ui/src/main/resources/ui/cypress/common/TaskUtils.ts index 4f7e10c4b2d..5955e093f0a 100644 --- a/openmetadata-ui/src/main/resources/ui/cypress/common/TaskUtils.ts +++ b/openmetadata-ui/src/main/resources/ui/cypress/common/TaskUtils.ts @@ -31,7 +31,10 @@ export type TaskDetails = { schemaName?: string; }; -export const verifyTaskDetails = (regexPattern: RegExp) => { +export const verifyTaskDetails = ( + regexPattern: RegExp, + taskAssignee?: string +) => { cy.get('#task-panel').should('be.visible'); cy.get('[data-testid="task-title"]') .invoke('text') @@ -43,7 +46,7 @@ export const verifyTaskDetails = (regexPattern: RegExp) => { cy.get('[data-testid="owner-link"]').should('contain', owner); - cy.get(`[data-testid="${assignee}"]`).should('be.visible'); + cy.get(`[data-testid="${taskAssignee ?? assignee}"]`).should('be.visible'); }; export const editAssignee = () => { diff --git a/openmetadata-ui/src/main/resources/ui/cypress/e2e/Flow/Task.spec.ts b/openmetadata-ui/src/main/resources/ui/cypress/e2e/Flow/Task.spec.ts index 92c6f00f440..e3025710636 100644 --- a/openmetadata-ui/src/main/resources/ui/cypress/e2e/Flow/Task.spec.ts +++ b/openmetadata-ui/src/main/resources/ui/cypress/e2e/Flow/Task.spec.ts @@ -16,7 +16,11 @@ import { toastNotification, verifyResponseStatusCode, } from '../../common/common'; -import { createEntityTable, hardDeleteService } from '../../common/EntityUtils'; +import { + createEntityTable, + deleteUserEntity, + hardDeleteService, +} from '../../common/EntityUtils'; import { createAndUpdateDescriptionTask, createDescriptionTask, @@ -26,8 +30,12 @@ import { import { visitEntityDetailsPage } from '../../common/Utils/Entity'; import { getToken } from '../../common/Utils/LocalStorage'; import { addOwner } from '../../common/Utils/Owner'; -import { DATA_ASSETS } from '../../constants/constants'; -import { DATABASE_SERVICE } from '../../constants/EntityConstant'; +import { DATA_ASSETS, uuid } from '../../constants/constants'; +import { + DATABASE_SERVICE, + USER_DETAILS, + USER_NAME, +} from '../../constants/EntityConstant'; import { SERVICE_CATEGORIES } from '../../constants/service.constants'; const ENTITY_TABLE = { @@ -39,30 +47,137 @@ const ENTITY_TABLE = { entityType: 'Table', }; +const POLICY_DETAILS = { + name: `cy-data-viewAll-policy-${uuid()}`, + rules: [ + { + name: 'viewRuleAllowed', + resources: ['All'], + operations: ['ViewAll'], + effect: 'allow', + }, + { + effect: 'deny', + name: 'editNotAllowed', + operations: ['EditAll'], + resources: ['All'], + }, + ], +}; +const ROLE_DETAILS = { + name: `cy-data-viewAll-role-${uuid()}`, + policies: [POLICY_DETAILS.name], +}; + +const TEAM_DETAILS = { + name: 'viewAllTeam', + displayName: 'viewAllTeam', + teamType: 'Group', +}; + describe('Task flow should work', { tags: 'DataAssets' }, () => { + const data = { + user: { id: '' }, + policy: { id: '' }, + role: { id: '' }, + team: { id: '' }, + }; + before(() => { cy.login(); - cy.getAllLocalStorage().then((data) => { - const token = getToken(data); + cy.getAllLocalStorage().then((storageData) => { + const token = getToken(storageData); createEntityTable({ token, ...DATABASE_SERVICE, tables: [DATABASE_SERVICE.entity], }); + + // Create ViewAll Policy + cy.request({ + method: 'POST', + url: `/api/v1/policies`, + headers: { Authorization: `Bearer ${token}` }, + body: POLICY_DETAILS, + }).then((policyResponse) => { + data.policy = policyResponse.body; + + // Create ViewAll Role + cy.request({ + method: 'POST', + url: `/api/v1/roles`, + headers: { Authorization: `Bearer ${token}` }, + body: ROLE_DETAILS, + }).then((roleResponse) => { + data.role = roleResponse.body; + + // Create a new user + cy.request({ + method: 'POST', + url: `/api/v1/users/signup`, + headers: { Authorization: `Bearer ${token}` }, + body: USER_DETAILS, + }).then((userResponse) => { + data.user = userResponse.body; + + // create team + cy.request({ + method: 'GET', + url: `/api/v1/teams/name/Organization`, + headers: { Authorization: `Bearer ${token}` }, + }).then((teamResponse) => { + cy.request({ + method: 'POST', + url: `/api/v1/teams`, + headers: { Authorization: `Bearer ${token}` }, + body: { + ...TEAM_DETAILS, + parents: [teamResponse.body.id], + users: [userResponse.body.id], + defaultRoles: [roleResponse.body.id], + }, + }).then((teamResponse) => { + data.team = teamResponse.body; + }); + }); + }); + }); + }); }); }); after(() => { cy.login(); - cy.getAllLocalStorage().then((data) => { - const token = getToken(data); + cy.getAllLocalStorage().then((storageData) => { + const token = getToken(storageData); hardDeleteService({ token, serviceFqn: ENTITY_TABLE.serviceName, serviceType: SERVICE_CATEGORIES.DATABASE_SERVICES, }); + + // Clean up for the created data + deleteUserEntity({ token, id: data.user.id }); + + cy.request({ + method: 'DELETE', + url: `/api/v1/teams/${data.team.id}?hardDelete=true&recursive=true`, + headers: { Authorization: `Bearer ${token}` }, + }); + + cy.request({ + method: 'DELETE', + url: `/api/v1/policies/${data.policy.id}?hardDelete=true&recursive=true`, + headers: { Authorization: `Bearer ${token}` }, + }); + + cy.request({ + method: 'DELETE', + url: `/api/v1/roles/${data.role.id}?hardDelete=true&recursive=true`, + headers: { Authorization: `Bearer ${token}` }, + }); }); }); @@ -193,7 +308,7 @@ describe('Task flow should work', { tags: 'DataAssets' }, () => { }); }); - it('Asignee field should be disabled for owned entity tasks', () => { + it('Assignee field should not be disabled for owned entity tasks', () => { interceptURL( 'GET', `/api/v1/${ENTITY_TABLE.entity}/name/*`, @@ -213,17 +328,49 @@ describe('Task flow should work', { tags: 'DataAssets' }, () => { cy.wait('@getEntityDetails').then((res) => { const entity = res.response.body; - // create description task and verify asignee field to have owner - // and should be disbaled - - createDescriptionTask( - { - ...ENTITY_TABLE, - assignee: 'Adam Rodriguez', - term: entity.displayName ?? entity.name, - }, - true - ); + createDescriptionTask({ + ...ENTITY_TABLE, + assignee: USER_NAME, + term: entity.displayName ?? entity.name, + }); }); }); + + it(`should throw error for not having edit permission for viewAll user`, () => { + // logout for the admin user + cy.logout(); + + // login to viewAll user + cy.login(USER_DETAILS.email, USER_DETAILS.password); + + interceptURL( + 'GET', + `/api/v1/${ENTITY_TABLE.entity}/name/*`, + 'getEntityDetails' + ); + + visitEntityDetailsPage({ + term: ENTITY_TABLE.term, + serviceName: ENTITY_TABLE.serviceName, + entity: ENTITY_TABLE.entity, + }); + + cy.get('[data-testid="activity_feed"]').click(); + + cy.get('[data-menu-id*="tasks"]').click(); + + // verify the task details + verifyTaskDetails(/#(\d+) Request to update description for/, USER_NAME); + + cy.get(`[data-testid="${USER_NAME}"]`).should('be.visible'); + + // Accept the description suggestion which is created + cy.get('.ant-btn-compact-first-item').contains('Accept Suggestion').click(); + + verifyResponseStatusCode('@taskResolve', 403); + + toastNotification( + `Principal: CatalogPrincipal{name='${USER_NAME}'} operation EditDescription denied by role ${ROLE_DETAILS.name}, policy ${POLICY_DETAILS.name}, rule editNotAllowed` + ); + }); }); diff --git a/openmetadata-ui/src/main/resources/ui/src/components/Entity/Task/TaskTab/TaskTab.component.tsx b/openmetadata-ui/src/main/resources/ui/src/components/Entity/Task/TaskTab/TaskTab.component.tsx index b4010bb9299..961b0fc5b9f 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/Entity/Task/TaskTab/TaskTab.component.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/components/Entity/Task/TaskTab/TaskTab.component.tsx @@ -83,6 +83,7 @@ import { getNameFromFQN } from '../../../../utils/CommonUtils'; import EntityLink from '../../../../utils/EntityLink'; import { getEntityFQN } from '../../../../utils/FeedUtils'; import { checkPermission } from '../../../../utils/PermissionsUtils'; +import { getErrorText } from '../../../../utils/StringsUtils'; import { fetchOptions, generateOptions, @@ -266,7 +267,9 @@ export const TaskTab = ({ rest.onAfterClose?.(); rest.onUpdateEntityDetails?.(); }) - .catch((err: AxiosError) => showErrorToast(err)); + .catch((err: AxiosError) => + showErrorToast(getErrorText(err, t('server.unexpected-error'))) + ); }; const onTaskResolve = () => { @@ -429,7 +432,9 @@ export const TaskTab = ({ rest.onAfterClose?.(); setShowEditTaskModel(false); } catch (error) { - showErrorToast(error as AxiosError); + showErrorToast( + getErrorText(error as AxiosError, t('server.unexpected-error')) + ); } finally { setIsActionLoading(false); } diff --git a/openmetadata-ui/src/main/resources/ui/src/pages/TasksPage/RequestDescriptionPage/RequestDescriptionPage.tsx b/openmetadata-ui/src/main/resources/ui/src/pages/TasksPage/RequestDescriptionPage/RequestDescriptionPage.tsx index 6c7069ff9a4..3351c573154 100644 --- a/openmetadata-ui/src/main/resources/ui/src/pages/TasksPage/RequestDescriptionPage/RequestDescriptionPage.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/pages/TasksPage/RequestDescriptionPage/RequestDescriptionPage.tsx @@ -240,7 +240,6 @@ const RequestDescription = () => { }, ]}> { }, ]}> { }, ]}> { }, ]}>