diff --git a/cmd/frontend/graphqlbackend/schema.graphql b/cmd/frontend/graphqlbackend/schema.graphql index a07535e5e80b..fff5f2958c6b 100755 --- a/cmd/frontend/graphqlbackend/schema.graphql +++ b/cmd/frontend/graphqlbackend/schema.graphql @@ -6833,10 +6833,19 @@ type SiteConfigurationChange implements Node { via an internal process (example: site startup or SITE_CONFIG_FILE being reloaded). """ author: User + + """ + A flag to indicate if the diff of changes to the previous site configuration + was reproduced or not. Sometimes it may not be possible to generate a diff + (for example the redacted contents are not available) in which case the + value of the flag will be false. + """ + reproducedDiff: Boolean! + """ - The diff string when diffed against the site config at previousID. + The diff string when diffed against the previous site config. When this is null there was no diff to the previous change. """ - diff: String! + diff: String """ The timestamp when this change in the site config was applied. """ diff --git a/cmd/frontend/graphqlbackend/site_config_change.go b/cmd/frontend/graphqlbackend/site_config_change.go index 74c168313d1b..2c98878024e6 100644 --- a/cmd/frontend/graphqlbackend/site_config_change.go +++ b/cmd/frontend/graphqlbackend/site_config_change.go @@ -2,11 +2,15 @@ package graphqlbackend import ( "context" + "fmt" "github.com/graph-gophers/graphql-go" "github.com/graph-gophers/graphql-go/relay" "github.com/sourcegraph/sourcegraph/internal/database" "github.com/sourcegraph/sourcegraph/internal/gqlutil" + + "github.com/hexops/gotextdiff" + "github.com/hexops/gotextdiff/myers" ) const siteConfigurationChangeKind = "SiteConfigurationChange" @@ -21,11 +25,6 @@ func (r SiteConfigurationChangeResolver) ID() graphql.ID { return marshalSiteConfigurationChangeID(r.siteConfig.ID) } -// One line wrapper to be able to use in tests as well. -func marshalSiteConfigurationChangeID(id int32) graphql.ID { - return relay.MarshalID(siteConfigurationChangeKind, &id) -} - func (r SiteConfigurationChangeResolver) Author(ctx context.Context) (*UserResolver, error) { if r.siteConfig.AuthorUserID == 0 { return nil, nil @@ -39,17 +38,48 @@ func (r SiteConfigurationChangeResolver) Author(ctx context.Context) (*UserResol return user, nil } -// TODO: Implement this. -func (r SiteConfigurationChangeResolver) Diff() string { - // TODO: We will do something like this, but for now return an empty string to not leak secrets - // until we have implemented redaction. - // - // if r.previousSiteConfig == nil { - // return "" - // } +func (r SiteConfigurationChangeResolver) ReproducedDiff() bool { + // If we were able to redact contents for both siteConfig and previousSiteConfig and store it in + // the DB, then we can generate a diff. + if r.siteConfig != nil { + // As a special case, if previousSiteConfig is nil (first entry in the DB) and we also have + // redactedContents available for this siteConfig, we can generate a diff (it will be all + // lines added). + if r.previousSiteConfig == nil && r.siteConfig.RedactedContents != "" { + return true + } + + if r.siteConfig.RedactedContents != "" && r.previousSiteConfig.RedactedContents != "" { + return true + } + } + + return false +} + +func (r SiteConfigurationChangeResolver) Diff() *string { + if !r.ReproducedDiff() { + return nil + } + + var prevID int32 + var prevRedactedContents string + if r.previousSiteConfig != nil { + prevID = r.previousSiteConfig.ID + + // 🚨 SECURITY: This should always use "siteConfig.RedactedContents" and never + // "siteConfig.Contents" to generate the diff because we do not want to leak secrets in the + // diff. + prevRedactedContents = r.previousSiteConfig.RedactedContents + } + + prettyID := func(id int32) string { return fmt.Sprintf("ID: %d", id) } + + // We're not diffing a file, so set an empty string for the URI argument. + edits := myers.ComputeEdits("", prevRedactedContents, r.siteConfig.RedactedContents) + diff := fmt.Sprint(gotextdiff.ToUnified(prettyID(prevID), prettyID(r.siteConfig.ID), prevRedactedContents, edits)) - // return cmp.Diff(r.siteConfig.Contents, r.previousSiteConfig.Contents) - return "" + return &diff } func (r SiteConfigurationChangeResolver) CreatedAt() gqlutil.DateTime { @@ -59,3 +89,8 @@ func (r SiteConfigurationChangeResolver) CreatedAt() gqlutil.DateTime { func (r SiteConfigurationChangeResolver) UpdatedAt() gqlutil.DateTime { return gqlutil.DateTime{Time: r.siteConfig.UpdatedAt} } + +// One line wrapper to be able to use in tests as well. +func marshalSiteConfigurationChangeID(id int32) graphql.ID { + return relay.MarshalID(siteConfigurationChangeKind, &id) +} diff --git a/cmd/frontend/graphqlbackend/site_config_change_connection_test.go b/cmd/frontend/graphqlbackend/site_config_change_connection_test.go index 993aa529072f..c292c6cdd995 100644 --- a/cmd/frontend/graphqlbackend/site_config_change_connection_test.go +++ b/cmd/frontend/graphqlbackend/site_config_change_connection_test.go @@ -15,9 +15,9 @@ import ( ) type siteConfigStubs struct { - db database.DB - users []*types.User - siteConfigs []*database.SiteConfig + db database.DB + users []*types.User + expectedDiffs map[int32]string } func toStringPtr(n int) *string { @@ -52,35 +52,35 @@ func setupSiteConfigStubs(t *testing.T) *siteConfigStubs { conf := db.Conf() siteConfigsToCreate := []*database.SiteConfig{ + // ID: 2 (because first time we create a config an initial config will be created first) { - Contents: ` -{ + Contents: `{ "auth.Providers": [] }`, }, + // ID: 3 { AuthorUserID: 2, // A new line is added. - Contents: ` -{ + Contents: `{ "disableAutoGitUpdates": true, "auth.Providers": [] }`, }, + // ID: 4 { AuthorUserID: 1, // Existing line is changed. - Contents: ` -{ + Contents: `{ "disableAutoGitUpdates": false, "auth.Providers": [] }`, }, + // ID: 5 { AuthorUserID: 1, // Existing line is removed. - Contents: ` -{ + Contents: `{ "auth.Providers": [] }`, }, @@ -98,15 +98,96 @@ func setupSiteConfigStubs(t *testing.T) *siteConfigStubs { lastID = siteConfig.ID } + expectedDiffs := map[int32]string{ + 5: `--- ID: 4 ++++ ID: 5 +@@ -1,4 +1,3 @@ + { +- "disableAutoGitUpdates": false, + "auth.Providers": [] + } +\ No newline at end of file +`, + + 4: `--- ID: 3 ++++ ID: 4 +@@ -1,4 +1,4 @@ + { +- "disableAutoGitUpdates": true, ++ "disableAutoGitUpdates": false, + "auth.Providers": [] + } +\ No newline at end of file +`, + + 3: `--- ID: 2 ++++ ID: 3 +@@ -1,3 +1,4 @@ + { ++ "disableAutoGitUpdates": true, + "auth.Providers": [] + } +\ No newline at end of file +`, + + 2: `--- ID: 1 ++++ ID: 2 +@@ -1,17 +1,3 @@ + { +- // The externally accessible URL for Sourcegraph (i.e., what you type into your browser) +- // This is required to be configured for Sourcegraph to work correctly. +- // "externalURL": "https://sourcegraph.example.com", +- // The authentication provider to use for identifying and signing in users. +- // Only one entry is supported. +- // +- // The builtin auth provider with signup disallowed (shown below) means that +- // after the initial site admin signs in, all other users must be invited. +- // +- // Other providers are documented at https://docs.sourcegraph.com/admin/auth. +- "auth.providers": [ +- { +- "type": "builtin" +- } +- ], ++ "auth.Providers": [] + } +\ No newline at end of file +`, + + 1: `--- ID: 0 ++++ ID: 1 +@@ -1 +1,17 @@ ++{ ++ // The externally accessible URL for Sourcegraph (i.e., what you type into your browser) ++ // This is required to be configured for Sourcegraph to work correctly. ++ // "externalURL": "https://sourcegraph.example.com", ++ // The authentication provider to use for identifying and signing in users. ++ // Only one entry is supported. ++ // ++ // The builtin auth provider with signup disallowed (shown below) means that ++ // after the initial site admin signs in, all other users must be invited. ++ // ++ // Other providers are documented at https://docs.sourcegraph.com/admin/auth. ++ "auth.providers": [ ++ { ++ "type": "builtin" ++ } ++ ], ++} +\ No newline at end of file +`, + } + return &siteConfigStubs{ - db: db, - users: users, - // siteConfigs: siteConfigs, + db: db, + users: users, + expectedDiffs: expectedDiffs, } } func TestSiteConfigConnection(t *testing.T) { stubs := setupSiteConfigStubs(t) + expectedDiffs := stubs.expectedDiffs // Create a context with an admin user as the actor. contextWithActor := actor.WithActor(context.Background(), &actor.Actor{UID: 1}) @@ -131,6 +212,8 @@ func TestSiteConfigConnection(t *testing.T) { username, displayName } + reproducedDiff + diff } pageInfo { hasNextPage @@ -158,7 +241,9 @@ func TestSiteConfigConnection(t *testing.T) { "id": "VXNlcjox", "username": "foo", "displayName": "foo user" - } + }, + "reproducedDiff": true, + "diff": %[3]q }, { "id": %[2]q, @@ -166,7 +251,9 @@ func TestSiteConfigConnection(t *testing.T) { "id": "VXNlcjox", "username": "foo", "displayName": "foo user" - } + }, + "reproducedDiff": true, + "diff": %[4]q } ], "pageInfo": { @@ -179,7 +266,7 @@ func TestSiteConfigConnection(t *testing.T) { } } } - `, marshalSiteConfigurationChangeID(5), marshalSiteConfigurationChangeID(4)), + `, marshalSiteConfigurationChangeID(5), marshalSiteConfigurationChangeID(4), expectedDiffs[5], expectedDiffs[4]), }, { Schema: mustParseGraphQLSchema(t, stubs.db), @@ -200,6 +287,8 @@ func TestSiteConfigConnection(t *testing.T) { username, displayName } + reproducedDiff + diff } pageInfo { hasNextPage @@ -227,15 +316,21 @@ func TestSiteConfigConnection(t *testing.T) { "id": "VXNlcjoy", "username": "bar", "displayName": "bar user" - } + }, + "reproducedDiff": true, + "diff": %[4]q }, { "id": %[2]q, - "author": null + "author": null, + "reproducedDiff": true, + "diff": %[5]q }, { "id": %[3]q, - "author": null + "author": null, + "reproducedDiff": true, + "diff": %[6]q } ], "pageInfo": { @@ -248,7 +343,9 @@ func TestSiteConfigConnection(t *testing.T) { } } } - `, marshalSiteConfigurationChangeID(3), marshalSiteConfigurationChangeID(2), marshalSiteConfigurationChangeID(1)), + `, marshalSiteConfigurationChangeID(3), marshalSiteConfigurationChangeID(2), marshalSiteConfigurationChangeID(1), + expectedDiffs[3], expectedDiffs[2], expectedDiffs[1], + ), }, { Schema: mustParseGraphQLSchema(t, stubs.db), @@ -269,6 +366,8 @@ func TestSiteConfigConnection(t *testing.T) { username, displayName } + reproducedDiff + diff } pageInfo { hasNextPage @@ -296,7 +395,9 @@ func TestSiteConfigConnection(t *testing.T) { "id": "VXNlcjox", "username": "foo", "displayName": "foo user" - } + }, + "reproducedDiff": true, + "diff": %[3]q }, { "id": %[2]q, @@ -304,7 +405,9 @@ func TestSiteConfigConnection(t *testing.T) { "id": "VXNlcjoy", "username": "bar", "displayName": "bar user" - } + }, + "reproducedDiff": true, + "diff": %[4]q } ], "pageInfo": { @@ -317,7 +420,7 @@ func TestSiteConfigConnection(t *testing.T) { } } } - `, marshalSiteConfigurationChangeID(4), marshalSiteConfigurationChangeID(3)), + `, marshalSiteConfigurationChangeID(4), marshalSiteConfigurationChangeID(3), expectedDiffs[4], expectedDiffs[3]), }, { Schema: mustParseGraphQLSchema(t, stubs.db), @@ -338,6 +441,8 @@ func TestSiteConfigConnection(t *testing.T) { username, displayName } + reproducedDiff + diff } pageInfo { hasNextPage @@ -360,16 +465,20 @@ func TestSiteConfigConnection(t *testing.T) { "totalCount": 5, "nodes": [ { - "id": %[1]q, - "author": { - "id": "VXNlcjoy", - "username": "bar", - "displayName": "bar user" - } + "id": %[1]q, + "author": { + "id": "VXNlcjoy", + "username": "bar", + "displayName": "bar user" + }, + "reproducedDiff": true, + "diff": %[3]q }, { - "id": %[2]q, - "author": null + "id": %[2]q, + "author": null, + "reproducedDiff": true, + "diff": %[4]q } ], "pageInfo": { @@ -382,7 +491,7 @@ func TestSiteConfigConnection(t *testing.T) { } } } - `, marshalSiteConfigurationChangeID(3), marshalSiteConfigurationChangeID(2)), + `, marshalSiteConfigurationChangeID(3), marshalSiteConfigurationChangeID(2), expectedDiffs[3], expectedDiffs[2]), }, }) } diff --git a/cmd/frontend/graphqlbackend/site_config_change_test.go b/cmd/frontend/graphqlbackend/site_config_change_test.go new file mode 100644 index 000000000000..6db9d18b2861 --- /dev/null +++ b/cmd/frontend/graphqlbackend/site_config_change_test.go @@ -0,0 +1,193 @@ +package graphqlbackend + +import ( + "context" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/sourcegraph/sourcegraph/cmd/frontend/graphqlbackend/graphqlutil" + "github.com/sourcegraph/sourcegraph/internal/actor" + "github.com/sourcegraph/sourcegraph/internal/database" + "github.com/sourcegraph/sourcegraph/internal/gitserver" +) + +func TestSiteConfigurationChangeResolverReproducedDiff(t *testing.T) { + testCases := []struct { + name string + resolver SiteConfigurationChangeResolver + expected bool + }{ + { + name: "both siteConfig and previousSiteConfig are nil", + resolver: SiteConfigurationChangeResolver{}, + expected: false, + }, + { + name: "siteConfig is nil", + resolver: SiteConfigurationChangeResolver{ + previousSiteConfig: &database.SiteConfig{}, + }, + expected: false, + }, + { + name: "previousSiteConfig is nil", + resolver: SiteConfigurationChangeResolver{ + siteConfig: &database.SiteConfig{}, + }, + expected: false, + }, + + { + name: "siteConfig.RedactedContents is non-empty but previousSiteConfig is nil", + resolver: SiteConfigurationChangeResolver{ + siteConfig: &database.SiteConfig{ + RedactedContents: "foo", + }, + }, + expected: true, + }, + + { + name: "both siteConfig.RedactedContents and previousSiteConfig.RedactedContents are empty", + resolver: SiteConfigurationChangeResolver{ + siteConfig: &database.SiteConfig{}, + previousSiteConfig: &database.SiteConfig{}, + }, + expected: false, + }, + + { + name: "siteConfig.RedactedContents is empty", + resolver: SiteConfigurationChangeResolver{ + siteConfig: &database.SiteConfig{}, + previousSiteConfig: &database.SiteConfig{RedactedContents: "foo"}, + }, + expected: false, + }, + { + name: "previousSiteConfig.RedactedContents is empty", + resolver: SiteConfigurationChangeResolver{ + siteConfig: &database.SiteConfig{RedactedContents: "foo"}, + previousSiteConfig: &database.SiteConfig{}, + }, + expected: false, + }, + + { + name: "both siteConfig.RedactedContents and previousSiteConfig.RedactedContents is non-empty", + resolver: SiteConfigurationChangeResolver{ + siteConfig: &database.SiteConfig{RedactedContents: "foo"}, + previousSiteConfig: &database.SiteConfig{RedactedContents: "foo"}, + }, + expected: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + if tc.resolver.ReproducedDiff() != tc.expected { + t.Errorf("mismatched value for ReproducedDiff, expected %v, but got %v", tc.expected, tc.resolver.ReproducedDiff()) + } + }) + } + +} + +func TestSiteConfigurationDiff(t *testing.T) { + stubs := setupSiteConfigStubs(t) + + ctx := actor.WithActor(context.Background(), &actor.Actor{UID: stubs.users[0].ID}) + schemaResolver, err := newSchemaResolver(stubs.db, gitserver.NewClient()).Site().Configuration(ctx) + if err != nil { + t.Fatalf("failed to create schemaResolver: %v", err) + } + + expectedDiffs := stubs.expectedDiffs + + expectedNodes := []struct { + ID int32 + AuthorUserID int32 + Diff *string + }{ + { + ID: 5, + AuthorUserID: 1, + Diff: stringPtr(expectedDiffs[5]), + }, + { + ID: 4, + AuthorUserID: 1, + Diff: stringPtr(expectedDiffs[4]), + }, + { + ID: 3, + AuthorUserID: 2, + Diff: stringPtr(expectedDiffs[3]), + }, + { + ID: 2, + AuthorUserID: 0, + Diff: stringPtr(expectedDiffs[2]), + }, + { + ID: 1, + AuthorUserID: 0, + Diff: stringPtr(expectedDiffs[1]), + }, + } + + testCases := []struct { + name string + args *graphqlutil.ConnectionResolverArgs + }{ + // We have tests for pagination so we can skip that here and just check for the diff for all + // the nodes in both the directions. + { + name: "first: 10", + args: &graphqlutil.ConnectionResolverArgs{First: int32Ptr(10)}, + }, + { + name: "last: 10", + args: &graphqlutil.ConnectionResolverArgs{Last: int32Ptr(10)}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + connectionResolver, err := schemaResolver.History(ctx, tc.args) + if err != nil { + t.Fatalf("failed to get history: %v", err) + } + + nodes, err := connectionResolver.Nodes(ctx) + if err != nil { + t.Fatalf("failed to get nodes: %v", err) + } + + totalNodes, totalExpectedNodes := len(nodes), len(expectedNodes) + if totalNodes != totalExpectedNodes { + t.Fatalf("mismatched number of nodes, expected %d, got: %d", totalExpectedNodes, totalNodes) + } + + for i := 0; i < totalNodes; i++ { + siteConfig, expectedNode := nodes[i].siteConfig, expectedNodes[i] + + if siteConfig.ID != expectedNode.ID { + t.Errorf("mismatched node ID, expected: %d, but got: %d", siteConfig.ID, expectedNode.ID) + } + + if siteConfig.AuthorUserID != expectedNode.AuthorUserID { + t.Errorf("mismatched node AuthorUserID, expected: %d, but got: %d", siteConfig.ID, expectedNode.ID) + } + + if !nodes[i].ReproducedDiff() { + t.Fatal("expected reproducedDiff to be true but got false") + } + + if diff := cmp.Diff(*expectedNode.Diff, *nodes[i].Diff()); diff != "" { + t.Errorf("mismatched node diff (-want, +got):\n%s ", diff) + } + } + }) + } +} diff --git a/go.mod b/go.mod index d0276e84c7d1..feb7173f5a72 100644 --- a/go.mod +++ b/go.mod @@ -349,7 +349,7 @@ require ( github.com/grpc-ecosystem/grpc-gateway/v2 v2.14.0 // indirect github.com/hashicorp/go-cleanhttp v0.5.2 // indirect github.com/hashicorp/go-retryablehttp v0.7.1 // indirect - github.com/hexops/gotextdiff v1.0.3 // indirect + github.com/hexops/gotextdiff v1.0.3 github.com/huandu/xstrings v1.3.2 // indirect github.com/imdario/mergo v0.3.13 // indirect github.com/inconshreveable/mousetrap v1.0.0 // indirect