Skip to content

Commit

Permalink
🐛 github: fix precedence flag vs env-var (#5144)
Browse files Browse the repository at this point in the history
* 🐛  github: fix precedence flag vs env-var

Github provide has two authentication methods.

1. Application credentials
2. Personal access token

We give precedence to the former and, if both auth methods are provided,
we will output a warning.

Closes #5036

---------

Signed-off-by: Salim Afiune Maya <[email protected]>
  • Loading branch information
afiune authored Feb 4, 2025
1 parent 3799ad0 commit a1105d2
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 8 deletions.
5 changes: 0 additions & 5 deletions providers/github/connection/connection.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"context"
"net/http"
"net/url"
"os"
"strconv"
"time"

Expand Down Expand Up @@ -71,10 +70,6 @@ func connectionOptionsFromConfigOptions(conf *inventory.Config) (opts githubConn
opts.EnterpriseURL = conf.Options[OPTION_ENTERPRISE_URL]
opts.Token = conf.Options[OPTION_TOKEN]

if opts.Token == "" {
opts.Token = os.Getenv("GITHUB_TOKEN")
}

for _, cred := range conf.Credentials {
switch cred.Type {

Expand Down
21 changes: 18 additions & 3 deletions providers/github/provider/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,13 @@ func (s *Service) ParseCLI(req *plugin.ParseCLIReq) (*plugin.ParseCLIRes, error)
conf.Options[connection.OPTION_ENTERPRISE_URL] = string(x.Value)
}

// Github provide has two authentication methods.
//
// 1. Application credentials
// 2. Personal access token
//
// We give precedence to the former and, if both auth methods are provided,
// we will output a warning.
isAppAuth := false
appId, ok := flags[connection.OPTION_APP_ID]
if ok && len(appId.Value) > 0 {
Expand All @@ -73,20 +80,28 @@ func (s *Service) ParseCLI(req *plugin.ParseCLIReq) (*plugin.ParseCLIRes, error)
pk := req.Flags[connection.OPTION_APP_PRIVATE_KEY]
conf.Options[connection.OPTION_APP_PRIVATE_KEY] = string(pk.Value)
isAppAuth = true
log.Debug().Msg("application credentials provided")
}

token := ""
if x, ok := flags["token"]; ok && len(x.Value) != 0 {
token = string(x.Value)
log.Debug().Msg("loaded token from flag")
}
if token == "" {
if token == "" && len(os.Getenv("GITHUB_TOKEN")) != 0 {
token = os.Getenv("GITHUB_TOKEN")
log.Debug().Msg("loaded token from GITHUB_TOKEN env variable")
}
if token == "" && !isAppAuth {
return nil, errors.New("a valid GitHub authentication is required, pass --token '<yourtoken>', set GITHUB_TOKEN environment variable or provide GitHub App credentials")
return nil, errors.New("a valid GitHub authentication is required, pass --token '<yourtoken>', " +
"set GITHUB_TOKEN environment variable or provide GitHub App credentials")
}
if token != "" {
conf.Credentials = append(conf.Credentials, vault.NewPasswordCredential("", token))
if isAppAuth {
log.Warn().Msg("both authentication methods provided, using application credentials")
} else {
conf.Credentials = append(conf.Credentials, vault.NewPasswordCredential("", token))
}
}

// discovery flags
Expand Down
18 changes: 18 additions & 0 deletions test/providers/github_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,4 +57,22 @@ func TestGithubScanFlags(t *testing.T) {
"could not read private key", // expected! it means we loaded the flags
)
})
t.Run("github scan with both auth methods, prefer app credentials", func(t *testing.T) {
// NOTE this will fail but, it will load the flags and fail with the right message
r := test.NewCliTestRunner("./cnquery", "scan", "github", "repo", "foo",
// personal access token
"--token", "abc",
// application credentials
"--app-id", "123", "--app-installation-id", "456", "--app-private-key", "private-key.pem",
)
err := r.Run()
require.NoError(t, err)
assert.Equal(t, 1, r.ExitCode())
assert.NotNil(t, r.Stdout())
assert.NotNil(t, r.Stderr())

assert.Contains(t, string(r.Stderr()),
"could not read private key", // expected! it means we use app credentials
)
})
}

0 comments on commit a1105d2

Please sign in to comment.