diff --git a/server/channels/api4/custom_profile_attributes.go b/server/channels/api4/custom_profile_attributes.go index 9817a4f2a6..c46f0bd82d 100644 --- a/server/channels/api4/custom_profile_attributes.go +++ b/server/channels/api4/custom_profile_attributes.go @@ -61,7 +61,7 @@ func createCPAField(c *Context, w http.ResponseWriter, r *http.Request) { return } - pf.SanitizeInput() + pf.Name = strings.TrimSpace(pf.Name) auditRec := c.MakeAuditRecord("createCPAField", audit.Fail) defer c.LogAuditRec(auditRec) @@ -106,7 +106,17 @@ func patchCPAField(c *Context, w http.ResponseWriter, r *http.Request) { return } - patch.SanitizeInput() + if patch.Name != nil { + *patch.Name = strings.TrimSpace(*patch.Name) + } + if err := patch.IsValid(); err != nil { + if appErr, ok := err.(*model.AppError); ok { + c.Err = appErr + } else { + c.Err = model.NewAppError("createCPAField", "api.custom_profile_attributes.invalid_field_patch", nil, "", http.StatusBadRequest) + } + return + } auditRec := c.MakeAuditRecord("patchCPAField", audit.Fail) defer c.LogAuditRec(auditRec) diff --git a/server/channels/app/custom_profile_attributes.go b/server/channels/app/custom_profile_attributes.go index 3c290fe621..997465dd7c 100644 --- a/server/channels/app/custom_profile_attributes.go +++ b/server/channels/app/custom_profile_attributes.go @@ -137,7 +137,9 @@ func (a *App) PatchCPAField(fieldID string, patch *model.PropertyFieldPatch) (*m return nil, appErr } - patchedField, err := a.Srv().propertyService.UpdatePropertyField(cpaField.ToPropertyField()) + // we've already ensured that the field exists for the CPA group, + // we don't need to specify the groupID for the update + patchedField, err := a.Srv().propertyService.UpdatePropertyField("", cpaField.ToPropertyField()) if err != nil { var nfErr *store.ErrNotFound switch { @@ -161,16 +163,7 @@ func (a *App) DeleteCPAField(id string) *model.AppError { return model.NewAppError("DeleteCPAField", "app.custom_profile_attributes.cpa_group_id.app_error", nil, "", http.StatusInternalServerError).Wrap(err) } - if _, err := a.Srv().propertyService.GetPropertyField(groupID, id); err != nil { - switch { - case errors.Is(err, sql.ErrNoRows): - return model.NewAppError("DeleteCPAField", "app.custom_profile_attributes.property_field_not_found.app_error", nil, "", http.StatusNotFound) - default: - return model.NewAppError("DeleteCPAField", "app.custom_profile_attributes.get_property_field.app_error", nil, "", http.StatusInternalServerError).Wrap(err) - } - } - - if err := a.Srv().propertyService.DeletePropertyField(id); err != nil { + if err := a.Srv().propertyService.DeletePropertyField(groupID, id); err != nil { var nfErr *store.ErrNotFound switch { case errors.As(err, &nfErr): diff --git a/server/channels/app/custom_profile_attributes_test.go b/server/channels/app/custom_profile_attributes_test.go index 9cec3a1a5b..0910622f32 100644 --- a/server/channels/app/custom_profile_attributes_test.go +++ b/server/channels/app/custom_profile_attributes_test.go @@ -390,9 +390,9 @@ func TestDeleteCPAField(t *testing.T) { } t.Run("should fail if the field doesn't exist", func(t *testing.T) { - appErr := th.App.DeleteCPAField(model.NewId()) - require.NotNil(t, appErr) - require.Equal(t, "app.custom_profile_attributes.property_field_not_found.app_error", appErr.Id) + err := th.App.DeleteCPAField(model.NewId()) + require.NotNil(t, err) + require.Equal(t, "app.custom_profile_attributes.property_field_delete.app_error", err.Id) }) t.Run("should not allow to delete a field outside of CPA", func(t *testing.T) { @@ -406,7 +406,7 @@ func TestDeleteCPAField(t *testing.T) { dErr := th.App.DeleteCPAField(field.ID) require.NotNil(t, dErr) - require.Equal(t, "app.custom_profile_attributes.property_field_not_found.app_error", dErr.Id) + require.Equal(t, "app.custom_profile_attributes.property_field_delete.app_error", dErr.Id) }) t.Run("should correctly delete the field", func(t *testing.T) { @@ -622,7 +622,7 @@ func TestPatchCPAValue(t *testing.T) { } createdField, err := th.App.Srv().propertyService.CreatePropertyField(newField) require.NoError(t, err) - err = th.App.Srv().propertyService.DeletePropertyField(createdField.ID) + err = th.App.Srv().propertyService.DeletePropertyField(cpaGroupID, createdField.ID) require.NoError(t, err) userID := model.NewId() diff --git a/server/channels/app/properties/property_field.go b/server/channels/app/properties/property_field.go index 322f53877b..ddf9387642 100644 --- a/server/channels/app/properties/property_field.go +++ b/server/channels/app/properties/property_field.go @@ -4,6 +4,8 @@ package properties import ( + "fmt" + "github.com/mattermost/mattermost/server/public/model" ) @@ -32,8 +34,8 @@ func (ps *PropertyService) SearchPropertyFields(groupID, targetID string, opts m return ps.fieldStore.SearchPropertyFields(opts) } -func (ps *PropertyService) UpdatePropertyField(field *model.PropertyField) (*model.PropertyField, error) { - fields, err := ps.UpdatePropertyFields([]*model.PropertyField{field}) +func (ps *PropertyService) UpdatePropertyField(groupID string, field *model.PropertyField) (*model.PropertyField, error) { + fields, err := ps.UpdatePropertyFields(groupID, []*model.PropertyField{field}) if err != nil { return nil, err } @@ -41,13 +43,21 @@ func (ps *PropertyService) UpdatePropertyField(field *model.PropertyField) (*mod return fields[0], nil } -func (ps *PropertyService) UpdatePropertyFields(fields []*model.PropertyField) ([]*model.PropertyField, error) { - return ps.fieldStore.Update(fields) +func (ps *PropertyService) UpdatePropertyFields(groupID string, fields []*model.PropertyField) ([]*model.PropertyField, error) { + return ps.fieldStore.Update(groupID, fields) } -func (ps *PropertyService) DeletePropertyField(id string) error { +func (ps *PropertyService) DeletePropertyField(groupID, id string) error { + // if groupID is not empty, we need to check first that the field belongs to the group + if groupID != "" { + if _, err := ps.GetPropertyField(groupID, id); err != nil { + return fmt.Errorf("error getting property field %q for group %q: %w", id, groupID, err) + } + } + if err := ps.valueStore.DeleteForField(id); err != nil { return err } - return ps.fieldStore.Delete(id) + + return ps.fieldStore.Delete(groupID, id) } diff --git a/server/channels/app/properties/property_value.go b/server/channels/app/properties/property_value.go index 11b65743d0..2a8775f572 100644 --- a/server/channels/app/properties/property_value.go +++ b/server/channels/app/properties/property_value.go @@ -27,8 +27,8 @@ func (ps *PropertyService) SearchPropertyValues(groupID, targetID string, opts m return ps.valueStore.SearchPropertyValues(opts) } -func (ps *PropertyService) UpdatePropertyValue(value *model.PropertyValue) (*model.PropertyValue, error) { - values, err := ps.UpdatePropertyValues([]*model.PropertyValue{value}) +func (ps *PropertyService) UpdatePropertyValue(groupID string, value *model.PropertyValue) (*model.PropertyValue, error) { + values, err := ps.UpdatePropertyValues(groupID, []*model.PropertyValue{value}) if err != nil { return nil, err } @@ -36,8 +36,8 @@ func (ps *PropertyService) UpdatePropertyValue(value *model.PropertyValue) (*mod return values[0], nil } -func (ps *PropertyService) UpdatePropertyValues(values []*model.PropertyValue) ([]*model.PropertyValue, error) { - return ps.valueStore.Update(values) +func (ps *PropertyService) UpdatePropertyValues(groupID string, values []*model.PropertyValue) ([]*model.PropertyValue, error) { + return ps.valueStore.Update(groupID, values) } func (ps *PropertyService) UpsertPropertyValue(value *model.PropertyValue) (*model.PropertyValue, error) { @@ -53,6 +53,6 @@ func (ps *PropertyService) UpsertPropertyValues(values []*model.PropertyValue) ( return ps.valueStore.Upsert(values) } -func (ps *PropertyService) DeletePropertyValue(id string) error { - return ps.valueStore.Delete(id) +func (ps *PropertyService) DeletePropertyValue(groupID, id string) error { + return ps.valueStore.Delete(groupID, id) } diff --git a/server/channels/store/retrylayer/retrylayer.go b/server/channels/store/retrylayer/retrylayer.go index de877740fa..be4cb0441d 100644 --- a/server/channels/store/retrylayer/retrylayer.go +++ b/server/channels/store/retrylayer/retrylayer.go @@ -9012,11 +9012,11 @@ func (s *RetryLayerPropertyFieldStore) Create(field *model.PropertyField) (*mode } -func (s *RetryLayerPropertyFieldStore) Delete(id string) error { +func (s *RetryLayerPropertyFieldStore) Delete(groupID string, id string) error { tries := 0 for { - err := s.PropertyFieldStore.Delete(id) + err := s.PropertyFieldStore.Delete(groupID, id) if err == nil { return nil } @@ -9096,11 +9096,11 @@ func (s *RetryLayerPropertyFieldStore) SearchPropertyFields(opts model.PropertyF } -func (s *RetryLayerPropertyFieldStore) Update(fields []*model.PropertyField) ([]*model.PropertyField, error) { +func (s *RetryLayerPropertyFieldStore) Update(groupID string, fields []*model.PropertyField) ([]*model.PropertyField, error) { tries := 0 for { - result, err := s.PropertyFieldStore.Update(fields) + result, err := s.PropertyFieldStore.Update(groupID, fields) if err == nil { return result, nil } @@ -9180,11 +9180,11 @@ func (s *RetryLayerPropertyValueStore) Create(value *model.PropertyValue) (*mode } -func (s *RetryLayerPropertyValueStore) Delete(id string) error { +func (s *RetryLayerPropertyValueStore) Delete(groupID string, id string) error { tries := 0 for { - err := s.PropertyValueStore.Delete(id) + err := s.PropertyValueStore.Delete(groupID, id) if err == nil { return nil } @@ -9285,11 +9285,11 @@ func (s *RetryLayerPropertyValueStore) SearchPropertyValues(opts model.PropertyV } -func (s *RetryLayerPropertyValueStore) Update(values []*model.PropertyValue) ([]*model.PropertyValue, error) { +func (s *RetryLayerPropertyValueStore) Update(groupID string, values []*model.PropertyValue) ([]*model.PropertyValue, error) { tries := 0 for { - result, err := s.PropertyValueStore.Update(values) + result, err := s.PropertyValueStore.Update(groupID, values) if err == nil { return result, nil } diff --git a/server/channels/store/sqlstore/property_field_store.go b/server/channels/store/sqlstore/property_field_store.go index 85f07c3ce6..27a294f42b 100644 --- a/server/channels/store/sqlstore/property_field_store.go +++ b/server/channels/store/sqlstore/property_field_store.go @@ -150,7 +150,7 @@ func (s *SqlPropertyFieldStore) SearchPropertyFields(opts model.PropertyFieldSea return fields, nil } -func (s *SqlPropertyFieldStore) Update(fields []*model.PropertyField) (_ []*model.PropertyField, err error) { +func (s *SqlPropertyFieldStore) Update(groupID string, fields []*model.PropertyField) (_ []*model.PropertyField, err error) { if len(fields) == 0 { return nil, nil } @@ -207,6 +207,10 @@ func (s *SqlPropertyFieldStore) Update(fields []*model.PropertyField) (_ []*mode Set("DeleteAt", deleteAtCase). Where(sq.Eq{"id": ids}) + if groupID != "" { + builder = builder.Where(sq.Eq{"GroupID": groupID}) + } + result, err := transaction.ExecBuilder(builder) if err != nil { return nil, errors.Wrap(err, "property_field_update_exec") @@ -227,12 +231,16 @@ func (s *SqlPropertyFieldStore) Update(fields []*model.PropertyField) (_ []*mode return fields, nil } -func (s *SqlPropertyFieldStore) Delete(id string) error { +func (s *SqlPropertyFieldStore) Delete(groupID string, id string) error { builder := s.getQueryBuilder(). Update("PropertyFields"). Set("DeleteAt", model.GetMillis()). Where(sq.Eq{"id": id}) + if groupID != "" { + builder = builder.Where(sq.Eq{"GroupID": groupID}) + } + result, err := s.GetMaster().ExecBuilder(builder) if err != nil { return errors.Wrapf(err, "failed to delete property field with id: %s", id) diff --git a/server/channels/store/sqlstore/property_value_store.go b/server/channels/store/sqlstore/property_value_store.go index a06b2d3547..30cc286409 100644 --- a/server/channels/store/sqlstore/property_value_store.go +++ b/server/channels/store/sqlstore/property_value_store.go @@ -141,7 +141,7 @@ func (s *SqlPropertyValueStore) SearchPropertyValues(opts model.PropertyValueSea return values, nil } -func (s *SqlPropertyValueStore) Update(values []*model.PropertyValue) (_ []*model.PropertyValue, err error) { +func (s *SqlPropertyValueStore) Update(groupID string, values []*model.PropertyValue) (_ []*model.PropertyValue, err error) { if len(values) == 0 { return nil, nil } @@ -186,6 +186,10 @@ func (s *SqlPropertyValueStore) Update(values []*model.PropertyValue) (_ []*mode Set("UpdateAt", updateTime). Where(sq.Eq{"id": ids}) + if groupID != "" { + builder = builder.Where(sq.Eq{"GroupID": groupID}) + } + result, err := transaction.ExecBuilder(builder) if err != nil { return nil, errors.Wrap(err, "property_value_update_exec") @@ -297,12 +301,16 @@ func (s *SqlPropertyValueStore) Upsert(values []*model.PropertyValue) (_ []*mode return updatedValues, nil } -func (s *SqlPropertyValueStore) Delete(id string) error { +func (s *SqlPropertyValueStore) Delete(groupID string, id string) error { builder := s.getQueryBuilder(). Update("PropertyValues"). Set("DeleteAt", model.GetMillis()). Where(sq.Eq{"id": id}) + if groupID != "" { + builder = builder.Where(sq.Eq{"GroupID": groupID}) + } + result, err := s.GetMaster().ExecBuilder(builder) if err != nil { return errors.Wrapf(err, "failed to delete property value with id: %s", id) diff --git a/server/channels/store/store.go b/server/channels/store/store.go index 5f7ebae115..7dd9560591 100644 --- a/server/channels/store/store.go +++ b/server/channels/store/store.go @@ -1092,8 +1092,8 @@ type PropertyFieldStore interface { GetMany(groupID string, ids []string) ([]*model.PropertyField, error) CountForGroup(groupID string, includeDeleted bool) (int64, error) SearchPropertyFields(opts model.PropertyFieldSearchOpts) ([]*model.PropertyField, error) - Update(fields []*model.PropertyField) ([]*model.PropertyField, error) - Delete(id string) error + Update(groupID string, fields []*model.PropertyField) ([]*model.PropertyField, error) + Delete(groupID string, id string) error } type PropertyValueStore interface { @@ -1101,9 +1101,9 @@ type PropertyValueStore interface { Get(groupID, id string) (*model.PropertyValue, error) GetMany(groupID string, ids []string) ([]*model.PropertyValue, error) SearchPropertyValues(opts model.PropertyValueSearchOpts) ([]*model.PropertyValue, error) - Update(values []*model.PropertyValue) ([]*model.PropertyValue, error) + Update(groupID string, values []*model.PropertyValue) ([]*model.PropertyValue, error) Upsert(values []*model.PropertyValue) ([]*model.PropertyValue, error) - Delete(id string) error + Delete(groupID string, id string) error DeleteForField(id string) error } diff --git a/server/channels/store/storetest/mocks/PropertyFieldStore.go b/server/channels/store/storetest/mocks/PropertyFieldStore.go index d4139251ad..58f7b571ce 100644 --- a/server/channels/store/storetest/mocks/PropertyFieldStore.go +++ b/server/channels/store/storetest/mocks/PropertyFieldStore.go @@ -72,17 +72,17 @@ func (_m *PropertyFieldStore) Create(field *model.PropertyField) (*model.Propert return r0, r1 } -// Delete provides a mock function with given fields: id -func (_m *PropertyFieldStore) Delete(id string) error { - ret := _m.Called(id) +// Delete provides a mock function with given fields: groupID, id +func (_m *PropertyFieldStore) Delete(groupID string, id string) error { + ret := _m.Called(groupID, id) if len(ret) == 0 { panic("no return value specified for Delete") } var r0 error - if rf, ok := ret.Get(0).(func(string) error); ok { - r0 = rf(id) + if rf, ok := ret.Get(0).(func(string, string) error); ok { + r0 = rf(groupID, id) } else { r0 = ret.Error(0) } @@ -180,9 +180,9 @@ func (_m *PropertyFieldStore) SearchPropertyFields(opts model.PropertyFieldSearc return r0, r1 } -// Update provides a mock function with given fields: fields -func (_m *PropertyFieldStore) Update(fields []*model.PropertyField) ([]*model.PropertyField, error) { - ret := _m.Called(fields) +// Update provides a mock function with given fields: groupID, fields +func (_m *PropertyFieldStore) Update(groupID string, fields []*model.PropertyField) ([]*model.PropertyField, error) { + ret := _m.Called(groupID, fields) if len(ret) == 0 { panic("no return value specified for Update") @@ -190,19 +190,19 @@ func (_m *PropertyFieldStore) Update(fields []*model.PropertyField) ([]*model.Pr var r0 []*model.PropertyField var r1 error - if rf, ok := ret.Get(0).(func([]*model.PropertyField) ([]*model.PropertyField, error)); ok { - return rf(fields) + if rf, ok := ret.Get(0).(func(string, []*model.PropertyField) ([]*model.PropertyField, error)); ok { + return rf(groupID, fields) } - if rf, ok := ret.Get(0).(func([]*model.PropertyField) []*model.PropertyField); ok { - r0 = rf(fields) + if rf, ok := ret.Get(0).(func(string, []*model.PropertyField) []*model.PropertyField); ok { + r0 = rf(groupID, fields) } else { if ret.Get(0) != nil { r0 = ret.Get(0).([]*model.PropertyField) } } - if rf, ok := ret.Get(1).(func([]*model.PropertyField) error); ok { - r1 = rf(fields) + if rf, ok := ret.Get(1).(func(string, []*model.PropertyField) error); ok { + r1 = rf(groupID, fields) } else { r1 = ret.Error(1) } diff --git a/server/channels/store/storetest/mocks/PropertyValueStore.go b/server/channels/store/storetest/mocks/PropertyValueStore.go index c2296d7183..4fae9dcd65 100644 --- a/server/channels/store/storetest/mocks/PropertyValueStore.go +++ b/server/channels/store/storetest/mocks/PropertyValueStore.go @@ -44,17 +44,17 @@ func (_m *PropertyValueStore) Create(value *model.PropertyValue) (*model.Propert return r0, r1 } -// Delete provides a mock function with given fields: id -func (_m *PropertyValueStore) Delete(id string) error { - ret := _m.Called(id) +// Delete provides a mock function with given fields: groupID, id +func (_m *PropertyValueStore) Delete(groupID string, id string) error { + ret := _m.Called(groupID, id) if len(ret) == 0 { panic("no return value specified for Delete") } var r0 error - if rf, ok := ret.Get(0).(func(string) error); ok { - r0 = rf(id) + if rf, ok := ret.Get(0).(func(string, string) error); ok { + r0 = rf(groupID, id) } else { r0 = ret.Error(0) } @@ -170,9 +170,9 @@ func (_m *PropertyValueStore) SearchPropertyValues(opts model.PropertyValueSearc return r0, r1 } -// Update provides a mock function with given fields: values -func (_m *PropertyValueStore) Update(values []*model.PropertyValue) ([]*model.PropertyValue, error) { - ret := _m.Called(values) +// Update provides a mock function with given fields: groupID, values +func (_m *PropertyValueStore) Update(groupID string, values []*model.PropertyValue) ([]*model.PropertyValue, error) { + ret := _m.Called(groupID, values) if len(ret) == 0 { panic("no return value specified for Update") @@ -180,19 +180,19 @@ func (_m *PropertyValueStore) Update(values []*model.PropertyValue) ([]*model.Pr var r0 []*model.PropertyValue var r1 error - if rf, ok := ret.Get(0).(func([]*model.PropertyValue) ([]*model.PropertyValue, error)); ok { - return rf(values) + if rf, ok := ret.Get(0).(func(string, []*model.PropertyValue) ([]*model.PropertyValue, error)); ok { + return rf(groupID, values) } - if rf, ok := ret.Get(0).(func([]*model.PropertyValue) []*model.PropertyValue); ok { - r0 = rf(values) + if rf, ok := ret.Get(0).(func(string, []*model.PropertyValue) []*model.PropertyValue); ok { + r0 = rf(groupID, values) } else { if ret.Get(0) != nil { r0 = ret.Get(0).([]*model.PropertyValue) } } - if rf, ok := ret.Get(1).(func([]*model.PropertyValue) error); ok { - r1 = rf(values) + if rf, ok := ret.Get(1).(func(string, []*model.PropertyValue) error); ok { + r1 = rf(groupID, values) } else { r1 = ret.Error(1) } diff --git a/server/channels/store/storetest/property_field_store.go b/server/channels/store/storetest/property_field_store.go index a37214c8d1..21a8148658 100644 --- a/server/channels/store/storetest/property_field_store.go +++ b/server/channels/store/storetest/property_field_store.go @@ -182,7 +182,7 @@ func testUpdatePropertyField(t *testing.T, _ request.CTX, ss store.Store) { Type: model.PropertyFieldTypeText, CreateAt: model.GetMillis(), } - updatedField, err := ss.PropertyField().Update([]*model.PropertyField{field}) + updatedField, err := ss.PropertyField().Update("", []*model.PropertyField{field}) require.Zero(t, updatedField) require.ErrorContains(t, err, "failed to update, some property fields were not found, got 0 of 1") }) @@ -198,13 +198,13 @@ func testUpdatePropertyField(t *testing.T, _ request.CTX, ss store.Store) { require.NotZero(t, field.ID) field.GroupID = "" - updatedField, err := ss.PropertyField().Update([]*model.PropertyField{field}) + updatedField, err := ss.PropertyField().Update("", []*model.PropertyField{field}) require.Zero(t, updatedField) require.ErrorContains(t, err, "model.property_field.is_valid.app_error") field.GroupID = model.NewId() field.Name = "" - updatedField, err = ss.PropertyField().Update([]*model.PropertyField{field}) + updatedField, err = ss.PropertyField().Update("", []*model.PropertyField{field}) require.Zero(t, updatedField) require.ErrorContains(t, err, "model.property_field.is_valid.app_error") }) @@ -248,7 +248,7 @@ func testUpdatePropertyField(t *testing.T, _ request.CTX, ss store.Store) { "options": []string{"x", "y", "z"}, } - _, err := ss.PropertyField().Update([]*model.PropertyField{field1, field2}) + _, err := ss.PropertyField().Update("", []*model.PropertyField{field1, field2}) require.NoError(t, err) // Verify first field @@ -303,16 +303,16 @@ func testUpdatePropertyField(t *testing.T, _ request.CTX, ss store.Store) { field1.Name = "Valid update" field2.GroupID = "Invalid ID" - _, err := ss.PropertyField().Update([]*model.PropertyField{field1, field2}) + _, err := ss.PropertyField().Update("", []*model.PropertyField{field1, field2}) require.ErrorContains(t, err, "model.property_field.is_valid.app_error") // Check that fields were not updated - updated1, err := ss.PropertyField().Get(groupID, field1.ID) + updated1, err := ss.PropertyField().Get("", field1.ID) require.NoError(t, err) require.Equal(t, "Field 1", updated1.Name) require.Equal(t, originalUpdateAt1, updated1.UpdateAt) - updated2, err := ss.PropertyField().Get(groupID, field2.ID) + updated2, err := ss.PropertyField().Get("", field2.ID) require.NoError(t, err) require.Equal(t, groupID, updated2.GroupID) require.Equal(t, originalUpdateAt2, updated2.UpdateAt) @@ -347,7 +347,7 @@ func testUpdatePropertyField(t *testing.T, _ request.CTX, ss store.Store) { field1.Name = "Updated First" - _, err = ss.PropertyField().Update([]*model.PropertyField{field1, field2}) + _, err = ss.PropertyField().Update("", []*model.PropertyField{field1, field2}) require.Error(t, err) require.ErrorContains(t, err, "failed to update, some property fields were not found") @@ -357,11 +357,88 @@ func testUpdatePropertyField(t *testing.T, _ request.CTX, ss store.Store) { require.Equal(t, "First field", updated1.Name) require.Equal(t, originalUpdateAt, updated1.UpdateAt) }) + + t.Run("should update fields with matching groupID", func(t *testing.T) { + // Create fields with the same groupID + groupID := model.NewId() + field1 := &model.PropertyField{ + GroupID: groupID, + Name: "Group Field 1", + Type: model.PropertyFieldTypeText, + } + field2 := &model.PropertyField{ + GroupID: groupID, + Name: "Group Field 2", + Type: model.PropertyFieldTypeText, + } + + for _, field := range []*model.PropertyField{field1, field2} { + _, err := ss.PropertyField().Create(field) + require.NoError(t, err) + } + + // Update the fields with the matching groupID + field1.Name = "Updated Group Field 1" + field2.Name = "Updated Group Field 2" + + updatedFields, err := ss.PropertyField().Update(groupID, []*model.PropertyField{field1, field2}) + require.NoError(t, err) + require.Len(t, updatedFields, 2) + + // Verify the fields were updated + for _, field := range []*model.PropertyField{field1, field2} { + updated, err := ss.PropertyField().Get("", field.ID) + require.NoError(t, err) + require.Contains(t, updated.Name, "Updated Group Field") + } + }) + + t.Run("should not update fields with non-matching groupID", func(t *testing.T) { + // Create fields with different groupIDs + groupID1 := model.NewId() + groupID2 := model.NewId() + + field1 := &model.PropertyField{ + GroupID: groupID1, + Name: "Field in Group 1", + Type: model.PropertyFieldTypeText, + } + field2 := &model.PropertyField{ + GroupID: groupID2, + Name: "Field in Group 2", + Type: model.PropertyFieldTypeText, + } + + for _, field := range []*model.PropertyField{field1, field2} { + _, err := ss.PropertyField().Create(field) + require.NoError(t, err) + } + + originalName1 := field1.Name + originalName2 := field2.Name + + // Try to update both fields but filter by groupID1 + field1.Name = "Updated Field in Group 1" + field2.Name = "Updated Field in Group 2" + + _, err := ss.PropertyField().Update(groupID1, []*model.PropertyField{field1, field2}) + require.Error(t, err) + require.ErrorContains(t, err, "failed to update, some property fields were not found") + + // Verify neither field was updated due to transaction rollback + updated1, err := ss.PropertyField().Get("", field1.ID) + require.NoError(t, err) + require.Equal(t, originalName1, updated1.Name) + + updated2, err := ss.PropertyField().Get("", field2.ID) + require.NoError(t, err) + require.Equal(t, originalName2, updated2.Name) + }) } func testDeletePropertyField(t *testing.T, _ request.CTX, ss store.Store) { t.Run("should fail on nonexisting field", func(t *testing.T) { - err := ss.PropertyField().Delete(model.NewId()) + err := ss.PropertyField().Delete("", model.NewId()) var enf *store.ErrNotFound require.ErrorAs(t, err, &enf) }) @@ -377,7 +454,7 @@ func testDeletePropertyField(t *testing.T, _ request.CTX, ss store.Store) { require.NoError(t, err) require.NotEmpty(t, field.ID) - err = ss.PropertyField().Delete(field.ID) + err = ss.PropertyField().Delete("", field.ID) require.NoError(t, err) // Verify the field was soft-deleted @@ -392,6 +469,49 @@ func testDeletePropertyField(t *testing.T, _ request.CTX, ss store.Store) { require.NoError(t, err) require.NotEmpty(t, field.ID) }) + + t.Run("should be able to delete a field with matching groupID", func(t *testing.T) { + groupID := model.NewId() + field := &model.PropertyField{ + GroupID: groupID, + Name: "Field with specific group", + Type: model.PropertyFieldTypeText, + } + _, err := ss.PropertyField().Create(field) + require.NoError(t, err) + require.NotZero(t, field.ID) + + err = ss.PropertyField().Delete(groupID, field.ID) + require.NoError(t, err) + + // Verify the field was soft-deleted + deletedField, err := ss.PropertyField().Get(groupID, field.ID) + require.NoError(t, err) + require.NotZero(t, deletedField.DeleteAt) + }) + + t.Run("should fail when deleting with non-matching groupID", func(t *testing.T) { + groupID := model.NewId() + field := &model.PropertyField{ + GroupID: groupID, + Name: "Another field with specific group", + Type: model.PropertyFieldTypeText, + } + _, err := ss.PropertyField().Create(field) + require.NoError(t, err) + require.NotZero(t, field.ID) + + // Try to delete with wrong groupID + err = ss.PropertyField().Delete(model.NewId(), field.ID) + require.Error(t, err) + var enf *store.ErrNotFound + require.ErrorAs(t, err, &enf) + + // Verify the field was not deleted + nonDeletedField, err := ss.PropertyField().Get(groupID, field.ID) + require.NoError(t, err) + require.Zero(t, nonDeletedField.DeleteAt) + }) } func testCountForGroup(t *testing.T, _ request.CTX, ss store.Store) { @@ -443,7 +563,7 @@ func testCountForGroup(t *testing.T, _ request.CTX, ss store.Store) { _, err := ss.PropertyField().Create(deletedField) require.NoError(t, err) - err = ss.PropertyField().Delete(deletedField.ID) + err = ss.PropertyField().Delete("", deletedField.ID) require.NoError(t, err) // Count should be 5 since the deleted field shouldn't be counted @@ -475,7 +595,7 @@ func testCountForGroup(t *testing.T, _ request.CTX, ss store.Store) { _, err := ss.PropertyField().Create(deletedField) require.NoError(t, err) - err = ss.PropertyField().Delete(deletedField.ID) + err = ss.PropertyField().Delete("", deletedField.ID) require.NoError(t, err) // Count should be 6 since we're including deleted fields @@ -527,7 +647,7 @@ func testSearchPropertyFields(t *testing.T, _ request.CTX, ss store.Store) { } // Delete one field for deletion tests - require.NoError(t, ss.PropertyField().Delete(field4.ID)) + require.NoError(t, ss.PropertyField().Delete("", field4.ID)) tests := []struct { name string diff --git a/server/channels/store/storetest/property_value_store.go b/server/channels/store/storetest/property_value_store.go index 7d458f639d..8c3fa64530 100644 --- a/server/channels/store/storetest/property_value_store.go +++ b/server/channels/store/storetest/property_value_store.go @@ -111,6 +111,43 @@ func testGetPropertyValue(t *testing.T, _ request.CTX, ss store.Store) { require.Zero(t, value) require.ErrorIs(t, err, sql.ErrNoRows) }) + + t.Run("should be able to retrieve an existing property value with matching groupID", func(t *testing.T) { + groupID := model.NewId() + newValue := &model.PropertyValue{ + TargetID: model.NewId(), + TargetType: "test_type", + GroupID: groupID, + FieldID: model.NewId(), + Value: json.RawMessage(`"test value with group"`), + } + _, err := ss.PropertyValue().Create(newValue) + require.NoError(t, err) + require.NotZero(t, newValue.ID) + + value, err := ss.PropertyValue().Get(groupID, newValue.ID) + require.NoError(t, err) + require.Equal(t, newValue.ID, value.ID) + require.Equal(t, newValue.Value, value.Value) + }) + + t.Run("should fail when retrieving a value with non-matching groupID", func(t *testing.T) { + newValue := &model.PropertyValue{ + TargetID: model.NewId(), + TargetType: "test_type", + GroupID: model.NewId(), + FieldID: model.NewId(), + Value: json.RawMessage(`"test value with specific group"`), + } + _, err := ss.PropertyValue().Create(newValue) + require.NoError(t, err) + require.NotZero(t, newValue.ID) + + // Try to get the value with a different group ID + value, err := ss.PropertyValue().Get(model.NewId(), newValue.ID) + require.Zero(t, value) + require.ErrorIs(t, err, sql.ErrNoRows) + }) } func testGetManyPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { @@ -185,7 +222,7 @@ func testUpdatePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { Value: json.RawMessage(`"test value"`), CreateAt: model.GetMillis(), } - updatedValue, err := ss.PropertyValue().Update([]*model.PropertyValue{value}) + updatedValue, err := ss.PropertyValue().Update("", []*model.PropertyValue{value}) require.Zero(t, updatedValue) require.ErrorContains(t, err, "failed to update, some property values were not found, got 0 of 1") }) @@ -203,13 +240,13 @@ func testUpdatePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { require.NotZero(t, value.ID) value.TargetID = "" - updatedValue, err := ss.PropertyValue().Update([]*model.PropertyValue{value}) + updatedValue, err := ss.PropertyValue().Update("", []*model.PropertyValue{value}) require.Zero(t, updatedValue) require.ErrorContains(t, err, "model.property_value.is_valid.app_error") value.TargetID = model.NewId() value.GroupID = "" - updatedValue, err = ss.PropertyValue().Update([]*model.PropertyValue{value}) + updatedValue, err = ss.PropertyValue().Update("", []*model.PropertyValue{value}) require.Zero(t, updatedValue) require.ErrorContains(t, err, "model.property_value.is_valid.app_error") }) @@ -241,7 +278,7 @@ func testUpdatePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { value1.Value = json.RawMessage(`"updated value 1"`) value2.Value = json.RawMessage(`"updated value 2"`) - _, err := ss.PropertyValue().Update([]*model.PropertyValue{value1, value2}) + _, err := ss.PropertyValue().Update("", []*model.PropertyValue{value1, value2}) require.NoError(t, err) // Verify first value @@ -288,7 +325,7 @@ func testUpdatePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { value1.Value = json.RawMessage(`"Valid update"`) value2.GroupID = "Invalid ID" - _, err := ss.PropertyValue().Update([]*model.PropertyValue{value1, value2}) + _, err := ss.PropertyValue().Update("", []*model.PropertyValue{value1, value2}) require.Error(t, err) require.Contains(t, err.Error(), "model.property_value.is_valid.app_error") @@ -332,7 +369,7 @@ func testUpdatePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { value1.Value = json.RawMessage(`"Updated Value 1"`) - _, err = ss.PropertyValue().Update([]*model.PropertyValue{value1, value2}) + _, err = ss.PropertyValue().Update("", []*model.PropertyValue{value1, value2}) require.Error(t, err) require.ErrorContains(t, err, "failed to update, some property values were not found") @@ -342,6 +379,91 @@ func testUpdatePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { require.Equal(t, json.RawMessage(`"Value 1"`), updated1.Value) require.Equal(t, originalUpdateAt, updated1.UpdateAt) }) + + t.Run("should update values with matching groupID", func(t *testing.T) { + // Create values with the same groupID + groupID := model.NewId() + value1 := &model.PropertyValue{ + TargetID: model.NewId(), + TargetType: "test_type", + GroupID: groupID, + FieldID: model.NewId(), + Value: json.RawMessage(`"Group Value 1"`), + } + value2 := &model.PropertyValue{ + TargetID: model.NewId(), + TargetType: "test_type", + GroupID: groupID, + FieldID: model.NewId(), + Value: json.RawMessage(`"Group Value 2"`), + } + + for _, value := range []*model.PropertyValue{value1, value2} { + _, err := ss.PropertyValue().Create(value) + require.NoError(t, err) + } + + // Update the values with the matching groupID + value1.Value = json.RawMessage(`"Updated Group Value 1"`) + value2.Value = json.RawMessage(`"Updated Group Value 2"`) + + updatedValues, err := ss.PropertyValue().Update(groupID, []*model.PropertyValue{value1, value2}) + require.NoError(t, err) + require.Len(t, updatedValues, 2) + + // Verify the values were updated + for _, value := range []*model.PropertyValue{value1, value2} { + updated, err := ss.PropertyValue().Get("", value.ID) + require.NoError(t, err) + require.Contains(t, string(updated.Value), "Updated Group Value") + } + }) + + t.Run("should not update values with non-matching groupID", func(t *testing.T) { + // Create values with different groupIDs + groupID1 := model.NewId() + groupID2 := model.NewId() + + value1 := &model.PropertyValue{ + TargetID: model.NewId(), + TargetType: "test_type", + GroupID: groupID1, + FieldID: model.NewId(), + Value: json.RawMessage(`"Value in Group 1"`), + } + value2 := &model.PropertyValue{ + TargetID: model.NewId(), + TargetType: "test_type", + GroupID: groupID2, + FieldID: model.NewId(), + Value: json.RawMessage(`"Value in Group 2"`), + } + + for _, value := range []*model.PropertyValue{value1, value2} { + _, err := ss.PropertyValue().Create(value) + require.NoError(t, err) + } + + originalValue1 := string(value1.Value) + originalValue2 := string(value2.Value) + + // Try to update both values but filter by groupID1 + value1.Value = json.RawMessage(`"Updated Value in Group 1"`) + value2.Value = json.RawMessage(`"Updated Value in Group 2"`) + + _, err := ss.PropertyValue().Update(groupID1, []*model.PropertyValue{value1, value2}) + require.Error(t, err) + require.ErrorContains(t, err, "failed to update, some property values were not found") + + // Verify neither value was updated due to transaction rollback + updated1, err := ss.PropertyValue().Get("", value1.ID) + require.NoError(t, err) + require.Equal(t, originalValue1, string(updated1.Value)) + + updated2, err := ss.PropertyValue().Get("", value2.ID) + require.NoError(t, err) + require.Equal(t, originalValue2, string(updated2.Value)) + }) } func testUpsertPropertyValue(t *testing.T, _ request.CTX, ss store.Store) { @@ -509,7 +631,7 @@ func testUpsertPropertyValue(t *testing.T, _ request.CTX, ss store.Store) { func testDeletePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { t.Run("should fail on nonexisting value", func(t *testing.T) { - err := ss.PropertyValue().Delete(model.NewId()) + err := ss.PropertyValue().Delete("", model.NewId()) var enf *store.ErrNotFound require.ErrorAs(t, err, &enf) }) @@ -526,7 +648,7 @@ func testDeletePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { require.NoError(t, err) require.NotEmpty(t, value.ID) - err = ss.PropertyValue().Delete(value.ID) + err = ss.PropertyValue().Delete("", value.ID) require.NoError(t, err) // Verify the value was soft-deleted @@ -548,6 +670,54 @@ func testDeletePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { require.NotEmpty(t, value.ID) require.Equal(t, sameDetailsValue.Value, value.Value) }) + + t.Run("should be able to delete a value with matching groupID", func(t *testing.T) { + groupID := model.NewId() + value := &model.PropertyValue{ + TargetID: model.NewId(), + TargetType: "test_type", + GroupID: groupID, + FieldID: model.NewId(), + Value: json.RawMessage(`"value with specific group"`), + } + _, err := ss.PropertyValue().Create(value) + require.NoError(t, err) + require.NotZero(t, value.ID) + + // Delete with matching groupID + err = ss.PropertyValue().Delete(groupID, value.ID) + require.NoError(t, err) + + // Verify the value was soft-deleted + deletedValue, err := ss.PropertyValue().Get(groupID, value.ID) + require.NoError(t, err) + require.NotZero(t, deletedValue.DeleteAt) + }) + + t.Run("should fail when deleting with non-matching groupID", func(t *testing.T) { + groupID := model.NewId() + value := &model.PropertyValue{ + TargetID: model.NewId(), + TargetType: "test_type", + GroupID: groupID, + FieldID: model.NewId(), + Value: json.RawMessage(`"another value with specific group"`), + } + _, err := ss.PropertyValue().Create(value) + require.NoError(t, err) + require.NotZero(t, value.ID) + + // Try to delete with wrong groupID + err = ss.PropertyValue().Delete(model.NewId(), value.ID) + require.Error(t, err) + var enf *store.ErrNotFound + require.ErrorAs(t, err, &enf) + + // Verify the value was not deleted + nonDeletedValue, err := ss.PropertyValue().Get(groupID, value.ID) + require.NoError(t, err) + require.Zero(t, nonDeletedValue.DeleteAt) + }) } func testSearchPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { @@ -595,7 +765,7 @@ func testSearchPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { } // Delete one value for deletion tests - require.NoError(t, ss.PropertyValue().Delete(value4.ID)) + require.NoError(t, ss.PropertyValue().Delete("", value4.ID)) tests := []struct { name string @@ -748,7 +918,7 @@ func testCreatePropertyValueWithArray(t *testing.T, _ request.CTX, ss store.Stor require.NotZero(t, created.ID) created.Value = json.RawMessage(`["updated1", "updated2", "updated3"]`) - updated, err := ss.PropertyValue().Update([]*model.PropertyValue{created}) + updated, err := ss.PropertyValue().Update("", []*model.PropertyValue{created}) require.NoError(t, err) require.NotZero(t, updated) diff --git a/server/channels/store/timerlayer/timerlayer.go b/server/channels/store/timerlayer/timerlayer.go index af46ab3b49..a8d45609a5 100644 --- a/server/channels/store/timerlayer/timerlayer.go +++ b/server/channels/store/timerlayer/timerlayer.go @@ -7153,10 +7153,10 @@ func (s *TimerLayerPropertyFieldStore) Create(field *model.PropertyField) (*mode return result, err } -func (s *TimerLayerPropertyFieldStore) Delete(id string) error { +func (s *TimerLayerPropertyFieldStore) Delete(groupID string, id string) error { start := time.Now() - err := s.PropertyFieldStore.Delete(id) + err := s.PropertyFieldStore.Delete(groupID, id) elapsed := float64(time.Since(start)) / float64(time.Second) if s.Root.Metrics != nil { @@ -7217,10 +7217,10 @@ func (s *TimerLayerPropertyFieldStore) SearchPropertyFields(opts model.PropertyF return result, err } -func (s *TimerLayerPropertyFieldStore) Update(fields []*model.PropertyField) ([]*model.PropertyField, error) { +func (s *TimerLayerPropertyFieldStore) Update(groupID string, fields []*model.PropertyField) ([]*model.PropertyField, error) { start := time.Now() - result, err := s.PropertyFieldStore.Update(fields) + result, err := s.PropertyFieldStore.Update(groupID, fields) elapsed := float64(time.Since(start)) / float64(time.Second) if s.Root.Metrics != nil { @@ -7281,10 +7281,10 @@ func (s *TimerLayerPropertyValueStore) Create(value *model.PropertyValue) (*mode return result, err } -func (s *TimerLayerPropertyValueStore) Delete(id string) error { +func (s *TimerLayerPropertyValueStore) Delete(groupID string, id string) error { start := time.Now() - err := s.PropertyValueStore.Delete(id) + err := s.PropertyValueStore.Delete(groupID, id) elapsed := float64(time.Since(start)) / float64(time.Second) if s.Root.Metrics != nil { @@ -7361,10 +7361,10 @@ func (s *TimerLayerPropertyValueStore) SearchPropertyValues(opts model.PropertyV return result, err } -func (s *TimerLayerPropertyValueStore) Update(values []*model.PropertyValue) ([]*model.PropertyValue, error) { +func (s *TimerLayerPropertyValueStore) Update(groupID string, values []*model.PropertyValue) ([]*model.PropertyValue, error) { start := time.Now() - result, err := s.PropertyValueStore.Update(values) + result, err := s.PropertyValueStore.Update(groupID, values) elapsed := float64(time.Since(start)) / float64(time.Second) if s.Root.Metrics != nil { diff --git a/server/i18n/en.json b/server/i18n/en.json index 2c4350c506..c6eb59669c 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -1873,6 +1873,10 @@ "id": "api.custom_profile_attributes.field_not_found", "translation": "trying to patch a field that does not exist" }, + { + "id": "api.custom_profile_attributes.invalid_field_patch", + "translation": "invalid custom profile attribute field patch" + }, { "id": "api.custom_profile_attributes.license_error", "translation": "Your license does not support Custom Profile Attributes." diff --git a/server/public/model/property_field.go b/server/public/model/property_field.go index 669ac275e6..67894c50cc 100644 --- a/server/public/model/property_field.go +++ b/server/public/model/property_field.go @@ -8,7 +8,6 @@ import ( "errors" "fmt" "net/http" - "strings" ) type PropertyFieldType string @@ -94,10 +93,6 @@ func (pf *PropertyField) IsValid() error { return nil } -func (pf *PropertyField) SanitizeInput() { - pf.Name = strings.TrimSpace(pf.Name) -} - type PropertyFieldPatch struct { Name *string `json:"name"` Type *PropertyFieldType `json:"type"` @@ -116,10 +111,22 @@ func (pfp *PropertyFieldPatch) Auditable() map[string]any { } } -func (pfp *PropertyFieldPatch) SanitizeInput() { - if pfp.Name != nil { - pfp.Name = NewPointer(strings.TrimSpace(*pfp.Name)) +func (pfp *PropertyFieldPatch) IsValid() error { + if pfp.Name != nil && *pfp.Name == "" { + return NewAppError("PropertyFieldPatch.IsValid", "model.property_field.is_valid.app_error", map[string]any{"FieldName": "name", "Reason": "value cannot be empty"}, "", http.StatusBadRequest) } + + if pfp.Type != nil && + *pfp.Type != PropertyFieldTypeText && + *pfp.Type != PropertyFieldTypeSelect && + *pfp.Type != PropertyFieldTypeMultiselect && + *pfp.Type != PropertyFieldTypeDate && + *pfp.Type != PropertyFieldTypeUser && + *pfp.Type != PropertyFieldTypeMultiuser { + return NewAppError("PropertyFieldPatch.IsValid", "model.property_field.is_valid.app_error", map[string]any{"FieldName": "type", "Reason": "unknown value"}, "", http.StatusBadRequest) + } + + return nil } func (pf *PropertyField) Patch(patch *PropertyFieldPatch) { diff --git a/server/public/model/property_field_test.go b/server/public/model/property_field_test.go index ff9c143831..7b6ecbd375 100644 --- a/server/public/model/property_field_test.go +++ b/server/public/model/property_field_test.go @@ -123,11 +123,38 @@ func TestPropertyField_IsValid(t *testing.T) { }) } -func TestPropertyField_SanitizeInput(t *testing.T) { - t.Run("trims spaces from name", func(t *testing.T) { - pf := &PropertyField{Name: " test field "} - pf.SanitizeInput() - assert.Equal(t, "test field", pf.Name) +func TestPropertyFieldPatch_IsValid(t *testing.T) { + t.Run("valid patch", func(t *testing.T) { + patch := &PropertyFieldPatch{ + Name: NewPointer("test field"), + Type: NewPointer(PropertyFieldTypeText), + } + require.NoError(t, patch.IsValid()) + }) + + t.Run("empty name", func(t *testing.T) { + patch := &PropertyFieldPatch{ + Name: NewPointer(""), + Type: NewPointer(PropertyFieldTypeText), + } + require.Error(t, patch.IsValid()) + }) + + t.Run("invalid type", func(t *testing.T) { + invalidType := PropertyFieldType("invalid") + patch := &PropertyFieldPatch{ + Name: NewPointer("test field"), + Type: &invalidType, + } + require.Error(t, patch.IsValid()) + }) + + t.Run("nil values are valid", func(t *testing.T) { + patch := &PropertyFieldPatch{ + Name: nil, + Type: nil, + } + require.NoError(t, patch.IsValid()) }) }