MM-69269 - Spaces permissions and RBAC: read gates, capabilities, auto-join - #10
MM-69269 - Spaces permissions and RBAC: read gates, capabilities, auto-join#10catalintomai wants to merge 31 commits into
Conversation
📝 WalkthroughWalkthroughThis change introduces capability-based Space RBAC, open/private view access, scheme-backed capabilities, ownership checks for page moves, member capability APIs, and Docker/Testcontainers end-to-end coverage with local and CI execution. ChangesSpace RBAC and capability model
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (8)
go.mod (1)
17-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTemporary pin to the paired core branch must not merge.
server/publicis pinned to a branch pseudo-version. Track replacing it with a released version once core PR#37685lands, otherwise the module is unbuildable for anyone once that branch is deleted or force-pushed. Want me to open a follow-up issue for the un-pin?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@go.mod` at line 17, Replace the branch-based pseudo-version for github.com/mattermost/mattermost/server/public in go.mod with the released version once core PR `#37685` is available, and remove the temporary pin so the module depends on a stable published release.server/e2e/container_test.go (1)
132-146: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPolling ignores context cancellation.
The loop only watches its own 2-minute wall-clock deadline; when the caller's context is cancelled it keeps sleeping and retrying failing calls instead of returning promptly.
♻️ Proposed refactor
for time.Now().Before(deadline) { _, _, err := adminClient.GetSchemes(ctx, "", 0, 1) if err == nil { return nil } lastErr = err - time.Sleep(2 * time.Second) + select { + case <-ctx.Done(): + return fmt.Errorf("waiting for advanced-permissions phase-2 migration: %w (last error: %v)", ctx.Err(), lastErr) + case <-time.After(2 * time.Second): + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/e2e/container_test.go` around lines 132 - 146, Update waitForPhase2Migration to honor ctx cancellation during both polling and the 2-second delay, returning ctx.Err() promptly when cancelled. Replace the unconditional time.Sleep and ensure the loop checks the context before retries while preserving the existing deadline and final timeout error behavior.server/e2e/helpers_test.go (1)
79-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
context.Contextshould come before*testing.T, or the lint config needs an exemption. revive’scontext-as-argumentrule is enabled, andserver/e2e/helpers_test.godoesn’t whitelist*testing.T, socreateActor,addSpaceMember,spaceHasMember, anddeleteSpacewill be flagged when thee2epackage is linted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/e2e/helpers_test.go` at line 79, Update createActor, addSpaceMember, spaceHasMember, and deleteSpace so context.Context is the first parameter, before *testing.T, and adjust every call site accordingly; alternatively, add the e2e test helpers to the revive context-as-argument exemption if that is the established linting approach.server/store/migrations/000007_add_viewaccess_to_spaces.up.sql (1)
5-5: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider
NOT VALIDfor the CHECK, and note this statement is not retry-safe.Two things about line 5:
ADD CONSTRAINT ... CHECKtakesACCESS EXCLUSIVEand scans the table to validate. Every row was just written by theDEFAULT 'private'on line 1, so the scan can only pass — adding itNOT VALID(enforced for new writes, no scan) avoids blocking writes on a largeDOCS_Space.- Unlike line 1, Postgres has no
ADD CONSTRAINT IF NOT EXISTS, so a re-run after a partially applied migration fails withduplicate_objectand leaves the version dirty for an operator to clear.♻️ Suggested change
-ALTER TABLE DOCS_Space ADD CONSTRAINT chk_docs_space_view_access CHECK (ViewAccess IN ('open', 'private')); +ALTER TABLE DOCS_Space ADD CONSTRAINT chk_docs_space_view_access CHECK (ViewAccess IN ('open', 'private')) NOT VALID;As per static analysis hint
constraint-missing-not-valid.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/store/migrations/000007_add_viewaccess_to_spaces.up.sql` at line 5, Update the CHECK constraint creation in this migration to use NOT VALID, avoiding the table-wide validation scan while continuing to enforce the constraint on new writes. Preserve the existing constraint name and condition; do not add retry handling beyond what PostgreSQL supports for ADD CONSTRAINT.Source: Linters/SAST tools
server/app/space.go (1)
372-375: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd the
space == nilguard the other exported space methods all have.
BuildSpaceWithAccessdereferencesspace.Idimmediately, whileCreateSpace,SetSpaceDefaultCapabilities,UpdateSpace,ListSpaceMembers, andAddSpaceMemberall open with an explicit nil check returning 400. Current callers pass a non-nil record, so this is convention/future-proofing rather than a live panic.🛡️ Suggested guard
func (s *Service) BuildSpaceWithAccess(space *model.Space, userID string) (*model.SpaceWithAccess, *mmmodel.AppError) { + if space == nil { + return nil, mmmodel.NewAppError("BuildSpaceWithAccess", "app.space.get.invalid_id.app_error", nil, "", http.StatusBadRequest) + } if appErr := s.requireClient("BuildSpaceWithAccess", "space_id", space.Id, "user_id", userID); appErr != nil {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/app/space.go` around lines 372 - 375, Add an explicit nil check at the start of BuildSpaceWithAccess before dereferencing space.Id, returning the same HTTP 400 AppError pattern used by CreateSpace, SetSpaceDefaultCapabilities, UpdateSpace, ListSpaceMembers, and AddSpaceMember.server/store/space_store.go (1)
104-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer building the
ORbranch in Go over binding a bareboolparameter as a predicate.
sq.Expr("?", callerHasOpenFallthrough)emits a placeholder in boolean position and leans on Postgres resolving the untyped parameter toboolean. It works with the current driver (the store tests cover both values), but dropping the branch when the caller lacks the fall-through is clearer and lets the planner skip theViewAccesspredicate entirely.♻️ Suggested change
- memberExists := sq.Expr("EXISTS (SELECT 1 FROM ChannelMembers cm WHERE cm.ChannelId = sp.ChannelId AND cm.UserId = ?)", userID) - openFallthrough := sq.And{sq.Eq{"sp.ViewAccess": model.ViewAccessOpen}, sq.Expr("?", callerHasOpenFallthrough)} - + memberExists := sq.Expr("EXISTS (SELECT 1 FROM ChannelMembers cm WHERE cm.ChannelId = sp.ChannelId AND cm.UserId = ?)", userID) + visible := sq.Or{memberExists} + if callerHasOpenFallthrough { + visible = append(visible, sq.Eq{"sp.ViewAccess": model.ViewAccessOpen}) + } + builder := s.getQueryBuilder(). Select(columnsWithAlias("sp", spaceSelectColumns)...). From("DOCS_Space sp"). Where(sq.Eq{"sp.TeamId": teamID, "sp.DeleteAt": 0}). - Where(sq.Or{memberExists, openFallthrough}). + Where(visible).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/store/space_store.go` around lines 104 - 111, Update the query construction around memberExists and openFallthrough to build the OR branches in Go: always include memberExists, but add the ViewAccess-open branch only when callerHasOpenFallthrough is true. Remove the boolean placeholder predicate while preserving the existing team, deletion, and membership filters.server/app/permissions.go (1)
254-313: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConfirm the WS publish inside the membership lock is intentional.
publishToChannelsruns whileWithSpaceMembershipLockstill holds its dedicated connection; a slow plugin-API RPC extends lock hold time and can push concurrent membership mutations intoReasonLockTimeout409s. Moving the publish after the lock closure (using the capturedjoined/member.UserId) would keep the critical section to DB + membership work only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/app/permissions.go` around lines 254 - 313, The WS publication in AutoJoinIfDefaultGranted currently runs inside WithSpaceMembershipLock, unnecessarily extending the membership lock during plugin/API work. Capture the joined member’s user ID while performing AddMember, then move publishToChannels outside the lock closure and invoke it only after a successful lock operation and join.server/model/space_capabilities_test.go (1)
112-121: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueModernize the backward loop per static analysis.
golangci-lint flags this as convertible to
slices.Backward(already imported).♻️ Proposed modernization
- shuffled := make([]string, 0, len(contribute)*2) - for i := len(contribute) - 1; i >= 0; i-- { - shuffled = append(shuffled, contribute[i], contribute[i]) - } + shuffled := make([]string, 0, len(contribute)*2) + for _, v := range slices.Backward(contribute) { + shuffled = append(shuffled, v, v) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/model/space_capabilities_test.go` around lines 112 - 121, Update the reverse iteration in the test’s shuffled construction to use the already imported slices.Backward helper instead of the manual index loop, while preserving the duplicate append order and existing assertions.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@build/build-core-image.sh`:
- Around line 14-17: Update the usage examples in build-core-image.sh to
reference build/build-core-image.sh instead of scripts/build-core-image.sh,
including the default, CORE_IMAGE, and --skip-build invocations.
In `@go.mod`:
- Line 37: Update the dependency resolution represented by go.mod so containerd
is at least 1.7.33 and Docker is at least 29.3.1, either by upgrading
testcontainers-go or adding explicit indirect overrides. Verify the resulting
module graph no longer retains the vulnerable containerd v1.7.18 or Docker
v27.1.1 pins.
In `@server/app/page_hierarchy.go`:
- Around line 165-169: Update MovePageToSpace’s same-space branch to enforce
non-empty requiredOwnerID before or during the
reparentWithinSpace/store.MovePage path, preserving the existing in-transaction
ownership semantics used for cross-space moves. Ensure same-space
delete_own_page callers cannot reparent pages outside the required owner, and
add coverage for a same-space move with requiredOwnerID set.
In `@server/e2e/container_test.go`:
- Around line 148-159: Update resolveBundlePath to select the most recently
modified matching bundle rather than matches[len(matches)-1]. Stat each
filepath.Glob result, compare modification times, and return the newest path
while preserving existing glob and error handling.
- Around line 106-127: Use a fresh, independent teardown context whenever
startup fails in the container setup flow around container.URL,
container.GetAdminClient, and waitForPhase2Migration. Replace each
container.Terminate(ctx) call with termination using a short-lived context
created specifically for cleanup, ensuring cleanup still runs when the 3-minute
startEnv context has expired.
In `@server/e2e/helpers_test.go`:
- Around line 61-69: Update the response helper around json.Unmarshal to
propagate decode failures instead of discarding them: capture the unmarshal
error and return it from the helper, wrapping it with the response context as
needed using fmt. Preserve the existing behavior for nil output targets and
empty response bodies so callers such as scenario3_private_team_space can assert
on actual decoded payloads.
In `@server/store/page_move.go`:
- Around line 250-255: Update MovePageToSpace so the IDs returned by
collectLiveSubtreeIDs are re-locked within the transaction before enforcing
requiredOwnerID or trusting the owners map. Re-read or obtain ownership data
from that locked subtree, then preserve the existing validation and
rewriteSubtreeSpace flow using the locked result.
In `@server/store/scheme_store.go`:
- Around line 235-256: Lock the target scheme row before counting channel
references in the surrounding deletion flow, using the existing scheme
identifier and transaction so retirement serializes with channel repointing.
Ensure the repoint path uses the same Schemes-row lock, then retain the
reference check and role/scheme deletion only after the lock is acquired.
---
Nitpick comments:
In `@go.mod`:
- Line 17: Replace the branch-based pseudo-version for
github.com/mattermost/mattermost/server/public in go.mod with the released
version once core PR `#37685` is available, and remove the temporary pin so the
module depends on a stable published release.
In `@server/app/permissions.go`:
- Around line 254-313: The WS publication in AutoJoinIfDefaultGranted currently
runs inside WithSpaceMembershipLock, unnecessarily extending the membership lock
during plugin/API work. Capture the joined member’s user ID while performing
AddMember, then move publishToChannels outside the lock closure and invoke it
only after a successful lock operation and join.
In `@server/app/space.go`:
- Around line 372-375: Add an explicit nil check at the start of
BuildSpaceWithAccess before dereferencing space.Id, returning the same HTTP 400
AppError pattern used by CreateSpace, SetSpaceDefaultCapabilities, UpdateSpace,
ListSpaceMembers, and AddSpaceMember.
In `@server/e2e/container_test.go`:
- Around line 132-146: Update waitForPhase2Migration to honor ctx cancellation
during both polling and the 2-second delay, returning ctx.Err() promptly when
cancelled. Replace the unconditional time.Sleep and ensure the loop checks the
context before retries while preserving the existing deadline and final timeout
error behavior.
In `@server/e2e/helpers_test.go`:
- Line 79: Update createActor, addSpaceMember, spaceHasMember, and deleteSpace
so context.Context is the first parameter, before *testing.T, and adjust every
call site accordingly; alternatively, add the e2e test helpers to the revive
context-as-argument exemption if that is the established linting approach.
In `@server/model/space_capabilities_test.go`:
- Around line 112-121: Update the reverse iteration in the test’s shuffled
construction to use the already imported slices.Backward helper instead of the
manual index loop, while preserving the duplicate append order and existing
assertions.
In `@server/store/migrations/000007_add_viewaccess_to_spaces.up.sql`:
- Line 5: Update the CHECK constraint creation in this migration to use NOT
VALID, avoiding the table-wide validation scan while continuing to enforce the
constraint on new writes. Preserve the existing constraint name and condition;
do not add retry handling beyond what PostgreSQL supports for ADD CONSTRAINT.
In `@server/store/space_store.go`:
- Around line 104-111: Update the query construction around memberExists and
openFallthrough to build the OR branches in Go: always include memberExists, but
add the ViewAccess-open branch only when callerHasOpenFallthrough is true.
Remove the boolean placeholder predicate while preserving the existing team,
deletion, and membership filters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 188835b3-ba09-46b3-b0fe-6018ce2f4806
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (41)
.github/workflows/ci.ymlMakefileassets/i18n/en.jsonbuild/build-core-image.shgo.modserver/api.goserver/api_handler_test.goserver/api_page.goserver/api_space.goserver/app/page_duplicate_test.goserver/app/page_hierarchy.goserver/app/page_move_test.goserver/app/page_move_to_space_test.goserver/app/page_reorder_test.goserver/app/permissions.goserver/app/service.goserver/app/service_test.goserver/app/space.goserver/app/space_members.goserver/app/space_test.goserver/app/ws_events.goserver/app/ws_events_test.goserver/e2e/README.mdserver/e2e/container_test.goserver/e2e/helpers_test.goserver/e2e/scenarios_test.goserver/internal/testutil/fixtures.goserver/internal/testutil/permstub.goserver/model/space.goserver/model/space_capabilities.goserver/model/space_capabilities_test.goserver/model/space_test.goserver/store/migrations/000007_add_viewaccess_to_spaces.down.sqlserver/store/migrations/000007_add_viewaccess_to_spaces.up.sqlserver/store/page_move.goserver/store/page_move_test.goserver/store/scheme_store.goserver/store/scheme_store_test.goserver/store/space_store.goserver/store/store.goserver/store/store_test.go
|
@coderabbitai - On two of the nitpicks from the review body (no inline threads on those, so answering here):
|
|
Tip For best results, initiate chat on the files or code changes.
I’m withdrawing both review-body nitpicks. ✏️ Learnings added
|
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
server/app/ws_events_test.go (2)
674-680: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe escalation guard is not reached, so the
HasPermissionToChannelstub is dead setup.Trace
SetSpaceMemberCapabilitieswith this input.capabilitiesis{edit_page}, sorequestedAdminis false.targetUserID != actingUserID, soselfTargetedis false. The mocked target hasSchemeAdminfalse. The conditionadminAffected || selfTargetedis therefore false andRequireSpaceAdminOrSysadminnever runs.Two consequences. The comment at Line 674 misstates what this test covers. And the stray stub would keep the test green if the guard later became unconditional, which would silently drop coverage of that guard.
Remove the stub and correct the comment. If the guard itself needs coverage, add a separate case that requests
CapabilityAdminSpaceor targets the acting user.🛠️ Proposed fix
- // The acting user must clear the admin-affecting escalation guard. - mockAPI.On("HasPermissionToChannel", actingUserID, space.ChannelId, mmmodel.PermissionAdminSpace).Return(true) + // A non-admin capability on another non-admin member does not reach the escalation guard, so + // no space-admin permission stub is needed here. mockAPI.On("GetChannelMember", space.ChannelId, targetUserID). Return(&mmmodel.ChannelMember{ChannelId: space.ChannelId, UserId: targetUserID}, nil)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/app/ws_events_test.go` around lines 674 - 680, Remove the unused HasPermissionToChannel stub from the SetSpaceMemberCapabilities test and revise the nearby comment so it accurately describes the non-admin capability scenario. Do not add guard coverage to this case; use a separate test case requesting CapabilityAdminSpace or targeting actingUserID if coverage is needed.
641-662: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
TestServiceAddSpaceMember_PublishesToChannelAndUseris a superset of the existing test.
TestServiceAddSpaceMember_PublishesMemberAddedEvent(Lines 598-613) uses the same helper, the sameAddChannelMemberstub, and asserts the channel-scoped half. This new test repeats all of that and adds only theUserIdassertion. Two tests now cover one behaviour, so each future signature change toAddSpaceMemberhas to be applied twice — a cost this PR already paid across many call sites in this file.Add the user-scoped assertion to the existing test and drop this one.
♻️ Proposed refactor
Extend the existing test at Lines 610-612:
- mockAPI.AssertCalled(t, "PublishWebSocketEvent", "space_member_added", - map[string]any{"space_id": space.Id, "user_id": member.UserId}, - &mmmodel.WebsocketBroadcast{ChannelId: space.ChannelId}) + // Both delivery targets: the channel-scoped broadcast for observers, and the direct publish + // that reaches the added user, whom the channel-scoped resolution may not yet include. + payload := map[string]any{"space_id": space.Id, "user_id": member.UserId} + mockAPI.AssertCalled(t, "PublishWebSocketEvent", "space_member_added", payload, + &mmmodel.WebsocketBroadcast{ChannelId: space.ChannelId}) + mockAPI.AssertCalled(t, "PublishWebSocketEvent", "space_member_added", payload, + &mmmodel.WebsocketBroadcast{UserId: member.UserId})Then remove Lines 642-662.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/app/ws_events_test.go` around lines 641 - 662, Merge the user-scoped PublishWebSocketEvent assertion from TestServiceAddSpaceMember_PublishesToChannelAndUser into TestServiceAddSpaceMember_PublishesMemberAddedEvent, preserving its existing channel assertion and setup. Remove the redundant TestServiceAddSpaceMember_PublishesToChannelAndUser test entirely.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Around line 388-389: Update the test-e2e target so ./build/build-core-image.sh
runs only when CORE_IMAGE is unset or refers to a local image; skip it for
namespaced, pullable CORE_IMAGE values and allow the existing e2e test command
to let Testcontainers pull the image.
In `@server/api_handler_test.go`:
- Around line 2259-2264: Remove the stale opening line from the doc comment
above TestHandler_SetSpaceDefaultCapabilities_ReusesPooledScheme, leaving the
description focused on pooled-scheme reuse and its non-retirement behavior.
In `@server/api_space.go`:
- Around line 122-127: Update UpdateSpace so failures from the post-commit
BuildSpaceWithAccess call are logged as projection errors rather than returned
via writeAppError. Return HTTP 200 with a degraded SpaceWithAccess response
based on the committed update, preserving non-nil empty default_capabilities and
capabilities arrays; do not return a bare Space.
In `@server/app/ws_events.go`:
- Line 55: Add the web app’s WebSocket handler registration for the
space_member_capabilities_updated event, and implement the corresponding
capability-state update flow using the existing event-consumer patterns and
state symbols. Ensure the handler is wired into the web app’s registration path
so incoming capability updates can update the UI.
In `@server/e2e/README.md`:
- Around line 7-9: Remove references to the nonexistent
scripts/smoke-scenarios.sh from server/e2e/README.md and the related comments in
scenarios_test.go and helpers_test.go, unless the script is being added to the
repository. Keep the documentation describing the Go test suite’s seven
canonical Confluence permission scenarios without identifying the missing script
as an authoritative source.
In `@server/e2e/scenarios_test.go`:
- Around line 90-96: Update the comment above the drive-by page request and the
assertion message in the test around outsider.id to describe outsider as a team
member who is not a space member, removing the misleading “team non-member”
wording while preserving the existing test behavior.
In `@server/internal/testutil/schemestub.go`:
- Around line 330-343: Update resolveChannelRoles so a non-empty, unregistered
channel.SchemeId returns a not-found *mmodel.AppError instead of empty role
names with nil error. Preserve the existing nil/empty scheme branch and
registered-scheme role lookup unchanged.
In `@server/model/space_capabilities_test.go`:
- Around line 17-25: The grantableCapabilities slice in the test is missing
model.CapabilityDeletePage, which prevents the round-trip test from covering all
64 valid member-grant combinations and leaves delete-any role behavior untested.
Add model.CapabilityDeletePage to the grantableCapabilities slice and update the
comment to reference five atomic per-page capabilities instead of four.
---
Nitpick comments:
In `@server/app/ws_events_test.go`:
- Around line 674-680: Remove the unused HasPermissionToChannel stub from the
SetSpaceMemberCapabilities test and revise the nearby comment so it accurately
describes the non-admin capability scenario. Do not add guard coverage to this
case; use a separate test case requesting CapabilityAdminSpace or targeting
actingUserID if coverage is needed.
- Around line 641-662: Merge the user-scoped PublishWebSocketEvent assertion
from TestServiceAddSpaceMember_PublishesToChannelAndUser into
TestServiceAddSpaceMember_PublishesMemberAddedEvent, preserving its existing
channel assertion and setup. Remove the redundant
TestServiceAddSpaceMember_PublishesToChannelAndUser test entirely.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 31e3cddd-5e41-4dfe-9d10-d804fb1b4235
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (49)
.github/workflows/ci.ymlMakefileassets/i18n/en.jsonbuild/build-core-image.shgo.modserver/api.goserver/api_handler_test.goserver/api_page.goserver/api_page_drafts.goserver/api_page_drafts_test.goserver/api_page_presence.goserver/api_space.goserver/app/page_draft.goserver/app/page_draft_test.goserver/app/page_duplicate_test.goserver/app/page_hierarchy.goserver/app/page_move_test.goserver/app/page_move_to_space_test.goserver/app/page_reorder_test.goserver/app/permissions.goserver/app/permissions_test.goserver/app/scheme.goserver/app/service.goserver/app/service_test.goserver/app/space.goserver/app/space_members.goserver/app/space_test.goserver/app/ws_events.goserver/app/ws_events_test.goserver/e2e/README.mdserver/e2e/container_test.goserver/e2e/helpers_test.goserver/e2e/scenarios_test.goserver/internal/testutil/fixtures.goserver/internal/testutil/permstub.goserver/internal/testutil/schemestub.goserver/model/space.goserver/model/space_capabilities.goserver/model/space_capabilities_test.goserver/model/space_test.goserver/store/migrations/000007_add_viewaccess_to_spaces.down.sqlserver/store/migrations/000007_add_viewaccess_to_spaces.up.sqlserver/store/page_move.goserver/store/page_move_test.goserver/store/page_store.goserver/store/space_store.goserver/store/store.goserver/store/store_test.gowebapp/i18n/en.json
💤 Files with no reviewable changes (1)
- webapp/i18n/en.json
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
build/build-core-image.sh (1)
138-138: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the duplicated default image tag.
Line 35 already applies
CORE_IMAGE="${CORE_IMAGE:-mm-docs-rbac-core:dev}", andset -uis active, soCORE_IMAGEis always non-empty here. The:-mm-docs-rbac-core:devfallback on line 138 is unreachable and states the default literal a second time. If one copy changes, the script builds an image under a name its documentation does not describe.♻️ Proposed simplification
-IMAGE_TAG="${CORE_IMAGE:-mm-docs-rbac-core:dev}" +IMAGE_TAG="$CORE_IMAGE"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build/build-core-image.sh` at line 138, Remove the duplicated default fallback from the IMAGE_TAG assignment, reusing the already-initialized CORE_IMAGE value established earlier in the script. Keep the existing image tag behavior unchanged while ensuring the default literal appears only in the initial CORE_IMAGE initialization.server/api_handler_test.go (1)
149-160: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
dbparameter fromseedSpaceInTeamand its forwarding calls. Thereviveunused-parameterrule is disabled, so this is not a lint failure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/api_handler_test.go` around lines 149 - 160, Remove the unused db parameter from seedSpaceInTeam and update seedSpace and every other caller to stop forwarding it, while preserving the existing space-creation behavior through testutil.MustCreateSpace.server/app/space_test.go (2)
646-648: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the pooled scheme was actually attached.
require.NotEmpty(t, pooledSchemeID)checks a locally generated id, so it can never fail. The test's stated intent is that the abandoned channel pointed at the pooled scheme and the scheme survived. Assert the attachment instead, asTestServiceCreateSpace_CustomSchemeConfiguredAfterChannelAttachdoes at Line 712.♻️ Proposed change
mockAPI.AssertCalled(t, "DeleteChannel", backingChannelID) mockAPI.AssertNotCalled(t, "DeleteScheme", mock.Anything) - require.NotEmpty(t, pooledSchemeID) + require.NotNil(t, channel.SchemeId, "the doomed channel must have carried the pooled scheme") + require.Equal(t, pooledSchemeID, *channel.SchemeId)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/app/space_test.go` around lines 646 - 648, Replace the ineffective require.NotEmpty assertion on pooledSchemeID in the test with an assertion that the abandoned channel’s scheme attachment references the pooled scheme, matching the established assertion pattern in TestServiceCreateSpace_CustomSchemeConfiguredAfterChannelAttach. Keep the existing DeleteChannel and DeleteScheme expectations unchanged.
588-611: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider one shared custom-scheme stub helper.
stubCustomSchemeCreateandstubCustomSchemeCreateFailingPatchare the same 15 lines of setup. Only the role-name prefix and thePatchRoleoutcome differ. A single helper that takes the role-name prefix and the patch result removes the duplication, and a later change to the scheme-creation contract then needs one edit instead of two.♻️ Sketch of the consolidated helper
// stubSchemeCreate wires the mock calls a non-preset default-capability set needs. patchErr, when // non-nil, makes every role patch fail, so the configure step cannot complete. Returns the new // scheme's id. func stubSchemeCreate(t *testing.T, mockAPI *plugintest.API, prefix string, patchErr *mmmodel.AppError) string { t.Helper() testutil.StubPooledSchemeMiss(mockAPI) schemeID := mmmodel.NewId() userRole, adminRole, guestRole := prefix+"_user_role", prefix+"_admin_role", prefix+"_guest_role" mockAPI.On("CreateScheme", mock.AnythingOfType("*model.Scheme")).Return(&mmmodel.Scheme{ Id: schemeID, Name: model.SharedSchemeNamePrefix + mmmodel.NewId(), Scope: mmmodel.SchemeScopeChannel, DefaultChannelUserRole: userRole, DefaultChannelAdminRole: adminRole, DefaultChannelGuestRole: guestRole, }, nil) testutil.RegisterSchemeRoles(schemeID, guestRole, userRole, adminRole) for _, r := range []string{userRole, adminRole, guestRole} { testutil.StubRole(mockAPI, r, nil) } if patchErr != nil { mockAPI.On("PatchRole", mock.AnythingOfType("string"), mock.AnythingOfType("*model.RolePatch")). Return(nil, patchErr) } else { testutil.StubPatchRole(mockAPI) } return schemeID }Also applies to: 731-754
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/app/space_test.go` around lines 588 - 611, The custom scheme setup is duplicated between stubCustomSchemeCreate and stubCustomSchemeCreateFailingPatch. Consolidate both into one shared stubSchemeCreate helper that accepts a role-name prefix and optional patch error, reuses the common scheme, role registration, and role stubbing setup, and configures PatchRole to either succeed or return the supplied error; update both callers to use it.server/e2e/helpers_test.go (1)
126-140: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
spaceHasMemberonly inspects the first response page.The request sends no
pageorper_pageparameter, so the handler returns the first page at the default size. If a scenario space ever holds more members than that page, this helper reportsfalsefor a member that exists, and the calling assertion fails for the wrong reason or passes silently. Today's scenarios use a few actors, so the defect is latent. Either followhas_moreor assert that the page was not full.♻️ Proposed guard
type spaceMembersResponse struct { Items []*spaceMemberJSON `json:"items"` + HasMore bool `json:"has_more"` }require.Equal(t, http.StatusOK, status, "list members of space %s: %s", spaceID, body) + require.False(t, resp.HasMore, "space %s has more members than one page; this helper would miss them", spaceID) for _, m := range resp.Items {Confirm the paginated response field name against
paginatedResponseinserver/api.gobefore applying.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/e2e/helpers_test.go` around lines 126 - 140, Update spaceHasMember to handle pagination instead of inspecting only the initial response page: follow the response’s has_more indicator using the pagination field names defined by paginatedResponse in server/api.go, requesting subsequent pages until the user is found or no pages remain. Alternatively, assert that the initial page is not full before returning false.server/app/space.go (1)
324-331: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
schemeAppErrorfor the default-capability lookup failure.
spaceDefaultCapabilitiesresolves through the pluginapi client (channel scheme, then role permissions), not through the plugin store.storeAppErrorrecognizes only store sentinels, so a core-supplied*AppError— for example a license denial or a refusal during the permissions migration — collapses to a generic 500 with the store error key.schemeAppError(server/app/service.go Lines 148-157) preserves that status and was added for this exact shape.SetSpaceDefaultCapabilitiesalready routes scheme errors through it.♻️ Proposed change
defaultCapabilities := knownDefaults if defaultCapabilities == nil { var err error defaultCapabilities, err = s.spaceDefaultCapabilities(space) if err != nil { - return nil, storeAppError("BuildSpaceWithAccess", err) + return nil, schemeAppError("BuildSpaceWithAccess", err) } }Note:
GetSpaceMembersandSetSpaceMemberCapabilitiesin server/app/space_members.go wrap the same lookups withstoreAppError, so the same change may apply there.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/app/space.go` around lines 324 - 331, Update the error handling in the default-capability lookup within the space-building flow to use schemeAppError instead of storeAppError, preserving core-supplied AppError status and keys from spaceDefaultCapabilities. Also inspect GetSpaceMembers and SetSpaceMemberCapabilities for the same pluginapi lookup pattern and route those lookup failures through schemeAppError consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 141-149: Update the gomod_commit extraction in the CI cross-check
to support both standard Go pseudo-version forms, including tagless versions
like v0.0.0-<timestamp>-<sha>, by matching the trailing timestamp and commit
revision rather than requiring a base-version counter. When server/public is
present but no revision can be parsed, fail the step instead of printing the
released-version message and exiting successfully; retain the skip behavior only
when the dependency is genuinely pinned to a released version.
In `@server/e2e/container_test.go`:
- Around line 143-159: Update waitForPhase2Migration to inspect each non-nil
error from adminClient.GetSchemes and continue polling only when its message
contains the known pending condition
app.schemes.is_phase_2_migration_completed.not_completed.app_error; return
unrelated errors immediately, preserving the existing timeout and context
handling for the pending-migration case. Add the errors import as needed and
reuse the existing strings import.
In `@server/e2e/README.md`:
- Around line 36-37: Update the “Running it” description in the e2e README to
qualify that make test-e2e runs build/build-core-image.sh only when CORE_IMAGE
is not namespaced (does not contain “/”); otherwise it uses the provided image.
Keep the existing plugin-bundle behavior and command flow unchanged.
---
Nitpick comments:
In `@build/build-core-image.sh`:
- Line 138: Remove the duplicated default fallback from the IMAGE_TAG
assignment, reusing the already-initialized CORE_IMAGE value established earlier
in the script. Keep the existing image tag behavior unchanged while ensuring the
default literal appears only in the initial CORE_IMAGE initialization.
In `@server/api_handler_test.go`:
- Around line 149-160: Remove the unused db parameter from seedSpaceInTeam and
update seedSpace and every other caller to stop forwarding it, while preserving
the existing space-creation behavior through testutil.MustCreateSpace.
In `@server/app/space_test.go`:
- Around line 646-648: Replace the ineffective require.NotEmpty assertion on
pooledSchemeID in the test with an assertion that the abandoned channel’s scheme
attachment references the pooled scheme, matching the established assertion
pattern in TestServiceCreateSpace_CustomSchemeConfiguredAfterChannelAttach. Keep
the existing DeleteChannel and DeleteScheme expectations unchanged.
- Around line 588-611: The custom scheme setup is duplicated between
stubCustomSchemeCreate and stubCustomSchemeCreateFailingPatch. Consolidate both
into one shared stubSchemeCreate helper that accepts a role-name prefix and
optional patch error, reuses the common scheme, role registration, and role
stubbing setup, and configures PatchRole to either succeed or return the
supplied error; update both callers to use it.
In `@server/app/space.go`:
- Around line 324-331: Update the error handling in the default-capability
lookup within the space-building flow to use schemeAppError instead of
storeAppError, preserving core-supplied AppError status and keys from
spaceDefaultCapabilities. Also inspect GetSpaceMembers and
SetSpaceMemberCapabilities for the same pluginapi lookup pattern and route those
lookup failures through schemeAppError consistently.
In `@server/e2e/helpers_test.go`:
- Around line 126-140: Update spaceHasMember to handle pagination instead of
inspecting only the initial response page: follow the response’s has_more
indicator using the pagination field names defined by paginatedResponse in
server/api.go, requesting subsequent pages until the user is found or no pages
remain. Alternatively, assert that the initial page is not full before returning
false.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 89ddf508-2943-4048-ba3f-b62b0911e9b0
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (52)
.github/workflows/ci.ymlMakefileassets/i18n/en.jsonbuild/build-core-image.shgo.modserver/api.goserver/api_handler_test.goserver/api_page.goserver/api_page_drafts.goserver/api_page_drafts_test.goserver/api_page_presence.goserver/api_space.goserver/app/page_draft.goserver/app/page_draft_test.goserver/app/page_duplicate_test.goserver/app/page_hierarchy.goserver/app/page_move_test.goserver/app/page_move_to_space_test.goserver/app/page_reorder_test.goserver/app/permissions.goserver/app/permissions_test.goserver/app/scheme.goserver/app/scheme_test.goserver/app/service.goserver/app/service_test.goserver/app/space.goserver/app/space_access.goserver/app/space_members.goserver/app/space_test.goserver/app/ws_events.goserver/app/ws_events_test.goserver/e2e/README.mdserver/e2e/container_test.goserver/e2e/helpers_test.goserver/e2e/scenarios_test.goserver/internal/testutil/fixtures.goserver/internal/testutil/permstub.goserver/internal/testutil/schemestub.goserver/model/space.goserver/model/space_capabilities.goserver/model/space_capabilities_test.goserver/model/space_test.goserver/store/migrations/000006_add_viewaccess_to_spaces.down.sqlserver/store/migrations/000006_add_viewaccess_to_spaces.up.sqlserver/store/page_move.goserver/store/page_move_test.goserver/store/page_store.goserver/store/space_store.goserver/store/store.goserver/store/store_test.gowebapp/i18n/en.jsonwebapp/src/components/spaces_sidebar/space_item_menu.tsx
| # The shape check above cannot catch the drift that matters. go.mod pins server/public to a | ||
| # core commit; CORE_IMAGE is a separate repository variable a human sets. Nothing ties them | ||
| # together, so bumping the pin without updating the variable leaves this suite green against | ||
| # a core image that predates the very changes under test. | ||
| gomod_commit="$(grep -oE 'server/public v[0-9]+\.[0-9]+\.[0-9]+-[0-9]+\.[0-9]+-[0-9a-f]+' go.mod | grep -oE '[0-9a-f]{12}$' || true)" | ||
| if [[ -z "$gomod_commit" ]]; then | ||
| echo "server/public is pinned to a released version, not a core commit; skipping the sha cross-check." | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The go.mod pseudo-version pattern misses the tagless shape, so the cross-check can silently self-disable.
The pattern requires a base release plus a pre-release counter: v[0-9]+\.[0-9]+\.[0-9]+-[0-9]+\.[0-9]+-[0-9a-f]+. It matches the current pin (v0.4.4-0.20260804125319-416829fe8ff3). It does not match the other standard Go pseudo-version shape, v0.0.0-20260804125319-416829fe8ff3, which has no -<n>. counter group. go mod tidy produces that shape whenever the pinned commit is not descended from a tag.
In that case gomod_commit is empty, the step prints "pinned to a released version" and exits 0. The drift the comment above describes then goes unreported, which is the outcome this step exists to prevent.
Match the revision by position instead of by base-version shape, and fail rather than exit 0 when server/public is present but the revision cannot be parsed.
🛠️ Proposed fix
- gomod_commit="$(grep -oE 'server/public v[0-9]+\.[0-9]+\.[0-9]+-[0-9]+\.[0-9]+-[0-9a-f]+' go.mod | grep -oE '[0-9a-f]{12}$' || true)"
- if [[ -z "$gomod_commit" ]]; then
+ gomod_version="$(grep -oE 'server/public v[^[:space:]]+' go.mod | head -1 | awk '{print $2}')"
+ if [[ -z "$gomod_version" ]]; then
+ echo "could not find a server/public requirement in go.mod." >&2
+ exit 1
+ fi
+ # A pseudo-version always ends in -<14-digit timestamp>-<12-hex revision>, whatever its
+ # base version looks like. Anything else is a real released version.
+ gomod_commit="$(printf '%s' "$gomod_version" | grep -oE '\-[0-9]{14}-[0-9a-f]{12}$' | grep -oE '[0-9a-f]{12}$' || true)"
+ if [[ -z "$gomod_commit" ]]; then
echo "server/public is pinned to a released version, not a core commit; skipping the sha cross-check."
exit 0
fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 141 - 149, Update the gomod_commit
extraction in the CI cross-check to support both standard Go pseudo-version
forms, including tagless versions like v0.0.0-<timestamp>-<sha>, by matching the
trailing timestamp and commit revision rather than requiring a base-version
counter. When server/public is present but no revision can be parsed, fail the
step instead of printing the released-version message and exiting successfully;
retain the skip behavior only when the dependency is genuinely pinned to a
released version.
| func waitForPhase2Migration(ctx context.Context, adminClient *mmmodel.Client4) error { | ||
| deadline := time.Now().Add(2 * time.Minute) | ||
| var lastErr error | ||
| for time.Now().Before(deadline) { | ||
| _, _, err := adminClient.GetSchemes(ctx, "", 0, 1) | ||
| if err == nil { | ||
| return nil | ||
| } | ||
| lastErr = err | ||
| select { | ||
| case <-ctx.Done(): | ||
| return fmt.Errorf("waiting for advanced-permissions phase-2 migration: %w (last error: %v)", ctx.Err(), lastErr) | ||
| case <-time.After(2 * time.Second): | ||
| } | ||
| } | ||
| return fmt.Errorf("advanced-permissions phase-2 migration did not complete within 2 minutes: %w", lastErr) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
The poll loop cannot distinguish "migration pending" from an unrelated failure.
Line 147 treats every non-nil error as "migration not complete". The comment at lines 127-132 names the specific condition being waited on, app.schemes.is_phase_2_migration_completed.not_completed.app_error, and the doc comment says the loop polls "until it stops reporting 'not completed'". The code never inspects the error id.
A bad admin token, a 500, or a connection reset therefore loops for the full two minutes and then reports that the migration did not complete. That consumes two of the caller's three boot minutes and points the reader at the wrong cause.
Return immediately on an error that is not the pending-migration condition.
🛠️ Proposed fix
for time.Now().Before(deadline) {
_, _, err := adminClient.GetSchemes(ctx, "", 0, 1)
if err == nil {
return nil
}
+ // Only the pending-migration condition is worth waiting on. Any other failure (auth, a
+ // 500, a dropped connection) will not resolve itself, so surface it instead of spending
+ // the remaining boot budget on it and then blaming the migration.
+ var appErr *mmmodel.AppError
+ if !errors.As(err, &appErr) || !strings.Contains(appErr.Id, "is_phase_2_migration_completed") {
+ return fmt.Errorf("polling for the advanced-permissions phase-2 migration failed: %w", err)
+ }
lastErr = errThe fix needs errors and strings in the import block; strings is already imported.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/e2e/container_test.go` around lines 143 - 159, Update
waitForPhase2Migration to inspect each non-nil error from adminClient.GetSchemes
and continue polling only when its message contains the known pending condition
app.schemes.is_phase_2_migration_completed.not_completed.app_error; return
unrelated errors immediately, preserving the existing timeout and context
handling for the pending-migration case. Add the errors import as needed and
reuse the existing strings import.
| This builds the core image (`build/build-core-image.sh`), ensures the plugin bundle exists | ||
| (`make dist` if missing), then runs: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
"Running it" contradicts the namespaced-CORE_IMAGE rule stated above.
This section says make test-e2e "builds the core image (build/build-core-image.sh)" without qualification. The test-e2e target now skips that build when CORE_IMAGE contains a /, which lines 24-26 describe. A reader who follows this section expects a local build in the CI-style configuration and does not get one.
📝 Proposed fix
-This builds the core image (`build/build-core-image.sh`), ensures the plugin bundle exists
-(`make dist` if missing), then runs:
+This ensures the plugin bundle exists (`make dist` if it is missing or carries no linux binary for
+the Docker daemon's architecture), builds the core image with `build/build-core-image.sh` unless
+`CORE_IMAGE` is namespaced, then runs:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| This builds the core image (`build/build-core-image.sh`), ensures the plugin bundle exists | |
| (`make dist` if missing), then runs: | |
| This ensures the plugin bundle exists (`make dist` if it is missing or carries no linux binary for | |
| the Docker daemon's architecture), builds the core image with `build/build-core-image.sh` unless | |
| `CORE_IMAGE` is namespaced, then runs: |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/e2e/README.md` around lines 36 - 37, Update the “Running it”
description in the e2e README to qualify that make test-e2e runs
build/build-core-image.sh only when CORE_IMAGE is not namespaced (does not
contain “/”); otherwise it uses the provided image. Keep the existing
plugin-bundle behavior and command flow unchanged.
1 participant
Summary
Puts every space and page route behind the Spaces permission set added in the paired core PR. Until now any logged-in user could reach any space or page; this change resolves a read gate first, then a capability gate, on every request.
Access model — A space is
openorprivate(ViewAccess, new column, defaults toopen). A read resolves to one of four outcomes: denied, sysadmin, backing-channel member, or the open-space fall-through for a team member who is not yet a member. Every denial — space missing, caller not a team member, caller no longer a member — returns the same 403, so existence cannot be probed by status code or message. A backend lookup failure is never collapsed into a denial; it surfaces as a 500 rather than misreporting an outage as "not permitted".Capabilities — A member's effective capability set is derived from the atomic capability roles on their backing-channel membership plus the space's default set.
SchemeAdminmembers additionally hold the full admin set regardless of what is recorded inExplicitRoles;SchemeGuestmembers are pinned toread_pageplus explicit grants only and cannot be given capabilities. Two new routes manage this:PATCH /spaces/{space_id}/members/{user_id}/capabilitiesfor one member, andPATCH /spaces/{space_id}/default-capabilitiesfor the space default. Both enforce a self-escalation guard and a last-admin invariant, so a space cannot be left without an authorized admin.Auto-join — A non-member admitted only through the open-space fall-through is joined to the backing channel before the write gate re-runs, but only when the space's current default set would grant that permission to a plain member. The join re-validates the open-read admission under the space's membership lock, so an
open→privateflip racing the request aborts the join rather than admitting on a stale read. A caller denied on the source side of a cross-space move is never left holding a membership the rejected request created.Cross-space move —
move-to-spaceis gated on sourceread_page, a remove-class delete permission over the whole moved subtree, and targetcreate_page. When the caller resolves to the own-scoped delete permission, subtree ownership is verified inside the move transaction against its own locked subtree read, closing the window a pre-scan would leave open to a concurrent reparent grafting another user's page into the subtree.Scheme lifecycle — A space with a non-default capability set gets its own custom scheme, created from the seeded presets and retired when nothing references it. Retirement is reference-counted over live and soft-deleted spaces alike (a soft-deleted space is restorable and keeps its
SchemeId), and refuses any scheme this plugin did not create. A backing channel archived by a failed space creation is excluded from that count, so an abandoned channel cannot pin a scheme forever.Testing — Adds a containerized end-to-end suite that stands up a real server with the paired core image and exercises the gates over HTTP, plus a build script for that image. Unit coverage extends across the model, store, app and handler layers.
Dependency
Requires mattermost/mattermost#37685.
server/publicis pinned to that branch's head and must be repinned to a release tag once the core change merges and ships.Ticket Link
Fixes: https://mattermost.atlassian.net/browse/MM-69269
Release Note