Conversation
@ShridharGoel @flodnv @JmillsExpensify was this an intentional change to not give auditors access to #admins anymore? |
|
No, it was decided that auditors should have access to #admins, hence that was addressed here. |
|
Cool, @mountiny so they should have access. 👍 |
|
There was some confusion around this, which indeed concluded with adding them in https://github.com/Expensify/Auth/pull/22404 |
|
Agree with Shridhar and Florent. |
|
@trjExpensify should be updated! |
There was a problem hiding this comment.
💡 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".
Explanation of Change
Documents the
Auditorrole in the member-management rules. The Workspace and #announce tables carried anAuditor?header with a literal question mark, and the #admins, workspace rooms and expense chats tables had no auditor column at all.Auditor?toAuditor. The existing values were already correct, so no cell values moved.Auditorcolumn.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.
PolicyUtils.tscanMemberWrite/canMemberAssignRolewriteCapability = admins;ReportAction::throwIfNotAuthorizedToCommentInAdminsRoomPolicy::buildRolePermissionsgrantsadminsRoomwrite toROLE_AUDITOR;Report::canWriteInChat;LeaveRoomPolicy::shareWithEmployeesskips onlyVISIBILITY_PRIVATE;Report::canAlterRoomMembershipReport::shareWithPolicyAuditors;RemoveFromRoom::getRemovableTargetAccountIDsWorth a careful read: auditors can comment in #admins, not just read it. #admins is created with no
writeCapability, unlike #announce, andReport::canWriteInChatlets anyone holding theauditorshare 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.
RemoveFromRoomfilters protected targets only whenisPolicyExpenseChatis true, and the unshare protections inReport::sharecover only Expensify system accounts, the report creator (0for 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:
contributingGuides/philosophies/SECURITY.mdon GitHub and verify the four changed tables (Workspace, #announce, #admins, workspace rooms) render with the correct number of columns.Auditor?column headers remain in the file.npm run spell-changed -- contributingGuides/philosophies/SECURITY.mdreports no issues.Offline tests
N/A, no app behaviour changes.
QA Steps
N/A, no app behaviour changes. The title carries [No QA].
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari