From cbfb1d49bda0cc51f418ef70569976ace7050fa6 Mon Sep 17 00:00:00 2001 From: Denis Arh Date: Mon, 8 Aug 2022 13:18:56 +0200 Subject: [PATCH] Refactor model ident validation --- compose/service/module.go | 83 ++++++---------- pkg/dal/conn.go | 48 +++++++++ pkg/dal/conn_test.go | 53 ++++++++++ pkg/dal/ident_formatter.go | 120 ----------------------- pkg/dal/ident_formatter_test.go | 109 -------------------- pkg/dal/service.go | 42 ++++---- pkg/provision/dal.go | 16 ++- system/service/dal_connection.go | 26 +++-- system/types/dal_connection.go | 2 +- tests/dal/main_test.go | 10 +- tests/system/dal_connection_crud_test.go | 3 - 11 files changed, 175 insertions(+), 337 deletions(-) create mode 100644 pkg/dal/conn.go create mode 100644 pkg/dal/conn_test.go delete mode 100644 pkg/dal/ident_formatter.go delete mode 100644 pkg/dal/ident_formatter_test.go diff --git a/compose/service/module.go b/compose/service/module.go index ac4548f01..e5d805a5a 100644 --- a/compose/service/module.go +++ b/compose/service/module.go @@ -79,10 +79,6 @@ type ( ReplaceModelAttribute(ctx context.Context, model *dal.Model, old, new *dal.Attribute, trans ...dal.TransformationFunction) (err error) SearchModelIssues(connectionID, ID uint64) []error } - - identFormatter interface { - Format(context.Context, string, ...string) (string, bool) - } ) const ( @@ -1173,10 +1169,22 @@ func DalModelRemove(ctx context.Context, dmm dalModelManager, mm ...*types.Modul return } +// modulesToModelSet takes a modules for a namespace and converts all of them +// into a model set for the DAL +// +// Ident partition placeholders are replaced here as well alongside +// with the revision models where revisions are enabled func modulesToModelSet(ctx context.Context, dmm dalModelManager, ns *types.Namespace, mm ...*types.Module) (out dal.ModelSet, err error) { var ( cm dal.ConnectionConfig model *dal.Model + + // partition replace pairs + modPartition []string + + // namespace partition replacement pairs + // {{namespace}} is replaced with the namespace handle (slug) + nsPartition = []string{"{{namespace}}", ns.Slug} ) for connectionID, modules := range modulesByConnection(mm...) { @@ -1186,27 +1194,22 @@ func modulesToModelSet(ctx context.Context, dmm dalModelManager, ns *types.Names return } - // Prepare the ident formatter for this connection - ff := dal.IdentFormatter(formatterNamespaceParams(ns)...) - if cm.PartitionValidator != "" { - ff, err = ff.WithValidationE(cm.PartitionValidator, nil) - if err != nil { - return - } - } - // Convert all modules to models for _, mod := range modules { + // convert each module to model model, err = moduleToModel(cm, mod) if err != nil { return } - // ensure partition placeholders are replaced with actual partition values - model.Ident, err = replaceModelIdentPlaceholders(ctx, ff, mod, model.Ident) - if err != nil { - return - } + // construct partition replacement pairs from namespace & module handles + // {{module}} is replaced with module handle + modPartition = append(nsPartition, "{{module}}", mod.Handle) + + // replace all partition replacement pairs + model.Ident = strings.NewReplacer(modPartition...).Replace(model.Ident) + + // @todo validate ident with connection's ident validator model.ConnectionID = connectionID out = append(out, model) @@ -1222,15 +1225,13 @@ func modulesToModelSet(ctx context.Context, dmm dalModelManager, ns *types.Names // the same ID and to avoid collisions with the model rModel.ResourceID = mod.ID + 1 - revIdent := mod.Config.RecordRevisions.Ident - if revIdent == "" { - revIdent = "compose_record_revisions" + if rModel.Ident = mod.Config.RecordRevisions.Ident; rModel.Ident == "" { + rModel.Ident = "compose_record_revisions" } - rModel.Ident, err = replaceModelIdentPlaceholders(ctx, ff, mod, revIdent) - if err != nil { - return - } + rModel.Ident = strings.NewReplacer(modPartition...).Replace(rModel.Ident) + + // @todo validate ident with connection's ident validator out = append(out, rModel) } @@ -1241,6 +1242,8 @@ func modulesToModelSet(ctx context.Context, dmm dalModelManager, ns *types.Names } // moduleToModel converts a module with fields to DAL model and attributes +// +// note: this function does not do any partition placeholder replacements func moduleToModel(cm dal.ConnectionConfig, mod *types.Module) (model *dal.Model, err error) { var ( attrAux dal.AttributeSet @@ -1278,20 +1281,6 @@ func moduleToModel(cm dal.ConnectionConfig, mod *types.Module) (model *dal.Model return } -func replaceModelIdentPlaceholders(ctx context.Context, ff identFormatter, mod *types.Module, ident string) (_ string, err error) { - var ( - ok bool - ) - - ident, ok = ff.Format(ctx, ident, formatterModuleParams(mod)...) - if !ok { - err = fmt.Errorf("invalid model ident generated: %s", ident) - return - } - - return ident, nil -} - // moduleFieldsToAttributes converts all user-defined module fields to attributes func moduleFieldsToAttributes(cm dal.ConnectionConfig, mod *types.Module) (out dal.AttributeSet, err error) { out = make(dal.AttributeSet, 0, len(mod.Fields)) @@ -1462,19 +1451,3 @@ func modulesByConnection(modules ...*types.Module) map[uint64]types.ModuleSet { return out } - -// formatterNamespaceParams returns the base namespace params used for ident formatting -func formatterNamespaceParams(ns *types.Namespace) []string { - nsHandle, _ := handle.Cast(nil, ns.Slug, strconv.FormatUint(ns.ID, 10)) - return []string{ - "namespace", nsHandle, - } -} - -// formatterModuleParams returns the base module params used for ident formatting -func formatterModuleParams(mod *types.Module) []string { - modHandle, _ := handle.Cast(nil, mod.Handle, strconv.FormatUint(mod.ID, 10)) - return []string{ - "module", modHandle, - } -} diff --git a/pkg/dal/conn.go b/pkg/dal/conn.go new file mode 100644 index 000000000..d7ba65e8c --- /dev/null +++ b/pkg/dal/conn.go @@ -0,0 +1,48 @@ +package dal + +import "regexp" + +type ( + ConnectionWrap struct { + connectionID uint64 + + connection Connection + params ConnectionParams + meta ConnectionConfig + operations OperationSet + } + + ConnectionConfig struct { + ConnectionID uint64 + SensitivityLevelID uint64 + Label string + + // When model does not specify the ident (table name for example), fallback to this + ModelIdent string + + // when a new model is added on a connection, it's ident + // is verified against this regexp + // + // ident is considered valid if it matches one of the expressions + // or if the list of checks is empty + ModelIdentCheck []*regexp.Regexp + + // If model attribute(s) do not specify + // @todo needs to be more explicit that this is for JSON encode attributes + AttributeIdent string + } +) + +func checkIdent(ident string, rr ...*regexp.Regexp) bool { + if len(rr) == 0 { + return true + } + + for _, r := range rr { + if r.MatchString(ident) { + return true + } + } + + return false +} diff --git a/pkg/dal/conn_test.go b/pkg/dal/conn_test.go new file mode 100644 index 000000000..c5fd62636 --- /dev/null +++ b/pkg/dal/conn_test.go @@ -0,0 +1,53 @@ +package dal + +import ( + "regexp" + "testing" +) + +func Test_checkIdent(t *testing.T) { + tests := []struct { + name string + ident string + rr []*regexp.Regexp + want bool + }{ + { + name: "empty", + ident: "", + rr: []*regexp.Regexp{}, + want: true, + }, + { + name: "one", + ident: "foo", + rr: []*regexp.Regexp{regexp.MustCompile("foo")}, + want: true, + }, + { + name: "false", + ident: "foo", + rr: []*regexp.Regexp{regexp.MustCompile("bar")}, + want: false, + }, + { + name: "two", + ident: "bar", + rr: []*regexp.Regexp{regexp.MustCompile("foo"), regexp.MustCompile("bar")}, + want: true, + }, + { + name: "two failed", + ident: "foo", + rr: []*regexp.Regexp{regexp.MustCompile("bar"), regexp.MustCompile("baz")}, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := checkIdent(tt.ident, tt.rr...); got != tt.want { + t.Errorf("checkIdent() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/pkg/dal/ident_formatter.go b/pkg/dal/ident_formatter.go deleted file mode 100644 index fc4ad814a..000000000 --- a/pkg/dal/ident_formatter.go +++ /dev/null @@ -1,120 +0,0 @@ -package dal - -import ( - "context" - "errors" - "fmt" - "strings" - - "github.com/PaesslerAG/gval" - "github.com/cortezaproject/corteza-server/pkg/expr" -) - -type ( - identFormatter struct { - identValidator string - identValidatorP gval.Evaluable - identValidatorParams map[string]any - - params []string - } -) - -// IdentFormatter returns an initialized ident formatter preconfigured with given -// base params. -// -// The ident formatter is primarily used for defining model and attribute identifiers. -// -// Base params are provided in key,value pairs and the function panics if an odd number of -// parameters is provided. -func IdentFormatter(baseParams ...string) identFormatter { - out := identFormatter{} - - // Validate params; following what string replacer does - err := out.validateFormatParams(baseParams...) - if err != nil { - panic(fmt.Sprintf("cannot initialize identFormatter: %s", err.Error())) - } - - // Some preprocessing - out.params = out.prepareFormatParams(baseParams...) - return out -} - -// WithValidation binds the given validation expression to the ident formatter -// -// The initial formatter remains unchanged. -func (f identFormatter) WithValidationE(validator string, params map[string]any) (_ identFormatter, err error) { - f.identValidator = validator - f.identValidatorP, err = expr.Parser().NewEvaluable(validator) - f.identValidatorParams = params - return f, err -} - -// WithValidation binds the given validation expression to the ident formatter -// -// The initial formatter remains unchanged. -// -// The function panics if the expression can not be parsed. -func (f identFormatter) WithValidation(validator string, params map[string]any) identFormatter { - out, err := f.WithValidationE(validator, params) - if err != nil { - panic(err) - } - return out -} - -// Format returns a formatted identifier and a flag wether the resulting identifier is valid or not -// -// Parameters are provided in key,value pairs and the function panics if an odd number of -// parameters is provided. -func (f identFormatter) Format(ctx context.Context, template string, params ...string) (out string, ok bool) { - var err error - ok = true - - err = f.validateFormatParams(params...) - if err != nil { - panic(fmt.Sprintf("cannot format template: %s", err.Error())) - } - - f.params = append(f.params, f.prepareFormatParams(params...)...) - - rpl := strings.NewReplacer(f.params...) - out = rpl.Replace(template) - - if f.identValidator != "" { - ok, err = f.identValidatorP.EvalBool(ctx, f.getEvalParams(out)) - ok = ok && (err == nil) - } - - return -} - -// getEvalParams is a helper to get a KV map of parameters for gval ident validation -func (f identFormatter) getEvalParams(ident string) (out map[string]any) { - out = map[string]any{ - "ident": ident, - } - - for k, v := range f.identValidatorParams { - out[k] = v - } - - return -} - -func (f identFormatter) validateFormatParams(params ...string) error { - if len(params)%2 == 1 { - return errors.New("expecting even number of parameters") - } - - return nil -} - -func (f identFormatter) prepareFormatParams(params ...string) []string { - for i := 0; i < len(params); i += 2 { - params[i] = fmt.Sprintf("{{%s}}", params[i]) - } - - return params -} diff --git a/pkg/dal/ident_formatter_test.go b/pkg/dal/ident_formatter_test.go deleted file mode 100644 index cbfe5e3ae..000000000 --- a/pkg/dal/ident_formatter_test.go +++ /dev/null @@ -1,109 +0,0 @@ -package dal - -import ( - "context" - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestIdentFormatterInit_noParams(t *testing.T) { - IdentFormatter() -} - -func TestIdentFormatterInit_okParams(t *testing.T) { - IdentFormatter("k", "v") -} - -func TestIdentFormatterInit_nokParams(t *testing.T) { - assert.Panics(t, func() { - IdentFormatter("k") - }) -} - -func TestWithValidation(t *testing.T) { - _, err := IdentFormatter().WithValidationE("true", nil) - assert.NoError(t, err) -} - -func TestWithValidation_nokExpr(t *testing.T) { - _, err := IdentFormatter().WithValidationE("1variable", nil) - assert.Error(t, err) -} - -func TestWithValidation_nokExpr_panic(t *testing.T) { - assert.Panics(t, func() { - IdentFormatter().WithValidation("1variable", nil) - }) -} - -func TestFormatting(t *testing.T) { - tcc := []struct { - name string - tpl string - params []string - validator string - ident string - ok bool - }{ - { - name: "template without params", - tpl: "identifier", - ident: "identifier", - ok: true, - }, - { - name: "template with params", - tpl: "identifier_{{k}}", - params: []string{"k", "v"}, - ident: "identifier_v", - ok: true, - }, - - { - name: "template without params; validated ok", - tpl: "identifier", - ident: "identifier", - validator: "true", - ok: true, - }, - { - name: "template with params; validated ok", - tpl: "identifier_{{k}}", - params: []string{"k", "v"}, - ident: "identifier_v", - validator: "true", - ok: true, - }, - - { - name: "template without params; validated nok", - tpl: "identifier", - ident: "identifier", - validator: "false", - ok: false, - }, - { - name: "template with params; validated nok", - tpl: "identifier_{{k}}", - params: []string{"k", "v"}, - ident: "identifier_v", - validator: "false", - ok: false, - }, - } - - ctx := context.Background() - for _, c := range tcc { - t.Run(c.name, func(t *testing.T) { - f := IdentFormatter() - if c.validator != "" { - f = f.WithValidation(c.validator, nil) - } - ident, ok := f.Format(ctx, c.tpl, c.params...) - - assert.Equal(t, c.ident, ident) - assert.Equal(t, c.ok, ok) - }) - } -} diff --git a/pkg/dal/service.go b/pkg/dal/service.go index 36c2b4b7e..34e45b48b 100644 --- a/pkg/dal/service.go +++ b/pkg/dal/service.go @@ -3,6 +3,7 @@ package dal import ( "context" "fmt" + "regexp" "strconv" "github.com/cortezaproject/corteza-server/pkg/filter" @@ -10,31 +11,6 @@ import ( ) type ( - ConnectionWrap struct { - connectionID uint64 - - connection Connection - params ConnectionParams - meta ConnectionConfig - operations OperationSet - } - - ConnectionConfig struct { - ConnectionID uint64 - SensitivityLevelID uint64 - Label string - - // When model does not specifiy the ident (table name for example), fallback to this - // @todo do we need a separate setting or can we get away with using just PartitionFormat - ModelIdent string - - // If model attribute(s) do not specify - // @todo needs to be more explicit that this is for JSON encode attributes - AttributeIdent string - - PartitionValidator string - } - service struct { connections map[uint64]*ConnectionWrap @@ -567,6 +543,11 @@ func (svc *service) ReplaceModel(ctx context.Context, model *Model) (err error) connection := svc.getConnectionByID(model.ConnectionID) if !modelIssues && !connectionIssues { + if !checkIdent(model.Ident, connection.meta.ModelIdentCheck...) { + log.Warn("can not add model to connection, invalid ident") + return nil + } + log.Debug("adding to connection") auxIssues, err = svc.registerModelToConnection(ctx, connection, model) issues.mergeWith(auxIssues) @@ -879,6 +860,17 @@ func (svc *service) registerModelToConnection(ctx context.Context, cw *Connectio return } + // make sure connection supports model's ident + var ( + rre []*regexp.Regexp + ) + + for _, re := range rre { + if re.MatchString(model.Ident) { + return + } + } + // Try to add to store err = cw.connection.CreateModel(ctx, model) if err != nil { diff --git a/pkg/provision/dal.go b/pkg/provision/dal.go index d4b3bf681..0635c6cd6 100644 --- a/pkg/provision/dal.go +++ b/pkg/provision/dal.go @@ -16,19 +16,18 @@ const ( ) // Injects primary connection -func defaultDalConnection(ctx context.Context, s store.DalConnections) (err error) { - cc, err := store.LookupDalConnectionByHandle(ctx, s, types.DalPrimaryConnectionHandle) +func defaultDalConnection(ctx context.Context, s store.DalConnections) error { + conn, err := store.LookupDalConnectionByHandle(ctx, s, types.DalPrimaryConnectionHandle) if err != nil && err != store.ErrNotFound { - return + return err } // Already exists - if cc != nil { - return + if conn != nil { + return nil } - // Create it - var conn = &types.DalConnection{ + conn = &types.DalConnection{ // Using id.Next since we dropped "special" ids a while ago. // If needed, use the handle ID: id.Next(), @@ -43,8 +42,7 @@ func defaultDalConnection(ctx context.Context, s store.DalConnections) (err erro DAL: types.ConnectionConfigDAL{ ModelIdent: DefaultComposeRecordTable, AttributeIdent: DefaultComposeRecordValueCol, - - Operations: dal.FullOperations(), + Operations: dal.FullOperations(), }, }, diff --git a/system/service/dal_connection.go b/system/service/dal_connection.go index 709ea49b9..73b055858 100644 --- a/system/service/dal_connection.go +++ b/system/service/dal_connection.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "reflect" + "regexp" "github.com/cortezaproject/corteza-server/pkg/errors" @@ -318,11 +319,29 @@ func dalConnectionReplace(ctx context.Context, primary dal.Connection, dcm dalCo var ( cw *dal.ConnectionWrap isPrimary bool + + connConfig dal.ConnectionConfig ) for _, c := range cc { isPrimary = c.Type == types.DalPrimaryConnectionResourceType + connConfig = dal.ConnectionConfig{ + SensitivityLevelID: c.Config.Privacy.SensitivityLevelID, + ModelIdent: c.Config.DAL.ModelIdent, + AttributeIdent: c.Config.DAL.AttributeIdent, + Label: c.Handle, + } + + if checks := len(c.Config.DAL.ModelIdentCheck); checks > 0 { + connConfig.ModelIdentCheck = make([]*regexp.Regexp, checks) + for i, m := range c.Config.DAL.ModelIdentCheck { + if connConfig.ModelIdentCheck[i], err = regexp.Compile(m); err != nil { + return fmt.Errorf("could not prepare connection model ident check for %q: %w", c.Handle, err) + } + } + } + cw = dal.MakeConnection( c.ID, // When connection is primary (type) we use the primary connection @@ -335,12 +354,7 @@ func dalConnectionReplace(ctx context.Context, primary dal.Connection, dcm dalCo return nil }(), c.Config.Connection, - dal.ConnectionConfig{ - SensitivityLevelID: c.Config.Privacy.SensitivityLevelID, - ModelIdent: c.Config.DAL.ModelIdent, - AttributeIdent: c.Config.DAL.AttributeIdent, - Label: c.Handle, - }, + connConfig, c.Config.DAL.Operations..., ) diff --git a/system/types/dal_connection.go b/system/types/dal_connection.go index 6f289ccf1..7bb1d1fe4 100644 --- a/system/types/dal_connection.go +++ b/system/types/dal_connection.go @@ -70,7 +70,7 @@ type ( ModelIdent string `json:"modelIdent"` AttributeIdent string `json:"attributeIdent"` - PartitionIdentValidator string `json:"partitionIdentValidator"` + ModelIdentCheck []string `json:"modelIdentCheck"` } DalConnectionFilter struct { diff --git a/tests/dal/main_test.go b/tests/dal/main_test.go index 8df74d9d5..028f84a10 100644 --- a/tests/dal/main_test.go +++ b/tests/dal/main_test.go @@ -259,9 +259,6 @@ func (h helper) createDalConnection(res *types.DalConnection) *types.DalConnecti if res.Config.DAL.AttributeIdent == "" { res.Config.DAL.AttributeIdent = "values" } - if res.Config.DAL.PartitionIdentValidator == "" { - res.Config.DAL.PartitionIdentValidator = "" - } if res.Config.Connection.Params == nil { res.Config.Connection = dal.NewDSNConnection("sqlite3://file::memory:?cache=shared&mode=memory") } @@ -301,12 +298,7 @@ func makeConnectionDefinition(dsn string) *types.DalConnection { DAL: types.ConnectionConfigDAL{ ModelIdent: "compose_record", AttributeIdent: "values", - - ModelIdent: "compose_record_{{namespace}}_{{module}}", - - PartitionIdentValidator: "", - - Operations: dal.FullOperations(), + Operations: dal.FullOperations(), }, Connection: dal.NewDSNConnection(dsn), diff --git a/tests/system/dal_connection_crud_test.go b/tests/system/dal_connection_crud_test.go index bfa9ec692..e95d77a56 100644 --- a/tests/system/dal_connection_crud_test.go +++ b/tests/system/dal_connection_crud_test.go @@ -63,9 +63,6 @@ func (h helper) createDalConnection(res *types.DalConnection) *types.DalConnecti if res.Config.DAL.AttributeIdent == "" { res.Config.DAL.AttributeIdent = "values" } - if res.Config.DAL.PartitionIdentValidator == "" { - res.Config.DAL.PartitionIdentValidator = "" - } if res.Config.Connection.Params == nil { res.Config.Connection = dal.NewDSNConnection("sqlite3://file::memory:?cache=shared&mode=memory") }