mirror of
https://github.com/mattermost/mattermost.git
synced 2026-06-11 20:07:34 +00:00
a9e574a826
* MM-68547: Tighten authorization on group syncable link and patch endpoints
Adds an additional permission check on the group syncable link and patch
endpoints. Callers must hold the role-management permission for the
target team or channel (or the sysconsole groups-management permission).
Made-with: Cursor
* Linting
* MM-68547: Extend group syncable scheme_admin authorization checks
Gate any explicit scheme_admin value (in either direction) on link and
patch. Populate SchemeAdmin in the singular getGroupSyncable so that
patches that do not touch scheme_admin no longer overwrite the persisted
value. Restrict PermittedSyncableAdmins to active syncables. Start the
link upsert from the existing active row to preserve fields the caller
did not, or could not, set.
Made-with: Cursor
* MM-68547: Add store-layer regression coverage for SchemeAdmin handling
Extend testGetGroupSyncable to round-trip SchemeAdmin: true through
UpdateGroupSyncable and re-fetch, locking in that getGroupSyncable
populates the field from the persisted row.
Strengthen groupTestPermittedSyncableAdmins{Team,Channel} to assert
that DeleteGroupSyncable preserves SchemeAdmin in the persisted row
and that PermittedSyncableAdmins still excludes the row, making the
coupling between the two store changes explicit.
Made-with: Cursor
* MM-68547: Fix group details role-change dedup on remove
The roleChangeKey helper was reading team_id/channel_id from the items
in itemsToRemove, but onRemoveTeamOrChannel pushes those items with a
generic id field. The deletion of the staged role change in
handleRemovedTeamsAndChannels therefore never matched the key produced
by onChangeRoles, and a stale patchGroupSyncable was dispatched after
the unlink.
Accept either id or team_id/channel_id when computing the key. Also
extend the e2e assertion to verify the channel removal took effect
(delete_at != 0) alongside the existing scheme_admin check.
Made-with: Cursor
* MM-68547: Mirror delete_at assertion on the removed-team e2e test
The team variant of "does not update the role of a removed X" was left
asserting only on scheme_admin. Add the matching delete_at != 0 check
already present in the channel variant so both tests verify the same
user-visible contract.
Made-with: Cursor
* Skip SyncSyncableRoles if no scheme_admin
(cherry picked from commit 8c72083414)
Co-authored-by: Maria A Nunez <maria.nunez@mattermost.com>
Server Channels Review Guidelines
When reviewing or writing code in the server channels package, focus on SQL query performance and API layer efficiency.
SQL Store Layer
- Run
EXPLAIN ANALYZEon new or modified queries against a large dataset before merging. A query that performs well on a 12M-post database may degrade significantly at 100M+ posts. - Watch for sequential scans on large tables. Ensure appropriate indexes exist for new query patterns.
- When adding new queries to the store, check whether an existing query already fetches the needed data. Avoid duplicate round trips to the database.
API Layer
- Minimize database round trips. If an endpoint calls a
Getfollowed by aDeleteon the same row, consider usingDELETE ... RETURNINGto combine them into a single query. - Don't add queries that are unnecessary for the operation. The most efficient work is the work you don't do.
- When adding new API endpoints, add them to the load test tooling so performance can be validated under realistic concurrency.
Permissions and Security
- Verify that new endpoints enforce appropriate permissions. Rely on the dedicated security review for thorough coverage, but flag anything obviously missing (e.g., an endpoint that skips permission checks entirely).