Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[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 all commits
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
4 changes: 2 additions & 2 deletions schema/perforce.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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).",
Expand Down
2 changes: 1 addition & 1 deletion schema/schema.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading