From ed445d120cb6a50690eb3cbbf253dd1120a865dd Mon Sep 17 00:00:00 2001 From: Denis Arh Date: Wed, 11 Mar 2020 18:52:52 +0100 Subject: [PATCH] Sanitization & validation refactoring (dates, refs) --- compose/service/record.go | 169 ++++++++++++----------- compose/service/record_test.go | 1 - compose/service/values/sanitizer.go | 34 +++-- compose/service/values/sanitizer_test.go | 107 ++++++++++++++ compose/service/values/shared.go | 4 + compose/types/record_value.go | 72 +++++++--- compose/types/record_value_test.go | 18 +-- 7 files changed, 281 insertions(+), 124 deletions(-) create mode 100644 compose/service/values/sanitizer_test.go diff --git a/compose/service/record.go b/compose/service/record.go index 203515abc..fa2133934 100644 --- a/compose/service/record.go +++ b/compose/service/record.go @@ -337,41 +337,6 @@ func (svc record) Export(filter types.RecordFilter, enc Encoder) error { func (svc record) Create(new *types.Record) (rec *types.Record, err error) { var invokerID = auth.GetIdentityFromContext(svc.ctx).Identity() - // Runs value sanitization, sets values that should be used - // and validates the final result - // - // This logic is kept in a utility function - it's used in the beginning - // of the creation procedure and after results are back from the automation scripts - // - // Both these points introduce external data that need to be checked fully in the same manner - procChanges := func(m *types.Module, new *types.Record) *types.RecordValueErrorSet { - // Before values are processed further and - // sent to automation scripts (if any) - // we need to make sure it does not get sanitized data - new.Values = svc.sanitizer.Run(m, new.Values) - - // Reset values to new record - // to make sure nobody slips in something we do not want - new.CreatedBy = invokerID - new.CreatedAt = *nowPtr() - new.UpdatedAt = nil - new.UpdatedBy = 0 - new.DeletedAt = nil - new.DeletedBy = 0 - - // Mark all values as updated (new) - new.Values.SetUpdatedFlag(true) - - if new.OwnedBy == 0 { - // If od owner is not set, make current user - // the owner of the record - new.OwnedBy = invokerID - } - - // Run validation of the updated records - return svc.validator.Run(m, new) - } - return rec, svc.db.Transaction(func() (err error) { var ( ns *types.Namespace @@ -396,7 +361,7 @@ func (svc record) Create(new *types.Record) (rec *types.Record, err error) { ) // Handle input payload - if rve = procChanges(m, new); !rve.IsValid() { + if rve = svc.procCreate(invokerID, m, new); !rve.IsValid() { return rve } @@ -410,7 +375,7 @@ func (svc record) Create(new *types.Record) (rec *types.Record, err error) { new.Values = svc.setDefaultValues(m, new.Values) // Handle payload from automation scripts - if rve = procChanges(m, new); !rve.IsValid() { + if rve = svc.procCreate(invokerID, m, new); !rve.IsValid() { return rve } @@ -430,53 +395,44 @@ func (svc record) Create(new *types.Record) (rec *types.Record, err error) { }) } +// Runs value sanitization, sets values that should be used +// and validates the final result +// +// This logic is kept in a utility function - it's used in the beginning +// of the creation procedure and after results are back from the automation scripts +// +// Both these points introduce external data that need to be checked fully in the same manner +func (svc record) procCreate(invokerID uint64, m *types.Module, new *types.Record) *types.RecordValueErrorSet { + // Mark all values as updated (new) + new.Values.SetUpdatedFlag(true) + + // Before values are processed further and + // sent to automation scripts (if any) + // we need to make sure it does not get un-sanitized data + new.Values = svc.sanitizer.Run(m, new.Values) + + // Reset values to new record + // to make sure nobody slips in something we do not want + new.CreatedBy = invokerID + new.CreatedAt = *nowPtr() + new.UpdatedAt = nil + new.UpdatedBy = 0 + new.DeletedAt = nil + new.DeletedBy = 0 + + if new.OwnedBy == 0 { + // If od owner is not set, make current user + // the owner of the record + new.OwnedBy = invokerID + } + + // Run validation of the updated records + return svc.validator.Run(m, new) +} + func (svc record) Update(upd *types.Record) (rec *types.Record, err error) { var invokerID = auth.GetIdentityFromContext(svc.ctx).Identity() - // Runs value sanitization, copies values that should updated - // and validates the final result - // - // This logic is kept in a utility function - it's used in the beginning - // of the update procedure and after results are back from the automation scripts - // - // Both these points introduce external data that need to be checked fully in the same maner - procChanges := func(m *types.Module, upd *types.Record, old *types.Record) *types.RecordValueErrorSet { - // First sanitization - // - // Before values are merged with existing data and - // sent to automation scripts (if any) - // we need to make sure it does not get sanitized data - upd.Values = svc.sanitizer.Run(m, upd.Values) - - // Copy values to updated record - // to make sure nobody slips in something we do not want - upd.CreatedAt = old.CreatedAt - upd.CreatedBy = old.CreatedBy - upd.UpdatedAt = nowPtr() - upd.UpdatedBy = invokerID - upd.DeletedAt = old.DeletedAt - upd.DeletedBy = old.DeletedBy - - // Merge new (updated) values with old ones - // This way we get list of updated, stale and deleted values - // that we can selectively update in the repository - upd.Values = old.Values.Merge(upd.Values) - - if upd.OwnedBy == 0 && old.OwnedBy > 0 { - // Owner not set/send in the payload - // - // Fallback to old owner (if set) - upd.OwnedBy = old.OwnedBy - } else { - // If od owner is not set, make current user - // the owner of the record - upd.OwnedBy = invokerID - } - - // Run validation of the updated records - return svc.validator.Run(m, upd) - } - return rec, svc.db.Transaction(func() (err error) { if upd.ID == 0 { return ErrInvalidID.withStack() @@ -510,7 +466,7 @@ func (svc record) Update(upd *types.Record) (rec *types.Record, err error) { ) // Handle input payload - if rve = procChanges(m, upd, old); !rve.IsValid() { + if rve = svc.procUpdate(invokerID, m, upd, old); !rve.IsValid() { return rve } @@ -526,7 +482,7 @@ func (svc record) Update(upd *types.Record) (rec *types.Record, err error) { } // Handle payload from automation scripts - if rve = procChanges(m, upd, old); !rve.IsValid() { + if rve = svc.procUpdate(invokerID, m, upd, old); !rve.IsValid() { return rve } @@ -548,6 +504,53 @@ func (svc record) Update(upd *types.Record) (rec *types.Record, err error) { }) } +// Runs value sanitization, copies values that should updated +// and validates the final result +// +// This logic is kept in a utility function - it's used in the beginning +// of the update procedure and after results are back from the automation scripts +// +// Both these points introduce external data that need to be checked fully in the same maner +func (svc record) procUpdate(invokerID uint64, m *types.Module, upd *types.Record, old *types.Record) *types.RecordValueErrorSet { + // Mark all values as updated (new) + upd.Values.SetUpdatedFlag(true) + + // First sanitization + // + // Before values are merged with existing data and + // sent to automation scripts (if any) + // we need to make sure it does not get sanitized data + upd.Values = svc.sanitizer.Run(m, upd.Values) + + // Copy values to updated record + // to make sure nobody slips in something we do not want + upd.CreatedAt = old.CreatedAt + upd.CreatedBy = old.CreatedBy + upd.UpdatedAt = nowPtr() + upd.UpdatedBy = invokerID + upd.DeletedAt = old.DeletedAt + upd.DeletedBy = old.DeletedBy + + // Merge new (updated) values with old ones + // This way we get list of updated, stale and deleted values + // that we can selectively update in the repository + upd.Values = old.Values.Merge(upd.Values) + + if upd.OwnedBy == 0 && old.OwnedBy > 0 { + // Owner not set/send in the payload + // + // Fallback to old owner (if set) + upd.OwnedBy = old.OwnedBy + } else { + // If od owner is not set, make current user + // the owner of the record + upd.OwnedBy = invokerID + } + + // Run validation of the updated records + return svc.validator.Run(m, upd) +} + func (svc record) recordInfoUpdate(r *types.Record) { now := time.Now() r.UpdatedAt = &now diff --git a/compose/service/record_test.go b/compose/service/record_test.go index d662cfb28..3cc2b4704 100644 --- a/compose/service/record_test.go +++ b/compose/service/record_test.go @@ -88,5 +88,4 @@ func TestDefaultValueSetting(t *testing.T) { chk(out, "single", 0, "s") chk(out, "multi", 0, "m1") chk(out, "multi", 1, "m2") - } diff --git a/compose/service/values/sanitizer.go b/compose/service/values/sanitizer.go index b94a54787..67e9415ab 100644 --- a/compose/service/values/sanitizer.go +++ b/compose/service/values/sanitizer.go @@ -39,13 +39,10 @@ func (s sanitizer) Run(m *types.Module, vv types.RecordValueSet) (out types.Reco continue } + c := v.Clone() + c.Place = uint(i) + out = append(out, c) i++ - out = append(out, &types.RecordValue{ - Name: f.Name, - Value: v.Value, - Ref: v.Ref, - Place: uint(i), - }) } } @@ -63,7 +60,7 @@ func (s sanitizer) Run(m *types.Module, vv types.RecordValueSet) (out types.Reco continue } - if v.IsDeleted() || !v.IsUpdated() { + if v.IsDeleted() || !v.Updated { // Ignore unchanged and deleted continue } @@ -75,8 +72,14 @@ func (s sanitizer) Run(m *types.Module, vv types.RecordValueSet) (out types.Reco v.Value = strings.TrimSpace(v.Value) } - if f.IsRef() && refy.MatchString(v.Value) { - v.Ref, _ = strconv.ParseUint(v.Value, 10, 64) + if f.IsRef() { + if refy.MatchString(v.Value) { + v.Ref, _ = strconv.ParseUint(v.Value, 10, 64) + } + + if v.Ref == 0 { + v.Value = "" + } } // Per field type validators @@ -166,17 +169,26 @@ func (sanitizer) sDatetime(v *types.RecordValue, f *types.ModuleField, m *types. "2019/_1/_2 15:04:05", "2019/_1/_2 15:04", } + + // if string looks like a RFC 3330 (ISO 8601), see if we need to suffix it with Z + if isoDaty.MatchString(v.Value) && !hasTimezone.MatchString(v.Value) { + // No timezone, add Z to satisfy parser + v.Value = v.Value + "Z" + + // Simplifiy list of rules + inputFormats = []string{time.RFC3339} + } } for _, format := range inputFormats { parsed, err := time.Parse(format, v.Value) - if err == nil { v.Value = parsed.UTC().Format(internalFormat) - break + return v } } + v.Value = "" return v } diff --git a/compose/service/values/sanitizer_test.go b/compose/service/values/sanitizer_test.go new file mode 100644 index 000000000..f53e903ed --- /dev/null +++ b/compose/service/values/sanitizer_test.go @@ -0,0 +1,107 @@ +package values + +import ( + "github.com/cortezaproject/corteza-server/compose/types" + "reflect" + "testing" +) + +func Test_sanitizer_Run(t *testing.T) { + tests := []struct { + name string + kind string + input string + output string + outref uint64 + }{ + { + name: "numbers should be trimmed", + kind: "Number", + input: " 42 ", + output: "42", + }, + { + name: "object reference should be processed", + kind: "Record", + input: " 133569629112020995 ", + output: "133569629112020995", + outref: 133569629112020995, + }, + { + name: "object reference should be numeric", + kind: "Record", + input: " foo ", + output: "", + }, + { + name: "user reference should be processed", + kind: "User", + input: " 133569629112020995 ", + output: "133569629112020995", + outref: 133569629112020995, + }, + { + name: "user reference should be numeric", + kind: "User", + input: " foo ", + output: "", + }, + { + name: "strings should be kept intact", + kind: "String", + input: " The answer ", + output: " The answer ", + }, + { + name: "booleans should be converted (t)", + kind: "Bool", + input: "t", + output: "1", + }, + { + name: "booleans should be converted (false)", + kind: "Bool", + input: "false", + output: "0", + }, + { + name: "booleans should be converted (garbage)", + kind: "Bool", + input: "%%#)%)')$)'", + output: "0", + }, + { + name: "dates should be converted to ISO", + kind: "DateTime", + input: "Mon Jan 2 15:04:05 2006", + output: "2006-01-02T15:04:05Z", + }, + { + name: "dates should be converted to UTC", + kind: "DateTime", + input: "2020-03-02T20:20:20+05:00", + output: "2020-03-02T15:20:20Z", + }, + { + name: "micro/mili seconds should be cut off", + kind: "DateTime", + input: "2020-03-11T11:20:08.471Z", + output: "2020-03-11T11:20:08Z", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + s := sanitizer{} + m := &types.Module{Fields: types.ModuleFieldSet{&types.ModuleField{Name: "testField", Kind: tt.kind}}} + v := types.RecordValueSet{&types.RecordValue{Name: "testField", Value: tt.input}} + o := types.RecordValueSet{&types.RecordValue{Name: "testField", Value: tt.output, Ref: tt.outref}} + + // Need to mark values as updated to trigger sanitization. + v.SetUpdatedFlag(true) + o.SetUpdatedFlag(true) + if sanitized := s.Run(m, v); !reflect.DeepEqual(sanitized, o) { + t.Errorf("\ninput value:\n%v\n\nresult of sanitization:\n%v\n\nexpected:\n%v\n", tt.input, sanitized, o) + } + }) + } +} diff --git a/compose/service/values/shared.go b/compose/service/values/shared.go index 9db35eded..829190d0d 100644 --- a/compose/service/values/shared.go +++ b/compose/service/values/shared.go @@ -29,6 +29,10 @@ var ( // value resembles something that can be a reference refy = regexp.MustCompile(`^[1-9](\d*)$`) + + // value resembels something that can be parsed as ISO 8601 + isoDaty = regexp.MustCompile(`^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}`) + hasTimezone = regexp.MustCompile(`(Z|\+\d{2}:\d{2})$`) ) func nowPtr() *time.Time { diff --git a/compose/types/record_value.go b/compose/types/record_value.go index 07fc0e068..1208bab26 100644 --- a/compose/types/record_value.go +++ b/compose/types/record_value.go @@ -19,19 +19,32 @@ type ( Place uint `db:"place" json:"-"` DeletedAt *time.Time `db:"deleted_at" json:"deletedAt,omitempty"` - updated bool - oldValue string + Updated bool `db:"-" json:"-"` + OldValue string `db:"-" json:"-"` } ) func (v RecordValue) IsUpdated() bool { - return v.updated + return v.Updated } func (v RecordValue) IsDeleted() bool { return v.DeletedAt != nil } +func (v RecordValue) Clone() *RecordValue { + return &RecordValue{ + RecordID: v.RecordID, + Name: v.Name, + Value: v.Value, + Ref: v.Ref, + Place: v.Place, + DeletedAt: v.DeletedAt, + Updated: v.Updated, + OldValue: v.OldValue, + } +} + func (set RecordValueSet) FilterByName(name string) (vv RecordValueSet) { for i := range set { if set[i].Name == name { @@ -108,14 +121,14 @@ func (set RecordValueSet) Has(name string, place uint) bool { func (set RecordValueSet) SetUpdatedFlag(updated bool) { for i := range set { - set[i].updated = updated + set[i].Updated = updated } } func (set RecordValueSet) GetUpdated() (out RecordValueSet) { out = make([]*RecordValue, 0, len(set)) for i := range set { - if !set[i].updated { + if !set[i].Updated { continue } @@ -153,7 +166,7 @@ func (set RecordValueSet) Merge(new RecordValueSet) (out RecordValueSet) { if len(set) == 0 { // Empty set, copy all new values and return them for i := range new { - new[i].updated = true + new[i].Updated = true } return new @@ -168,8 +181,8 @@ func (set RecordValueSet) Merge(new RecordValueSet) (out RecordValueSet) { Ref: set[s].Ref, Place: set[s].Place, DeletedAt: &time.Time{}, - updated: true, - oldValue: set[s].Value, + Updated: true, + OldValue: set[s].Value, }) } @@ -178,12 +191,12 @@ func (set RecordValueSet) Merge(new RecordValueSet) (out RecordValueSet) { // Reset deleted flag ex.DeletedAt = new[n].DeletedAt - if ex.oldValue == new[n].Value { - ex.updated = false - } else if !ex.updated { + if ex.OldValue == new[n].Value { + ex.Updated = false + } else if !ex.Updated { // Did value change? - ex.updated = ex.Value != new[n].Value - ex.oldValue = ex.Value + ex.Updated = ex.Value != new[n].Value + ex.OldValue = ex.Value } ex.Value = new[n].Value @@ -195,10 +208,10 @@ func (set RecordValueSet) Merge(new RecordValueSet) (out RecordValueSet) { Value: new[n].Value, Ref: new[n].Ref, Place: new[n].Place, - updated: true, + Updated: true, // verbose & explicit for clarity - oldValue: "", + OldValue: "", DeletedAt: nil, }) } @@ -232,10 +245,28 @@ func (set RecordValueSet) String() (o string) { return "" } - const tpl = "%-10s %2d %-10s %-20d %-10s %v %v\n" + is := func(in interface{}) string { + switch in := in.(type) { + case bool: + if in { + return "✔" + } + case *time.Time: + if in != nil { + return "✔" + } + } + + return "x" + } + + o += "━━━━━━━━━━━┳━━━━┳━━━┳━━━┳━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━━━━━━\n" + o += "name ┃ ## ┃ u ┃ d ┃ value ┃ ref ┃ old value \n" + o += "━━━━━━━━━━━╋━━━━╋━━━╋━━━╋━━━━━━━━━━━━━━━━━━━━━━━━━━━╋━━━━━━━━━━━━━━━━━━━━━━╋━━━━━━━━━━━━━━━━━━━━━━━━━━━\n" + const tpl = "%-10s ┃ %2d ┃ %s ┃ %s ┃ %-25s ┃ %-20d ┃ %-25s\n" for _, v := range set { if v == nil { - o += "\n" + o += "<--------> ┃ -- ┃ - ┃ - ┃ <------------------------> ┃ <------------------> ┃ <------------------> \n" continue } @@ -243,13 +274,14 @@ func (set RecordValueSet) String() (o string) { tpl, v.Name, v.Place, + is(v.Updated), + is(v.DeletedAt), v.Value, v.Ref, - v.oldValue, - v.updated, - v.DeletedAt, + v.OldValue, ) } + o += "━━━━━━━━━━━┻━━━━┻━━━┻━━━┻━━━━━━━━━━━━━━━━━━━━━━━━━━━┻━━━━━━━━━━━━━━━━━━━━━━┻━━━━━━━━━━━━━━━━━━━━━━━━━━━\n" return o } diff --git a/compose/types/record_value_test.go b/compose/types/record_value_test.go index 1abcf0958..1bd4345a0 100644 --- a/compose/types/record_value_test.go +++ b/compose/types/record_value_test.go @@ -53,41 +53,41 @@ func TestRecordValueSet_Merge(t *testing.T) { name: "simple update of an empty set", set: RecordValueSet{}, new: RecordValueSet{{Name: "n", Value: "v"}}, - want: RecordValueSet{{Name: "n", Value: "v", updated: true}}, + want: RecordValueSet{{Name: "n", Value: "v", Updated: true}}, }, { name: "update nil", set: nil, new: RecordValueSet{{Name: "n", Value: "v"}}, - want: RecordValueSet{{Name: "n", Value: "v", oldValue: "", updated: true}}, + want: RecordValueSet{{Name: "n", Value: "v", OldValue: "", Updated: true}}, }, { name: "update with nil", set: RecordValueSet{{Name: "n", Value: "v"}}, new: nil, - want: RecordValueSet{{Name: "n", Value: "v", oldValue: "v", DeletedAt: &time.Time{}, updated: true}}, + want: RecordValueSet{{Name: "n", Value: "v", OldValue: "v", DeletedAt: &time.Time{}, Updated: true}}, }, { name: "update with new value", set: RecordValueSet{{Name: "n", Value: "1"}}, new: RecordValueSet{{Name: "n", Value: "2"}}, - want: RecordValueSet{{Name: "n", Value: "2", oldValue: "1", updated: true}}, + want: RecordValueSet{{Name: "n", Value: "2", OldValue: "1", Updated: true}}, }, { name: "update with less values", set: RecordValueSet{{Name: "n", Value: "1"}, {Name: "deleted", Value: "d"}}, new: RecordValueSet{{Name: "n", Value: "2"}}, - want: RecordValueSet{{Name: "n", Value: "2", oldValue: "1", updated: true}, {Name: "deleted", Value: "d", oldValue: "d", updated: true, DeletedAt: &time.Time{}}}, + want: RecordValueSet{{Name: "n", Value: "2", OldValue: "1", Updated: true}, {Name: "deleted", Value: "d", OldValue: "d", Updated: true, DeletedAt: &time.Time{}}}, }, { name: "update multi value", set: RecordValueSet{{Name: "c", Value: "1st", Place: 1}, {Name: "c", Value: "2nd", Place: 2}, {Name: "c", Value: "3rd", Place: 3}, {Name: "c", Value: "4th", Place: 4}}, new: RecordValueSet{{Name: "c", Value: "1st", Place: 1}, {Name: "c", Value: "2nd", Place: 2}, {Name: "c", Value: "4th", Place: 3}}, want: RecordValueSet{ - {Name: "c", Value: "1st", Place: 1, oldValue: "1st"}, - {Name: "c", Value: "2nd", Place: 2, oldValue: "2nd"}, - {Name: "c", Value: "4th", Place: 3, oldValue: "3rd", updated: true}, - {Name: "c", Value: "4th", Place: 4, oldValue: "4th", updated: true, DeletedAt: &time.Time{}}, + {Name: "c", Value: "1st", Place: 1, OldValue: "1st"}, + {Name: "c", Value: "2nd", Place: 2, OldValue: "2nd"}, + {Name: "c", Value: "4th", Place: 3, OldValue: "3rd", Updated: true}, + {Name: "c", Value: "4th", Place: 4, OldValue: "4th", Updated: true, DeletedAt: &time.Time{}}, }, }, }