Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

[fix] Perforce auth provider panics when only IP is provided #64533

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

[fix] Perforce auth provider panics when only IP is provided #64533

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6c0c274
If only an IP address is provided and URL parsing fails, provide a mo…
pjlast Aug 19, 2024
fce14c6
Add another example to the p4.port examples
pjlast Aug 19, 2024
a3dd70d
sg generate
pjlast Aug 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion internal/authz/providers/perforce/authz.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 6 additions & 3 deletions internal/authz/providers/perforce/perforce.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
Expand Down
49 changes: 33 additions & 16 deletions internal/authz/providers/perforce/perforce_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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: "[email protected]"}}, 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)
Expand All @@ -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: "[email protected]"}}, nil)
got, err := p.FetchAccount(ctx, user)
if err != nil {
Expand Down Expand Up @@ -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: "[email protected]"}}, nil)
got, err := p.FetchAccount(ctx, user)
if err != nil {
Expand Down Expand Up @@ -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 {
Expand All @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 {
Expand All @@ -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{
Expand Down Expand Up @@ -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",
Expand Down
18 changes: 12 additions & 6 deletions internal/authz/providers/perforce/protects_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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/",
Expand Down Expand Up @@ -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/",
Expand Down Expand Up @@ -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/",
}
Expand Down Expand Up @@ -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),
}
Expand Down Expand Up @@ -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/",
}
Expand Down Expand Up @@ -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"},
}
Expand Down
Loading