[HOLD for payment 2023-07-26] [$1000] Web - UserB can access the room "Welcome message" page and update it without permission

@ayazhussain79

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Action Performed:

Login to userA's account
Go to the "New room" section
Enter a room name and select userA's workspace
Set the visibility to "Public"
Click on "Create room"
Go to the room details.
Go to Share code > Copy the URL to the clipboard
Return to the room details
Navigate to Settings > Welcome message
Open a different browser and login to userB's account
Join the room by pasting the room URL
Now copy the “Welcome message” page URL from userA account
Paste it on userB’s browser
Update the "Welcome message"
Now copy the room URL from userA's account
Paste the URL in another browser

Expected Result:

UserB should be unable to access the "Welcome message" page and should not be able to modify it

Actual Result:

UserB can access the "Welcome message" page and update it without permission

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Version Number: 1.3.34-1
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

screen-recording-2023-06-30-at-125608-am_4BuHkCsb.1.mp4

Recording.5257.mp4

Expensify/Expensify Issue URL:
Issue reported by: @ayazhussain79
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1688069999157569

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~0112e180fbc88a066d
Upwork Job ID: 1676001917057400832
Last Price Increase: 2023-07-03

Proposal

Please re-state the problem

Unauthorized users are able to edit the welcome message of the workspace room.

What is the root cause of that problem?

The root cause is that there are no checks in place either on the backend or the front-end if the user has access to the workspace as an admin.

What changes do you think we should make to solve this problem?

On the front-end, we should prevent the user from accessing the welcomeMessage page if the user is not an admin of the workspace.

First we need to add the withOnyx HOC to access the policy in ReportWelcomeMessagePage.js:

withOnyx({
    policy: {
        key: ({report}) => `${ONYXKEYS.COLLECTION.POLICY}${report.policyID}`
    }
}),

Here are the possible approaches:

We can navigate the user back if they are not an admin.
We can show the not found page if they are not an admin.

Option 1
We can add a userEffect hook to check if the user is admin and if they not, just navigate them back like this:

useEffect(() => {
      if (PolicyUtils.isPolicyAdmin(props.policy)) {
          return;
      }
      Navigation.goBack();
  }, [props.policy]);

Option 2
We are already using FullPageNotFoundView and we can add an additional condition for that as follows:

const shouldHide = _.isEmpty(props.report) || !PolicyUtils.isPolicyAdmin(props.policy);

Additional Note

We should also do this for settings like who can post and room name.
Currently, we allow the user to see the edit room name page, but there is an error as soon as we try to change the name from the backend. I believe we should add this backend functionality to all other settings (like who can post, welcome message) also while adding the front-end safeguards proposed above.

What other approach did you explore

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

@puneetlath

Triggered auto assignment to @puneetlath (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

Bug0 Triage Checklist (Main S/O)

This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

Proposal

Please re-state the problem that we are trying to solve in this issue.

Web - UserB can access the room "Welcome message" page and update it without permission

What is the root cause of that problem?

In current ReportWelcomeMessagePage we didn't have a check if current user has permission to open it, we only prevent it from ReportSettingsPage.

What changes do you think we should make in order to solve the problem?

Since we need to reuse this in few other places, the best way to reuse the logic is to write a HOC. Let's call it withPolicyAdmin, we will wrap this HOC with any page that only can be access by admin user ie: Welcome message, who can post....etc.

import PropTypes from 'prop-types';
import React from 'react';
import {withOnyx} from 'react-native-onyx';
import _ from 'underscore';
import getComponentDisplayName from '../../../libs/getComponentDisplayName';
import NotFoundPage from '../../ErrorPage/NotFoundPage';
import ONYXKEYS from '../../../ONYXKEYS';
import reportPropTypes from '../../reportPropTypes';
import FullscreenLoadingIndicator from '../../../components/FullscreenLoadingIndicator';
import * as ReportUtils from '../../../libs/ReportUtils';
import * as PolicyUtils from "../../../libs/PolicyUtils";

export default function (WrappedComponent) {
    const propTypes = {
        /** The HOC takes an optional ref as a prop and passes it as a ref to the wrapped component.
         * That way, if a ref is passed to a component wrapped in the HOC, the ref is a reference to the wrapped component, not the HOC. */
        forwardedRef: PropTypes.func,

        /** The report currently being looked at */
        report: reportPropTypes,

        /** The policies which the user has access to */
        policies: PropTypes.objectOf(
            PropTypes.shape({
                /** The policy name */
                name: PropTypes.string,

                /** The type of the policy */
                type: PropTypes.string,
            }),
        ),

        /** Beta features list */
        betas: PropTypes.arrayOf(PropTypes.string),

        /** Indicated whether the report data is loading */
        isLoadingReportData: PropTypes.bool,
    };

    const defaultProps = {
        forwardedRef: () => {},
        report: {},
        policies: {},
        betas: [],
        isLoadingReportData: true,
    };

    // eslint-disable-next-line rulesdir/no-negated-variables
    function WithPolicyAdmin(props) {
        if (props.isLoadingReportData && (_.isEmpty(props.report) || !props.report.reportID)) {
            return <FullscreenLoadingIndicator />;
        } 
      // if we don't have permission, show not found page.
        if (_.isEmpty(props.report) || !props.report.reportID || !ReportUtils.canAccessReport(props.report, props.policies, props.betas) || !PolicyUtils.isPolicyAdmin(props.policy)) {
            return <NotFoundPage />;
        }
        const rest = _.omit(props, ['forwardedRef']);
        return (
            <WrappedComponent
                // eslint-disable-next-line react/jsx-props-no-spreading
                {...rest}
                ref={props.forwardedRef}
            />
        );
    }

    WithPolicyAdmin.propTypes = propTypes;
    WithPolicyAdmin.defaultProps = defaultProps;
    WithPolicyAdmin.displayName = `withPolicyAdmin(${getComponentDisplayName(WrappedComponent)})`;

    // eslint-disable-next-line rulesdir/no-negated-variables
    const withPolicyAdmin = React.forwardRef((props, ref) => (
        <WithPolicyAdmin
            // eslint-disable-next-line react/jsx-props-no-spreading
            {...props}
            forwardedRef={ref}
        />
    ));

    return withOnyx({
        report: {
            key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT}${route.params.reportID}`,
        },
        isLoadingReportData: {
            key: ONYXKEYS.IS_LOADING_REPORT_DATA,
        },
        betas: {
            key: ONYXKEYS.BETAS,
        },
        policies: {
            key: ONYXKEYS.COLLECTION.POLICY,
        },
    })(withPolicyAdmin);
}

Since this logic will cover withReportOrNotFound we can deprecate it.

What alternative solutions did you explore? (Optional)

N/A

Interesting. I agree that we need to restrict this from both the front-end and back-end.

Job added to Upwork: https://www.upwork.com/jobs/~0112e180fbc88a066d

Reviewing label has been removed, please complete the "BugZero Checklist".

@ayazhussain79

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.42-26 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

Restrict workspace settings to admins #22820

If no regressions arise, payment will be issued on 2023-07-26. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

External issue reporter @ayazhussain79
Contributor that fixed the issue @esh-g
Contributor+ that helped on the issue and/or PR @mollfpr

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

Merged PR within 3 business days of assignment - 50% bonus
Merged PR more than 9 business days after assignment - 50% penalty

@mollfpr

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[@mollfpr] The PR that introduced the bug has been identified. Link to the PR:
[@mollfpr] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
[@mollfpr] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
[@mollfpr] Determine if we should create a regression test for this bug.
[@mollfpr] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
[@puneetlath] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@mollfpr

Looks like everyone has had contracts sent.

@mollfpr friendly reminder on the checklist.

@ayazhussain79

Issue reporter: @ayazhussain79 $250
Contributor: @esh-g $1500 (including bonus)
C+: @mollfpr $1500 (including bonus)

I've paid the first two. @mollfpr just waiting on the checklist for you.

@mollfpr

Sorry, I swear I already complete the checklist of the issues near the payment date 😅

[@mollfpr] The PR that introduced the bug has been identified. Link to the PR:

#18662
#21857

[@mollfpr] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:

https://github.com/Expensify/App/pull/18662/files#r1278793008
https://github.com/Expensify/App/pull/21857/files#r1278797201

[@mollfpr] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

The regression step should be enough.

[@mollfpr] Determine if we should create a regression test for this bug.
[@mollfpr] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Test Setup

From account A, create a new workspace.
Invite account B.
Create a workspace from account B.

Therefore, B is an admin in workspace B and not workspace A.

We need to prepare a chat with deep links to the correct page for testing. This setup can be done once on web, and then can be tested on other platforms with the same account.

✅ The page will open
❌ The not found page will be open
Please look at the 'Test Setup (Web)' in the screenshots section to ensure steps are followed correctly.

We first need to copy the reportID of #announce of workspace A and workspace B then also #admin of workspace B. This can be done by opening the room and copying the number in the URL localhost:8080/r/[reportID].
[#announce B] means the reportID of the #announce room of workspace B. The same format applies for all other reportIDs copied.
Go to any chat on account B.
Add a comment 'Welcome message as admin' and add a link like this: https://staging.new.expensify.com/r/[#announce B]/welcomeMessage ✅
Add another comment 'Welcome message not as admin' and add a link like this: https://staging.new.expensify.com/r/[#announce A]/welcomeMessage. ❌
Add another comment 'Who can post (in #admin)' and add this a link like this: https://staging.new.expensify.com/r/[#admin B]/settings-who-can-post. ❌
Add another comment 'Who can post (as admin)' and add a link like this: https://staging.new.expensify.com/r/[#announce B]/settings/who-can-post. ✅
Add another comment 'Who can post (not as admin)' and add a link like this: https://staging.new.expensify.com/r/[#announce A]/settings/who-can-post. ❌
Click on each link and verify that it opens correctly according to ✅ and ❌.

Great, thanks. Paid!

Navigation Menu

Repository navigation

[HOLD for payment 2023-07-26] [$1000] Web - UserB can access the room "Welcome message" page and update it without permission #22013

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Proposal

Please re-state the problem

What is the root cause of that problem?

What changes do you think we should make to solve this problem?

Additional Note

What other approach did you explore

Bug0 Triage Checklist (Main S/O)

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

What alternative solutions did you explore? (Optional)

32 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

[HOLD for payment 2023-07-26] [$1000] Web - UserB can access the room "Welcome message" page and update it without permission #22013

Description

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Activity

esh-g commented on Jul 1, 2023

Proposal

Please re-state the problem

What is the root cause of that problem?

What changes do you think we should make to solve this problem?

Additional Note

What other approach did you explore

melvin-bot commented on Jul 1, 2023

melvin-bot commented on Jul 1, 2023

melvin-bot commented on Jul 1, 2023

Bug0 Triage Checklist (Main S/O)

hungvu193 commented on Jul 1, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

What alternative solutions did you explore? (Optional)

puneetlath commented on Jul 3, 2023

melvin-bot commented on Jul 3, 2023

32 remaining items

melvin-bot commented on Jul 19, 2023

melvin-bot commented on Jul 19, 2023

melvin-bot commented on Jul 19, 2023

puneetlath commented on Jul 24, 2023

puneetlath commented on Jul 27, 2023

mollfpr commented on Jul 31, 2023

puneetlath commented on Jul 31, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions