Conversation
* Checking if the last board id is present from the api * nit
* Fixed critical bugs in Mattermost Boards plugin * Linter fixes * Linter * Minor
* Use webapp exported markdown to card and board descriptions * review comments:
📝 WalkthroughWalkthroughThis pull request introduces comprehensive authentication and authorization hardening across API endpoints, refactors test infrastructure to use plugin-mode setup with dynamic IDs, adds database schema utilities for integration testing, and strengthens error handling with nil checks throughout the application and store layers. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant PluginTestStore as TestStore/DB
participant FileStore
Client->>Server: Integration test requests (authenticated/anon)
Server->>Server: requireUserID(getUserID(r)) check
alt authenticated
Server->>PluginTestStore: Query/modify boards, blocks, members
PluginTestStore-->>Server: Data/OK
Server->>FileStore: GetFileReader / Copy / FileExists (when needed)
FileStore-->>Server: Reader/Status
Server-->>Client: HTTP response
else unauthenticated
Server-->>Client: 401 Unauthorized (early return)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Rajat-Dabade
requested review from
harshilsharma63,
larkox and
yasserfaraazkhan
Rajat-Dabade
added
2: Dev Review
|
@coderabbitai please review this PR. |
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (14)
server/services/store/storetests/users.go (2)
104-108:⚠️ Potential issue | 🟡 MinorCopy-paste bug: "GetUserByEmail nonexistent" calls
GetUserByIDinstead ofGetUserByEmail.Same issue as above — line 105 calls
store.GetUserByID("nonexistent-email")instead ofstore.GetUserByEmail(...).🐛 Proposed fix
t.Run("GetUserByEmail nonexistent", func(t *testing.T) { - got, err := store.GetUserByID("nonexistent-email") + got, err := store.GetUserByEmail("nonexistent-email") var nf *model.ErrNotFound require.ErrorAs(t, err, &nf) require.Nil(t, got) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/services/store/storetests/users.go` around lines 104 - 108, The test named "GetUserByEmail nonexistent" mistakenly calls store.GetUserByID("nonexistent-email"); change this to call store.GetUserByEmail("nonexistent-email") so the test exercises the intended function; update the call in the test function (around the t.Run block using got, err := ...) to use GetUserByEmail and keep the existing error assertions using model.ErrNotFound and require.Nil(t, got).
89-93:⚠️ Potential issue | 🟡 MinorCopy-paste bug: "GetUserByUsername nonexistent" calls
GetUserByIDinstead ofGetUserByUsername.Line 90 calls
store.GetUserByID("nonexistent-username")but this subtest is supposed to verifyGetUserByUsernamewith a nonexistent username. The test still passes becauseGetUserByIDalso returns not-found, but it doesn't exercise the intended code path. Since this PR is specifically about fixing tests, this should be corrected.🐛 Proposed fix
t.Run("GetUserByUsername nonexistent", func(t *testing.T) { - got, err := store.GetUserByID("nonexistent-username") + got, err := store.GetUserByUsername("nonexistent-username") var nf *model.ErrNotFound require.ErrorAs(t, err, &nf) require.Nil(t, got) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/services/store/storetests/users.go` around lines 89 - 93, The "GetUserByUsername nonexistent" subtest is calling the wrong helper; replace the call to store.GetUserByID("nonexistent-username") with store.GetUserByUsername("nonexistent-username") so the test exercises the intended code path (keep the same assertions checking for a model.ErrNotFound and nil result); update the call in the t.Run block for "GetUserByUsername nonexistent" to use GetUserByUsername.server/api/channels.go (1)
78-78:⚠️ Potential issue | 🟡 MinorBug: audit metadata logs
teamIDinstead ofchannelID.Line 78 passes
teamIDas the value for the"channelID"audit metadata key. This means the audit trail will record incorrect data, making it harder to trace channel-related actions.🐛 Proposed fix
- auditRec.AddMeta("channelID", teamID) + auditRec.AddMeta("channelID", channelID)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/channels.go` at line 78, The audit metadata call is using the wrong variable: change the auditRec.AddMeta invocation that currently passes teamID to instead pass the channelID variable so the key "channelID" records the actual channel identifier (update the call auditRec.AddMeta("channelID", teamID) to use channelID); ensure the channelID symbol is in scope where auditRec.AddMeta is invoked.server/api/cards.go (1)
199-209:⚠️ Potential issue | 🟠 MajorMissing
returnafter error responses for invalidpageandper_pageparameters.When
strconv.Atoifails forpageorper_page,a.errorResponseis called but execution continues. This means the handler will proceed withpage=0/perPage=0(the zero values from the failed conversion) and may write a second response tow.This appears to be a pre-existing bug, but since this handler was touched in this PR, it's worth fixing now.
🐛 Proposed fix
page, err := strconv.Atoi(strPage) if err != nil { message := fmt.Sprintf("invalid `page` parameter: %s", err) a.errorResponse(w, r, model.NewErrBadRequest(message)) + return } perPage, err := strconv.Atoi(strPerPage) if err != nil { message := fmt.Sprintf("invalid `per_page` parameter: %s", err) a.errorResponse(w, r, model.NewErrBadRequest(message)) + return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/cards.go` around lines 199 - 209, The handler currently calls strconv.Atoi for strPage and strPerPage and invokes a.errorResponse(...) on error but does not return, so execution continues with zero values; update the error branches after the two a.errorResponse calls to immediately return from the handler (i.e., add a return after the page parse failure and after the perPage parse failure) so the handler stops processing when strconv.Atoi for page or per_page fails; reference the strconv.Atoi calls and the a.errorResponse(...) invocations in server/api/cards.go to locate the fixes.server/api/teams.go (1)
260-282:⚠️ Potential issue | 🟡 Minor
handleGetTeamUsersByIDis missing the earlyuserIDempty check.All other handlers in this file and across the PR consistently guard against an empty
userIDbefore proceeding. This handler extractsuserIDon line 277 but doesn't validate it, which is inconsistent with the hardening pattern applied everywhere else.🛡️ Proposed fix
vars := mux.Vars(r) teamID := vars["teamID"] userID := getUserID(r) + if userID == "" { + a.errorResponse(w, r, model.NewErrUnauthorized("access denied to team users")) + return + } + if !a.permissions.HasPermissionToTeam(userID, teamID, model.PermissionViewTeam) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/teams.go` around lines 260 - 282, The handler handleGetTeamUsersByID extracts userID via getUserID(r) but does not validate it; add an early guard after userID := getUserID(r) to check for empty string and return a permission/unauthorized error using a.errorResponse with model.NewErrPermission("access denied to team") (matching the pattern used elsewhere in this file) before any permission checks or further processing so the request is hardened against missing user context.server/web/webserver.go (1)
124-128:⚠️ Potential issue | 🟡 Minor
port == -1path doesn't setstarted, allowing duplicateregisterRoutescalls.When
ws.port == -1,Start()returns early without settingstarted = true. A second call toStart()would callws.registerRoutes()again, which could panic on duplicate route registration ingorilla/mux. Consider settingstarted = truebefore the early return, or guardingregisterRoutesseparately.Proposed fix
+ ws.started = true ws.registerRoutes() if ws.port == -1 { ws.logger.Debug("server not bind to any port") return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/web/webserver.go` around lines 124 - 128, The Start() path that returns when ws.port == -1 currently skips setting the ws.started flag, allowing subsequent Start() calls to call ws.registerRoutes() again and cause duplicate route registration; update Start() to set ws.started = true before the early return (or alternatively add a guard so registerRoutes() is only called once) and ensure the ws.registerRoutes() invocation is idempotent by checking ws.started or a separate routesRegistered flag to prevent double registration.server/api/blocks.go (1)
492-494:⚠️ Potential issue | 🟡 MinorIncorrect error message: "access denied to modify board members" should reference board cards.
This handler is about undeleting blocks, but the permission-denied message references "board members." Likely a copy-paste from a different handler.
📝 Suggested fix
if !a.permissions.HasPermissionToBoard(userID, boardID, model.PermissionManageBoardCards) { - a.errorResponse(w, r, model.NewErrPermission("access denied to modify board members")) + a.errorResponse(w, r, model.NewErrPermission("access denied to make board changes")) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/blocks.go` around lines 492 - 494, The permission-denied error message is wrong (mentions "board members"); update the message passed to model.NewErrPermission in the branch that checks a.permissions.HasPermissionToBoard(userID, boardID, model.PermissionManageBoardCards) to reference board cards (e.g., "access denied to modify board cards" or similar) so it accurately reflects the permission being checked; modify the string where a.errorResponse(w, r, model.NewErrPermission(...)) is called.server/api/users.go (1)
407-412:⚠️ Potential issue | 🟡 MinorMissing authentication guard in
handleGetUserPreferences.Every other handler in this file now validates
userID != ""before proceeding, buthandleGetUserPreferencesdoes not. IfgetUserID(r)returns an empty string, the handler will callGetUserPreferences("")which may produce unexpected results.🛡️ Suggested fix for consistency
userID := getUserID(r) + if userID == "" { + a.errorResponse(w, r, model.NewErrUnauthorized("access denied to user preferences")) + return + } auditRec := a.makeAuditRecord(r, "getUserConfig", audit.Fail)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/users.go` around lines 407 - 412, handleGetUserPreferences is missing the auth guard: after calling getUserID(r) and before creating/using auditRec or calling a.app.GetUserPreferences(userID), check that userID != "" and short-circuit with an authentication error response (e.g., write a 401/403 and return) and mark the audit record appropriately (audit.Fail) before returning; update the logic around getUserID, a.makeAuditRecord, and a.app.GetUserPreferences to follow the same pattern as other handlers in this file.server/services/store/storetests/files.go (1)
21-33:⚠️ Potential issue | 🟡 MinorTest coverage regression: no test verifies that a saved
FileInfocan be retrieved.The retrieval assertion was removed from this test, and no replacement test was added. The remaining "not found" subtest only validates the error path. Consider adding a subtest that calls
GetFileInfoafterSaveFileInfoto confirm the round-trip works.Would you like me to draft a "should save and retrieve fileinfo" subtest to restore the coverage?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/services/store/storetests/files.go` around lines 21 - 33, The test removed verification that a saved FileInfo can be retrieved; add a subtest (e.g., "should save and retrieve") after calling sqlStore.SaveFileInfo(fileInfo) that calls sqlStore.GetFileInfo(fileInfo.Id), asserts require.NoError(t, err) and that the returned *mmModel.FileInfo matches the original (use require.NotNil and require.Equal on important fields or require.EqualValues for the struct) to restore the round-trip coverage for SaveFileInfo and GetFileInfo.server/integrationtests/sharing_test.go (1)
82-84:⚠️ Potential issue | 🟡 MinorMissing
th.Server.UpdateAppConfig()after settingEnablePublicSharedBoards = true.At line 59–60, the config-false test explicitly calls
th.Server.UpdateAppConfig()after changing the config. Here at line 83,EnablePublicSharedBoardsis set totruebutUpdateAppConfig()is not called. IfUpdateAppConfig()is needed to propagate config changes (as the config-false test suggests), this omission could mean the test passes by accident (e.g., if the server still reads the pointer directly).Proposed fix
t.Run("POST sharing, config = true", func(t *testing.T) { th.Server.Config().EnablePublicSharedBoards = true + th.Server.UpdateAppConfig() sharing := model.Sharing{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/integrationtests/sharing_test.go` around lines 82 - 84, The test sets th.Server.Config().EnablePublicSharedBoards = true in the "POST sharing, config = true" case but never calls th.Server.UpdateAppConfig(), unlike the config=false test; update the test to call th.Server.UpdateAppConfig() after setting EnablePublicSharedBoards so the server picks up the new setting (locate the change in the t.Run "POST sharing, config = true" block and add a call to th.Server.UpdateAppConfig() immediately after setting EnablePublicSharedBoards).server/integrationtests/clienttestlib.go (1)
572-603:⚠️ Potential issue | 🟠 MajorServer polling loop has no timeout — test hangs forever if the server fails to start.
If the server never responds (e.g., port conflict, panic in goroutine), this loop spins indefinitely. Add a deadline.
Proposed fix: add a timeout
func (th *TestHelper) Start() *TestHelper { go func() { if err := th.Server.Start(); err != nil { panic(err) } }() + deadline := time.After(30 * time.Second) for { - URL := th.Server.Config().ServerRoot - th.Server.Logger().Info("Polling server", mlog.String("url", URL)) - resp, err := http.Get(URL) //nolint:gosec - if err != nil { - th.Server.Logger().Error("Polling failed", mlog.Err(err)) - time.Sleep(100 * time.Millisecond) - continue + select { + case <-deadline: + th.T.Fatal("test server did not start within 30 seconds") + default: + URL := th.Server.Config().ServerRoot + th.Server.Logger().Info("Polling server", mlog.String("url", URL)) + resp, err := http.Get(URL) //nolint:gosec + if err != nil { + th.Server.Logger().Error("Polling failed", mlog.Err(err)) + time.Sleep(100 * time.Millisecond) + continue + } + resp.Body.Close() + th.Server.Logger().Info("Server ping OK", mlog.Int("statusCode", resp.StatusCode)) + return th } - resp.Body.Close() - th.Server.Logger().Info("Server ping OK", mlog.Int("statusCode", resp.StatusCode)) - break } - - return th }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/integrationtests/clienttestlib.go` around lines 572 - 603, The Start method's polling loop (TestHelper.Start) can hang indefinitely if th.Server.Start() never makes the HTTP endpoint reachable; wrap the poll with a deadline (e.g., create a time.After timeout or context.WithTimeout before the for loop) and stop polling when the deadline is reached, logging the failure and returning/panicking appropriately instead of looping forever; make sure to reference the same Start() receiver and use th.Server.Config().ServerRoot and th.Server.Logger() for the timeout error path so callers see why polling failed.server/integrationtests/compliance_test.go (1)
18-22:⚠️ Potential issue | 🟡 MinorFix pre-existing bug:
OneHourconstant is 360,000 ms (6 minutes), not 1 hour.One hour in milliseconds is 3,600,000. The current value
360000is off by 10×, causingOneDayto be 2.4 hours instead of 24 hours, andOneYearto be roughly 36.5 days instead of 365 days. This affects all compliance tests relying on these constants for time window calculations—they use much smaller windows than intended.🐛 Proposed fix
var ( - OneHour int64 = 360000 + OneHour int64 = 3600000 OneDay int64 = OneHour * 24 OneYear int64 = OneDay * 365 )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/integrationtests/compliance_test.go` around lines 18 - 22, The time constants are wrong: OneHour is set to 360000 (6 minutes) causing OneDay and OneYear to be off; update OneHour to 3600000 (3,600,000 ms) and recompute OneDay and OneYear (OneDay = OneHour * 24, OneYear = OneDay * 365) or convert them to consts using those multiplications to ensure correct millisecond values for OneHour, OneDay and OneYear.server/integrationtests/boards_and_blocks_test.go (1)
115-189:⚠️ Potential issue | 🟡 MinorThe
AddMemberToBoardcalls afterCreateBoardsAndBlocksare redundant.
CreateBoardsAndBlocksis invoked with the API's default parameter ofaddMember=true(line 132), which already adds the calling user as bothSchemeAdminandSchemeEditorfor each board. The subsequentAddMemberToBoardloop (lines 141–149) is unnecessary becauseAddMemberToBoardchecks for existing membership and returns it unchanged; it will not modify an existing admin membership to editor. The test passes only due to this idempotent behaviour.Remove the
AddMemberToBoardloop (lines 141–149) since the user is already a member with admin privileges fromCreateBoardsAndBlocks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/integrationtests/boards_and_blocks_test.go` around lines 115 - 189, The test is redundantly calling AddMemberToBoard after creating boards because CreateBoardsAndBlocks (called via th.Client.CreateBoardsAndBlocks) already adds the caller as a board member/admin; remove the loop that iterates over bab.Boards and calls th.Server.App().AddMemberToBoard to avoid duplicate/no-op membership operations and rely on the existing assertions (GetMembersForBoard, etc.) to verify membership.server/integrationtests/board_test.go (1)
870-920:⚠️ Potential issue | 🟡 Minor"No license" and "Professional license" subtests appear functionally identical.
Both subtests set
FOCALBOARD_UNIT_TESTING_COMPLIANCEto"false"and both assertCheckNotImplemented. If the intent is to differentiate between "no license" and "Professional license," the Professional license test should configure a different license/environment to be meaningful. As-is, they test the exact same code path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/integrationtests/board_test.go` around lines 870 - 920, The two subtests "getBoardMetadata should fail with no license" and "getBoardMetadata should fail on Professional license" are identical because both set FOCALBOARD_UNIT_TESTING_COMPLIANCE="false" and assert th.CheckNotImplemented; update the Professional-license subtest to actually simulate a Professional license (e.g., set FOCALBOARD_UNIT_TESTING_COMPLIANCE="true" or apply the appropriate license flag used by your test harness) and adjust the expected result for th.Client.GetBoardMetadata accordingly (replace th.CheckNotImplemented with the correct assertion for a professional license path), referencing the environment variable FOCALBOARD_UNIT_TESTING_COMPLIANCE and the call th.Client.GetBoardMetadata(rBoard.ID, "") as the change points.
🧹 Nitpick comments (24)
server/integrationtests/work_template_test.go (1)
16-16: Unconditionalt.Skiprenders the entire test body dead code.This is fine as a temporary measure, but consider adding a tracking issue or TODO so it doesn't stay skipped indefinitely. Skipped tests tend to be forgotten.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/integrationtests/work_template_test.go` at line 16, The test contains an unconditional t.Skip in TestGetTemplatesForWorkTemplate which makes the test body dead code; update the t.Skip so the test is either conditionally skipped (e.g., check testing.Short(), an environment flag, or a CI marker) or replace the unconditional skip with t.Skipf including a short TODO/issue tracker reference (e.g., "TODO: enable after templates sync - see ISSUE-1234") so it won't be forgotten; locate the t.Skip call inside TestGetTemplatesForWorkTemplate and change it to conditional logic or add the issue URL/text to the skip message.server/app/category_test.go (1)
14-20: Confusing names:testCategoryIDDefaultvstestDefaultCategoryID.These two constants differ only in word order, making them easy to mix up. Each is used exactly once (lines 285 and 380 respectively) for different conceptual roles — one for a
"default"-type category and the other for a newly created"system"-type "Boards" category. Consider renaming to make the distinction clear.Suggested rename
- testCategoryIDDefault = "7defghijklmnopqrstuvwxyzabc" - testDefaultCategoryID = "7efghijklmnopqrstuvwxyzabcd" + testCategoryIDDefaultType = "7defghijklmnopqrstuvwxyzabc" + testCategoryIDSystemBoards = "7efghijklmnopqrstuvwxyzabcd"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/app/category_test.go` around lines 14 - 20, The two similarly named constants testCategoryIDDefault and testDefaultCategoryID are confusing; rename them to clearly reflect their roles (for example, rename testCategoryIDDefault -> testTypeDefaultCategoryID or testDefaultTypeCategoryID for the one used as a `"default"`-type category, and rename testDefaultCategoryID -> testSystemBoardsCategoryID or testSystemBoardsCategoryIDForNew for the one used as the newly created `"system"`-type "Boards" category), then update all references to these constants (the single usages in the tests) to the new names and adjust any assertion messages or helper calls that mention the old names (look for usages in the test functions that create/assert the `"default"`-type category and the system "Boards" category).server/app/cards_test.go (1)
41-43: Overly relaxed mock expectations reduce test value.Using
gomock.Any()for bothGetBlockandGetMembersForBoard, combined with.AnyTimes(), means this test won't catch regressions where:
GetBlockis called with an incorrect/unexpected ID.GetMembersForBoardis called with a board ID other thanboard.ID, or called more/fewer times than expected.If the block ID is generated internally by
CreateCardand isn't known ahead of time,gomock.Any()onGetBlockis understandable. However,GetMembersForBoardshould still be matchable againstboard.ID(as done inTestPatchCardon line 155), and.AnyTimes()could be tightened to.Times(1)(or removed) to assert the expected call count.Suggested tightening
- th.Store.EXPECT().GetMembersForBoard(gomock.Any()).Return([]*model.BoardMember{}, nil).AnyTimes() + th.Store.EXPECT().GetMembersForBoard(board.ID).Return([]*model.BoardMember{}, nil)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/app/cards_test.go` around lines 41 - 43, The mock expectations are too loose: tighten the GetMembersForBoard and call-count assertions so the test verifies correct board ID and exact call frequency; replace gomock.Any() for GetMembersForBoard with gomock.Eq(board.ID) (as used in TestPatchCard) and remove .AnyTimes(), using .Times(1) (or no Times to default to once), and keep GetBlock as-is if CreateCard generates the ID internally; ensure InsertBlock still uses gomock.AssignableToTypeOf(reflect.TypeOf(block)) and userID as before so the test asserts the expected interactions.server/model/category_test.go (1)
100-100: Semantically misleading error message: "Invalid Block ID" for Category ID validation.The tests expect
"Invalid Block ID"when a Category ID fails validation. Category.IsValid() calls the sharedIsValidId()validator from base.go (which returns this generic block-specific error message), then wraps it withNewErrInvalidCategory(). While the wrapping acknowledges this is category validation, the underlying message remains "Invalid Block ID"—confusing since categories are not blocks.Improve clarity by updating the error wrapping in Category.IsValid() (lines 96–97) to return a context-appropriate message like
"invalid category ID"so that error messages are meaningful to callers and in logs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/model/category_test.go` at line 100, Category.IsValid() currently uses the shared IsValidId() validator and then wraps its error with NewErrInvalidCategory(), but the underlying message still reads "Invalid Block ID" which is misleading; update the error wrapping in Category.IsValid() so that when IsValidId() returns an error you call NewErrInvalidCategory() with a clear, category-specific message (e.g., "invalid category ID") or create a new error using that text before wrapping, ensuring callers see "invalid category ID" instead of "Invalid Block ID"; reference the Category.IsValid(), IsValidId(), and NewErrInvalidCategory() symbols when making this change.server/app/boards.go (1)
629-632: Usemodel.IsErrNotFound()instead of string-matching on error messages.The code already checks for typed errors at line 624 (
if model.IsErrNotFound(bErr)), but then falls back to brittle string-matching at line 630 (if strings.Contains(err.Error(), "user not found")). This inconsistency suggests thatGetMemberForBoardshould returnmodel.ErrNotFounderrors like the rest of the codebase (as confirmed by the store tests atstoretests/boards.go:688–691). Align this error check with the typed error pattern already established in the same method.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/app/boards.go` around lines 629 - 632, Replace the brittle string check with the typed error helper: where the code currently does strings.Contains(err.Error(), "user not found") (after calling GetMemberForBoard / handling bErr), use model.IsErrNotFound(err) instead; ensure you check the same err variable and return nil when model.IsErrNotFound(err) is true so the behavior is consistent with the earlier model.IsErrNotFound(bErr) usage and store tests expecting ErrNotFound.server/api/api.go (1)
171-173: Good nil-safety guard preventing a panic onerr.Error().This is a sensible defensive measure. Note that
ErrUnknownErroris also defined inserver/integrationtests/clienttestlib.go— if these are intended to be the same sentinel, consider extracting it to a shared location to avoid drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/api.go` around lines 171 - 173, You already guard against nil by assigning ErrUnknownError when err == nil; if that sentinel is intended to be the same as the one in server/integrationtests/clienttestlib.go, extract ErrUnknownError into a shared package (e.g., a new package errors or common) and replace the duplicate declarations so both api.go and clienttestlib.go import and reference the single exported ErrUnknownError; ensure the exported identifier and its underlying value/type remain unchanged and update imports where ErrUnknownError is used.server/model/board_test.go (1)
31-35: Prefert.Setenvover manualos.Setenv+ defer.
t.Setenv(Go 1.17+) handles save/restore automatically, marks the test as incompatible witht.Parallel(), and is the idiomatic approach for setting env vars in tests.♻️ Suggested simplification
t.Run("Should allow global team ID for valid board in test scenarios", func(t *testing.T) { - // Set test environment variable to allow GlobalTeamID - origEnv := os.Getenv("FOCALBOARD_UNIT_TESTING") - os.Setenv("FOCALBOARD_UNIT_TESTING", "1") - defer os.Setenv("FOCALBOARD_UNIT_TESTING", origEnv) + t.Setenv("FOCALBOARD_UNIT_TESTING", "1")This also lets you remove the
"os"import if it's not used elsewhere in the file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/model/board_test.go` around lines 31 - 35, Replace the manual environment save/restore in the test "Should allow global team ID for valid board in test scenarios" with Go's t.Setenv: call t.Setenv("FOCALBOARD_UNIT_TESTING", "1") at the start of the t.Run body (remove origEnv/os.Setenv/defer lines) so the test framework handles restore and test compatibility; also remove the "os" import if it becomes unused.server/api/categories.go (2)
603-612:handleHideBoardandhandleUnhideBoardare missing the empty-userIDguard.Both use
getUserID(r)but skip theuserID == ""check that every other handler in this file now includes. WhileHasPermissionToTeamwith an empty userID would return false (resulting in a permission error rather than an unauthorized error), this is inconsistent and returns the wrong HTTP status/error type.Proposed fix for handleHideBoard (apply similar to handleUnhideBoard)
userID := getUserID(r) + if userID == "" { + a.errorResponse(w, r, model.NewErrUnauthorized("access denied to hide board")) + return + } vars := mux.Vars(r)Also applies to: 665-674
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/categories.go` around lines 603 - 612, handleHideBoard and handleUnhideBoard call getUserID(r) but omit the empty-userID guard present elsewhere; add a check after userID := getUserID(r) in both functions that if userID == "" you call a.errorResponse(w, r, model.NewErrUnauthorized("not logged in")) (or the project-standard unauthorized error) and return before calling a.permissions.HasPermissionToTeam, so unauthenticated requests return an unauthorized error instead of a permission-denied error.
437-439: Inconsistent auth pattern:handleReorderCategoriesandhandleReorderCategoryBoardsstill use session context directly.These two handlers extract
userIDviactx.Value(sessionContextKey).(*model.Session)instead ofgetUserID(r), and lack the empty-userIDguard present in all other handlers updated in this PR. WhilesessionRequiredmiddleware should prevent issues, this inconsistency makes the code harder to maintain and could mask bugs if the middleware changes.Proposed fix for handleReorderCategories (apply similar to handleReorderCategoryBoards)
vars := mux.Vars(r) teamID := vars["teamID"] - ctx := r.Context() - session := ctx.Value(sessionContextKey).(*model.Session) - userID := session.UserID + userID := getUserID(r) + if userID == "" { + a.errorResponse(w, r, model.NewErrUnauthorized("access denied to reorder categories")) + return + }Also applies to: 515-517
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/categories.go` around lines 437 - 439, Replace direct session extraction in handleReorderCategories and handleReorderCategoryBoards (currently using ctx.Value(sessionContextKey).(*model.Session) and referencing model.Session) with the standard getUserID(r) call used by other handlers, and add the same empty-userID guard that returns http.Error/unauthorized when getUserID returns an empty string; keep the existing sessionRequired middleware but make these handlers consistent by using getUserID(r) and checking for "" before proceeding.server/services/store/sqlstore/mattermost_tables.go (1)
384-408: Postgres schemas forFileInfoandSessionsuse PascalCase table names, unlike other tables.Tables like
channels,channelmembers,teammembers,teams,users,preferences, andbotsall use lowercase names in their Postgres schemas.FileInfoandSessionsuse PascalCase. PostgreSQL treats unquoted identifiers as case-insensitive, so this isn't a functional issue, but it's an inconsistency worth noting for readability.Also applies to: 447-460
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/services/store/sqlstore/mattermost_tables.go` around lines 384 - 408, The Postgres CREATE TABLE statements use PascalCase table names ("FileInfo" and "Sessions") causing inconsistency with other lowercase table names; update the SQL string values (the createTableSQL constants that define the FileInfo and Sessions tables) to use lowercase identifiers (e.g., fileinfo and sessions) and ensure the PRIMARY KEY and column definitions remain unchanged, and then search for any direct string references to "FileInfo" or "Sessions" in the same package to update them to the new lowercase names so code and schema naming is consistent.server/integrationtests/pluginteststore.go (1)
106-109: Permissive fallback inGetTeammay mask test bugs.Any valid 26-character Mattermost ID now silently returns a mock team, meaning tests won't catch cases where an incorrect or unexpected team ID is accidentally used. Consider whether this catch-all is intentional or if it would be safer to restrict it further (e.g., only IDs that were explicitly created in the test setup).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/integrationtests/pluginteststore.go` around lines 106 - 109, The GetTeam implementation currently treats any 26-char Mattermost ID (mmModel.IsValidId(id)) as a valid team and returns a generic mock Team, which can mask tests using the wrong IDs; change GetTeam to only return a Team if that ID was explicitly created in the test store (e.g., check the test store's teams map or createdTeams set) and otherwise return nil with an error; update the GetTeam function and any helper/in-memory structures used by the pluginteststore (reference: GetTeam, mmModel.IsValidId, model.Team) so the permissive fallback is removed and only known/test-created IDs succeed.server/app/files_test.go (1)
810-818: Environment variable restoration does not cover the case whereFOCALBOARD_UNIT_TESTINGwas explicitly set to"".If the env var was set to an empty string before the test, the
deferwon't restore it (becauseorigEnv == ""). This could affect subsequent tests that rely on the var being set. Useos.Setenvunconditionally or track presence separately.♻️ Suggested fix
origEnv := os.Getenv("FOCALBOARD_UNIT_TESTING") + _, origSet := os.LookupEnv("FOCALBOARD_UNIT_TESTING") os.Unsetenv("FOCALBOARD_UNIT_TESTING") defer func() { - if origEnv != "" { + if origSet { os.Setenv("FOCALBOARD_UNIT_TESTING", origEnv) + } else { + os.Unsetenv("FOCALBOARD_UNIT_TESTING") } }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/app/files_test.go` around lines 810 - 818, The deferred restore for FOCALBOARD_UNIT_TESTING in the t.Run("Should reject global team ID for non-templates for security", func(t *testing.T) { ... }) block only calls os.Setenv when origEnv != "" and thus fails to restore an original empty-string value; change the restore logic to always call os.Setenv("FOCALBOARD_UNIT_TESTING", origEnv) (or alternatively capture presence with a bool origPresent and call os.Unsetenv when it was absent) so the environment is correctly restored after the test.server/services/store/sqlstore/helpers_test.go (1)
296-380: Test setup looks solid — a few observations.The dynamic DB config via
storetest.MakeSqlSettings, Mattermost table bootstrapping, and test user map are well-structured. Thet.Cleanupwithrecover(lines 312-322) is unusual — it suppresses panics fromCleanupSqlSettings. This prevents cascading failures in test teardown but may hide legitimate cleanup errors.Also, the
tearDownfunction (line 373) callsos.Remove(connectionString), which only applies to file-based SQLite databases. Since SQLite is excluded at line 302-303, this is effectively dead code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/services/store/sqlstore/helpers_test.go` around lines 296 - 380, The t.Cleanup wrapper in SetupTests currently swallows panics from storetest.CleanupSqlSettings which can hide real cleanup errors; change it to call storetest.CleanupSqlSettings directly (or log and return the error) instead of using a defer recover inside t.Cleanup so failures surface during test teardown (refer to t.Cleanup and storetest.CleanupSqlSettings). Also update the tearDown closure so it only attempts os.Remove(connectionString) for SQLite/file-based DBs (check DBType or sqlSettings.DriverName) or remove that call entirely since SQLite is skipped; ensure you still call store.Shutdown and restore FOCALBOARD_UNIT_TESTING as before (refer to tearDown and Shutdown).server/api/members.go (1)
76-89: Redundantelse if !board.IsTemplatecondition.Line 83's
else if !board.IsTemplateis always true whenboard.IsTemplateis false (the only way to reach this branch). A plainelsewould suffice.♻️ Suggested simplification
if board.IsTemplate { // Check for explicit board membership (not synthetic) member, err2 := a.app.GetMemberForBoard(boardID, userID) if err2 != nil || member == nil || member.Synthetic { a.errorResponse(w, r, model.NewErrPermission("access denied to board members")) return } - } else if !board.IsTemplate { + } else { // For non-template boards, use standard permission check if !a.permissions.HasPermissionToBoard(userID, boardID, model.PermissionViewBoard) { a.errorResponse(w, r, model.NewErrPermission("access denied to board members")) return } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/members.go` around lines 76 - 89, The branch condition is redundant: replace the `else if !board.IsTemplate` branch with a plain `else` to simplify logic in the members handler; specifically, in the block that checks `board.IsTemplate`, leave the explicit template-check branch that uses `a.app.GetMemberForBoard(boardID, userID)` and `member.Synthetic`, and change the subsequent `else if !board.IsTemplate` to `else` so the standard permission check using `a.permissions.HasPermissionToBoard(userID, boardID, model.PermissionViewBoard)` and the `a.errorResponse` handling remain but without the unnecessary boolean re-check.server/api/blocks.go (1)
670-670:boardIDfrom the route is extracted but explicitly discarded.The PATCH
/boards/{boardID}/blocksendpoint advertises aboardIDin the URL but never validates that patched blocks belong to it. While per-block permission checks exist below, silently ignoring the URL path segment can confuse API consumers and violates the principle of least surprise.Consider either removing
boardIDfrom the route (breaking change) or validating that every block in the batch belongs to the specified board.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/blocks.go` at line 670, The code ignores vars["boardID"] in the PATCH /boards/{boardID}/blocks handler (line with _ = vars["boardID"]) so incoming blocks are not validated against the URL board; parse and normalize the boardID from vars["boardID"] and, before per-block permission checks, iterate the incoming blocks payload and verify each block's BoardID (or parent board reference) equals that parsed boardID, returning a 400/403 error if any mismatch is found; update the handler that processes the batch (the PATCH /boards/{boardID}/blocks function where vars["boardID"] is read) to enforce this validation and add a unit/integration test for a mismatched boardID case.server/integrationtests/user_test.go (1)
22-28: Variableclientshadows the imported package name.
client := client.NewClient(...)shadows theclientpackage import. While it works in this narrow scope, it's confusing. Consider using a different variable name (e.g.,unauthClient) for clarity, consistent with the naming used insharing_test.go(unauthClient).Proposed fix
t.Run("not login yet", func(t *testing.T) { // Create a client without authentication - client := client.NewClient(th.Server.Config().ServerRoot, "") - me, resp := client.GetMe() + unauthClient := client.NewClient(th.Server.Config().ServerRoot, "") + me, resp := unauthClient.GetMe() require.Error(t, resp.Error) require.Nil(t, me) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/integrationtests/user_test.go` around lines 22 - 28, The test's local variable "client" shadows the imported package "client"; rename the local variable used when calling client.NewClient and client.GetMe (e.g., change "client := client.NewClient(...)" to "unauthClient := client.NewClient(...)" and update subsequent calls to unauthClient.GetMe()) to avoid shadowing and match the naming used in sharing_test.go.server/integrationtests/clienttestlib.go (3)
146-157:HasPermissionToChannelis overly permissive — returnstruefor any valid Mattermost ID.This mock grants channel permission to every user (including guests and non-team-members) for any channel with a valid 26-char ID. This may mask authorization bugs in tests. Consider checking the
userIDagainst expected roles, similar to howHasPermissionToTeamrestrictsuserNoTeamMember.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/integrationtests/clienttestlib.go` around lines 146 - 157, The HasPermissionToChannel mock is too permissive; modify FakePermissionPluginAPI.HasPermissionToChannel to also validate the userID (similar to HasPermissionToTeam) before granting permission: explicitly deny for the test user ID that should have no channel access (e.g., "userNoTeamMember") and only return true for allowed test users or when both channelID is a test ID (like "valid-channel-id" / "valid-channel-id-2") and the userID is not a denied user; otherwise return false. Ensure you update the function logic around HasPermissionToChannel to check userID first, mirror the user-based checks used in HasPermissionToTeam, and keep acceptance of the test channel IDs but only for permitted users.
786-820:AddUserToTeamMembersopens a new DB connection instead of reusing the existing one.Each call to this method opens a fresh
sql.Open+db.Close. The server already has a DB connection (used by the store). Opening a separate connection is wasteful and risks connection pool exhaustion in parallel tests. Consider accepting the*sql.DBor accessing it from the server's store.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/integrationtests/clienttestlib.go` around lines 786 - 820, The AddUserToTeamMembers function opens its own DB connection via sql.Open and defers db.Close, which wastes connections; change the function to reuse the existing DB by either adding a parameter (e.g., func (th *TestHelper) AddUserToTeamMembers(db *sql.DB, teamID, userID string) error) and remove sql.Open/sql.Close, or obtain the DB from the server/store (use th.Server.Store or equivalent accessor) instead of calling th.Server.Config() to build a new connection string; keep the existing logic around Postgres vs MySQL table names (teammembers vs TeamMembers) and simply replace db usage to operate on the shared *sql.DB so no per-call opening/closing occurs.
61-103:setupClientsmutates package-level variables — not safe for parallel tests.Lines 93–100 reassign global
userAnonID,userNoTeamMemberID, etc. If two tests usingsetupClientsrun in parallel (t.Parallel()), there's a data race. Currently the tests don't appear to uset.Parallel(), but this is a latent hazard worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/integrationtests/clienttestlib.go` around lines 61 - 103, The setupClients function mutates package-level variables (userAnonID, userNoTeamMemberID, userTeamMemberID, userViewerID, userCommenterID, userEditorID, userAdminID, userGuestID), which causes data races if tests run in parallel; remove these global assignments and instead return any needed IDs from setupClients (e.g., change signature to return (Clients, map[string]string) or embed the IDs on the returned Clients struct), update call sites to use the returned IDs or client fields, and ensure no other code relies on those package globals being set by setupClients so you avoid mutating shared state in setupClients.server/integrationtests/sidebar_test.go (1)
25-25: Minor inconsistency: raw string"O"instead ofmodel.BoardTypeOpen.Other test files in this PR consistently use
model.BoardTypeOpen. Using the constant improves readability and avoids breakage if the value ever changes.♻️ Suggested fix
- board := th.CreateBoard(teamID, "O") + board := th.CreateBoard(teamID, model.BoardTypeOpen)Apply the same change at lines 35 and 77.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/integrationtests/sidebar_test.go` at line 25, Replace the hard-coded string "O" used when creating boards with the board type constant: change calls to th.CreateBoard(teamID, "O") to th.CreateBoard(teamID, model.BoardTypeOpen) (apply this to all occurrences of that call, e.g., the other CreateBoard calls near the same test). Ensure you import or reference model.BoardTypeOpen where needed.server/integrationtests/board_test.go (3)
756-759: Environment variable restoration may not restore "unset" state correctly.If
FOCALBOARD_UNIT_TESTING_COMPLIANCEwas not originally set,os.Getenvreturns""and the deferredos.Setenvwill set it to""rather than unsetting it. Consider usingos.UnsetenvwhenoriginalEnvis empty and the variable was not originally defined, or uset.Setenvwhich handles cleanup automatically.Simpler alternative using t.Setenv (Go 1.17+)
- originalEnv := os.Getenv("FOCALBOARD_UNIT_TESTING_COMPLIANCE") - os.Setenv("FOCALBOARD_UNIT_TESTING_COMPLIANCE", "false") - defer os.Setenv("FOCALBOARD_UNIT_TESTING_COMPLIANCE", originalEnv) + t.Setenv("FOCALBOARD_UNIT_TESTING_COMPLIANCE", "false")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/integrationtests/board_test.go` around lines 756 - 759, The current pattern saves originalEnv := os.Getenv("FOCALBOARD_UNIT_TESTING_COMPLIANCE") and defers os.Setenv(..., originalEnv), which wrongly leaves the variable set to an empty string if it was originally unset; change the cleanup to restore the original state by deferring a function that checks originalEnv: if originalEnv == "" then call os.Unsetenv("FOCALBOARD_UNIT_TESTING_COMPLIANCE") else call os.Setenv("FOCALBOARD_UNIT_TESTING_COMPLIANCE", originalEnv); alternatively replace the os.Getenv/os.Setenv logic with t.Setenv("FOCALBOARD_UNIT_TESTING_COMPLIANCE", "false") at the start of the test to let the testing framework handle restoration automatically.
2220-2220: Return value fromAddMemberToBoardis discarded without checking the error.Both here and at line 2309, the response from
th.Client.AddMemberToBoard(newUserMember)is ignored. If the member addition fails silently, subsequent assertions about member counts could produce confusing failures.Proposed fix
- th.Client.AddMemberToBoard(newUserMember) + _, addResp := th.Client.AddMemberToBoard(newUserMember) + th.CheckOK(addResp)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/integrationtests/board_test.go` at line 2220, The call to th.Client.AddMemberToBoard(newUserMember) is ignoring its return value; update the test to capture and assert the result (e.g., err := th.Client.AddMemberToBoard(newUserMember)) and fail the test if it returns an error (use t.Fatalf/t.Fatalf-like helper or require.NoError) so member-add failures surface immediately before further assertions about member counts; apply the same change for the second occurrence at the other test location.
507-507: EntireTestSearchBoardsis skipped — ensure this is tracked.
t.Skipat the top-level means all subtests below (lines 508–623) are dead code. If this is a known deferral, consider adding a tracking issue reference so it doesn't get forgotten.Would you like me to open an issue to track re-enabling
TestSearchBoards?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/integrationtests/board_test.go` at line 507, TestSearchBoards is fully skipped which leaves all its subtests as dead code; replace the bare t.Skip("Skipping TestSearchBoards - will be fixed separately") in TestSearchBoards with a skip that includes a tracking reference (e.g., t.Skipf("Skipping TestSearchBoards - tracked in ISSUE-<num>: <url>")) or add a clear TODO comment with the issue ID and create the referenced issue; update the Skip call or surrounding comment in the TestSearchBoards test so future readers can find and un-skip it.server/services/store/storetests/boards.go (1)
139-156: Raw SQL user insertion may break if theUserstable schema changes or has additional NOT NULL constraints.The direct SQL insertion bypasses the store layer and is coupled to the exact column names and casing of the
Userstable for both Postgres and MySQL. If the schema evolves (e.g., new NOT NULL columns without defaults), these inserts will silently fail at test time.Also, the hard-coded
usernameand"test-user","test@example.com") would collide if this pattern is used in multiple tests sharing the same DB. Currently each test gets its own store instance, so this works, but it's fragile.Consider extracting this into a shared test helper (e.g.,
insertTestUser(t, store, userID)) to centralize the SQL and make future schema changes easier to maintain, especially since this same pattern is repeated intestGetMembersForUserandtestSearchBoardsForUser.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/services/store/storetests/boards.go` around lines 139 - 156, The raw SQL insertion in testGetBoardsForUserAndTeam tightly couples tests to the Users table schema and reuses fixed username/email which can cause collisions; extract this logic into a shared helper (e.g., insertTestUser(t, store, userID)) and replace the inline block in testGetBoardsForUserAndTeam (and the other tests testGetMembersForUser and testSearchBoardsForUser) to call that helper; the helper should accept t, store, and userID, derive dbHandle via dbHandle assertion, build and execute the appropriate parameterized INSERT SQL for Postgres and MySQL centrally, and generate unique username/email (or accept them as optional args) to avoid collisions and simplify future schema changes.
larkox
left a comment
larkox
left a comment
There was a problem hiding this comment.
LGTM, but a few minor things to check.
Also, it would be nice to answer why the code rabbit comments are valid or not.
server/api/blocks.go
Outdated
|
|
||
| vars := mux.Vars(r) | ||
| teamID := vars["teamID"] | ||
| _ = vars["boardID"] // boardID is available but not used directly here |
There was a problem hiding this comment.
This and the above line, both seem useless.
There was a problem hiding this comment.
Yes not required, will remove it.
There was a problem hiding this comment.
No not required, will delete it.
| // the work template are present in the default templates. | ||
| // If this fails, you might need to sync with the channels team. | ||
| func TestGetTemplatesForWorkTemplate(t *testing.T) { | ||
| t.Skip("Skipping TestGetTemplatesForWorkTemplate - requires template data sync with channels team") |
There was a problem hiding this comment.
Do we have a ticket to remove this skip?
| got, err := store.GetActiveUserCount(60) | ||
| require.NoError(t, err) | ||
| require.Equal(t, count, got) | ||
| t.Skip("Skipping standalone-only test: GetActiveUserCount is only used in standalone server telemetry") |
There was a problem hiding this comment.
Dow e have a ticket to remove this skip?
There was a problem hiding this comment.
This is fixed now. don't have to skip it anymore.
| store, tearDown := setup(t) | ||
| defer tearDown() | ||
| testUpsertTeamSignupToken(t, store) | ||
| t.Skip("Skipping standalone-only test: UpsertTeamSignupToken is only used in standalone server initialization") |
There was a problem hiding this comment.
We no longer support standalone mode of Boards in this repository so we should get rid of any such tests and the logic as well.
There was a problem hiding this comment.
Removed the tests.
harshilsharma63
left a comment
harshilsharma63
left a comment
There was a problem hiding this comment.
The Coderabbit comments are also valid concerns.
| runs-on: ubuntu-22.04 | ||
| services: | ||
| postgres: | ||
| image: postgres:14 |
There was a problem hiding this comment.
Can you confirm this is the same version we use for Mattermost server CI? If not, can we use the same one here?
server/api/blocks.go
Outdated
| boardID := mux.Vars(r)["boardID"] | ||
| userID := getUserID(r) | ||
|
|
||
| if userID == "" { |
There was a problem hiding this comment.
I don't think this is a good way to add this check. If this is needed, it should be moved to middleware, similar to attachSession.
server/api/blocks.go
Outdated
|
|
||
| vars := mux.Vars(r) | ||
| teamID := vars["teamID"] | ||
| _ = vars["boardID"] // boardID is available but not used directly here |
There was a problem hiding this comment.
This and the above line, both seem useless.
| } | ||
|
|
||
| if board.IsTemplate { | ||
| // Check for explicit board membership (not synthetic) |
There was a problem hiding this comment.
Do templates even have explicit membership?
server/api/statistics.go
Outdated
|
|
||
| // user must have right to access analytics | ||
| userID := getUserID(r) | ||
| if userID == "" { |
There was a problem hiding this comment.
We need to get rid of all these if userId == "" checks.
| // Insert user into Users table | ||
| dbType := dbStore.DBType() | ||
| var insertSQL string | ||
| if dbType == model.PostgresDBType { |
There was a problem hiding this comment.
We no longer support MySQL and SQLite, so there is no need of any DB type check. This check, and potentially the dbStore.DBType() function itself can be removed.
| dbStore, ok := store.(dbHandle) | ||
| require.True(t, ok, "store must implement dbHandle interface") | ||
|
|
||
| // Get the database connection from the store | ||
| db := dbStore.DBHandle() | ||
| require.NotNil(t, db) | ||
|
|
||
| // Insert user into Users table | ||
| dbType := dbStore.DBType() | ||
| var insertSQL string | ||
| if dbType == model.PostgresDBType { | ||
| insertSQL = `INSERT INTO users (id, username, email, createat, updateat, deleteat) VALUES ($1, $2, $3, $4, $5, $6)` | ||
| } else { | ||
| insertSQL = `INSERT INTO Users (Id, Username, Email, CreateAt, UpdateAt, DeleteAt) VALUES (?, ?, ?, ?, ?, ?)` | ||
| } | ||
| _, err := db.Exec(insertSQL, user.ID, user.Username, user.Email, user.CreateAt, user.UpdateAt, 0) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
This whole logic of inserting user into DB for test is repeated in many places. This needs to be fixed. Also check for other similar duplications.
| store, tearDown := setup(t) | ||
| defer tearDown() | ||
| testUpsertTeamSignupToken(t, store) | ||
| t.Skip("Skipping standalone-only test: UpsertTeamSignupToken is only used in standalone server initialization") |
There was a problem hiding this comment.
We no longer support standalone mode of Boards in this repository so we should get rid of any such tests and the logic as well.
| BlockType: model.TypeCard, | ||
| BlockID: utils.NewID(utils.IDTypeBlock), | ||
| ModifiedByID: utils.NewID(utils.IDTypeUser), | ||
| ModifiedByID: mmModel.NewId(), // Mattermost user IDs are 26 characters |
There was a problem hiding this comment.
Unnecessary and repeated code comment about the nature of IDs.
| } | ||
| if err != nil { | ||
| // If the error is "user not found", treat it as idempotent (member doesn't exist) | ||
| if strings.Contains(err.Error(), "user not found") { |
There was a problem hiding this comment.
This is bad. We should compare the error with ErrUserNotFound. See isNotFoundError function in mattermost server codebase.
|
will be late to get to this. But its good to merge after de review |
|
This pull request introduces a high-severity issue where handlePatchBlocks determines teamID from the first block in a batch and uses it to broadcast all updates, allowing a crafted request with blocks from multiple boards to leak updates across team websocket audiences if the requester has permissions on each board. In short: batch patching can cause cross-team info disclosure via mis-scoped notifications and should be fixed to compute and use the correct teamID per block (or restrict batching).
🟠 Cross-Team Info Disclosure via Batch Block Patch in
|
| Vulnerability | Cross-Team Info Disclosure via Batch Block Patch |
|---|---|
| Description | In handlePatchBlocks, the teamID is determined based on the board of the first block in the patch array. The PatchBlocksAndNotify function then broadcasts updates for all blocks in that array using the same teamID. If a user crafts a batch request containing blocks from multiple boards (even if they have permission to access all of them), the notification service will broadcast changes for all these blocks to the websocket clients listening to the first block's team. This effectively leaks block updates from one team to another team's websocket audience, as long as the requesting user has PermissionManageBoardCards on all boards involved. |
mattermost-plugin-boards/server/api/blocks.go
Line 723 in 218bd2e
Comment to provide feedback on these findings.
Report false positive: @dryrunsecurity fp [FINDING ID] [FEEDBACK]
Report low-impact: @dryrunsecurity nit [FINDING ID] [FEEDBACK]
Example: @dryrunsecurity fp drs_90eda195 This code is not user-facing
All finding details can be found in the DryRun Security Dashboard.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/services/store/storetests/blocks.go (1)
1098-1118:⚠️ Potential issue | 🟡 MinorCleanup defer misses appended blocks.
The cleanup defers
DeleteBlocks(t, store, subtreeSampleBlocks, "test")which only includes the 6 original blocks, butblocksToInsertcontains 8 blocks after appendinggrandchild1aandgrandchild2a. While this doesn't affect test correctness (each test gets a fresh store), it's inconsistent with the cleanup pattern.🧹 Proposed fix
InsertBlocks(t, store, blocksToInsert, "user-id-1") time.Sleep(1 * time.Millisecond) - defer DeleteBlocks(t, store, subtreeSampleBlocks, "test") + defer DeleteBlocks(t, store, blocksToInsert, "test")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/services/store/storetests/blocks.go` around lines 1098 - 1118, The defer cleanup currently calls DeleteBlocks with subtreeSampleBlocks (only the original 6), but the test actually inserts blocksToInsert (which includes the appended grandchild1a and grandchild2a); update the defer to call DeleteBlocks(t, store, blocksToInsert, "test") so the cleanup matches the inserted set (refer to the InsertBlocks call and variables blocksToInsert and subtreeSampleBlocks to locate the change).
🧹 Nitpick comments (12)
server/services/store/sqlstore/helpers_test.go (3)
377-379: Dead code:os.Removewill never succeed for current configurations.Since SQLite is explicitly skipped (lines 304-308 default to
postgres),connectionStringwill always be a database URL rather than a file path. Theos.Removecall will silently fail for PostgreSQL/MySQL connection strings.Consider removing this SQLite-specific cleanup or adding a condition:
🧹 Suggested cleanup
- if err = os.Remove(connectionString); err == nil { - logger.Debug("Removed test database", mlog.String("file", connectionString)) - } + // Note: File removal only applies to SQLite (currently not supported in plugin mode)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/services/store/sqlstore/helpers_test.go` around lines 377 - 379, The os.Remove call attempting to delete connectionString is dead for non-SQLite DBs; update the cleanup in the test to only attempt filesystem removal when using SQLite (e.g., guard around the os.Remove and logger.Debug using the DB type check used earlier in the test) or remove the SQLite-specific cleanup entirely; target the block that references connectionString and logger.Debug to apply the conditional or delete the call.
81-94: Database errors are silently converted to "not found".If
QueryRow.Scanfails for reasons other than the user not existing (e.g., connection issues, malformed data), the error is discarded and the method returnsErrNotFound. This could mask real database problems during test debugging.Consider checking for
sql.ErrNoRowsexplicitly:♻️ Suggested improvement
err := t.db.QueryRow(query, args...).Scan( &dbUser.ID, &dbUser.Username, &dbUser.Email, &dbUser.CreateAt, &dbUser.UpdateAt, &dbUser.DeleteAt) - if err == nil { + if err != nil { + if !errors.Is(err, sql.ErrNoRows) { + return nil, err // Propagate real DB errors + } + // User not found in database + return nil, model.NewErrNotFound("user ID=" + userID) + } + return &mmModel.User{ + Id: dbUser.ID, + Username: dbUser.Username, + Email: dbUser.Email, + CreateAt: dbUser.CreateAt, + UpdateAt: dbUser.UpdateAt, + DeleteAt: dbUser.DeleteAt, + }, nil - return &mmModel.User{ - Id: dbUser.ID, - Username: dbUser.Username, - Email: dbUser.Email, - CreateAt: dbUser.CreateAt, - UpdateAt: dbUser.UpdateAt, - DeleteAt: dbUser.DeleteAt, - }, nil - } - // User not found in database, return ErrNotFound - return nil, model.NewErrNotFound("user ID=" + userID) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/services/store/sqlstore/helpers_test.go` around lines 81 - 94, The test helper currently treats any Scan error as "not found"; change the error handling after QueryRow(...).Scan(...) to check if err == sql.ErrNoRows and only then return model.NewErrNotFound("user ID="+userID), otherwise return the original database error (or a wrapped/db-specific error) so real DB issues are propagated; update the block around QueryRow.Scan in helpers_test.go to explicitly handle sql.ErrNoRows versus other errors.
135-149: Same error handling pattern as GetUserByID.The same silent error suppression issue applies here. The fix pattern suggested for
GetUserByIDshould also be applied to this method andGetUserByUsername(lines 187-199).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/services/store/sqlstore/helpers_test.go` around lines 135 - 149, The QueryRow Scan block in the helper for getting users by email (and likewise in GetUserByUsername) currently swallows all errors and only returns NotFound; change the error handling to mirror GetUserByID's fix: after Scan, if err == sql.ErrNoRows return model.NewErrNotFound("user email="+email) (or username) else if err != nil return nil, err so non-NotFound DB errors propagate; update the scan/error branches in the functions referenced (GetUserByEmail and GetUserByUsername) to distinguish sql.ErrNoRows from other errors and return the real error for troubleshooting.server/integrationtests/file_test.go (1)
117-123: Add ownership assertion onfileInfo.Path(board-scoped path), not only ID equality.This test currently proves ID normalization, but not board ownership semantics. Please also assert the returned path contains the expected board segment.
Based on learnings: file ownership for boards is tracked via storage path (e.g., `boards/YYYYMMDD/{boardID}/{filename}`), so ownership assertions should validate path structure rather than relying only on other fields.✅ Suggested assertion add-on
import ( "bytes" + "strings" "testing" @@ require.NotNil(t, fileInfo) require.NotNil(t, fileInfo.Id) fileIDWithoutPrefix := utils.RetrieveFileIDFromBlockFieldStorage(file.FileID) require.Equal(t, fileIDWithoutPrefix, fileInfo.Id) + require.NotEmpty(t, fileInfo.Path) + parts := strings.Split(fileInfo.Path, "/") + require.GreaterOrEqual(t, len(parts), 4) + require.Equal(t, "boards", parts[0]) + require.Equal(t, testBoard.ID, parts[2]) }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/integrationtests/file_test.go` around lines 117 - 123, Add an assertion that verifies board ownership by checking fileInfo.Path contains the board-scoped segment derived from testBoard.ID (instead of only checking IDs). In the TeamUploadFileInfo result handling (where fileInfo is obtained and utils.RetrieveFileIDFromBlockFieldStorage is used), construct the expected board segment (e.g., "boards/.../{testBoard.ID}/" or simply "/{testBoard.ID}/") and assert that fileInfo.Path contains that segment (using strings.Contains or a small regex) to ensure the file path is scoped to the correct board.server/api/categories.go (4)
598-607: MissingrequireUserIDguard inhandleHideBoard.This handler uses
getUserID(r)but doesn't have therequireUserIDguard that was added to other category handlers. An emptyuserIDwould pass through to the permission check, which might behave unexpectedly.Proposed fix
userID := getUserID(r) + if a.requireUserID(w, r, userID, "access denied to hide board") { + return + } + vars := mux.Vars(r) teamID := vars["teamID"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/categories.go` around lines 598 - 607, handleHideBoard is calling getUserID(r) but lacks the requireUserID guard found in other handlers; add a check using requireUserID(r) at the top of handleHideBoard and return early with a.errorResponse (like other handlers) when it fails, before calling getUserID or permissions.HasPermissionToTeam. Locate handleHideBoard, insert the requireUserID call and its error response path so userID is guaranteed non-empty before using getUserID, and leave the subsequent permissions.HasPermissionToTeam and a.errorResponse logic unchanged.
660-669: MissingrequireUserIDguard inhandleUnhideBoard.Same issue as
handleHideBoard- usesgetUserID(r)without therequireUserIDguard.Proposed fix
userID := getUserID(r) + if a.requireUserID(w, r, userID, "access denied to unhide board") { + return + } + vars := mux.Vars(r) teamID := vars["teamID"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/categories.go` around lines 660 - 669, The handleUnhideBoard handler currently calls getUserID(r) directly like handleHideBoard did; change it to call requireUserID(w, r) and check its boolean return, returning early if it fails, then use the returned userID for permission checks (mirroring handleHideBoard). Update references to userID in that function (and leave teamID/boardID/categoryID usage intact) so permission check uses the validated userID with a.permissions.HasPermissionToTeam.
432-434: Inconsistent authentication pattern inhandleReorderCategories.This handler still uses the old session context pattern (
ctx.Value(sessionContextKey).(*model.Session)) without therequireUserIDguard that was added to other category handlers in this file. If the session is nil or the type assertion fails, this will panic.Proposed fix
vars := mux.Vars(r) teamID := vars["teamID"] - ctx := r.Context() - session := ctx.Value(sessionContextKey).(*model.Session) - userID := session.UserID + userID := getUserID(r) + if a.requireUserID(w, r, userID, "access denied to reorder categories") { + return + } if !a.permissions.HasPermissionToTeam(userID, teamID, model.PermissionViewTeam) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/categories.go` around lines 432 - 434, handleReorderCategories currently pulls the session directly via ctx.Value(sessionContextKey).(*model.Session) which can panic; replace that with the existing auth helper by calling requireUserID(r.Context()) (the same guard used in other category handlers) to obtain userID and early-return on error/unauthorized, remove the direct type assertion of model.Session, and use the returned userID variable for downstream logic so the handler follows the same safe authentication pattern.
510-512: Inconsistent authentication pattern inhandleReorderCategoryBoards.Similar to
handleReorderCategories, this handler uses the old session context pattern without therequireUserIDguard.Proposed fix
vars := mux.Vars(r) teamID := vars["teamID"] categoryID := vars["categoryID"] - ctx := r.Context() - session := ctx.Value(sessionContextKey).(*model.Session) - userID := session.UserID + userID := getUserID(r) + if a.requireUserID(w, r, userID, "access denied to reorder category boards") { + return + } if !a.permissions.HasPermissionToTeam(userID, teamID, model.PermissionViewTeam) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/categories.go` around lines 510 - 512, The handler handleReorderCategoryBoards is using the old sessionContextKey extraction (ctx.Value(sessionContextKey).(*model.Session)) instead of the unified guard; replace that code with a call to requireUserID(ctx) to obtain (userID, err) and handle the error path consistently (return/write error and stop) just like handleReorderCategories; remove the sessionContextKey/model.Session extraction and use the returned userID for subsequent logic.server/api/teams.go (1)
272-279: MissingrequireUserIDguard inhandleGetTeamUsersByID.Other team handlers (
handleGetTeams,handleGetTeam,handleGetTeamUsers) now have therequireUserIDguard, buthandleGetTeamUsersByIDonly checksHasPermissionToTeamwithout the earlyrequireUserIDgate. This is inconsistent with the pattern applied elsewhere in this PR.Proposed fix
vars := mux.Vars(r) teamID := vars["teamID"] userID := getUserID(r) + if a.requireUserID(w, r, userID, "access denied to team users") { + return + } + if !a.permissions.HasPermissionToTeam(userID, teamID, model.PermissionViewTeam) { a.errorResponse(w, r, model.NewErrPermission("access denied to team")) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/teams.go` around lines 272 - 279, The handler handleGetTeamUsersByID is missing the requireUserID guard used elsewhere; add a call to requireUserID(w, r) at the start of the function (before reading vars/getUserID and before permission checks) and if it returns an error respond via a.errorResponse and return, mirroring the pattern in handleGetTeams/handleGetTeam/handleGetTeamUsers so that requireUserID enforces authentication prior to calling a.permissions.HasPermissionToTeam.server/integrationtests/board_test.go (3)
763-768: Unnecessary client assignment immediately overwritten.Lines 763-764 set
th.Clientto an unauthenticated client, but lines 767-768 immediately override it withclients.TeamMemberfor board creation. The first assignment has no effect.♻️ Suggested simplification
th := SetupTestHelperPluginMode(t) defer th.TearDown() - // Create a client with no authentication headers - th.Client = client.NewClient(th.Server.Config().ServerRoot, "") // Create a board first so we can test authentication properly clients := setupClients(th) th.Client = clients.TeamMember🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/integrationtests/board_test.go` around lines 763 - 768, The assignment that creates an unauthenticated client is redundant because th.Client is immediately overwritten; remove the unnecessary line that sets th.Client = client.NewClient(th.Server.Config().ServerRoot, "") and rely on setupClients() to initialize th.Client via clients.TeamMember when creating the board; update or run tests to ensure setupClients(), clients.TeamMember and any subsequent uses of th.Client still behave as expected.
506-507: Entire test is skipped, disabling significant coverage.The
t.Skip()at the start prevents all subtests from running. Consider adding a tracked issue reference (e.g.,t.Skip("Skipping TestSearchBoards - see MM-XXXXX")) so the skip can be revisited. Alternatively, if only specific scenarios fail, skip those individually rather than the entire test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/integrationtests/board_test.go` around lines 506 - 507, TestSearchBoards is entirely skipped via t.Skip which disables coverage; remove or update the top-level t.Skip in TestSearchBoards and either add a tracked issue reference in the skip message (e.g., t.Skip("Skipping TestSearchBoards - see MM-XXXXX")) or move the skip to only failing subtests (skip inside specific t.Run subtests) so the rest of the subtests execute; update the TestSearchBoards function accordingly and ensure any test-specific skips reference the issue ID to allow future re-enablement.
880-880: Inconsistent team ID usage compared to other tests.This test uses
testTeamIDdirectly, while other migrated tests (e.g.,TestGetBoardMetadataat line 792-793) useth.GetTestTeamIDs()for dynamic team IDs. The same inconsistency appears at lines 906 and 927. Consider aligning with the dynamic ID pattern for consistency:- teamID := testTeamID + testTeamID, _, _ := th.GetTestTeamIDs() + teamID := testTeamID🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/integrationtests/board_test.go` at line 880, The test uses a hardcoded teamID variable (teamID := testTeamID) instead of the dynamic pattern used elsewhere; change the assignment to use the test helper that returns dynamic IDs (call th.GetTestTeamIDs() and select the appropriate ID) so this test aligns with other migrated tests like TestGetBoardMetadata; apply the same replacement at the other occurrences referenced (around the spots matching lines 906 and 927) to ensure consistent use of th.GetTestTeamIDs() across the board_test.go tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/api/auth.go`:
- Around line 14-15: Update the doc comment for requireUserID to fix the
spelling: change "signalling" to "signaling" in the comment that starts with "//
requireUserID writes an unauthorized error response and returns true when userID
is empty, signalling the caller to abort request handling." so the word is
corrected to "signaling".
In `@server/integrationtests/file_test.go`:
- Around line 67-79: Capture the original MaxFileSize before mutating it (e.g.,
orig := th.Server.App().GetConfig().MaxFileSize), set the smaller value for the
first upload test, and defer restoring the original via
th.Server.App().SetConfig(...) so the config is returned to its prior state
instead of using the hardcoded 100000; update the block around
GetConfig/SetConfig and TeamUploadFile to use this captured orig value and a
defer to reset it.
---
Outside diff comments:
In `@server/services/store/storetests/blocks.go`:
- Around line 1098-1118: The defer cleanup currently calls DeleteBlocks with
subtreeSampleBlocks (only the original 6), but the test actually inserts
blocksToInsert (which includes the appended grandchild1a and grandchild2a);
update the defer to call DeleteBlocks(t, store, blocksToInsert, "test") so the
cleanup matches the inserted set (refer to the InsertBlocks call and variables
blocksToInsert and subtreeSampleBlocks to locate the change).
---
Nitpick comments:
In `@server/api/categories.go`:
- Around line 598-607: handleHideBoard is calling getUserID(r) but lacks the
requireUserID guard found in other handlers; add a check using requireUserID(r)
at the top of handleHideBoard and return early with a.errorResponse (like other
handlers) when it fails, before calling getUserID or
permissions.HasPermissionToTeam. Locate handleHideBoard, insert the
requireUserID call and its error response path so userID is guaranteed non-empty
before using getUserID, and leave the subsequent permissions.HasPermissionToTeam
and a.errorResponse logic unchanged.
- Around line 660-669: The handleUnhideBoard handler currently calls
getUserID(r) directly like handleHideBoard did; change it to call
requireUserID(w, r) and check its boolean return, returning early if it fails,
then use the returned userID for permission checks (mirroring handleHideBoard).
Update references to userID in that function (and leave
teamID/boardID/categoryID usage intact) so permission check uses the validated
userID with a.permissions.HasPermissionToTeam.
- Around line 432-434: handleReorderCategories currently pulls the session
directly via ctx.Value(sessionContextKey).(*model.Session) which can panic;
replace that with the existing auth helper by calling requireUserID(r.Context())
(the same guard used in other category handlers) to obtain userID and
early-return on error/unauthorized, remove the direct type assertion of
model.Session, and use the returned userID variable for downstream logic so the
handler follows the same safe authentication pattern.
- Around line 510-512: The handler handleReorderCategoryBoards is using the old
sessionContextKey extraction (ctx.Value(sessionContextKey).(*model.Session))
instead of the unified guard; replace that code with a call to
requireUserID(ctx) to obtain (userID, err) and handle the error path
consistently (return/write error and stop) just like handleReorderCategories;
remove the sessionContextKey/model.Session extraction and use the returned
userID for subsequent logic.
In `@server/api/teams.go`:
- Around line 272-279: The handler handleGetTeamUsersByID is missing the
requireUserID guard used elsewhere; add a call to requireUserID(w, r) at the
start of the function (before reading vars/getUserID and before permission
checks) and if it returns an error respond via a.errorResponse and return,
mirroring the pattern in handleGetTeams/handleGetTeam/handleGetTeamUsers so that
requireUserID enforces authentication prior to calling
a.permissions.HasPermissionToTeam.
In `@server/integrationtests/board_test.go`:
- Around line 763-768: The assignment that creates an unauthenticated client is
redundant because th.Client is immediately overwritten; remove the unnecessary
line that sets th.Client = client.NewClient(th.Server.Config().ServerRoot, "")
and rely on setupClients() to initialize th.Client via clients.TeamMember when
creating the board; update or run tests to ensure setupClients(),
clients.TeamMember and any subsequent uses of th.Client still behave as
expected.
- Around line 506-507: TestSearchBoards is entirely skipped via t.Skip which
disables coverage; remove or update the top-level t.Skip in TestSearchBoards and
either add a tracked issue reference in the skip message (e.g., t.Skip("Skipping
TestSearchBoards - see MM-XXXXX")) or move the skip to only failing subtests
(skip inside specific t.Run subtests) so the rest of the subtests execute;
update the TestSearchBoards function accordingly and ensure any test-specific
skips reference the issue ID to allow future re-enablement.
- Line 880: The test uses a hardcoded teamID variable (teamID := testTeamID)
instead of the dynamic pattern used elsewhere; change the assignment to use the
test helper that returns dynamic IDs (call th.GetTestTeamIDs() and select the
appropriate ID) so this test aligns with other migrated tests like
TestGetBoardMetadata; apply the same replacement at the other occurrences
referenced (around the spots matching lines 906 and 927) to ensure consistent
use of th.GetTestTeamIDs() across the board_test.go tests.
In `@server/integrationtests/file_test.go`:
- Around line 117-123: Add an assertion that verifies board ownership by
checking fileInfo.Path contains the board-scoped segment derived from
testBoard.ID (instead of only checking IDs). In the TeamUploadFileInfo result
handling (where fileInfo is obtained and
utils.RetrieveFileIDFromBlockFieldStorage is used), construct the expected board
segment (e.g., "boards/.../{testBoard.ID}/" or simply "/{testBoard.ID}/") and
assert that fileInfo.Path contains that segment (using strings.Contains or a
small regex) to ensure the file path is scoped to the correct board.
In `@server/services/store/sqlstore/helpers_test.go`:
- Around line 377-379: The os.Remove call attempting to delete connectionString
is dead for non-SQLite DBs; update the cleanup in the test to only attempt
filesystem removal when using SQLite (e.g., guard around the os.Remove and
logger.Debug using the DB type check used earlier in the test) or remove the
SQLite-specific cleanup entirely; target the block that references
connectionString and logger.Debug to apply the conditional or delete the call.
- Around line 81-94: The test helper currently treats any Scan error as "not
found"; change the error handling after QueryRow(...).Scan(...) to check if err
== sql.ErrNoRows and only then return model.NewErrNotFound("user ID="+userID),
otherwise return the original database error (or a wrapped/db-specific error) so
real DB issues are propagated; update the block around QueryRow.Scan in
helpers_test.go to explicitly handle sql.ErrNoRows versus other errors.
- Around line 135-149: The QueryRow Scan block in the helper for getting users
by email (and likewise in GetUserByUsername) currently swallows all errors and
only returns NotFound; change the error handling to mirror GetUserByID's fix:
after Scan, if err == sql.ErrNoRows return model.NewErrNotFound("user
email="+email) (or username) else if err != nil return nil, err so non-NotFound
DB errors propagate; update the scan/error branches in the functions referenced
(GetUserByEmail and GetUserByUsername) to distinguish sql.ErrNoRows from other
errors and return the real error for troubleshooting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e3502358-7b91-47c1-a91f-9ac0211db879
📒 Files selected for processing (35)
server/api/archive.goserver/api/auth.goserver/api/blocks.goserver/api/boards.goserver/api/boards_and_blocks.goserver/api/cards.goserver/api/categories.goserver/api/channels.goserver/api/content_blocks.goserver/api/files.goserver/api/members.goserver/api/onboarding.goserver/api/search.goserver/api/sharing.goserver/api/statistics.goserver/api/subscriptions.goserver/api/teams.goserver/api/templates.goserver/api/users.goserver/app/import.goserver/integrationtests/board_test.goserver/integrationtests/file_test.goserver/integrationtests/work_template_test.goserver/model/board.goserver/model/board_test.goserver/services/store/sqlstore/cloud.goserver/services/store/sqlstore/helpers_test.goserver/services/store/sqlstore/mattermost_tables.goserver/services/store/sqlstore/sqlstore_test.goserver/services/store/sqlstore/support_for_test.goserver/services/store/storetests/blocks.goserver/services/store/storetests/cloud.goserver/services/store/storetests/data_retention.goserver/services/store/storetests/session.goserver/services/store/storetests/teams.go
💤 Files with no reviewable changes (3)
- server/services/store/sqlstore/sqlstore_test.go
- server/services/store/storetests/teams.go
- server/services/store/storetests/session.go
✅ Files skipped from review due to trivial changes (2)
- server/api/statistics.go
- server/api/members.go
🚧 Files skipped from review as they are similar to previous changes (14)
- server/api/channels.go
- server/api/files.go
- server/api/templates.go
- server/api/sharing.go
- server/api/search.go
- server/api/onboarding.go
- server/services/store/sqlstore/mattermost_tables.go
- server/api/users.go
- server/api/cards.go
- server/api/blocks.go
- server/services/store/storetests/data_retention.go
- server/services/store/storetests/cloud.go
- server/app/import.go
- server/services/store/sqlstore/support_for_test.go
| // requireUserID writes an unauthorized error response and returns true when | ||
| // userID is empty, signalling the caller to abort request handling. |
There was a problem hiding this comment.
Fix spelling: "signalling" → "signaling".
The pipeline linter flagged a misspelling in the doc comment.
Proposed fix
-// requireUserID writes an unauthorized error response and returns true when
-// userID is empty, signalling the caller to abort request handling.
+// requireUserID writes an unauthorized error response and returns true when
+// userID is empty, signaling the caller to abort request handling.📝 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.
| // requireUserID writes an unauthorized error response and returns true when | |
| // userID is empty, signalling the caller to abort request handling. | |
| // requireUserID writes an unauthorized error response and returns true when | |
| // userID is empty, signaling the caller to abort request handling. |
🧰 Tools
🪛 GitHub Actions: golangci-lint
[error] 15-15: golangci-lint failed: signalling is a misspelling of signaling (misspell)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/api/auth.go` around lines 14 - 15, Update the doc comment for
requireUserID to fix the spelling: change "signalling" to "signaling" in the
comment that starts with "// requireUserID writes an unauthorized error response
and returns true when userID is empty, signalling the caller to abort request
handling." so the word is corrected to "signaling".
| config := th.Server.App().GetConfig() | ||
| config.MaxFileSize = 1 | ||
| th.Server.App().SetConfig(config) | ||
|
|
||
| file, resp := th.Client.TeamUploadFile(testTeamID, testBoard.ID, bytes.NewBuffer([]byte("test"))) | ||
| file, resp := th.Client.TeamUploadFile(teamID, testBoard.ID, bytes.NewBuffer([]byte("test"))) | ||
| th.CheckRequestEntityTooLarge(resp) | ||
| require.Nil(t, file) | ||
|
|
||
| config.MaxFileSize = 100000 | ||
| th.Server.App().SetConfig(config) | ||
|
|
||
| file, resp = th.Client.TeamUploadFile(testTeamID, testBoard.ID, bytes.NewBuffer([]byte("test"))) | ||
| file, resp = th.Client.TeamUploadFile(teamID, testBoard.ID, bytes.NewBuffer([]byte("test"))) | ||
| th.CheckOK(resp) |
There was a problem hiding this comment.
Restore MaxFileSize to its original value, not a hardcoded constant.
Using 100000 on Line 75 can leak config state across tests. Capture the original value and restore it with defer.
🔧 Proposed fix
config := th.Server.App().GetConfig()
+ originalMaxFileSize := config.MaxFileSize
+ defer func() {
+ cfg := th.Server.App().GetConfig()
+ cfg.MaxFileSize = originalMaxFileSize
+ th.Server.App().SetConfig(cfg)
+ }()
+
config.MaxFileSize = 1
th.Server.App().SetConfig(config)
@@
- config.MaxFileSize = 100000
- th.Server.App().SetConfig(config)
-
file, resp = th.Client.TeamUploadFile(teamID, testBoard.ID, bytes.NewBuffer([]byte("test")))📝 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.
| config := th.Server.App().GetConfig() | |
| config.MaxFileSize = 1 | |
| th.Server.App().SetConfig(config) | |
| file, resp := th.Client.TeamUploadFile(testTeamID, testBoard.ID, bytes.NewBuffer([]byte("test"))) | |
| file, resp := th.Client.TeamUploadFile(teamID, testBoard.ID, bytes.NewBuffer([]byte("test"))) | |
| th.CheckRequestEntityTooLarge(resp) | |
| require.Nil(t, file) | |
| config.MaxFileSize = 100000 | |
| th.Server.App().SetConfig(config) | |
| file, resp = th.Client.TeamUploadFile(testTeamID, testBoard.ID, bytes.NewBuffer([]byte("test"))) | |
| file, resp = th.Client.TeamUploadFile(teamID, testBoard.ID, bytes.NewBuffer([]byte("test"))) | |
| th.CheckOK(resp) | |
| config := th.Server.App().GetConfig() | |
| originalMaxFileSize := config.MaxFileSize | |
| defer func() { | |
| cfg := th.Server.App().GetConfig() | |
| cfg.MaxFileSize = originalMaxFileSize | |
| th.Server.App().SetConfig(cfg) | |
| }() | |
| config.MaxFileSize = 1 | |
| th.Server.App().SetConfig(config) | |
| file, resp := th.Client.TeamUploadFile(teamID, testBoard.ID, bytes.NewBuffer([]byte("test"))) | |
| th.CheckRequestEntityTooLarge(resp) | |
| require.Nil(t, file) | |
| config.MaxFileSize = originalMaxFileSize | |
| th.Server.App().SetConfig(config) | |
| file, resp = th.Client.TeamUploadFile(teamID, testBoard.ID, bytes.NewBuffer([]byte("test"))) | |
| th.CheckOK(resp) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/integrationtests/file_test.go` around lines 67 - 79, Capture the
original MaxFileSize before mutating it (e.g., orig :=
th.Server.App().GetConfig().MaxFileSize), set the smaller value for the first
upload test, and defer restoring the original via th.Server.App().SetConfig(...)
so the config is returned to its prior state instead of using the hardcoded
100000; update the block around GetConfig/SetConfig and TeamUploadFile to use
this captured orig value and a defer to reset it.
5 participants
Summary
Fixing all the server tests for boards.
Ticket Link
https://mattermost.atlassian.net/browse/MM-67641
Change Impact: 🔴 High
Reasoning: The PR makes wide-ranging changes across API authentication/authorization, data persistence (SQL store, test DB setup, migration helpers), core server lifecycle, and many integration/unit tests—affecting critical control flows and shared modules.
Regression Risk: High — authentication/authorization logic was altered extensively (new requireUserID guards), data-layer/table creation and SQL behavior changed, and new exported errors/fields were added; these touch core paths and risk breaking existing behavior if any guard or DB migration step is incorrect.
** QA Recommendation:** Run full CI with Postgres integration and perform thorough manual QA focusing on auth-protected endpoints, file/export/import flows, webserver start/shutdown behavior, and cross-database store tests. Skipping manual QA is not recommended.
Generated by CodeRabbitAI
Summary by CodeRabbit
Release Notes
Bug Fixes
Chores