Skip to content

Commit

Permalink
[Fix] Fail fast when authenticating if host is not configured (#1033)
Browse files Browse the repository at this point in the history
## Changes
The SDK currently hangs when making API requests if the Host is set to a
non-empty but invalid endpoint, such as `https://:443`. This PR adds a
check during authentication to ensure that the hostname is defined,
failing early if not. The underlying error is exported so other clients,
such as the CLI or Terraform Provider, can provide specific advice when
this happens.

## Tests
Added a unit test for this case.

- [ ] `make test` passing
- [ ] `make fmt` applied
- [ ] relevant integration tests applied
  • Loading branch information
mgyucht authored Aug 30, 2024
1 parent 796dae1 commit 4886afe
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 3 deletions.
20 changes: 17 additions & 3 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package config

import (
"context"
"errors"
"fmt"
"net/http"
"net/url"
Expand Down Expand Up @@ -336,7 +337,8 @@ func (c *Config) WithTesting() *Config {
}

func (c *Config) CanonicalHostName() string {
c.fixHostIfNeeded()
// Missing host is tolerated here.
_ = c.fixHostIfNeeded()
return c.Host
}

Expand All @@ -361,7 +363,9 @@ func (c *Config) authenticateIfNeeded() error {
if c.Credentials == nil {
c.Credentials = &DefaultCredentials{}
}
c.fixHostIfNeeded()
if err := c.fixHostIfNeeded(); err != nil && !errors.Is(err, ErrNoHostConfigured) {
return err
}
ctx := c.refreshClient.InContextForOAuth2(c.refreshCtx)
credentialsProvider, err := c.Credentials.Configure(ctx, c)
if err != nil {
Expand All @@ -372,7 +376,9 @@ func (c *Config) authenticateIfNeeded() error {
}
c.credentialsProvider = credentialsProvider
c.AuthType = c.Credentials.Name()
c.fixHostIfNeeded()
if err := c.fixHostIfNeeded(); err != nil {
return err
}
// TODO: error customization
return nil
}
Expand All @@ -393,6 +399,9 @@ func (c *Config) fixHostIfNeeded() error {
return err
}
}
if parsedHost.Hostname() == "" {
return ErrNoHostConfigured
}
// Create new instance to ensure other fields are initialized as empty.
parsedHost = &url.URL{
Scheme: parsedHost.Scheme,
Expand All @@ -403,6 +412,11 @@ func (c *Config) fixHostIfNeeded() error {
return nil
}

// ErrNoHostConfigured is the error returned when a user tries to authenticate
// without a host configured. Applications can check for this error to provide
// more user-friendly error messages.
var ErrNoHostConfigured = fmt.Errorf("no host configured")

func (c *Config) refreshTokenErrorMapper(ctx context.Context, resp common.ResponseWrapper) error {
defaultErr := httpclient.DefaultErrorMapper(ctx, resp)
if defaultErr == nil {
Expand Down
14 changes: 14 additions & 0 deletions config/config_test.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
package config

import (
"context"
"net/http"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestIsAccountClient_AwsAccount(t *testing.T) {
Expand Down Expand Up @@ -52,3 +55,14 @@ func TestNewWithWorkspaceHost(t *testing.T) {
// The new config will not be automatically resolved.
assert.False(t, c2.resolved)
}

func TestAuthenticate_InvalidHostSet(t *testing.T) {
c := &Config{
Host: "https://:443",
Token: "abcdefg",
}
req, err := http.NewRequestWithContext(context.Background(), "GET", c.Host, nil)
require.NoError(t, err)
err = c.Authenticate(req)
assert.ErrorIs(t, err, ErrNoHostConfigured)
}

0 comments on commit 4886afe

Please sign in to comment.