From 6c0c2740fec71b9a803e6f941407af61e8747c79 Mon Sep 17 00:00:00 2001 From: Petri-Johan Last Date: Mon, 19 Aug 2024 13:22:21 +0200 Subject: [PATCH 1/3] If only an IP address is provided and URL parsing fails, provide a more helpful error message --- internal/authz/providers/perforce/authz.go | 2 +- internal/authz/providers/perforce/perforce.go | 9 ++-- .../authz/providers/perforce/perforce_test.go | 49 +++++++++++++------ .../authz/providers/perforce/protects_test.go | 18 ++++--- 4 files changed, 52 insertions(+), 26 deletions(-) diff --git a/internal/authz/providers/perforce/authz.go b/internal/authz/providers/perforce/authz.go index 5078fba65c8f0..6235ec8b5411d 100644 --- a/internal/authz/providers/perforce/authz.go +++ b/internal/authz/providers/perforce/authz.go @@ -70,7 +70,7 @@ func newAuthzProvider( } } - return NewProvider(logger, db, gitserver.NewClient("authz.perforce"), urn, host, user, password, depotIDs, a.IgnoreRulesWithHost), nil + return NewProvider(logger, db, gitserver.NewClient("authz.perforce"), urn, host, user, password, depotIDs, a.IgnoreRulesWithHost) } // ValidateAuthz validates the authorization fields of the given Perforce diff --git a/internal/authz/providers/perforce/perforce.go b/internal/authz/providers/perforce/perforce.go index 8fb3a630ea1d8..d7cc27e55015b 100644 --- a/internal/authz/providers/perforce/perforce.go +++ b/internal/authz/providers/perforce/perforce.go @@ -59,8 +59,11 @@ func cacheIsUpToDate(lastUpdate time.Time) bool { // host, user and password to talk to a Perforce Server that is the source of // truth for permissions. It assumes emails of Sourcegraph accounts match 1-1 // with emails of Perforce Server users. -func NewProvider(logger log.Logger, db database.DB, gitserverClient gitserver.Client, urn, host, user, password string, depots []extsvc.RepoID, ignoreRulesWithHost bool) *Provider { - baseURL, _ := url.Parse(host) +func NewProvider(logger log.Logger, db database.DB, gitserverClient gitserver.Client, urn, host, user, password string, depots []extsvc.RepoID, ignoreRulesWithHost bool) (*Provider, error) { + baseURL, err := url.Parse(host) + if err != nil { + return nil, errors.Wrap(err, "could not parse Perforce host. Consider prefixing p4.port with `tcp:` or `ssl:`") + } return &Provider{ db: db, logger: logger, @@ -73,7 +76,7 @@ func NewProvider(logger log.Logger, db database.DB, gitserverClient gitserver.Cl gitserverClient: gitserverClient, cachedGroupMembers: make(map[string][]string), ignoreRulesWithHost: ignoreRulesWithHost, - } + }, nil } // FetchAccount uses given user's verified emails to match users on the Perforce diff --git a/internal/authz/providers/perforce/perforce_test.go b/internal/authz/providers/perforce/perforce_test.go index d1c5ce2a6b81f..1e860056d2975 100644 --- a/internal/authz/providers/perforce/perforce_test.go +++ b/internal/authz/providers/perforce/perforce_test.go @@ -9,6 +9,7 @@ import ( "github.com/google/go-cmp/cmp" jsoniter "github.com/json-iterator/go" "github.com/sourcegraph/log/logtest" + "github.com/stretchr/testify/require" "github.com/sourcegraph/sourcegraph/internal/api" "github.com/sourcegraph/sourcegraph/internal/authz" @@ -44,7 +45,8 @@ func TestProvider_FetchAccount(t *testing.T) { t.Run("no matching account", func(t *testing.T) { mockUserEmails.ListByUserFunc.SetDefaultReturn([]*database.UserEmail{{Email: "bob@example.com"}}, nil) - p := NewProvider(logger, db, gitserverClient, "", "ssl:111.222.333.444:1666", "admin", "password", []extsvc.RepoID{}, false) + p, err := NewProvider(logger, db, gitserverClient, "", "ssl:111.222.333.444:1666", "admin", "password", []extsvc.RepoID{}, false) + require.NoError(t, err) got, err := p.FetchAccount(ctx, user) if err != nil { t.Fatal(err) @@ -56,7 +58,8 @@ func TestProvider_FetchAccount(t *testing.T) { }) t.Run("found matching account", func(t *testing.T) { - p := NewProvider(logger, db, gitserverClient, "", "ssl:111.222.333.444:1666", "admin", "password", []extsvc.RepoID{}, false) + p, err := NewProvider(logger, db, gitserverClient, "", "ssl:111.222.333.444:1666", "admin", "password", []extsvc.RepoID{}, false) + require.NoError(t, err) mockUserEmails.ListByUserFunc.SetDefaultReturn([]*database.UserEmail{{Email: "alice@example.com"}}, nil) got, err := p.FetchAccount(ctx, user) if err != nil { @@ -89,8 +92,14 @@ func TestProvider_FetchAccount(t *testing.T) { } }) + t.Run("provider with only IP address returns error", func(t *testing.T) { + _, err := NewProvider(logger, db, gitserverClient, "", "111.222.333.444:1666", "admin", "password", []extsvc.RepoID{}, false) + require.Error(t, err) + }) + t.Run("found matching account case insensitive", func(t *testing.T) { - p := NewProvider(logger, db, gitserverClient, "", "ssl:111.222.333.444:1666", "admin", "password", []extsvc.RepoID{}, false) + p, err := NewProvider(logger, db, gitserverClient, "", "ssl:111.222.333.444:1666", "admin", "password", []extsvc.RepoID{}, false) + require.NoError(t, err) mockUserEmails.ListByUserFunc.SetDefaultReturn([]*database.UserEmail{{Email: "Alice@example.com"}}, nil) got, err := p.FetchAccount(ctx, user) if err != nil { @@ -131,8 +140,9 @@ func TestProvider_FetchUserPerms(t *testing.T) { t.Run("nil account", func(t *testing.T) { logger := logtest.Scoped(t) - p := NewProvider(logger, db, gitserver.NewTestClient(t), "", "ssl:111.222.333.444:1666", "admin", "password", []extsvc.RepoID{}, false) - _, err := p.FetchUserPerms(ctx, nil, authz.FetchPermsOptions{}) + p, err := NewProvider(logger, db, gitserver.NewTestClient(t), "", "ssl:111.222.333.444:1666", "admin", "password", []extsvc.RepoID{}, false) + require.NoError(t, err) + _, err = p.FetchUserPerms(ctx, nil, authz.FetchPermsOptions{}) want := "no account provided" got := fmt.Sprintf("%v", err) if got != want { @@ -142,8 +152,9 @@ func TestProvider_FetchUserPerms(t *testing.T) { t.Run("not the code host of the account", func(t *testing.T) { logger := logtest.Scoped(t) - p := NewProvider(logger, db, gitserver.NewTestClient(t), "", "ssl:111.222.333.444:1666", "admin", "password", []extsvc.RepoID{}, false) - _, err := p.FetchUserPerms(context.Background(), + p, err := NewProvider(logger, db, gitserver.NewTestClient(t), "", "ssl:111.222.333.444:1666", "admin", "password", []extsvc.RepoID{}, false) + require.NoError(t, err) + _, err = p.FetchUserPerms(context.Background(), &extsvc.Account{ AccountSpec: extsvc.AccountSpec{ ServiceType: extsvc.TypeGitLab, @@ -161,8 +172,9 @@ func TestProvider_FetchUserPerms(t *testing.T) { t.Run("no user found in account data", func(t *testing.T) { logger := logtest.Scoped(t) - p := NewProvider(logger, db, gitserver.NewTestClient(t), "", "ssl:111.222.333.444:1666", "admin", "password", []extsvc.RepoID{}, false) - _, err := p.FetchUserPerms(ctx, + p, err := NewProvider(logger, db, gitserver.NewTestClient(t), "", "ssl:111.222.333.444:1666", "admin", "password", []extsvc.RepoID{}, false) + require.NoError(t, err) + _, err = p.FetchUserPerms(ctx, &extsvc.Account{ AccountSpec: extsvc.AccountSpec{ ServiceType: extsvc.TypePerforce, @@ -311,7 +323,8 @@ read user alice * //Sourcegraph/Security/... ## give access to alice agai gc := gitserver.NewStrictMockClient() gc.PerforceProtectsForUserFunc.SetDefaultReturn(test.protects, nil) - p := NewProvider(logger, db, gc, "", "ssl:111.222.333.444:1666", "admin", "password", []extsvc.RepoID{}, false) + p, err := NewProvider(logger, db, gc, "", "ssl:111.222.333.444:1666", "admin", "password", []extsvc.RepoID{}, false) + require.NoError(t, err) got, err := p.FetchUserPerms(ctx, &extsvc.Account{ AccountSpec: extsvc.AccountSpec{ @@ -387,7 +400,8 @@ read user alice * //Sourcegraph/Security/... ## give access to alice agai gitserverClient.PerforceProtectsForDepotFunc.SetDefaultReturn(test.input, nil) gitserverClient.PerforceProtectsForUserFunc.SetDefaultReturn(test.input, nil) - p := NewProvider(logger, db, gitserverClient, "", "ssl:111.222.333.444:1666", "admin", "password", []extsvc.RepoID{}, false) + p, err := NewProvider(logger, db, gitserverClient, "", "ssl:111.222.333.444:1666", "admin", "password", []extsvc.RepoID{}, false) + require.NoError(t, err) p.depots = append(p.depots, "//Sourcegraph/") got, err := p.FetchUserPerms(ctx, @@ -420,8 +434,9 @@ func TestProvider_FetchRepoPerms(t *testing.T) { db := dbmocks.NewMockDB() t.Run("nil repository", func(t *testing.T) { - p := NewProvider(logger, db, gitserver.NewTestClient(t), "", "ssl:111.222.333.444:1666", "admin", "password", []extsvc.RepoID{}, false) - _, err := p.FetchRepoPerms(ctx, nil, authz.FetchPermsOptions{}) + p, err := NewProvider(logger, db, gitserver.NewTestClient(t), "", "ssl:111.222.333.444:1666", "admin", "password", []extsvc.RepoID{}, false) + require.NoError(t, err) + _, err = p.FetchRepoPerms(ctx, nil, authz.FetchPermsOptions{}) want := "no repository provided" got := fmt.Sprintf("%v", err) if got != want { @@ -430,8 +445,9 @@ func TestProvider_FetchRepoPerms(t *testing.T) { }) t.Run("not the code host of the repository", func(t *testing.T) { - p := NewProvider(logger, db, gitserver.NewTestClient(t), "", "ssl:111.222.333.444:1666", "admin", "password", []extsvc.RepoID{}, false) - _, err := p.FetchRepoPerms(ctx, + p, err := NewProvider(logger, db, gitserver.NewTestClient(t), "", "ssl:111.222.333.444:1666", "admin", "password", []extsvc.RepoID{}, false) + require.NoError(t, err) + _, err = p.FetchRepoPerms(ctx, &extsvc.Repository{ URI: "gitlab.com/user/repo", ExternalRepoSpec: api.ExternalRepoSpec{ @@ -476,7 +492,8 @@ func TestProvider_FetchRepoPerms(t *testing.T) { {Level: "list", EntityType: "user", EntityName: "david", Host: "*", Match: "//Sourcegraph/..."}, // "list" can't grant read access }, nil) - p := NewProvider(logger, db, gitserverClient, "", "ssl:111.222.333.444:1666", "admin", "password", []extsvc.RepoID{}, false) + p, err := NewProvider(logger, db, gitserverClient, "", "ssl:111.222.333.444:1666", "admin", "password", []extsvc.RepoID{}, false) + require.NoError(t, err) got, err := p.FetchRepoPerms(ctx, &extsvc.Repository{ URI: "gitlab.com/user/repo", diff --git a/internal/authz/providers/perforce/protects_test.go b/internal/authz/providers/perforce/protects_test.go index 99f181d8e9748..c79ee2f322aac 100644 --- a/internal/authz/providers/perforce/protects_test.go +++ b/internal/authz/providers/perforce/protects_test.go @@ -234,7 +234,8 @@ func TestScanFullRepoPermissions(t *testing.T) { db := dbmocks.NewMockDB() - p := NewProvider(logger, db, gitserver.NewStrictMockClient(), "", "ssl:111.222.333.444:1666", "admin", "password", []extsvc.RepoID{}, false) + p, err := NewProvider(logger, db, gitserver.NewStrictMockClient(), "", "ssl:111.222.333.444:1666", "admin", "password", []extsvc.RepoID{}, false) + require.NoError(t, err) p.depots = []extsvc.RepoID{ "//depot/main/", "//depot/training/", @@ -317,7 +318,8 @@ func TestScanIPPermissions(t *testing.T) { db := dbmocks.NewMockDB() - p := NewProvider(logger, db, gitserver.NewStrictMockClient(), "", "ssl:111.222.333.444:1666", "admin", "password", []extsvc.RepoID{}, false) + p, err := NewProvider(logger, db, gitserver.NewStrictMockClient(), "", "ssl:111.222.333.444:1666", "admin", "password", []extsvc.RepoID{}, false) + require.NoError(t, err) p.depots = []extsvc.RepoID{ "//depot/src/", "//depot/project1/", @@ -411,7 +413,8 @@ func TestScanFullRepoPermissionsWithWildcardMatchingDepot(t *testing.T) { db := dbmocks.NewMockDB() - p := NewProvider(logger, db, gitserver.NewStrictMockClient(), "", "ssl:111.222.333.444:1666", "admin", "password", []extsvc.RepoID{}, false) + p, err := NewProvider(logger, db, gitserver.NewStrictMockClient(), "", "ssl:111.222.333.444:1666", "admin", "password", []extsvc.RepoID{}, false) + require.NoError(t, err) p.depots = []extsvc.RepoID{ "//depot/main/base/", } @@ -735,7 +738,8 @@ read group Dev1 * //depot/main/.../*.go db := dbmocks.NewMockDB() - p := NewProvider(logger, db, gitserver.NewStrictMockClient(), "", "ssl:111.222.333.444:1666", "admin", "password", []extsvc.RepoID{}, false) + p, err := NewProvider(logger, db, gitserver.NewStrictMockClient(), "", "ssl:111.222.333.444:1666", "admin", "password", []extsvc.RepoID{}, false) + require.NoError(t, err) p.depots = []extsvc.RepoID{ extsvc.RepoID(tc.depot), } @@ -798,7 +802,8 @@ func TestFullScanWildcardDepotMatching(t *testing.T) { db := dbmocks.NewMockDB() - p := NewProvider(logger, db, gitserver.NewStrictMockClient(), "", "ssl:111.222.333.444:1666", "admin", "password", []extsvc.RepoID{}, false) + p, err := NewProvider(logger, db, gitserver.NewStrictMockClient(), "", "ssl:111.222.333.444:1666", "admin", "password", []extsvc.RepoID{}, false) + require.NoError(t, err) p.depots = []extsvc.RepoID{ "//depot/654/deploy/base/", } @@ -948,7 +953,8 @@ func TestScanAllUsers(t *testing.T) { db := dbmocks.NewMockDB() - p := NewProvider(logger, db, gc, "", "ssl:111.222.333.444:1666", "admin", "password", []extsvc.RepoID{}, false) + p, err := NewProvider(logger, db, gc, "", "ssl:111.222.333.444:1666", "admin", "password", []extsvc.RepoID{}, false) + require.NoError(t, err) p.cachedGroupMembers = map[string][]string{ "dev": {"user1", "user2"}, } From fce14c6a654dc36d708cb19f2534bc2136041d67 Mon Sep 17 00:00:00 2001 From: Petri-Johan Last Date: Mon, 19 Aug 2024 14:32:35 +0200 Subject: [PATCH 2/3] Add another example to the p4.port examples --- schema/perforce.schema.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/schema/perforce.schema.json b/schema/perforce.schema.json index 7e66bf20a2064..b03eedb2661b7 100644 --- a/schema/perforce.schema.json +++ b/schema/perforce.schema.json @@ -9,9 +9,9 @@ "required": ["p4.port", "p4.user", "p4.passwd"], "properties": { "p4.port": { - "description": "The Perforce Server address to be used for p4 CLI (P4PORT).", + "description": "The Perforce Server address to be used for p4 CLI (P4PORT). It's recommended to specify the protocol prefix (e.g. tcp: or ssl:) as part of the address.", "type": "string", - "examples": ["ssl:111.222.333.444:1666"] + "examples": ["ssl:111.222.333.444:1666", "tcp:111.222.333.444:1666"] }, "p4.user": { "description": "The user to be authenticated for p4 CLI (P4USER).", From a3dd70d42f40bcedbb0a835e8baeab3b562bea37 Mon Sep 17 00:00:00 2001 From: Petri-Johan Last Date: Mon, 19 Aug 2024 14:38:48 +0200 Subject: [PATCH 3/3] sg generate --- schema/schema.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/schema/schema.go b/schema/schema.go index aa00ff8fe287c..2893161cd7304 100644 --- a/schema/schema.go +++ b/schema/schema.go @@ -2266,7 +2266,7 @@ type PerforceConnection struct { P4Client string `json:"p4.client,omitempty"` // P4Passwd description: The ticket value for the user (P4PASSWD). You can get this by running `p4 login -p` or `p4 login -pa`. It should look like `6211C5E719EDE6925855039E8F5CC3D2`. P4Passwd string `json:"p4.passwd"` - // P4Port description: The Perforce Server address to be used for p4 CLI (P4PORT). + // P4Port description: The Perforce Server address to be used for p4 CLI (P4PORT). It's recommended to specify the protocol prefix (e.g. tcp: or ssl:) as part of the address. P4Port string `json:"p4.port"` // P4User description: The user to be authenticated for p4 CLI (P4USER). P4User string `json:"p4.user"`