Skip to content

[No QA] Document the Auditor role in the security philosophy - #101433

Open
mountiny wants to merge 4 commits into
mainfrom
vit-auditor-security-docs
Open

mountiny wants to merge 4 commits into
mainfrom
vit-auditor-security-docs

Conversation

@mountiny

@mountiny mountiny commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Explanation of Change

Documents the Auditor role in the member-management rules. The Workspace and #announce tables carried an Auditor? header with a literal question mark, and the #admins, workspace rooms and expense chats tables had no auditor column at all.

  • Workspace, #announce: resolved Auditor? to Auditor. The existing values were already correct, so no cell values moved.
  • #admins, workspace rooms: added an Auditor column.
  • Expense chats: bullets only, no column — the answer differs between an auditor's own expense chat and another member's.

Room membership and write access are decided in Auth, so those rows come from the backend. The workspace-level rows come from the App, which is where those are gated.

Chat type Auditor behaviour Source
Workspace No invite, no remove, can leave, can be removed PolicyUtils.ts canMemberWrite / canMemberAssignRole
#announce Added on joining, can't leave, can't be removed, can't post #announce is created with writeCapability = admins; ReportAction::throwIfNotAuthorizedToCommentInAdminsRoom
#admins Added with the role, reads and comments, can't leave, can be removed Policy::buildRolePermissions grants adminsRoom write to ROLE_AUDITOR; Report::canWriteInChat; LeaveRoom
Workspace rooms Added to the non-private ones with the role, can invite and remove others, can leave, can be removed Policy::shareWithEmployees skips only VISIBILITY_PRIVATE; Report::canAlterRoomMembership
Expense chats Added to all of them, can comment, can't be removed, can leave another member's Report::shareWithPolicyAuditors; RemoveFromRoom::getRemovableTargetAccountIDs

Worth a careful read: auditors can comment in #admins, not just read it. #admins is created with no writeCapability, unlike #announce, and Report::canWriteInChat lets anyone holding the auditor share permission write.

Auditors are documented as non-removable from #admins, matching the MUST rule at the top of the doc and the Admin column beside it. Membership follows the role, so losing #admins is a role change rather than a removal. The rules section gains the auditor case explicitly.

Context worth knowing, but not work for this PR: nothing enforces that rule server-side for the default rooms. RemoveFromRoom filters protected targets only when isPolicyExpenseChat is true, and the unshare protections in Report::share cover only Expensify system accounts, the report creator (0 for default rooms) and trip creators. So an admin is as removable from #admins as an auditor. The App simply offers no removal UI for default rooms.

cc @tgolen as code owner of contributingGuides/philosophies/.

Fixed Issues

$ N/A, documentation update
PROPOSAL:

Tests

N/A, documentation only. Verified by:

  1. Render contributingGuides/philosophies/SECURITY.md on GitHub and verify the four changed tables (Workspace, #announce, #admins, workspace rooms) render with the correct number of columns.
  2. Verify no Auditor? column headers remain in the file.
  3. Verify npm run spell-changed -- contributingGuides/philosophies/SECURITY.md reports no issues.
  • Verify that no errors appear in the JS console

Offline tests

N/A, no app behaviour changes.

QA Steps

N/A, no app behaviour changes. The title carries [No QA].

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
  • If new assets were added or existing ones were modified, I verified that:
    • The assets are optimized and compressed (for SVG files, run npm run compress-svg)
    • The assets load correctly across all supported platforms.
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari

@mountiny
mountiny requested a review from tgolen as a code owner September 17, 2026 14:45
@mountiny
mountiny removed the request for review from tgolen September 17, 2026 14:55
@mountiny
mountiny marked this pull request as draft September 17, 2026 14:55
tgolen
tgolen previously approved these changes Sep 17, 2026
@trjExpensify

Copy link
Copy Markdown
Contributor

PR #92740 — "Add Card Admin workspace access": this is the PR that actually dropped Auditor from the admins-room check (the 2026-06-12 commit "Update policy admins room role membership"), while adding Card Admin, People Admin, and Payments Admin to the list.

@ShridharGoel @flodnv @JmillsExpensify was this an intentional change to not give auditors access to #admins anymore?

@ShridharGoel

Copy link
Copy Markdown
Contributor

No, it was decided that auditors should have access to #admins, hence that was addressed here.

@trjExpensify

Copy link
Copy Markdown
Contributor

Cool, @mountiny so they should have access. 👍

@flodnv

flodnv commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

There was some confusion around this, which indeed concluded with adding them in https://github.com/Expensify/Auth/pull/22404

@JmillsExpensify

Copy link
Copy Markdown
Contributor

Agree with Shridhar and Florent.

@mountiny
mountiny marked this pull request as ready for review September 17, 2026 19:36
@mountiny

Copy link
Copy Markdown
Contributor Author

@trjExpensify should be updated!

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c52a545b3f

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread contributingGuides/philosophies/SECURITY.md Outdated

@trjExpensify trjExpensify left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants