From ae5e7eb03e766735abd8348eed8425e09375f2e6 Mon Sep 17 00:00:00 2001 From: Milan Freml Date: Wed, 19 Oct 2022 17:57:01 +0200 Subject: [PATCH] Revive #42039 (#43168) * Revert "Revert "Revert "Revert "validate settings and site config in DB layer (#43041)" This reverts commit 1360fcd7495c2360bc5f102e5522b28afab481b1. * Fix true/false boolean problem when creating default site-config --- cmd/frontend/graphqlbackend/site.go | 6 ---- cmd/frontend/internal/cli/config.go | 9 ++++-- internal/database/conf.go | 31 +++++++++++++------ internal/database/conf_test.go | 48 +++++++++++++++++------------ internal/database/mocks_temp.go | 29 +++++++++-------- internal/database/settings.go | 8 +---- internal/database/settings_test.go | 2 +- 7 files changed, 75 insertions(+), 58 deletions(-) diff --git a/cmd/frontend/graphqlbackend/site.go b/cmd/frontend/graphqlbackend/site.go index 885f051602a..821288aee28 100644 --- a/cmd/frontend/graphqlbackend/site.go +++ b/cmd/frontend/graphqlbackend/site.go @@ -172,12 +172,6 @@ func (r *schemaResolver) UpdateSiteConfiguration(ctx context.Context, args *stru return false, errors.Errorf("blank site configuration is invalid (you can clear the site configuration by entering an empty JSON object: {})") } - if problems, err := conf.ValidateSite(args.Input); err != nil { - return false, errors.Errorf("failed to validate site configuration: %w", err) - } else if len(problems) > 0 { - return false, errors.Errorf("site configuration is invalid: %s", strings.Join(problems, ",")) - } - prev := conf.Raw() unredacted, err := conf.UnredactSecrets(args.Input, prev) if err != nil { diff --git a/cmd/frontend/internal/cli/config.go b/cmd/frontend/internal/cli/config.go index 05188efa6f1..41ac32cba69 100644 --- a/cmd/frontend/internal/cli/config.go +++ b/cmd/frontend/internal/cli/config.go @@ -119,7 +119,7 @@ func overrideSiteConfig(ctx context.Context, logger log.Logger, db database.DB) } raw.Site = string(site) - err = cs.Write(ctx, raw, raw.ID) + err = cs.WriteWithOverride(ctx, raw, raw.ID, true) if err != nil { return errors.Wrap(err, "writing site config overrides to database") } @@ -452,6 +452,10 @@ func (c *configurationSource) Read(ctx context.Context) (conftypes.RawUnified, e } func (c *configurationSource) Write(ctx context.Context, input conftypes.RawUnified, lastID int32) error { + return c.WriteWithOverride(ctx, input, lastID, false) +} + +func (c *configurationSource) WriteWithOverride(ctx context.Context, input conftypes.RawUnified, lastID int32, isOverride bool) error { site, err := c.db.Conf().SiteGetLatest(ctx) if err != nil { return errors.Wrap(err, "ConfStore.SiteGetLatest") @@ -459,8 +463,9 @@ func (c *configurationSource) Write(ctx context.Context, input conftypes.RawUnif if site.ID != lastID { return errors.New("site config has been modified by another request, write not allowed") } - _, err = c.db.Conf().SiteCreateIfUpToDate(ctx, &site.ID, input.Site) + _, err = c.db.Conf().SiteCreateIfUpToDate(ctx, &site.ID, input.Site, isOverride) if err != nil { + log.Error(errors.Wrap(err, "SiteConfig creation failed")) return errors.Wrap(err, "ConfStore.SiteCreateIfUpToDate") } return nil diff --git a/internal/database/conf.go b/internal/database/conf.go index 58da3f5bdc9..9ad0e891aca 100644 --- a/internal/database/conf.go +++ b/internal/database/conf.go @@ -3,12 +3,14 @@ package database import ( "context" "database/sql" + "strings" "time" "github.com/keegancsmith/sqlf" - "github.com/sourcegraph/jsonx" + "github.com/sourcegraph/sourcegraph/internal/conf" "github.com/sourcegraph/sourcegraph/internal/conf/confdefaults" + "github.com/sourcegraph/sourcegraph/internal/conf/conftypes" "github.com/sourcegraph/sourcegraph/internal/database/basestore" "github.com/sourcegraph/sourcegraph/internal/database/dbutil" "github.com/sourcegraph/sourcegraph/lib/errors" @@ -27,7 +29,7 @@ type ConfStore interface { // // 🚨 SECURITY: This method does NOT verify the user is an admin. The caller is // responsible for ensuring this or that the response never makes it to a user. - SiteCreateIfUpToDate(ctx context.Context, lastID *int32, contents string) (*SiteConfig, error) + SiteCreateIfUpToDate(ctx context.Context, lastID *int32, contents string, isOverride bool) (*SiteConfig, error) // SiteGetLatest returns the site config that was most recently saved to the database. // This returns nil, nil if there is not yet a site config in the database. @@ -76,7 +78,7 @@ func (s *confStore) transact(ctx context.Context) (*confStore, error) { return &confStore{Store: txBase}, nil } -func (s *confStore) SiteCreateIfUpToDate(ctx context.Context, lastID *int32, contents string) (_ *SiteConfig, err error) { +func (s *confStore) SiteCreateIfUpToDate(ctx context.Context, lastID *int32, contents string, isOverride bool) (_ *SiteConfig, err error) { tx, err := s.transact(ctx) if err != nil { return nil, err @@ -90,7 +92,7 @@ func (s *confStore) SiteCreateIfUpToDate(ctx context.Context, lastID *int32, con if newLastID != nil { lastID = newLastID } - return tx.createIfUpToDate(ctx, lastID, contents) + return tx.createIfUpToDate(ctx, lastID, contents, isOverride) } func (s *confStore) SiteGetLatest(ctx context.Context) (_ *SiteConfig, err error) { @@ -118,7 +120,7 @@ func (s *confStore) addDefault(ctx context.Context, contents string) (newLastID return nil, nil } - latest, err = s.createIfUpToDate(ctx, nil, contents) + latest, err = s.createIfUpToDate(ctx, nil, contents, true) if err != nil { return nil, err } @@ -131,10 +133,21 @@ VALUES ('site', %s) RETURNING %s -- siteConfigColumns ` -func (s *confStore) createIfUpToDate(ctx context.Context, lastID *int32, contents string) (*SiteConfig, error) { - // Validate JSON syntax before saving. - if _, errs := jsonx.Parse(contents, jsonx.ParseOptions{Comments: true, TrailingCommas: true}); len(errs) > 0 { - return nil, errors.Errorf("invalid settings JSON: %v", errs) +func (s *confStore) createIfUpToDate(ctx context.Context, lastID *int32, contents string, isOverride bool) (*SiteConfig, error) { + // Validate config for syntax and by the JSON Schema. + var problems []string + var err error + if isOverride { + var problemStruct conf.Problems + problemStruct, err = conf.Validate(conftypes.RawUnified{Site: contents}) + problems = problemStruct.Messages() + } else { + problems, err = conf.ValidateSite(contents) + } + if err != nil { + return nil, errors.Errorf("failed to validate site configuration: %w", err) + } else if len(problems) > 0 { + return nil, errors.Errorf("site configuration is invalid: %s", strings.Join(problems, ",")) } latest, err := s.getLatest(ctx) diff --git a/internal/database/conf_test.go b/internal/database/conf_test.go index f4c527c5dbb..8410fc5d1f5 100644 --- a/internal/database/conf_test.go +++ b/internal/database/conf_test.go @@ -5,6 +5,8 @@ import ( "strings" "testing" + "github.com/sourcegraph/sourcegraph/lib/errors" + "github.com/sourcegraph/log/logtest" "github.com/sourcegraph/sourcegraph/internal/database/dbtest" @@ -41,9 +43,9 @@ func TestSiteCreate_RejectInvalidJSON(t *testing.T) { malformedJSON := "[This is malformed.}" - _, err := db.Conf().SiteCreateIfUpToDate(ctx, nil, malformedJSON) + _, err := db.Conf().SiteCreateIfUpToDate(ctx, nil, malformedJSON, false) - if err == nil || !strings.Contains(err.Error(), "invalid settings JSON") { + if err == nil || !strings.Contains(err.Error(), "failed to parse JSON") { t.Fatalf("expected parse error after creating configuration with malformed JSON, got: %+v", err) } } @@ -80,11 +82,11 @@ func TestSiteCreateIfUpToDate(t *testing.T) { { input{ lastID: 0, - contents: `"This is a test."`, + contents: `{"defaultRateLimit": 0,"auth.providers": []}`, }, output{ ID: 2, - contents: `"This is a test."`, + contents: `{"defaultRateLimit": 0,"auth.providers": []}`, }, }, }, @@ -95,21 +97,21 @@ func TestSiteCreateIfUpToDate(t *testing.T) { { input{ lastID: 0, - contents: `"This is the first one."`, + contents: `{"defaultRateLimit": 0,"auth.providers": []}`, }, output{ ID: 2, - contents: `"This is the first one."`, + contents: `{"defaultRateLimit": 0,"auth.providers": []}`, }, }, { input{ lastID: 2, - contents: `"This is the second one."`, + contents: `{"defaultRateLimit": 1,"auth.providers": []}`, }, output{ ID: 3, - contents: `"This is the second one."`, + contents: `{"defaultRateLimit": 1,"auth.providers": []}`, }, }, }, @@ -120,22 +122,23 @@ func TestSiteCreateIfUpToDate(t *testing.T) { { input{ lastID: 0, - contents: `"This is the first one."`, + contents: `{"defaultRateLimit": 0,"auth.providers": []}`, }, output{ ID: 2, - contents: `"This is the first one."`, + contents: `{"defaultRateLimit": 0,"auth.providers": []}`, }, }, { input{ - lastID: 0, - contents: `"This configuration is now behind the first one, so it shouldn't be saved."`, + lastID: 0, + // This configuration is now behind the first one, so it shouldn't be saved + contents: `{"defaultRateLimit": 1,"auth.providers": []}`, }, output{ ID: 2, - contents: `"This is the first one."`, - err: ErrNewerEdit, + contents: `{"defaultRateLimit": 1,"auth.providers": []}`, + err: errors.Append(ErrNewerEdit), }, }, }, @@ -146,32 +149,37 @@ func TestSiteCreateIfUpToDate(t *testing.T) { { input{ lastID: 0, - contents: `{"fieldA": "valueA", + contents: `{"disableBuiltInSearches": true, // This is a comment. - "fieldB": "valueB", + "defaultRateLimit": 42, + "auth.providers": [], }`, }, output{ ID: 2, - contents: `{"fieldA": "valueA", + contents: `{"disableBuiltInSearches": true, // This is a comment. - "fieldB": "valueB", + "defaultRateLimit": 42, + "auth.providers": [], }`, }, }, }, }, } { + // we were running the same test all the time, see this gist for more information + // https://gist.github.com/posener/92a55c4cd441fc5e5e85f27bca008721 + test := test t.Run(test.name, func(t *testing.T) { t.Parallel() db := NewDB(logger, dbtest.NewDB(logger, t)) ctx := context.Background() for _, p := range test.sequence { - output, err := db.Conf().SiteCreateIfUpToDate(ctx, &p.input.lastID, p.input.contents) + output, err := db.Conf().SiteCreateIfUpToDate(ctx, &p.input.lastID, p.input.contents, false) if err != nil { - if err == p.expected.err { + if errors.Is(err, p.expected.err) { continue } t.Fatal(err) diff --git a/internal/database/mocks_temp.go b/internal/database/mocks_temp.go index ba6d792cb72..618e788a8c2 100644 --- a/internal/database/mocks_temp.go +++ b/internal/database/mocks_temp.go @@ -3020,7 +3020,7 @@ func NewMockConfStore() *MockConfStore { }, }, SiteCreateIfUpToDateFunc: &ConfStoreSiteCreateIfUpToDateFunc{ - defaultHook: func(context.Context, *int32, string) (r0 *SiteConfig, r1 error) { + defaultHook: func(context.Context, *int32, string, bool) (r0 *SiteConfig, r1 error) { return }, }, @@ -3052,7 +3052,7 @@ func NewStrictMockConfStore() *MockConfStore { }, }, SiteCreateIfUpToDateFunc: &ConfStoreSiteCreateIfUpToDateFunc{ - defaultHook: func(context.Context, *int32, string) (*SiteConfig, error) { + defaultHook: func(context.Context, *int32, string, bool) (*SiteConfig, error) { panic("unexpected invocation of MockConfStore.SiteCreateIfUpToDate") }, }, @@ -3294,24 +3294,24 @@ func (c ConfStoreHandleFuncCall) Results() []interface{} { // SiteCreateIfUpToDate method of the parent MockConfStore instance is // invoked. type ConfStoreSiteCreateIfUpToDateFunc struct { - defaultHook func(context.Context, *int32, string) (*SiteConfig, error) - hooks []func(context.Context, *int32, string) (*SiteConfig, error) + defaultHook func(context.Context, *int32, string, bool) (*SiteConfig, error) + hooks []func(context.Context, *int32, string, bool) (*SiteConfig, error) history []ConfStoreSiteCreateIfUpToDateFuncCall mutex sync.Mutex } // SiteCreateIfUpToDate delegates to the next hook function in the queue and // stores the parameter and result values of this invocation. -func (m *MockConfStore) SiteCreateIfUpToDate(v0 context.Context, v1 *int32, v2 string) (*SiteConfig, error) { - r0, r1 := m.SiteCreateIfUpToDateFunc.nextHook()(v0, v1, v2) - m.SiteCreateIfUpToDateFunc.appendCall(ConfStoreSiteCreateIfUpToDateFuncCall{v0, v1, v2, r0, r1}) +func (m *MockConfStore) SiteCreateIfUpToDate(v0 context.Context, v1 *int32, v2 string, v3 bool) (*SiteConfig, error) { + r0, r1 := m.SiteCreateIfUpToDateFunc.nextHook()(v0, v1, v2, v3) + m.SiteCreateIfUpToDateFunc.appendCall(ConfStoreSiteCreateIfUpToDateFuncCall{v0, v1, v2, v3, r0, r1}) return r0, r1 } // SetDefaultHook sets function that is called when the SiteCreateIfUpToDate // method of the parent MockConfStore instance is invoked and the hook queue // is empty. -func (f *ConfStoreSiteCreateIfUpToDateFunc) SetDefaultHook(hook func(context.Context, *int32, string) (*SiteConfig, error)) { +func (f *ConfStoreSiteCreateIfUpToDateFunc) SetDefaultHook(hook func(context.Context, *int32, string, bool) (*SiteConfig, error)) { f.defaultHook = hook } @@ -3319,7 +3319,7 @@ func (f *ConfStoreSiteCreateIfUpToDateFunc) SetDefaultHook(hook func(context.Con // SiteCreateIfUpToDate method of the parent MockConfStore instance invokes // the hook at the front of the queue and discards it. After the queue is // empty, the default hook function is invoked for any future action. -func (f *ConfStoreSiteCreateIfUpToDateFunc) PushHook(hook func(context.Context, *int32, string) (*SiteConfig, error)) { +func (f *ConfStoreSiteCreateIfUpToDateFunc) PushHook(hook func(context.Context, *int32, string, bool) (*SiteConfig, error)) { f.mutex.Lock() f.hooks = append(f.hooks, hook) f.mutex.Unlock() @@ -3328,19 +3328,19 @@ func (f *ConfStoreSiteCreateIfUpToDateFunc) PushHook(hook func(context.Context, // SetDefaultReturn calls SetDefaultHook with a function that returns the // given values. func (f *ConfStoreSiteCreateIfUpToDateFunc) SetDefaultReturn(r0 *SiteConfig, r1 error) { - f.SetDefaultHook(func(context.Context, *int32, string) (*SiteConfig, error) { + f.SetDefaultHook(func(context.Context, *int32, string, bool) (*SiteConfig, error) { return r0, r1 }) } // PushReturn calls PushHook with a function that returns the given values. func (f *ConfStoreSiteCreateIfUpToDateFunc) PushReturn(r0 *SiteConfig, r1 error) { - f.PushHook(func(context.Context, *int32, string) (*SiteConfig, error) { + f.PushHook(func(context.Context, *int32, string, bool) (*SiteConfig, error) { return r0, r1 }) } -func (f *ConfStoreSiteCreateIfUpToDateFunc) nextHook() func(context.Context, *int32, string) (*SiteConfig, error) { +func (f *ConfStoreSiteCreateIfUpToDateFunc) nextHook() func(context.Context, *int32, string, bool) (*SiteConfig, error) { f.mutex.Lock() defer f.mutex.Unlock() @@ -3383,6 +3383,9 @@ type ConfStoreSiteCreateIfUpToDateFuncCall struct { // Arg2 is the value of the 3rd argument passed to this method // invocation. Arg2 string + // Arg3 is the value of the 4th argument passed to this method + // invocation. + Arg3 bool // Result0 is the value of the 1st result returned from this method // invocation. Result0 *SiteConfig @@ -3394,7 +3397,7 @@ type ConfStoreSiteCreateIfUpToDateFuncCall struct { // Args returns an interface slice containing the arguments of this // invocation. func (c ConfStoreSiteCreateIfUpToDateFuncCall) Args() []interface{} { - return []interface{}{c.Arg0, c.Arg1, c.Arg2} + return []interface{}{c.Arg0, c.Arg1, c.Arg2, c.Arg3} } // Results returns an interface slice containing the results of this diff --git a/internal/database/settings.go b/internal/database/settings.go index 1eea7785f97..35344889afe 100644 --- a/internal/database/settings.go +++ b/internal/database/settings.go @@ -6,7 +6,6 @@ import ( "strings" "github.com/keegancsmith/sqlf" - "github.com/sourcegraph/jsonx" "github.com/sourcegraph/sourcegraph/internal/api" "github.com/sourcegraph/sourcegraph/internal/conf" @@ -47,12 +46,7 @@ func (s *settingsStore) Transact(ctx context.Context) (SettingsStore, error) { } func (o *settingsStore) CreateIfUpToDate(ctx context.Context, subject api.SettingsSubject, lastID *int32, authorUserID *int32, contents string) (latestSetting *api.Settings, err error) { - // Validate JSON syntax before saving. - if _, errs := jsonx.Parse(contents, jsonx.ParseOptions{Comments: true, TrailingCommas: true}); len(errs) > 0 { - return nil, errors.Errorf("invalid settings JSON: %v", errs) - } - - // Validate setting schema + // Validate settings for syntax and by the JSON Schema. if problems := conf.ValidateSettings(contents); len(problems) > 0 { return nil, errors.Errorf("invalid settings: %s", strings.Join(problems, ",")) } diff --git a/internal/database/settings_test.go b/internal/database/settings_test.go index 14ef55ae229..e17a889a103 100644 --- a/internal/database/settings_test.go +++ b/internal/database/settings_test.go @@ -115,7 +115,7 @@ func TestCreateIfUpToDate(t *testing.T) { t.Run("syntactically invalid settings", func(t *testing.T) { contents := `{` - wantErr := "invalid settings JSON: [CloseBraceExpected]" + wantErr := "invalid settings: failed to parse JSON: [CloseBraceExpected]" _, err := db.Settings().CreateIfUpToDate(ctx, api.SettingsSubject{User: &u.ID}, nil, nil, contents) if err == nil || err.Error() != wantErr { t.Errorf("got err %q, want %q", err, wantErr)