MM-69622 Refactoring board validators - #224
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds shared per-board-type authorization to board creation, boards-and-blocks creation, and archive import. Archive imports now validate all boards before persistence, with new callback contracts, parsing support, fake permission controls, and unit/integration tests. ChangesBoard creation permission enforcement
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ArchiveAPI
participant ImportArchive
participant parseBoardsAndBlocks
participant authorizeBoardCreate
participant CreateBoardsAndBlocks
Client->>ArchiveAPI: Import archive
ArchiveAPI->>ImportArchive: ImportArchive(BoardValidator)
ImportArchive->>parseBoardsAndBlocks: Parse all archive boards
parseBoardsAndBlocks->>authorizeBoardCreate: Validate each board
authorizeBoardCreate-->>ImportArchive: Allow or deny
ImportArchive->>CreateBoardsAndBlocks: Persist validated boards
ImportArchive-->>ArchiveAPI: Success or validation error
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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.
🧹 Nitpick comments (2)
server/integrationtests/board_creation_permissions_test.go (1)
51-56: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a mixed multi-board payload case.
The
createBoardsAndBlockspath accepts multiple boards, but this test only sends one board per request. Add a request containing both an allowed public board and a denied private board so the integration test proves any denied board rejects the whole batch.Proposed test coverage extension
_, resp := th.Client.CreateBoardsAndBlocks(newBoardAndBlocks(teamID, model.BoardTypePrivate)) th.CheckForbidden(resp) + _, resp = th.Client.CreateBoardsAndBlocks(newBoardsAndBlocks(teamID, model.BoardTypeOpen, model.BoardTypePrivate)) + th.CheckForbidden(resp) + bab, resp := th.Client.CreateBoardsAndBlocks(newBoardAndBlocks(teamID, model.BoardTypeOpen)) th.CheckOK(resp) require.Len(t, bab.Boards, 1)func newBoardAndBlocks(teamID string, boardType model.BoardType) *model.BoardsAndBlocks { - boardID := utils.NewID(utils.IDTypeBoard) - return &model.BoardsAndBlocks{ - Boards: []*model.Board{{ + return newBoardsAndBlocks(teamID, boardType) +} + +func newBoardsAndBlocks(teamID string, boardTypes ...model.BoardType) *model.BoardsAndBlocks { + bab := &model.BoardsAndBlocks{} + for _, boardType := range boardTypes { + boardID := utils.NewID(utils.IDTypeBoard) + bab.Boards = append(bab.Boards, &model.Board{ ID: boardID, Title: "Test Board", TeamID: teamID, Type: boardType, - }}, - Blocks: []*model.Block{{ + }) + bab.Blocks = append(bab.Blocks, &model.Block{ ID: utils.NewID(utils.IDTypeCard), Title: "Test Block", BoardID: boardID, Type: model.TypeCard, CreateAt: model.GetMillis(), UpdateAt: model.GetMillis(), - }}, + }) } + return bab }Also applies to: 90-108
🤖 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/integrationtests/board_creation_permissions_test.go` around lines 51 - 56, The current CreateBoardsAndBlocks integration coverage only verifies single-board requests, so extend the board_creation_permissions_test cases to use createBoardsAndBlocks with a mixed payload containing both an allowed open/public board and a denied private board. Update the test to assert that the presence of any forbidden board causes the entire batch to be rejected via th.CheckForbidden, and apply the same mixed-case coverage in the other related test block referenced in the suite.server/api/boards_and_blocks.go (1)
74-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo separate passes over
newBab.Boards.The new authorization loop (Lines 113-119) and the earlier validation/team-consistency loop (Lines 74-96) both iterate
newBab.Boardsindependently. Could be folded into a single pass for a small readability/perf win, but not essential given the low board counts typical for this endpoint.♻️ Optional consolidation
for _, board := range newBab.Boards { if validationErr := board.IsValid(); validationErr != nil { a.errorResponse(w, r, model.NewErrBadRequest(validationErr.Error())) return } boardIDs[board.ID] = true if teamID == "" { teamID = board.TeamID continue } if teamID != board.TeamID { a.errorResponse(w, r, model.NewErrBadRequest("cannot create boards for multiple teams")) return } if board.ID == "" { a.errorResponse(w, r, model.NewErrBadRequest("boards need an ID to be referenced from the blocks")) return } } + + // authorizeBoardCreate is deferred until after the ViewTeam/guest checks below, + // so it stays a separate loop here rather than being merged into the one above.Also applies to: 113-119
🤖 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/boards_and_blocks.go` around lines 74 - 96, The handler in boards_and_blocks.go iterates over newBab.Boards twice for validation/team checks and then authorization, so consolidate those checks into a single pass in the BoardsAndBlocks creation flow. Keep the existing IsValid, teamID consistency, board ID, and authorization logic together so the request is validated and authorized per board without a second traversal, using the newBab.Boards loop and the surrounding create/update handling as the main place to refactor.
🤖 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.
Nitpick comments:
In `@server/api/boards_and_blocks.go`:
- Around line 74-96: The handler in boards_and_blocks.go iterates over
newBab.Boards twice for validation/team checks and then authorization, so
consolidate those checks into a single pass in the BoardsAndBlocks creation
flow. Keep the existing IsValid, teamID consistency, board ID, and authorization
logic together so the request is validated and authorized per board without a
second traversal, using the newBab.Boards loop and the surrounding create/update
handling as the main place to refactor.
In `@server/integrationtests/board_creation_permissions_test.go`:
- Around line 51-56: The current CreateBoardsAndBlocks integration coverage only
verifies single-board requests, so extend the board_creation_permissions_test
cases to use createBoardsAndBlocks with a mixed payload containing both an
allowed open/public board and a denied private board. Update the test to assert
that the presence of any forbidden board causes the entire batch to be rejected
via th.CheckForbidden, and apply the same mixed-case coverage in the other
related test block referenced in the suite.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fc4a57bb-6e1c-4165-a29e-a0924434c5ce
📒 Files selected for processing (9)
server/api/archive.goserver/api/boards.goserver/api/boards_and_blocks.goserver/app/import.goserver/app/import_test.goserver/integrationtests/board_creation_permissions_test.goserver/integrationtests/clienttestlib.goserver/model/block.goserver/model/import_export.go
| } | ||
|
|
||
| // validateImportedBoards runs the optional authorization callback against every | ||
| // board parsed from an archive, aborting the import if any board is rejected. |
There was a problem hiding this comment.
validateImportedBoards is invoked from ImportBoardJSONL which ImportArchive calls once per board.jsonl entry and persists immediately via CreateBoardsAndBlocks before the next entry is read. For multi-board archives if board #2 fails auth, board #1 is already committed.
Suggestion: hoist validation into ImportArchive, drop the per-jsonl call afterwards.
nang2049
left a comment
nang2049
left a comment
There was a problem hiding this comment.
Thanks @avasconcelos114 just one suggestion!
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
server/app/import.go (2)
32-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test coverage for the non-seekable-reader path.
No test exercises
errArchiveNotSeekable(e.g., passing aBoardValidatorwith a reader that doesn't implementio.Seeker). Worth a quick unit test given this is a brand-new failure mode.🤖 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/import.go` around lines 32 - 34, Add a unit test for the import path using a BoardValidator whose reader does not implement io.Seeker, and assert that the operation returns errArchiveNotSeekable. Reuse the existing import test setup and verify this new failure mode without changing production behavior.
43-100: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDouble full-parse overhead when a validator is set.
When
opt.BoardValidator != nil, the archive is fully parsed twice: once invalidateArchiveBoards(viaparseBoardsAndBlocks, which also builds the fullBlocks/BoardMembersslices just to discard them) and once more inimportArchive. For zip archives this also means decompressing everyboard.jsonlentry twice. For large boards/archives this doubles CPU and transient memory usage on the validation path.Since a
board.jsonlfile has itsboard/board_blockline first, a lighter-weight parse that stops as soon as the board is captured (skipping block/member parsing entirely during validation) would avoid this overhead.🤖 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/import.go` around lines 43 - 100, Reduce validator overhead in validateArchiveBoards by using a lightweight parser that reads each board.jsonl only until its initial board record is captured, without constructing Blocks or BoardMembers. Apply this to both legacy and zip archive paths, validate the captured boards with validateImportedBoards, and preserve the existing rewind before importArchive.
🤖 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 `@server/app/import.go`:
- Around line 294-297: Update the error wrapping in the blockToBoard call to
identify the failed conversion as a board conversion rather than a block
conversion, while preserving the archive line number and wrapped error.
---
Nitpick comments:
In `@server/app/import.go`:
- Around line 32-34: Add a unit test for the import path using a BoardValidator
whose reader does not implement io.Seeker, and assert that the operation returns
errArchiveNotSeekable. Reuse the existing import test setup and verify this new
failure mode without changing production behavior.
- Around line 43-100: Reduce validator overhead in validateArchiveBoards by
using a lightweight parser that reads each board.jsonl only until its initial
board record is captured, without constructing Blocks or BoardMembers. Apply
this to both legacy and zip archive paths, validate the captured boards with
validateImportedBoards, and preserve the existing rewind before importArchive.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a71a582c-0b43-4098-b703-28823b34c609
📒 Files selected for processing (3)
server/app/import.goserver/app/import_test.goserver/integrationtests/board_creation_permissions_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/integrationtests/board_creation_permissions_test.go
# Conflicts: # server/app/import.go
# Conflicts: # server/app/import_test.go
edgarbellot
left a comment
edgarbellot
left a comment
There was a problem hiding this comment.
@avasconcelos114 I noticed this while verifying the changes - POST /boards/{boardID}/duplicate also needs the same check.
The other endpoints are covered (both happy path and edge cases) :)
|
/cherry-pick release-9.3 |
|
Cherry pick is scheduled. |
|
Error trying doing the automated Cherry picking. Please do this manually |
|
/cherry-pick release-9.3 |
|
Cherry pick is scheduled. |
* MM-69622 Refactoring board validators * Improving validation of import flow * Addressing PR feedback from coderabbit * Adding checks to board duplication (cherry picked from commit 261bc51) Co-authored-by: Andre Vasconcelos <a-andre.vasconcelos@mattermost.com>
|
/cherry-pick release-9.2 |
|
Cherry pick is scheduled. |
* MM-69622 Refactoring board validators * Improving validation of import flow * Addressing PR feedback from coderabbit * Adding checks to board duplication (cherry picked from commit 261bc51) Co-authored-by: Andre Vasconcelos <a-andre.vasconcelos@mattermost.com>
4 participants
Summary
This PR refactors some of the checks done on create boards to make it easy to run the same ones in other flows
Ticket Link
Fixes https://mattermost.atlassian.net/browse/MM-69622
Change Impact: High 🟥
Regression Risk: Authorization logic is now centralized and enforced across multiple user-facing board creation paths, including archive import. Archive import adds a new validation pre-pass that requires reader seekability and can abort earlier; this changes failure modes and import control flow (potentially affecting parsing, ordering, and “no partial persistence” behavior).
QA Recommendation: Run focused end-to-end permission allow/deny tests across: (1) single board creation, (2) create boards+blocks, and (3) archive import (including multi-board archives and verification that denied boards don’t partially persist). Additionally, manually validate archive import behavior for non-seekable/non-standard inputs to confirm the expected early failure.
Generated by CodeRabbitAI