Skip to content

Commit

Permalink
Fix GetWorkspaceClient for GCP (#803)
Browse files Browse the repository at this point in the history
## Changes
The current implementation of GetWorkspaceClient copies the entire
config, critically reusing the cached authenticator so as to use the
same auth mechanism when getting a workspace client. However, at least
in GCP, account-level OAuth tokens can't be used to authenticate to a
workspace (probably because the audience for the account-level and
workspace-level tokens is different).

This PR fixes this by only copying the exported fields and not copying
the authenticator or refresh client. Subsequent use of the config in
WorkspaceClient will trigger config resolution. For GCP, this means
creating a new token source using the correct host as the audience.

## Tests
Manually ran this integration test in all non-UC (Azure, AWS, GCP) and
UC (AWS, GCP) account-level environments.

- [ ] `make test` passing
- [ ] `make fmt` applied
- [ ] relevant integration tests applied
  • Loading branch information
mgyucht authored Feb 21, 2024
1 parent 943b309 commit 018b8f7
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 18 deletions.
15 changes: 7 additions & 8 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -179,15 +179,14 @@ func (c *Config) NewWithWorkspaceHost(host string) (*Config, error) {
}

res.Host = host
// We can reuse the same OAuth token refresh client and context. The
// reuseTokenSource internally locks.
res.refreshClient = c.refreshClient
res.refreshCtx = c.refreshCtx
// The config does not need to be re-resolved, as we reuse all attributes
// from the original config.
res.resolved = c.resolved
res.auth = c.auth
res.isTesting = c.isTesting
// We need to reresolve the config with the updated host in general. For
// example, the audience for OAuth tokens provided by GCP is derived from
// the host, so account-level tokens cannot be used at workspace-level or
// vice-versa.
//
// In the future, when unified login is widely available, we may be able to
// reuse the authentication visitor specifically for in-house OAuth.
return res, nil
}

Expand Down
3 changes: 2 additions & 1 deletion config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,5 +49,6 @@ func TestNewWithWorkspaceHost(t *testing.T) {
// Other fields should be preserved
assert.Equal(t, "client-id", c2.ClientID)
assert.Equal(t, "http://", c2.MetadataServiceURL)
assert.True(t, c2.resolved)
// The new config will not be automatically resolved.
assert.False(t, c2.resolved)
}
12 changes: 3 additions & 9 deletions internal/account_client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,10 @@ import (

func TestMwsAccAccountClient_GetWorkspaceClient_NoTranspile(t *testing.T) {
ctx, a := accountTest(t)
if !a.Config.IsAws() {
skipf(t)("Only works on AWS")
}
wss, err := a.Workspaces.List(ctx)
workspaceId := MustParseInt64(GetEnvOrSkipTest(t, "TEST_WORKSPACE_ID"))
ws, err := a.Workspaces.GetByWorkspaceId(ctx, int64(workspaceId))
require.NoError(t, err)

if len(wss) == 0 {
t.Skip("No workspaces found")
}
w, err := a.GetWorkspaceClient(wss[0])
w, err := a.GetWorkspaceClient(*ws)
assert.NoError(t, err)
me, err := w.CurrentUser.Me(ctx)
assert.NoError(t, err)
Expand Down

0 comments on commit 018b8f7

Please sign in to comment.