artin/sourcegraph
1
0
Fork 0
mirror of https://github.com/sourcegraph/sourcegraph.git synced 2026-02-06 18:31:54 +00:00
Code Issues Packages Projects Releases Wiki Activity

Revive #42039 (#43168)

Browse Source 
* Revert "Revert "Revert "Revert "validate settings and site config in DB layer (#43041)"

This reverts commit 1360fcd749.

* Fix true/false boolean problem when creating default site-config
This commit is contained in:
Milan Freml 2022-10-19 17:57:01 +02:00 committed by GitHub
parent 46ef216178
commit ae5e7eb03e
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 75 additions and 58 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
cmd/frontend/graphqlbackend/site.go
View File

@ -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 {

9
cmd/frontend/internal/cli/config.go
View File

@ -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

31
internal/database/conf.go
View File

@ -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)

48
internal/database/conf_test.go
View File

@ -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)

29
internal/database/mocks_temp.go
View File

@ -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

8
internal/database/settings.go
View File

@ -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, ","))
}

2
internal/database/settings_test.go
View File

@ -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)