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

Fix/31863: Don't allow member to edit task in room #32165

Merged
merged 10 commits into from
Dec 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion src/components/ReportActionItem/TaskPreview.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,12 @@ const propTypes = {
ownerAccountID: PropTypes.number,
}),

/** The policy of root parent report */
rootParentReportpolicy: PropTypes.shape({
/** The role of current user */
role: PropTypes.string,
}),

/** The chat report associated with taskReport */
chatReportID: PropTypes.string.isRequired,

Expand All @@ -72,6 +78,7 @@ const propTypes = {
const defaultProps = {
...withCurrentUserPersonalDetailsDefaultProps,
taskReport: {},
rootParentReportpolicy: {},
isHovered: false,
};

Expand Down Expand Up @@ -116,7 +123,7 @@ function TaskPreview(props) {
style={[styles.mr2]}
containerStyle={[styles.taskCheckbox]}
isChecked={isTaskCompleted}
disabled={!Task.canModifyTask(props.taskReport, props.currentUserPersonalDetails.accountID)}
disabled={!Task.canModifyTask(props.taskReport, props.currentUserPersonalDetails.accountID, lodashGet(props.rootParentReportpolicy, 'role', ''))}
onPress={Session.checkIfActionIsAllowed(() => {
if (isTaskCompleted) {
Task.reopenTask(props.taskReport);
Expand Down Expand Up @@ -149,5 +156,9 @@ export default compose(
key: ({taskReportID}) => `${ONYXKEYS.COLLECTION.REPORT}${taskReportID}`,
initialValue: {},
},
rootParentReportpolicy: {
key: ({policyID}) => `${ONYXKEYS.COLLECTION.POLICY}${policyID || '0'}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need in this case to get the ReportUtils.getRootParentReport(report); first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated name of props. policyID props here was already policyID of root parent report

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aldo-expensify Updated. please check again

selector: (policy) => _.pick(policy, ['role']),
},
}),
)(TaskPreview);
21 changes: 20 additions & 1 deletion src/components/ReportActionItem/TaskView.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import PropTypes from 'prop-types';
import React, {useEffect} from 'react';
import {View} from 'react-native';
import {withOnyx} from 'react-native-onyx';
import _ from 'underscore';
import Checkbox from '@components/Checkbox';
import Hoverable from '@components/Hoverable';
import Icon from '@components/Icon';
Expand Down Expand Up @@ -37,6 +38,12 @@ const propTypes = {
/** The report currently being looked at */
report: reportPropTypes.isRequired,

/** The policy of root parent report */
policy: PropTypes.shape({
/** The role of current user */
role: PropTypes.string,
}),

/** Whether we should display the horizontal rule below the component */
shouldShowHorizontalRule: PropTypes.bool.isRequired,

Expand All @@ -45,6 +52,10 @@ const propTypes = {
...withCurrentUserPersonalDetailsPropTypes,
};

const defaultProps = {
policy: {},
};

function TaskView(props) {
const theme = useTheme();
const styles = useThemeStyles();
Expand All @@ -56,7 +67,7 @@ function TaskView(props) {
const assigneeTooltipDetails = ReportUtils.getDisplayNamesWithTooltips(OptionsListUtils.getPersonalDetailsForAccountIDs([props.report.managerID], props.personalDetails), false);
const isCompleted = ReportUtils.isCompletedTaskReport(props.report);
const isOpen = ReportUtils.isOpenTaskReport(props.report);
const canModifyTask = Task.canModifyTask(props.report, props.currentUserPersonalDetails.accountID);
const canModifyTask = Task.canModifyTask(props.report, props.currentUserPersonalDetails.accountID, lodashGet(props.policy, 'role', ''));
const disableState = !canModifyTask;
const isDisableInteractive = !canModifyTask || !isOpen;
const personalDetails = usePersonalDetails() || CONST.EMPTY_OBJECT;
Expand Down Expand Up @@ -189,6 +200,7 @@ function TaskView(props) {
}

TaskView.propTypes = propTypes;
TaskView.defaultProps = defaultProps;
TaskView.displayName = 'TaskView';

export default compose(
Expand All @@ -199,5 +211,12 @@ export default compose(
personalDetails: {
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
},
policy: {
key: ({report}) => {
const rootParentReport = ReportUtils.getRootParentReport(report);
return `${ONYXKEYS.COLLECTION.POLICY}${rootParentReport ? rootParentReport.policyID : '0'}`;
},
selector: (policy) => _.pick(policy, ['role']),
},
}),
)(TaskView);
13 changes: 11 additions & 2 deletions src/components/TaskHeaderActionButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,22 +13,25 @@ import Button from './Button';
type TaskHeaderActionButtonOnyxProps = {
/** Current user session */
session: OnyxEntry<OnyxTypes.Session>;

/** The policy of root parent report */
policy: OnyxEntry<OnyxTypes.Policy>;
};

type TaskHeaderActionButtonProps = TaskHeaderActionButtonOnyxProps & {
/** The report currently being looked at */
report: OnyxTypes.Report;
};

function TaskHeaderActionButton({report, session}: TaskHeaderActionButtonProps) {
function TaskHeaderActionButton({report, session, policy}: TaskHeaderActionButtonProps) {
const {translate} = useLocalize();
const styles = useThemeStyles();

return (
<View style={[styles.flexRow, styles.alignItemsCenter, styles.justifyContentEnd]}>
<Button
success
isDisabled={!Task.canModifyTask(report, session?.accountID ?? 0)}
isDisabled={!Task.canModifyTask(report, session?.accountID ?? 0, policy?.role ?? '')}
medium
text={translate(ReportUtils.isCompletedTaskReport(report) ? 'task.markAsIncomplete' : 'task.markAsComplete')}
onPress={Session.checkIfActionIsAllowed(() => (ReportUtils.isCompletedTaskReport(report) ? Task.reopenTask(report) : Task.completeTask(report)))}
Expand All @@ -44,4 +47,10 @@ export default withOnyx<TaskHeaderActionButtonProps, TaskHeaderActionButtonOnyxP
session: {
key: ONYXKEYS.SESSION,
},
policy: {
key: ({report}) => {
const rootParentReport = ReportUtils.getRootParentReport(report);
return `${ONYXKEYS.COLLECTION.POLICY}${rootParentReport ? rootParentReport.policyID : '0'}`;
},
},
})(TaskHeaderActionButton);
11 changes: 9 additions & 2 deletions src/libs/actions/Task.js
Original file line number Diff line number Diff line change
Expand Up @@ -866,9 +866,11 @@ function getTaskOwnerAccountID(taskReport) {
* Check if you're allowed to modify the task - anyone that has write access to the report can modify the task
* @param {Object} taskReport
* @param {Number} sessionAccountID
* @param {String} policyRole
*
* @returns {Boolean}
*/
function canModifyTask(taskReport, sessionAccountID) {
function canModifyTask(taskReport, sessionAccountID, policyRole = '') {
if (ReportUtils.isCanceledTaskReport(taskReport)) {
return false;
}
Expand All @@ -877,10 +879,15 @@ function canModifyTask(taskReport, sessionAccountID) {
return true;
}

const parentReport = ReportUtils.getParentReport(taskReport);

if (policyRole && (ReportUtils.isChatRoom(parentReport) || ReportUtils.isPolicyExpenseChat(parentReport)) && policyRole !== CONST.POLICY.ROLE.ADMIN) {
return false;
}
Comment on lines +882 to +886
Copy link
Contributor

Choose a reason for hiding this comment

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

For non-member of workspace, policyRole is falsy, so this condition wasn't met.
So it caused regression - #33420

Details: #33420 (comment)


// If you don't have access to the task report (maybe haven't opened it yet), check if you can access the parent report
// - If the parent report is an #admins only room
// - If you are a policy admin
const parentReport = ReportUtils.getParentReport(taskReport);
return ReportUtils.isAllowedToComment(parentReport);
}

Expand Down
16 changes: 15 additions & 1 deletion src/pages/home/HeaderView.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,12 @@ const propTypes = {
accountID: PropTypes.number,
}),

/** The policy of root parent report */
rootParentReportPolicy: PropTypes.shape({
/** The role of current user */
role: PropTypes.string,
}),

/** The current policy of the report */
policy: PropTypes.shape({
/** The policy name */
Expand All @@ -77,6 +83,7 @@ const defaultProps = {
accountID: 0,
},
policy: {},
rootParentReportPolicy: {},
};

function HeaderView(props) {
Expand Down Expand Up @@ -112,7 +119,7 @@ function HeaderView(props) {
// these users via alternative means. It is possible to request a call with Concierge so we leave the option for them.
const threeDotMenuItems = [];
if (isTaskReport && !isCanceledTaskReport) {
const canModifyTask = Task.canModifyTask(props.report, props.session.accountID);
const canModifyTask = Task.canModifyTask(props.report, props.session.accountID, lodashGet(props.rootParentReportPolicy, 'role', ''));

// Task is marked as completed
if (ReportUtils.isCompletedTaskReport(props.report) && canModifyTask) {
Expand Down Expand Up @@ -311,5 +318,12 @@ export default memo(
key: ({report}) => `${ONYXKEYS.COLLECTION.POLICY}${report ? report.policyID : '0'}`,
selector: (policy) => _.pick(policy, ['name', 'avatar', 'pendingAction']),
},
rootParentReportPolicy: {
key: ({report}) => {
const rootParentReport = ReportUtils.getRootParentReport(report);
return `${ONYXKEYS.COLLECTION.POLICY}${rootParentReport ? rootParentReport.policyID : '0'}`;
},
selector: (policy) => _.pick(policy, ['role']),
},
})(HeaderView),
);
1 change: 1 addition & 0 deletions src/pages/home/report/ReportActionItem.js
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,7 @@ function ReportActionItem(props) {
<TaskPreview
taskReportID={props.action.originalMessage.taskReportID.toString()}
chatReportID={props.report.reportID}
policyID={ReportUtils.getRootParentReport(props.report).policyID}
action={props.action}
isHovered={hovered}
contextMenuAnchor={popoverAnchorRef}
Expand Down
16 changes: 15 additions & 1 deletion src/pages/tasks/TaskAssigneeSelectorModal.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,12 @@ const propTypes = {
report: reportPropTypes,
}),

/** The policy of root parent report */
rootParentReportPolicy: PropTypes.shape({
/** The role of current user */
role: PropTypes.string,
}),

...withLocalizePropTypes,
};

Expand All @@ -66,6 +72,7 @@ const defaultProps = {
session: {},
route: {},
task: {},
rootParentReportPolicy: {},
};

function TaskAssigneeSelectorModal(props) {
Expand Down Expand Up @@ -204,7 +211,7 @@ function TaskAssigneeSelectorModal(props) {
};

const isOpen = ReportUtils.isOpenTaskReport(report);
const canModifyTask = Task.canModifyTask(report, props.currentUserPersonalDetails.accountID);
const canModifyTask = Task.canModifyTask(report, props.currentUserPersonalDetails.accountID, lodashGet(props.rootParentReportPolicy, 'role', ''));
const isTaskNonEditable = ReportUtils.isTaskReport(report) && (!canModifyTask || !isOpen);

return (
Expand Down Expand Up @@ -259,5 +266,12 @@ export default compose(
session: {
key: ONYXKEYS.SESSION,
},
rootParentReportPolicy: {
key: ({report}) => {
const rootParentReport = ReportUtils.getRootParentReport(report);
return `${ONYXKEYS.COLLECTION.POLICY}${rootParentReport ? rootParentReport.policyID : '0'}`;
},
selector: (policy) => _.pick(policy, ['role']),
},
}),
)(TaskAssigneeSelectorModal);
19 changes: 18 additions & 1 deletion src/pages/tasks/TaskDescriptionPage.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
import {useFocusEffect} from '@react-navigation/native';
import ExpensiMark from 'expensify-common/lib/ExpensiMark';
import lodashGet from 'lodash/get';
import PropTypes from 'prop-types';
import React, {useCallback, useRef} from 'react';
import {View} from 'react-native';
import {withOnyx} from 'react-native-onyx';
import _ from 'underscore';
import FullPageNotFoundView from '@components/BlockingViews/FullPageNotFoundView';
import FormProvider from '@components/Form/FormProvider';
import InputWrapper from '@components/Form/InputWrapper';
Expand All @@ -28,12 +31,19 @@ const propTypes = {
/** The report currently being looked at */
report: reportPropTypes,

/** The policy of parent report */
rootParentReportPolicy: PropTypes.shape({
/** The role of current user */
role: PropTypes.string,
}),

/* Onyx Props */
...withLocalizePropTypes,
};

const defaultProps = {
report: {},
rootParentReportPolicy: {},
};

const parser = new ExpensiMark();
Expand Down Expand Up @@ -64,7 +74,7 @@ function TaskDescriptionPage(props) {
const focusTimeoutRef = useRef(null);

const isOpen = ReportUtils.isOpenTaskReport(props.report);
const canModifyTask = Task.canModifyTask(props.report, props.currentUserPersonalDetails.accountID);
const canModifyTask = Task.canModifyTask(props.report, props.currentUserPersonalDetails.accountID, lodashGet(props.rootParentReportPolicy, 'role', ''));
const isTaskNonEditable = ReportUtils.isTaskReport(props.report) && (!canModifyTask || !isOpen);

useFocusEffect(
Expand Down Expand Up @@ -138,5 +148,12 @@ export default compose(
report: {
key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT}${route.params.reportID}`,
},
rootParentReportPolicy: {
key: ({report}) => {
const rootParentReport = ReportUtils.getRootParentReport(report);
return `${ONYXKEYS.COLLECTION.POLICY}${rootParentReport ? rootParentReport.policyID : '0'}`;
},
selector: (policy) => _.pick(policy, ['role']),
},
}),
)(TaskDescriptionPage);
18 changes: 17 additions & 1 deletion src/pages/tasks/TaskTitlePage.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import lodashGet from 'lodash/get';
import PropTypes from 'prop-types';
import React, {useCallback, useRef} from 'react';
import {View} from 'react-native';
import {withOnyx} from 'react-native-onyx';
Expand All @@ -24,12 +26,19 @@ const propTypes = {
/** The report currently being looked at */
report: reportPropTypes,

/** The policy of parent report */
rootParentReportPolicy: PropTypes.shape({
/** The role of current user */
role: PropTypes.string,
}),

/* Onyx Props */
...withLocalizePropTypes,
};

const defaultProps = {
report: {},
rootParentReportPolicy: {},
};

function TaskTitlePage(props) {
Expand Down Expand Up @@ -70,7 +79,7 @@ function TaskTitlePage(props) {

const inputRef = useRef(null);
const isOpen = ReportUtils.isOpenTaskReport(props.report);
const canModifyTask = Task.canModifyTask(props.report, props.currentUserPersonalDetails.accountID);
const canModifyTask = Task.canModifyTask(props.report, props.currentUserPersonalDetails.accountID, lodashGet(props.rootParentReportPolicy, 'role', ''));
const isTaskNonEditable = ReportUtils.isTaskReport(props.report) && (!canModifyTask || !isOpen);

return (
Expand Down Expand Up @@ -130,5 +139,12 @@ export default compose(
report: {
key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT}${route.params.reportID}`,
},
rootParentReportPolicy: {
key: ({report}) => {
const rootParentReport = ReportUtils.getRootParentReport(report);
return `${ONYXKEYS.COLLECTION.POLICY}${rootParentReport ? rootParentReport.policyID : '0'}`;
},
selector: (policy) => _.pick(policy, ['role']),
},
}),
)(TaskTitlePage);
Loading