diff --git a/server/channels/api4/content_flagging.go b/server/channels/api4/content_flagging.go index f2def9c8398..548696283e2 100644 --- a/server/channels/api4/content_flagging.go +++ b/server/channels/api4/content_flagging.go @@ -239,9 +239,9 @@ func getContentFlaggingFields(c *Context, w http.ResponseWriter, r *http.Request return } - groupId, appErr := c.App.ContentFlaggingGroupId() - if appErr != nil { - c.Err = appErr + groupId, err := c.App.ContentFlaggingGroupId() + if err != nil { + c.Err = model.NewAppError("getContentFlaggingGroupId", "app.data_spillage.get_group.error", nil, "", http.StatusInternalServerError).Wrap(err) return } diff --git a/server/channels/app/content_flagging.go b/server/channels/app/content_flagging.go index 47bea653792..fee88a01b20 100644 --- a/server/channels/app/content_flagging.go +++ b/server/channels/app/content_flagging.go @@ -89,9 +89,9 @@ func (a *App) FlagPost(rctx request.CTX, post *model.Post, teamId, reportingUser return appErr } - groupId, appErr := a.ContentFlaggingGroupId() - if appErr != nil { - return appErr + groupId, err := a.ContentFlaggingGroupId() + if err != nil { + return model.NewAppError("FlagPost", "app.data_spillage.get_group.error", nil, "", http.StatusInternalServerError).Wrap(err) } reportingUser, appErr := a.GetUser(reportingUserId) @@ -232,18 +232,19 @@ func (a *App) setContentFlaggingPropertiesForThreadReplies(post *model.Post, con return nil } -func (a *App) ContentFlaggingGroupId() (string, *model.AppError) { - group, err := a.Srv().propertyAccessService.GetPropertyGroup(model.ContentFlaggingGroupName) +func (a *App) ContentFlaggingGroupId() (string, error) { + group, err := a.Srv().propertyAccessService.Group(model.ContentFlaggingGroupName) if err != nil { - return "", model.NewAppError("getContentFlaggingGroupId", "app.data_spillage.get_group.error", nil, "", http.StatusInternalServerError) + return "", fmt.Errorf("cannot retrieve content flagging property group ID: %w", err) } + return group.ID, nil } func (a *App) GetPostContentFlaggingPropertyValue(postId, propertyFieldName string) (*model.PropertyValue, *model.AppError) { - groupId, appErr := a.ContentFlaggingGroupId() - if appErr != nil { - return nil, appErr + groupId, err := a.ContentFlaggingGroupId() + if err != nil { + return nil, model.NewAppError("GetPostContentFlaggingPropertyValue", "app.data_spillage.get_group.error", nil, "", http.StatusInternalServerError).Wrap(err) } statusPropertyField, err := a.Srv().propertyAccessService.GetPropertyFieldByName(anonymousCallerId, groupId, "", propertyFieldName) @@ -538,9 +539,9 @@ func (a *App) IsUserTeamContentReviewer(userId, teamId string) (bool, *model.App } func (a *App) GetPostContentFlaggingPropertyValues(postId string) ([]*model.PropertyValue, *model.AppError) { - groupId, appErr := a.ContentFlaggingGroupId() - if appErr != nil { - return nil, appErr + groupId, err := a.ContentFlaggingGroupId() + if err != nil { + return nil, model.NewAppError("GetPostContentFlaggingPropertyValues", "app.data_spillage.get_group.error", nil, "", http.StatusInternalServerError).Wrap(err) } propertyValues, err := a.PropertyAccessService().SearchPropertyValues(anonymousCallerId, groupId, model.PropertyValueSearchOpts{TargetIDs: []string{postId}, PerPage: CONTENT_FLAGGING_MAX_PROPERTY_VALUES}) @@ -585,9 +586,9 @@ func (a *App) PermanentDeleteFlaggedPost(rctx request.CTX, actionRequest *model. return appErr } - groupId, appErr := a.ContentFlaggingGroupId() - if appErr != nil { - return appErr + groupId, err := a.ContentFlaggingGroupId() + if err != nil { + return model.NewAppError("PermanentDeleteFlaggedPost", "app.data_spillage.get_group.error", nil, "", http.StatusInternalServerError).Wrap(err) } mappedFields, appErr := a.GetContentFlaggingMappedFields(groupId) @@ -619,7 +620,7 @@ func (a *App) PermanentDeleteFlaggedPost(rctx request.CTX, actionRequest *model. }, } - _, err := a.Srv().propertyAccessService.CreatePropertyValues(anonymousCallerId, propertyValues) + _, err = a.Srv().propertyAccessService.CreatePropertyValues(anonymousCallerId, propertyValues) if err != nil { return model.NewAppError("PermanentlyRemoveFlaggedPost", "app.data_spillage.create_property_values.app_error", nil, "", http.StatusInternalServerError).Wrap(err) } @@ -727,9 +728,9 @@ func (a *App) KeepFlaggedPost(rctx request.CTX, actionRequest *model.FlagContent return model.NewAppError("KeepFlaggedPost", "api.data_spillage.error.post_not_in_progress", nil, "", http.StatusBadRequest) } - groupId, appErr := a.ContentFlaggingGroupId() - if appErr != nil { - return appErr + groupId, err := a.ContentFlaggingGroupId() + if err != nil { + return model.NewAppError("KeepFlaggedPost", "app.data_spillage.get_group.error", nil, "", http.StatusInternalServerError).Wrap(err) } mappedFields, appErr := a.GetContentFlaggingMappedFields(groupId) @@ -756,8 +757,8 @@ func (a *App) KeepFlaggedPost(rctx request.CTX, actionRequest *model.FlagContent } // Restore the post, its replies, and all associated files - if err := a.Srv().Store().Post().RestoreContentFlaggedPost(flaggedPost, statusField.ID, contentFlaggingManagedField.ID); err != nil { - return model.NewAppError("KeepFlaggedPost", "app.data_spillage.keep_post.undelete.app_error", nil, "", http.StatusInternalServerError).Wrap(err) + if rErr := a.Srv().Store().Post().RestoreContentFlaggedPost(flaggedPost, statusField.ID, contentFlaggingManagedField.ID); rErr != nil { + return model.NewAppError("KeepFlaggedPost", "app.data_spillage.keep_post.undelete.app_error", nil, "", http.StatusInternalServerError).Wrap(rErr) } } @@ -989,9 +990,9 @@ func (a *App) AssignFlaggedPostReviewer(rctx request.CTX, flaggedPostId, flagged status := strings.Trim(string(statusPropertyValue.Value), `"`) - groupId, appErr := a.ContentFlaggingGroupId() - if appErr != nil { - return appErr + groupId, err := a.ContentFlaggingGroupId() + if err != nil { + return model.NewAppError("AssignFlaggedPostReviewer", "app.data_spillage.get_group.error", nil, "", http.StatusInternalServerError).Wrap(err) } mappedFields, appErr := a.GetContentFlaggingMappedFields(groupId) @@ -1011,7 +1012,7 @@ func (a *App) AssignFlaggedPostReviewer(rctx request.CTX, flaggedPostId, flagged Value: json.RawMessage(fmt.Sprintf(`"%s"`, reviewerId)), } - assigneePropertyValue, err := a.Srv().propertyAccessService.UpsertPropertyValue(anonymousCallerId, assigneePropertyValue) + assigneePropertyValue, err = a.Srv().propertyAccessService.UpsertPropertyValue(anonymousCallerId, assigneePropertyValue) if err != nil { return model.NewAppError("AssignFlaggedPostReviewer", "app.data_spillage.assign_reviewer.upsert_property_value.app_error", nil, "", http.StatusInternalServerError).Wrap(err) } diff --git a/server/channels/app/content_flagging_test.go b/server/channels/app/content_flagging_test.go index 30225b3b02b..93b6ceedf13 100644 --- a/server/channels/app/content_flagging_test.go +++ b/server/channels/app/content_flagging_test.go @@ -38,8 +38,8 @@ func setBaseConfig(th *TestHelper) *model.AppError { func searchPropertyValue(t *testing.T, th *TestHelper, postId, fieldName string) []*model.PropertyValue { t.Helper() - groupId, appErr := th.App.ContentFlaggingGroupId() - require.Nil(t, appErr) + groupId, err := th.App.ContentFlaggingGroupId() + require.NoError(t, err) mappedFields, appErr := th.App.GetContentFlaggingMappedFields(groupId) require.Nil(t, appErr) @@ -197,8 +197,8 @@ func TestAssignFlaggedPostReviewer(t *testing.T) { require.Equal(t, `"`+model.ContentFlaggingStatusAssigned+`"`, string(statusValue.Value)) // Verify reviewer property was created - groupId, appErr := th.App.ContentFlaggingGroupId() - require.Nil(t, appErr) + groupId, err := th.App.ContentFlaggingGroupId() + require.NoError(t, err) mappedFields, appErr := th.App.GetContentFlaggingMappedFields(groupId) require.Nil(t, appErr) @@ -232,8 +232,8 @@ func TestAssignFlaggedPostReviewer(t *testing.T) { require.Equal(t, `"`+model.ContentFlaggingStatusAssigned+`"`, string(statusValue.Value)) // Verify reviewer property was updated - groupId, appErr := th.App.ContentFlaggingGroupId() - require.Nil(t, appErr) + groupId, err := th.App.ContentFlaggingGroupId() + require.NoError(t, err) mappedFields, appErr := th.App.GetContentFlaggingMappedFields(groupId) require.Nil(t, appErr) @@ -276,8 +276,8 @@ func TestAssignFlaggedPostReviewer(t *testing.T) { require.Equal(t, `"`+model.ContentFlaggingStatusAssigned+`"`, string(statusValue.Value)) // Verify reviewer property still exists with correct value - groupId, appErr := th.App.ContentFlaggingGroupId() - require.Nil(t, appErr) + groupId, err := th.App.ContentFlaggingGroupId() + require.NoError(t, err) mappedFields, appErr := th.App.GetContentFlaggingMappedFields(groupId) require.Nil(t, appErr) @@ -306,8 +306,8 @@ func TestAssignFlaggedPostReviewer(t *testing.T) { require.Equal(t, `"`+model.ContentFlaggingStatusAssigned+`"`, string(statusValue.Value)) // Verify reviewer property was created with empty value - groupId, appErr := th.App.ContentFlaggingGroupId() - require.Nil(t, appErr) + groupId, err := th.App.ContentFlaggingGroupId() + require.NoError(t, err) mappedFields, appErr := th.App.GetContentFlaggingMappedFields(groupId) require.Nil(t, appErr) @@ -334,15 +334,15 @@ func TestAssignFlaggedPostReviewer(t *testing.T) { post := setupFlaggedPost(t, th) - groupId, appErr := th.App.ContentFlaggingGroupId() - require.Nil(t, appErr) + groupId, err := th.App.ContentFlaggingGroupId() + require.NoError(t, err) statusValue, appErr := th.App.GetPostContentFlaggingPropertyValue(post.Id, ContentFlaggingPropertyNameStatus) require.Nil(t, appErr) // Set the status to Assigned statusValue.Value = json.RawMessage(fmt.Sprintf(`"%s"`, model.ContentFlaggingStatusAssigned)) - _, err := th.App.PropertyAccessService().UpdatePropertyValue(anonymousCallerId, groupId, statusValue) + _, err = th.App.PropertyAccessService().UpdatePropertyValue(anonymousCallerId, groupId, statusValue) require.NoError(t, err) appErr = th.App.AssignFlaggedPostReviewer(th.Context, post.Id, th.BasicChannel.TeamId, th.BasicUser.Id, th.SystemAdminUser.Id) @@ -992,18 +992,18 @@ func TestCanFlagPost(t *testing.T) { t.Run("should be able to flag post which has not already been flagged", func(t *testing.T) { post := th.CreatePost(t, th.BasicChannel) - groupId, appErr := th.App.ContentFlaggingGroupId() - require.Nil(t, appErr) + groupId, err := th.App.ContentFlaggingGroupId() + require.NoError(t, err) - appErr = th.App.canFlagPost(groupId, post.Id, "en") + appErr := th.App.canFlagPost(groupId, post.Id, "en") require.Nil(t, appErr) }) t.Run("should not be able to flag post which has already been flagged", func(t *testing.T) { post := th.CreatePost(t, th.BasicChannel) - groupId, appErr := th.App.ContentFlaggingGroupId() - require.Nil(t, appErr) + groupId, err := th.App.ContentFlaggingGroupId() + require.NoError(t, err) statusField, err := th.Server.propertyAccessService.GetPropertyFieldByName(anonymousCallerId, groupId, "", ContentFlaggingPropertyNameStatus) require.NoError(t, err) @@ -1018,7 +1018,7 @@ func TestCanFlagPost(t *testing.T) { require.NoError(t, err) // Can't fleg when post already flagged in pending status - appErr = th.App.canFlagPost(groupId, post.Id, "en") + appErr := th.App.canFlagPost(groupId, post.Id, "en") require.NotNil(t, appErr) require.Equal(t, "Cannot quarantine this post as it is already quarantined for review.", appErr.Id) @@ -1079,8 +1079,8 @@ func TestFlagPost(t *testing.T) { require.Nil(t, appErr) // Verify property values were created - groupId, appErr := th.App.ContentFlaggingGroupId() - require.Nil(t, appErr) + groupId, err := th.App.ContentFlaggingGroupId() + require.NoError(t, err) mappedFields, appErr := th.App.GetContentFlaggingMappedFields(groupId) require.Nil(t, appErr) @@ -1264,8 +1264,8 @@ func TestFlagPost(t *testing.T) { require.Nil(t, appErr) // Verify property values were created with empty comment - groupId, appErr := th.App.ContentFlaggingGroupId() - require.Nil(t, appErr) + groupId, err := th.App.ContentFlaggingGroupId() + require.NoError(t, err) mappedFields, appErr := th.App.GetContentFlaggingMappedFields(groupId) require.Nil(t, appErr) @@ -1298,8 +1298,8 @@ func TestFlagPost(t *testing.T) { require.Nil(t, appErr) // Verify reporting time property was set - groupId, appErr := th.App.ContentFlaggingGroupId() - require.Nil(t, appErr) + groupId, err := th.App.ContentFlaggingGroupId() + require.NoError(t, err) mappedFields, appErr := th.App.GetContentFlaggingMappedFields(groupId) require.Nil(t, appErr) @@ -1630,8 +1630,8 @@ func TestGetReviewerPostsForFlaggedPost(t *testing.T) { post := setupFlaggedPost(t, th) - groupId, appErr := th.App.ContentFlaggingGroupId() - require.Nil(t, appErr) + groupId, err := th.App.ContentFlaggingGroupId() + require.NoError(t, err) mappedFields, appErr := th.App.GetContentFlaggingMappedFields(groupId) require.Nil(t, appErr) @@ -1655,8 +1655,8 @@ func TestGetReviewerPostsForFlaggedPost(t *testing.T) { require.Nil(t, setBaseConfig(th)) post := th.CreatePost(t, th.BasicChannel) - groupId, appErr := th.App.ContentFlaggingGroupId() - require.Nil(t, appErr) + groupId, err := th.App.ContentFlaggingGroupId() + require.NoError(t, err) mappedFields, appErr := th.App.GetContentFlaggingMappedFields(groupId) require.Nil(t, appErr) @@ -1689,8 +1689,8 @@ func TestGetReviewerPostsForFlaggedPost(t *testing.T) { // Wait for async reviewer post creation to complete time.Sleep(2 * time.Second) - groupId, appErr := th.App.ContentFlaggingGroupId() - require.Nil(t, appErr) + groupId, err := th.App.ContentFlaggingGroupId() + require.NoError(t, err) mappedFields, appErr := th.App.GetContentFlaggingMappedFields(groupId) require.Nil(t, appErr) @@ -1714,8 +1714,8 @@ func TestGetReviewerPostsForFlaggedPost(t *testing.T) { t.Run("should handle invalid flagged post ID", func(t *testing.T) { require.Nil(t, setBaseConfig(th)) - groupId, appErr := th.App.ContentFlaggingGroupId() - require.Nil(t, appErr) + groupId, err := th.App.ContentFlaggingGroupId() + require.NoError(t, err) mappedFields, appErr := th.App.GetContentFlaggingMappedFields(groupId) require.Nil(t, appErr) @@ -1740,11 +1740,11 @@ func TestPostReviewerMessage(t *testing.T) { post := setupFlaggedPost(t, th) - groupId, appErr := th.App.ContentFlaggingGroupId() - require.Nil(t, appErr) + groupId, err := th.App.ContentFlaggingGroupId() + require.NoError(t, err) testMessage := "Test reviewer message" - _, appErr = th.App.postReviewerMessage(th.Context, testMessage, groupId, post.Id) + _, appErr := th.App.postReviewerMessage(th.Context, testMessage, groupId, post.Id) require.Nil(t, appErr) // Verify message was posted to the reviewer thread @@ -1797,8 +1797,8 @@ func TestPostReviewerMessage(t *testing.T) { // Wait for async reviewer post creation to complete time.Sleep(2 * time.Second) - groupId, appErr := th.App.ContentFlaggingGroupId() - require.Nil(t, appErr) + groupId, err := th.App.ContentFlaggingGroupId() + require.NoError(t, err) testMessage := "Test message for multiple reviewers" _, appErr = th.App.postReviewerMessage(th.Context, testMessage, groupId, post.Id) @@ -1854,11 +1854,11 @@ func TestPostReviewerMessage(t *testing.T) { require.Nil(t, setBaseConfig(th)) post := th.CreatePost(t, th.BasicChannel) - groupId, appErr := th.App.ContentFlaggingGroupId() - require.Nil(t, appErr) + groupId, err := th.App.ContentFlaggingGroupId() + require.NoError(t, err) testMessage := "Test message for non-flagged post" - _, appErr = th.App.postReviewerMessage(th.Context, testMessage, groupId, post.Id) + _, appErr := th.App.postReviewerMessage(th.Context, testMessage, groupId, post.Id) require.Nil(t, appErr) }) @@ -1867,11 +1867,11 @@ func TestPostReviewerMessage(t *testing.T) { post := setupFlaggedPost(t, th) - groupId, appErr := th.App.ContentFlaggingGroupId() - require.Nil(t, appErr) + groupId, err := th.App.ContentFlaggingGroupId() + require.NoError(t, err) testMessage := "Test message with special chars: @user #channel ~team & " - _, appErr = th.App.postReviewerMessage(th.Context, testMessage, groupId, post.Id) + _, appErr := th.App.postReviewerMessage(th.Context, testMessage, groupId, post.Id) require.Nil(t, appErr) // Verify message was posted correctly with special characters preserved @@ -1935,8 +1935,8 @@ func TestSendFlaggedPostRemovalNotification(t *testing.T) { post := setupFlaggedPost(t, th) - groupId, appErr := th.App.ContentFlaggingGroupId() - require.Nil(t, appErr) + groupId, err := th.App.ContentFlaggingGroupId() + require.NoError(t, err) actorComment := "This post violates community guidelines" createdPosts := th.App.sendFlaggedPostRemovalNotification(th.Context, post, th.SystemAdminUser.Id, actorComment, groupId) @@ -2000,8 +2000,8 @@ func TestSendFlaggedPostRemovalNotification(t *testing.T) { }) require.Nil(t, appErr) - groupId, appErr := th.App.ContentFlaggingGroupId() - require.Nil(t, appErr) + groupId, err := th.App.ContentFlaggingGroupId() + require.NoError(t, err) createdPosts := th.App.sendFlaggedPostRemovalNotification(th.Context, post, th.SystemAdminUser.Id, "Test comment", groupId) @@ -2023,8 +2023,8 @@ func TestSendFlaggedPostRemovalNotification(t *testing.T) { post := setupFlaggedPost(t, th) - groupId, appErr := th.App.ContentFlaggingGroupId() - require.Nil(t, appErr) + groupId, err := th.App.ContentFlaggingGroupId() + require.NoError(t, err) createdPosts := th.App.sendFlaggedPostRemovalNotification(th.Context, post, th.SystemAdminUser.Id, "", groupId) @@ -2042,8 +2042,8 @@ func TestSendFlaggedPostRemovalNotification(t *testing.T) { post := setupFlaggedPost(t, th) - groupId, appErr := th.App.ContentFlaggingGroupId() - require.Nil(t, appErr) + groupId, err := th.App.ContentFlaggingGroupId() + require.NoError(t, err) specialComment := "Comment with @mentions #channels ~teams & " createdPosts := th.App.sendFlaggedPostRemovalNotification(th.Context, post, th.SystemAdminUser.Id, specialComment, groupId) @@ -2070,8 +2070,8 @@ func TestSendKeepFlaggedPostNotification(t *testing.T) { post := setupFlaggedPost(t, th) - groupId, appErr := th.App.ContentFlaggingGroupId() - require.Nil(t, appErr) + groupId, err := th.App.ContentFlaggingGroupId() + require.NoError(t, err) actorComment := "This post is acceptable after review" createdPosts := th.App.sendKeepFlaggedPostNotification(th.Context, post, th.SystemAdminUser.Id, actorComment, groupId) @@ -2129,8 +2129,8 @@ func TestSendKeepFlaggedPostNotification(t *testing.T) { post := setupFlaggedPost(t, th) - groupId, appErr := th.App.ContentFlaggingGroupId() - require.Nil(t, appErr) + groupId, err := th.App.ContentFlaggingGroupId() + require.NoError(t, err) comment := "Test comment" createdPosts := th.App.sendKeepFlaggedPostNotification(th.Context, post, th.SystemAdminUser.Id, comment, groupId) @@ -2153,8 +2153,8 @@ func TestSendKeepFlaggedPostNotification(t *testing.T) { post := setupFlaggedPost(t, th) - groupId, appErr := th.App.ContentFlaggingGroupId() - require.Nil(t, appErr) + groupId, err := th.App.ContentFlaggingGroupId() + require.NoError(t, err) createdPosts := th.App.sendKeepFlaggedPostNotification(th.Context, post, th.SystemAdminUser.Id, "", groupId) @@ -2172,8 +2172,8 @@ func TestSendKeepFlaggedPostNotification(t *testing.T) { post := setupFlaggedPost(t, th) - groupId, appErr := th.App.ContentFlaggingGroupId() - require.Nil(t, appErr) + groupId, err := th.App.ContentFlaggingGroupId() + require.NoError(t, err) specialComment := "Comment with @mentions #channels ~teams & " createdPosts := th.App.sendKeepFlaggedPostNotification(th.Context, post, th.SystemAdminUser.Id, specialComment, groupId) @@ -2192,8 +2192,8 @@ func TestSendKeepFlaggedPostNotification(t *testing.T) { post := setupFlaggedPost(t, th) - groupId, appErr := th.App.ContentFlaggingGroupId() - require.Nil(t, appErr) + groupId, err := th.App.ContentFlaggingGroupId() + require.NoError(t, err) // Use BasicUser as actor instead of SystemAdminUser createdPosts := th.App.sendKeepFlaggedPostNotification(th.Context, post, th.BasicUser.Id, "Reviewed by different user", groupId) @@ -2233,8 +2233,8 @@ func TestPermanentDeleteFlaggedPost(t *testing.T) { require.Equal(t, `"`+model.ContentFlaggingStatusRemoved+`"`, string(statusValue.Value)) // Verify actor properties were created - groupId, appErr := th.App.ContentFlaggingGroupId() - require.Nil(t, appErr) + groupId, err := th.App.ContentFlaggingGroupId() + require.NoError(t, err) mappedFields, appErr := th.App.GetContentFlaggingMappedFields(groupId) require.Nil(t, appErr) @@ -2319,14 +2319,14 @@ func TestPermanentDeleteFlaggedPost(t *testing.T) { post := setupFlaggedPost(t, th) // Set status to removed - groupId, appErr := th.App.ContentFlaggingGroupId() - require.Nil(t, appErr) + groupId, err := th.App.ContentFlaggingGroupId() + require.NoError(t, err) statusValue, appErr := th.App.GetPostContentFlaggingPropertyValue(post.Id, ContentFlaggingPropertyNameStatus) require.Nil(t, appErr) statusValue.Value = json.RawMessage(fmt.Sprintf(`"%s"`, model.ContentFlaggingStatusRemoved)) - _, err := th.App.PropertyAccessService().UpdatePropertyValue(anonymousCallerId, groupId, statusValue) + _, err = th.App.PropertyAccessService().UpdatePropertyValue(anonymousCallerId, groupId, statusValue) require.NoError(t, err) actionRequest := &model.FlagContentActionRequest{ @@ -2343,14 +2343,14 @@ func TestPermanentDeleteFlaggedPost(t *testing.T) { post := setupFlaggedPost(t, th) // Set status to retained - groupId, appErr := th.App.ContentFlaggingGroupId() - require.Nil(t, appErr) + groupId, err := th.App.ContentFlaggingGroupId() + require.NoError(t, err) statusValue, appErr := th.App.GetPostContentFlaggingPropertyValue(post.Id, ContentFlaggingPropertyNameStatus) require.Nil(t, appErr) statusValue.Value = json.RawMessage(fmt.Sprintf(`"%s"`, model.ContentFlaggingStatusRetained)) - _, err := th.App.PropertyAccessService().UpdatePropertyValue(anonymousCallerId, groupId, statusValue) + _, err = th.App.PropertyAccessService().UpdatePropertyValue(anonymousCallerId, groupId, statusValue) require.NoError(t, err) actionRequest := &model.FlagContentActionRequest{ @@ -2386,8 +2386,8 @@ func TestPermanentDeleteFlaggedPost(t *testing.T) { require.Nil(t, appErr) // Verify empty comment was stored - groupId, appErr := th.App.ContentFlaggingGroupId() - require.Nil(t, appErr) + groupId, err := th.App.ContentFlaggingGroupId() + require.NoError(t, err) mappedFields, appErr := th.App.GetContentFlaggingMappedFields(groupId) require.Nil(t, appErr) @@ -2414,8 +2414,8 @@ func TestPermanentDeleteFlaggedPost(t *testing.T) { require.Nil(t, appErr) // Verify special characters were properly escaped and stored - groupId, appErr := th.App.ContentFlaggingGroupId() - require.Nil(t, appErr) + groupId, err := th.App.ContentFlaggingGroupId() + require.NoError(t, err) mappedFields, appErr := th.App.GetContentFlaggingMappedFields(groupId) require.Nil(t, appErr) @@ -2704,14 +2704,14 @@ func TestKeepFlaggedPost(t *testing.T) { post := setupFlaggedPost(t, th) // Set status to removed - groupId, appErr := th.App.ContentFlaggingGroupId() - require.Nil(t, appErr) + groupId, err := th.App.ContentFlaggingGroupId() + require.NoError(t, err) statusValue, appErr := th.App.GetPostContentFlaggingPropertyValue(post.Id, ContentFlaggingPropertyNameStatus) require.Nil(t, appErr) statusValue.Value = json.RawMessage(fmt.Sprintf(`"%s"`, model.ContentFlaggingStatusRemoved)) - _, err := th.App.Srv().propertyAccessService.UpdatePropertyValue("", groupId, statusValue) + _, err = th.Server.propertyAccessService.UpdatePropertyValue("", groupId, statusValue) require.NoError(t, err) actionRequest := &model.FlagContentActionRequest{ @@ -2728,14 +2728,14 @@ func TestKeepFlaggedPost(t *testing.T) { post := setupFlaggedPost(t, th) // Set status to retained - groupId, appErr := th.App.ContentFlaggingGroupId() - require.Nil(t, appErr) + groupId, err := th.App.ContentFlaggingGroupId() + require.NoError(t, err) statusValue, appErr := th.App.GetPostContentFlaggingPropertyValue(post.Id, ContentFlaggingPropertyNameStatus) require.Nil(t, appErr) statusValue.Value = json.RawMessage(fmt.Sprintf(`"%s"`, model.ContentFlaggingStatusRetained)) - _, err := th.App.Srv().propertyAccessService.UpdatePropertyValue("", groupId, statusValue) + _, err = th.Server.propertyAccessService.UpdatePropertyValue("", groupId, statusValue) require.NoError(t, err) actionRequest := &model.FlagContentActionRequest{ diff --git a/server/channels/app/custom_profile_attributes.go b/server/channels/app/custom_profile_attributes.go index 82d69f5bb73..a579c26eed3 100644 --- a/server/channels/app/custom_profile_attributes.go +++ b/server/channels/app/custom_profile_attributes.go @@ -6,6 +6,7 @@ package app import ( "database/sql" "encoding/json" + "fmt" "net/http" "sort" @@ -19,26 +20,13 @@ const ( CustomProfileAttributesFieldLimit = 20 ) -var cpaGroupID string - func (a *App) CpaGroupID() (string, error) { - return getCpaGroupID(a.Srv().propertyAccessService) -} - -// ToDo: we should explore moving this to the database cache layer -// instead of maintaining the ID cached at the application level -func getCpaGroupID(service *PropertyAccessService) (string, error) { - if cpaGroupID != "" { - return cpaGroupID, nil - } - - cpaGroup, err := service.RegisterPropertyGroup(model.CustomProfileAttributesPropertyGroupName) + group, err := a.Srv().propertyAccessService.Group(model.CustomProfileAttributesPropertyGroupName) if err != nil { - return "", errors.Wrap(err, "cannot register Custom Profile Attributes property group") + return "", fmt.Errorf("cannot retrieve custom profile attributes property group ID: %w", err) } - cpaGroupID = cpaGroup.ID - return cpaGroupID, nil + return group.ID, nil } func (a *App) GetCPAField(callerId, fieldID string) (*model.CPAField, *model.AppError) { diff --git a/server/channels/app/properties/property_group.go b/server/channels/app/properties/property_group.go index b9fa938ee7b..1843fb5793b 100644 --- a/server/channels/app/properties/property_group.go +++ b/server/channels/app/properties/property_group.go @@ -4,11 +4,48 @@ package properties import ( + "fmt" + "github.com/mattermost/mattermost/server/public/model" ) +// RegisterBuiltinGroups registers a set of property groups at startup +// and populates the cache so that subsequent Group calls are free of +// database round-trips. +func (ps *PropertyService) RegisterBuiltinGroups(groups []*model.PropertyGroup) error { + for _, group := range groups { + if _, err := ps.RegisterPropertyGroup(group.Name); err != nil { + return fmt.Errorf("failed to register builtin property group %q: %w", group.Name, err) + } + } + return nil +} + +// Group returns the cached PropertyGroup for a given name. If the +// group is not in the cache (e.g. a plugin-registered group), it +// falls back to a database lookup and caches the result. +func (ps *PropertyService) Group(name string) (*model.PropertyGroup, error) { + if cached, ok := ps.groupCache.Load(name); ok { + return cached.(*model.PropertyGroup), nil + } + + group, err := ps.groupStore.Get(name) + if err != nil { + return nil, fmt.Errorf("property group %q not found: %w", name, err) + } + + ps.groupCache.Store(name, group) + return group, nil +} + func (ps *PropertyService) RegisterPropertyGroup(name string) (*model.PropertyGroup, error) { - return ps.groupStore.Register(name) + group, err := ps.groupStore.Register(name) + if err != nil { + return nil, err + } + + ps.groupCache.Store(name, group) + return group, nil } func (ps *PropertyService) GetPropertyGroup(name string) (*model.PropertyGroup, error) { diff --git a/server/channels/app/properties/service.go b/server/channels/app/properties/service.go index f736d4efb11..11472fe5503 100644 --- a/server/channels/app/properties/service.go +++ b/server/channels/app/properties/service.go @@ -5,6 +5,7 @@ package properties import ( "errors" + "sync" "github.com/mattermost/mattermost/server/v8/channels/store" ) @@ -13,6 +14,7 @@ type PropertyService struct { groupStore store.PropertyGroupStore fieldStore store.PropertyFieldStore valueStore store.PropertyValueStore + groupCache sync.Map // name -> *model.PropertyGroup } type ServiceConfig struct { diff --git a/server/channels/app/property_access.go b/server/channels/app/property_access.go index 2d88a2875f1..80c3d48ab8d 100644 --- a/server/channels/app/property_access.go +++ b/server/channels/app/property_access.go @@ -75,6 +75,12 @@ func (pas *PropertyAccessService) GetPropertyGroup(name string) (*model.Property return pas.propertyService.GetPropertyGroup(name) } +// Group returns the cached PropertyGroup for a given name. If the +// group is not in the cache, it falls back to a database lookup. +func (pas *PropertyAccessService) Group(name string) (*model.PropertyGroup, error) { + return pas.propertyService.Group(name) +} + // Property Field Methods // CreatePropertyField creates a new property field. @@ -1300,9 +1306,9 @@ func (pas *PropertyAccessService) applyValueReadAccessControl(values []*model.Pr } func (pas *PropertyAccessService) groupHasAccessRestrictions(groupId string) (bool, error) { - cpaID, err := getCpaGroupID(pas) + cpaGroup, err := pas.Group(model.CustomProfileAttributesPropertyGroupName) if err != nil { return false, err } - return groupId == cpaID, nil + return groupId == cpaGroup.ID, nil } diff --git a/server/channels/app/property_access_test.go b/server/channels/app/property_access_test.go index 13df22219e5..15741df536e 100644 --- a/server/channels/app/property_access_test.go +++ b/server/channels/app/property_access_test.go @@ -18,9 +18,8 @@ func TestGetPropertyFieldReadAccess(t *testing.T) { pas := th.App.PropertyAccessService() // Register the CPA group - group, err := pas.RegisterPropertyGroup("cpa") + group, err := pas.RegisterPropertyGroup(model.CustomProfileAttributesPropertyGroupName) require.NoError(t, err) - cpaGroupID = group.ID pluginID1 := "plugin-1" pluginID2 := "plugin-2" @@ -371,9 +370,8 @@ func TestGetPropertyFieldsReadAccess(t *testing.T) { pas := th.App.PropertyAccessService() // Register the CPA group - group, err := pas.RegisterPropertyGroup("cpa") + group, err := pas.RegisterPropertyGroup(model.CustomProfileAttributesPropertyGroupName) require.NoError(t, err) - cpaGroupID = group.ID pluginID := "plugin-1" userID := model.NewId() @@ -474,9 +472,8 @@ func TestSearchPropertyFieldsReadAccess(t *testing.T) { pas := th.App.PropertyAccessService() // Register the CPA group - group, err := pas.RegisterPropertyGroup("cpa") + group, err := pas.RegisterPropertyGroup(model.CustomProfileAttributesPropertyGroupName) require.NoError(t, err) - cpaGroupID = group.ID pluginID := "plugin-1" userID := model.NewId() @@ -581,9 +578,8 @@ func TestGetPropertyFieldByNameReadAccess(t *testing.T) { pas := th.App.PropertyAccessService() // Register the CPA group - group, err := pas.RegisterPropertyGroup("cpa") + group, err := pas.RegisterPropertyGroup(model.CustomProfileAttributesPropertyGroupName) require.NoError(t, err) - cpaGroupID = group.ID pluginID := "plugin-1" userID := model.NewId() @@ -626,11 +622,9 @@ func TestCreatePropertyField_SourcePluginIDValidation(t *testing.T) { pas := th.App.PropertyAccessService() // Register the CPA group - group, err := pas.RegisterPropertyGroup("cpa") + group, err := pas.RegisterPropertyGroup(model.CustomProfileAttributesPropertyGroupName) require.NoError(t, err) - cpaGroupID = group.ID - - groupID := cpaGroupID + groupID := group.ID t.Run("allows field creation without source_plugin_id", func(t *testing.T) { field := &model.PropertyField{ @@ -778,11 +772,9 @@ func TestCreatePropertyFieldForPlugin(t *testing.T) { pas := th.App.PropertyAccessService() // Register the CPA group - group, err := pas.RegisterPropertyGroup("cpa") + group, err := pas.RegisterPropertyGroup(model.CustomProfileAttributesPropertyGroupName) require.NoError(t, err) - cpaGroupID = group.ID - - groupID := cpaGroupID + groupID := group.ID t.Run("automatically sets source_plugin_id", func(t *testing.T) { field := &model.PropertyField{ @@ -858,11 +850,9 @@ func TestUpdatePropertyField_WriteAccessControl(t *testing.T) { pas := th.App.PropertyAccessService() // Register the CPA group - group, err := pas.RegisterPropertyGroup("cpa") + group, err := pas.RegisterPropertyGroup(model.CustomProfileAttributesPropertyGroupName) require.NoError(t, err) - cpaGroupID = group.ID - - groupID := cpaGroupID + groupID := group.ID t.Run("allows update of unprotected field", func(t *testing.T) { field := &model.PropertyField{ @@ -1039,11 +1029,9 @@ func TestUpdatePropertyFields_BulkWriteAccessControl(t *testing.T) { pas := th.App.PropertyAccessService() // Register the CPA group - group, err := pas.RegisterPropertyGroup("cpa") + group, err := pas.RegisterPropertyGroup(model.CustomProfileAttributesPropertyGroupName) require.NoError(t, err) - cpaGroupID = group.ID - - groupID := cpaGroupID + groupID := group.ID t.Run("allows bulk update of unprotected fields", func(t *testing.T) { field1 := &model.PropertyField{GroupID: groupID, Name: "Field1", Type: model.PropertyFieldTypeText} @@ -1137,11 +1125,9 @@ func TestDeletePropertyField_WriteAccessControl(t *testing.T) { pas := th.App.PropertyAccessService() // Register the CPA group - group, err := pas.RegisterPropertyGroup("cpa") + group, err := pas.RegisterPropertyGroup(model.CustomProfileAttributesPropertyGroupName) require.NoError(t, err) - cpaGroupID = group.ID - - groupID := cpaGroupID + groupID := group.ID t.Run("allows deletion of unprotected field", func(t *testing.T) { field := &model.PropertyField{GroupID: groupID, Name: "Unprotected", Type: model.PropertyFieldTypeText} @@ -1219,11 +1205,9 @@ func TestCreatePropertyValue_WriteAccessControl(t *testing.T) { pas := th.App.PropertyAccessService() // Register the CPA group - group, err := pas.RegisterPropertyGroup("cpa") + group, err := pas.RegisterPropertyGroup(model.CustomProfileAttributesPropertyGroupName) require.NoError(t, err) - cpaGroupID = group.ID - - groupID := cpaGroupID + groupID := group.ID t.Run("allows creating value for public field", func(t *testing.T) { field := &model.PropertyField{GroupID: groupID, Name: "Public", Type: model.PropertyFieldTypeText} @@ -1334,11 +1318,9 @@ func TestDeletePropertyValue_WriteAccessControl(t *testing.T) { pas := th.App.PropertyAccessService() // Register the CPA group - group, err := pas.RegisterPropertyGroup("cpa") + group, err := pas.RegisterPropertyGroup(model.CustomProfileAttributesPropertyGroupName) require.NoError(t, err) - cpaGroupID = group.ID - - groupID := cpaGroupID + groupID := group.ID t.Run("allows deleting value for public field", func(t *testing.T) { field := &model.PropertyField{GroupID: groupID, Name: "Public", Type: model.PropertyFieldTypeText} @@ -1394,11 +1376,9 @@ func TestDeletePropertyValuesForTarget_WriteAccessControl(t *testing.T) { pas := th.App.PropertyAccessService() // Register the CPA group - group, err := pas.RegisterPropertyGroup("cpa") + group, err := pas.RegisterPropertyGroup(model.CustomProfileAttributesPropertyGroupName) require.NoError(t, err) - cpaGroupID = group.ID - - groupID := cpaGroupID + groupID := group.ID t.Run("allows deleting all values when caller has write access to all fields", func(t *testing.T) { field1 := &model.PropertyField{GroupID: groupID, Name: "Field1", Type: model.PropertyFieldTypeText} @@ -1470,9 +1450,8 @@ func TestGetPropertyValueReadAccess(t *testing.T) { pas := th.App.PropertyAccessService() // Register the CPA group - group, rerr := pas.RegisterPropertyGroup("cpa") + group, rerr := pas.RegisterPropertyGroup(model.CustomProfileAttributesPropertyGroupName) require.NoError(t, rerr) - cpaGroupID = group.ID pluginID1 := "plugin-1" pluginID2 := "plugin-2" @@ -1857,9 +1836,8 @@ func TestGetPropertyValuesReadAccess(t *testing.T) { pas := th.App.PropertyAccessService() // Register the CPA group - group, rerr := pas.RegisterPropertyGroup("cpa") + group, rerr := pas.RegisterPropertyGroup(model.CustomProfileAttributesPropertyGroupName) require.NoError(t, rerr) - cpaGroupID = group.ID pluginID1 := "plugin-1" pluginID2 := "plugin-2" @@ -1943,9 +1921,8 @@ func TestSearchPropertyValuesReadAccess(t *testing.T) { pas := th.App.PropertyAccessService() // Register the CPA group - group, rerr := pas.RegisterPropertyGroup("cpa") + group, rerr := pas.RegisterPropertyGroup(model.CustomProfileAttributesPropertyGroupName) require.NoError(t, rerr) - cpaGroupID = group.ID pluginID1 := "plugin-1" pluginID2 := "plugin-2" @@ -2089,9 +2066,8 @@ func TestCreatePropertyValues_WriteAccessControl(t *testing.T) { pas := th.App.PropertyAccessService() // Register the CPA group - group, err := pas.RegisterPropertyGroup("cpa") + group, err := pas.RegisterPropertyGroup(model.CustomProfileAttributesPropertyGroupName) require.NoError(t, err) - cpaGroupID = group.ID pluginID1 := "plugin-1" pluginID2 := "plugin-2" diff --git a/server/channels/app/server.go b/server/channels/app/server.go index bb6abb9386c..4a78ac35dcf 100644 --- a/server/channels/app/server.go +++ b/server/channels/app/server.go @@ -243,6 +243,13 @@ func NewServer(options ...Option) (*Server, error) { return nil, errors.Wrapf(err, "unable to create properties service") } + if err = propertyService.RegisterBuiltinGroups([]*model.PropertyGroup{ + {Name: model.CustomProfileAttributesPropertyGroupName}, + {Name: model.ContentFlaggingGroupName}, + }); err != nil { + return nil, errors.Wrap(err, "failed to register builtin property groups") + } + // Wrap PropertyService with access control layer to enforce caller-based permissions s.propertyAccessService = NewPropertyAccessService(propertyService, func(pluginID string) bool { _, err := s.ch.GetPluginStatus(pluginID) diff --git a/server/i18n/en.json b/server/i18n/en.json index a64e3266e1d..490be82721f 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -5528,7 +5528,7 @@ }, { "id": "app.custom_profile_attributes.cpa_group_id.app_error", - "translation": "Cannot register Custom Profile Attributes property group" + "translation": "Unable to retrieve the custom profile attributes property group." }, { "id": "app.custom_profile_attributes.create_property_field.app_error", @@ -5640,7 +5640,7 @@ }, { "id": "app.data_spillage.get_group.error", - "translation": "Failed to get Content Flagging bot." + "translation": "Unable to retrieve the content flagging property group." }, { "id": "app.data_spillage.get_reviewer_settings.app_error",