mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
feat!: resolve agent external auth by template, not config order (#27854)
## TL;DR
**Problem.** A template can declare which external auth provider it
wants via `data "coder_external_auth" { id = "..." }`, and that
declaration is honored at every stage of the build. It was ignored at
runtime. Any git operation going through `GIT_ASKPASS` supplies only a
hostname, never a provider ID, and the handler scanned *every* provider
configured on the deployment and returned whichever matched the hostname
**last in config order**, with no reference to what the requesting
workspace's own template declared. Reordering
`CODER_EXTERNAL_AUTH_<N>_*` silently redirected a plain `git clone` from
one OAuth client's token to a completely different one.
**Fix.** For hostname-only requests, resolve the calling agent's
workspace and build *before* selecting a provider, then narrow
candidates to the providers declared by that build's template version.
Exactly one match wins regardless of config order. No matching declared
provider falls back to today's deployment-wide scan, so a template that
declares only a GitHub provider can still clone an unrelated host. Two
or more matching declared providers return `409` naming them, rather
than picking one arbitrarily: `external_auth_providers` is stored sorted
by ID, so HCL declaration order is already unavailable and no principled
tie-break exists.
Requests supplying an explicit provider ID are untouched. Server-side
only: no wire protocol, proto, manifest, or database schema change, so
already-running agents get the corrected behavior on their next askpass
call with no restart.
Refs #23718
<details>
<summary><b>Call flow</b></summary>
```mermaid
flowchart TD
subgraph Push["1. Template import: coder templates push"]
A1["Terraform extracts coder_external_auth id/optional attrs"]
A2["CompleteJob(TemplateImport) validates each id<br/>against deployment config"]
A4["template_versions.external_auth_providers persisted"]
A1 --> A2 --> A4
end
subgraph PreBuild["2. Pre-build and workspace build (unaffected)"]
B1["User authenticates declared provider(s), exact-ID lookup"]
B2["Build resolves token by exact ID<br/>(provisionerdserver.go)"]
A4 --> B1 --> B2
end
subgraph Runtime["3. Workspace running: a credential is needed"]
B2 --> C0{"Caller supplies id or match?"}
C0 -->|"id (explicit)"| D1["Exact-ID match<br/>UNCHANGED, already deterministic<br/>(coder external-auth access-token)"]
C0 -->|"match only (GIT_ASKPASS)"| C1["git needs credentials for a hostname<br/>GIT_ASKPASS invoked, unchanged"]
C1 --> C2["coder gitaskpass sends ExternalAuthRequest{Match: host}<br/>unchanged (cli/gitaskpass.go)"]
C2 --> C3["workspaceAgentsExternalAuth<br/>(coderd/workspaceagents.go)"]
C3 --> C4["CHANGED:<br/>1. resolve workspace/build BEFORE matching<br/>2. read that build's declared provider IDs<br/>3. filter: declared AND regex matches host"]
C4 --> C5{"how many candidates?"}
C5 -->|"exactly 1"| C6["use it, regardless of config order"]
C5 -->|"0"| C7["fall back to deployment-wide scan<br/>(unchanged legacy behavior)"]
C5 -->|"2 or more"| C8["409 naming every matching ID"]
end
D1 --> E1["Token returned"]
C6 --> E1
C7 --> E1
style C4 fill:#1f4d2e,stroke:#4caf50,color:#fff
style C6 fill:#1f4d2e,stroke:#4caf50,color:#fff
style C8 fill:#1f4d2e,stroke:#4caf50,color:#fff
style D1 fill:#333,stroke:#888,color:#fff
```
</details>
## Verification
Two test functions were added in `coderd/workspaceagents_test.go`, and
the behavior no unit test can reach was verified against a local dev
cluster with two real GitHub OAuth Apps whose regexes both match
`github.com`.
| Behavior | Unit | Manual |
|---|---|---|
| Declared provider wins over a colliding one | yes | yes |
| Outcome independent of deployment config order | yes | yes |
| No declared match falls back to the full scan | yes | yes |
| Host the template never declared still resolves | yes | via fallback |
| Two declared providers matching one host return `409` | yes | not run
|
| Declared but unauthenticated provider returns its auth URL | yes | not
run |
| Two templates resolve independently and concurrently | yes | no |
| Explicit-ID path unaffected | no | yes |
| Running agent corrected with no restart | **no** | **yes** |
| Declared ID since removed from config falls back | **no** | **yes** |
| Recomputed per build after a template update | **no** | **yes** |
The last three are properties a unit test cannot express: they involve
swapping the server binary underneath a live agent, removing deployment
configuration, and rebuilding a workspace against a new template
version.
<details>
<summary><b>Unit test detail</b></summary>
`TestWorkspaceAgentsExternalAuthTemplateScoped` builds a deployment with
two providers sharing a regex, a template declaring one of them, and a
seeded token for **every** provider, so a mis-selection returns a valid
token with the wrong identity rather than an error. Subtests:
- `DeclaredProviderLast` / `DeclaredProviderFirst`: the declared
provider wins in both config orders. Only the `First` arm is
discriminating, since the pre-change loop had no `break` and returned
the last regex match, which the `Last` arm happens to agree with.
- `NoDeclaredProvidersFallsBackToFullScan`: a template declaring nothing
keeps today's behavior exactly, pinning the legacy last-match rule.
- `UnrelatedHostStillResolvesViaFallback`: a template declaring only a
GitHub provider still resolves a GitLab host.
- `AmbiguousDeclaredSetReturnsError`: `409` whose message names both
colliding provider IDs.
- `OptionalUnauthenticatedDeclaredProviderReturnsAuthURL`: returns the
auth URL for the *declared* provider, not for an unrelated one the user
happens to hold a token for.
`TestWorkspaceAgentsExternalAuthMultipleTemplates` runs two workspaces
from two templates, each declaring a different provider, issuing
requests concurrently. Each resolves to its own template's provider.
</details>
<details>
<summary><b>Manual verification detail</b></summary>
Local dev cluster, two GitHub OAuth Apps both defaulting to
`^(https?://)?github\.com(/.*)?$`, both authorized by the workspace
owner so a wrong selection yields a usable token rather than an error.
Workspace built from a template declaring only `github-dotfiles`. Tokens
redacted.
**Order independence.** Same workspace, never rebuilt, config order
reversed between runs:
| Deployment config order | Token returned |
|---|---|
| `[github-broad, github-dotfiles]` | `gho_<dotfiles>` |
| `[github-dotfiles, github-broad]` | `gho_<dotfiles>` |
**A/B against the pre-fix binary.** Everything held constant except the
coderd build, with `/api/v2/buildinfo` checked on both sides so the
comparison rests on verified binary identity. The workspace was never
stopped, rebuilt, or re-authorized:
| coderd | buildinfo | Token | Honors declaration |
|---|---|---|---|
| pre-fix | `v2.35.3-devel+11e03cfb3a` | `gho_<broad>` | no |
| this branch | `v2.35.3-devel+e8b87d0333` | `gho_<dotfiles>` | yes |
This doubles as the demonstration that a coderd-only upgrade corrects
behavior on a live agent's next askpass call.
**Declared provider removed from config.** `github-dotfiles` deleted
from deployment configuration while the workspace's template still
declared it. Result: `HTTP/2 200` with `gho_<broad>` via the fallback.
No `500`, no fail-closed `404`. The orphaned `external_auth_link` row
remained in the database throughout and correctly had no effect.
**Recomputation after a template update.**
| Workspace state | Build's declared provider | Token returned |
|---|---|---|
| new version pushed, workspace not updated | `github-dotfiles` |
`gho_<dotfiles>` |
| after `coder update` | `github-broad` | `gho_<broad>` |
The pair is what makes it conclusive: the first rules out following the
template's newest version, the second rules out a cached value.
**Explicit-ID path.** `coder external-auth access-token github-broad`
returned that provider's result even though the template declared only
`github-dotfiles`, and did not substitute the declared provider's
already-valid token.
Raw traces were captured with `GIT_CURL_VERBOSE=1 git -c
credential.helper="" ls-remote <private repo>`, reading the unredacted
`== Info: Server auth using Basic with user '<token>'` line. A private
repo is required, since a public one never triggers a `401` and
therefore never invokes `GIT_ASKPASS`.
</details>
This commit is contained in:
Bobby Ho
+153
-33
@@ -2083,39 +2083,10 @@ func (api *API) workspaceAgentsExternalAuth(rw http.ResponseWriter, r *http.Requ
|
||||
// new token to be issued!
|
||||
listen := query.Has("listen")
|
||||
|
||||
var externalAuthConfig *externalauth.Config
|
||||
for _, extAuth := range api.ExternalAuthConfigs {
|
||||
if extAuth.ID == id {
|
||||
externalAuthConfig = extAuth
|
||||
break
|
||||
}
|
||||
if match == "" || extAuth.Regex == nil {
|
||||
continue
|
||||
}
|
||||
matches := extAuth.Regex.MatchString(match)
|
||||
if !matches {
|
||||
continue
|
||||
}
|
||||
externalAuthConfig = extAuth
|
||||
}
|
||||
if externalAuthConfig == nil {
|
||||
detail := "External auth provider not found."
|
||||
if len(api.ExternalAuthConfigs) > 0 {
|
||||
regexURLs := make([]string, 0, len(api.ExternalAuthConfigs))
|
||||
for _, extAuth := range api.ExternalAuthConfigs {
|
||||
if extAuth.Regex == nil {
|
||||
continue
|
||||
}
|
||||
regexURLs = append(regexURLs, fmt.Sprintf("%s=%q", extAuth.ID, extAuth.Regex.String()))
|
||||
}
|
||||
detail = fmt.Sprintf("The configured external auth provider have regex filters that do not match the url. Provider url regex: %s", strings.Join(regexURLs, ","))
|
||||
}
|
||||
httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{
|
||||
Message: fmt.Sprintf("No matching external auth provider found in Coder for the url %q.", match),
|
||||
Detail: detail,
|
||||
})
|
||||
return
|
||||
}
|
||||
// Resolve the calling agent's own workspace/build before selecting a
|
||||
// provider below, so the match-only (GIT_ASKPASS) path can be scoped to
|
||||
// this workspace's own template-declared providers instead of scanning
|
||||
// every provider configured on the deployment.
|
||||
workspaceAgent := httpmw.WorkspaceAgent(r)
|
||||
// We must get the workspace to get the owner ID!
|
||||
resource, err := api.Database.GetWorkspaceResourceByID(ctx, workspaceAgent.ResourceID)
|
||||
@@ -2143,6 +2114,83 @@ func (api *API) workspaceAgentsExternalAuth(rw http.ResponseWriter, r *http.Requ
|
||||
return
|
||||
}
|
||||
|
||||
var externalAuthConfig *externalauth.Config
|
||||
if id != "" {
|
||||
// Explicit ID path: exact match only. Deterministic, and deliberately
|
||||
// unaffected by the template-scoped narrowing below.
|
||||
for _, extAuth := range api.ExternalAuthConfigs {
|
||||
if extAuth.ID == id {
|
||||
externalAuthConfig = extAuth
|
||||
break
|
||||
}
|
||||
}
|
||||
} else {
|
||||
// match-only path (GIT_ASKPASS supplies a hostname, never an ID):
|
||||
// narrow to the workspace's own template-declared providers first.
|
||||
// Only fall back to the full deployment-wide scan when every declared
|
||||
// provider is configured and none of them match this hostname (e.g. a
|
||||
// host the template never declared). Report an error rather than guess
|
||||
// when several declared providers match, or when the declaration set is
|
||||
// stale.
|
||||
//
|
||||
// Both errors use 404 so `coder gitaskpass` warns and defers to git's
|
||||
// own credential behavior instead of failing the git operation.
|
||||
declared, err := api.workspaceAgentsExternalAuthDeclaredCandidates(ctx, build, match)
|
||||
if err != nil {
|
||||
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
|
||||
Message: "Failed to resolve the workspace's template-declared external auth providers.",
|
||||
Detail: err.Error(),
|
||||
})
|
||||
return
|
||||
}
|
||||
switch {
|
||||
case len(declared.matchedIDs) > 1:
|
||||
httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{
|
||||
Message: fmt.Sprintf("Multiple external auth providers declared by this workspace's template match %q: %s.", match, strings.Join(declared.matchedIDs, ", ")),
|
||||
Detail: "Coder cannot tell which of them to use. Request a token with an explicit provider ID instead, using `coder external-auth access-token <id>`.",
|
||||
})
|
||||
return
|
||||
case declared.config != nil:
|
||||
externalAuthConfig = declared.config
|
||||
case len(declared.missingIDs) > 0:
|
||||
// A declared provider that the deployment no longer configures
|
||||
// leaves no way to tell whether it was the one meant to serve this
|
||||
// hostname, so another provider's token could silently stand in for
|
||||
// it. Refuse instead, which keeps the template scoping above intact
|
||||
// even while a template and the deployment config disagree.
|
||||
httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{
|
||||
Message: fmt.Sprintf("This workspace's template declares external auth provider(s) that this deployment no longer configures: %s.", strings.Join(declared.missingIDs, ", ")),
|
||||
Detail: "Coder will not substitute a different provider's token. Restore that provider's configuration, or update the template to declare a configured provider.",
|
||||
})
|
||||
return
|
||||
default:
|
||||
for _, extAuth := range api.ExternalAuthConfigs {
|
||||
if extAuth.Regex == nil || !extAuth.Regex.MatchString(match) {
|
||||
continue
|
||||
}
|
||||
externalAuthConfig = extAuth
|
||||
}
|
||||
}
|
||||
}
|
||||
if externalAuthConfig == nil {
|
||||
detail := "External auth provider not found."
|
||||
if len(api.ExternalAuthConfigs) > 0 {
|
||||
regexURLs := make([]string, 0, len(api.ExternalAuthConfigs))
|
||||
for _, extAuth := range api.ExternalAuthConfigs {
|
||||
if extAuth.Regex == nil {
|
||||
continue
|
||||
}
|
||||
regexURLs = append(regexURLs, fmt.Sprintf("%s=%q", extAuth.ID, extAuth.Regex.String()))
|
||||
}
|
||||
detail = fmt.Sprintf("The configured external auth provider have regex filters that do not match the url. Provider url regex: %s", strings.Join(regexURLs, ","))
|
||||
}
|
||||
httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{
|
||||
Message: fmt.Sprintf("No matching external auth provider found in Coder for the url %q.", match),
|
||||
Detail: detail,
|
||||
})
|
||||
return
|
||||
}
|
||||
|
||||
// Pre-check if the caller can read the external auth links for the owner of the
|
||||
// workspace. Do this up front because a sql.ErrNoRows is expected if the user is
|
||||
// in the flow of authenticating. If no row is present, the auth check is delayed
|
||||
@@ -2239,6 +2287,78 @@ func (api *API) workspaceAgentsExternalAuth(rw http.ResponseWriter, r *http.Requ
|
||||
httpapi.Write(ctx, rw, http.StatusOK, resp)
|
||||
}
|
||||
|
||||
// declaredExternalAuthCandidates describes how a workspace template's declared
|
||||
// external auth providers relate to one hostname-only (GIT_ASKPASS) request.
|
||||
type declaredExternalAuthCandidates struct {
|
||||
// config is the provider to use, set only when exactly one declared and
|
||||
// configured provider matches the hostname.
|
||||
config *externalauth.Config
|
||||
// matchedIDs holds the ID of every declared and configured provider whose
|
||||
// regex matches the hostname, in declaration order.
|
||||
matchedIDs []string
|
||||
// missingIDs holds the ID of every declared provider that the deployment
|
||||
// does not configure.
|
||||
missingIDs []string
|
||||
}
|
||||
|
||||
// workspaceAgentsExternalAuthDeclaredCandidates resolves which configured
|
||||
// external auth provider should service a hostname-only (GIT_ASKPASS) request,
|
||||
// scoped to the given build's template version's declared providers. The zero
|
||||
// value means the template declares no providers at all.
|
||||
//
|
||||
// config is set only when exactly one declared provider matched, so callers
|
||||
// should distinguish four outcomes:
|
||||
// - several matchedIDs: report the collision rather than guess between them.
|
||||
// - config set: use it.
|
||||
// - no match but some missingIDs: the declaration set is stale, so report
|
||||
// that instead of substituting a provider the template never declared.
|
||||
// - no match and nothing missing: the template's providers simply do not
|
||||
// serve this hostname, so fall back to a deployment-wide scan.
|
||||
func (api *API) workspaceAgentsExternalAuthDeclaredCandidates(ctx context.Context, build database.WorkspaceBuild, match string) (declaredExternalAuthCandidates, error) {
|
||||
// Template reads authorize through the template's ACL, which the owner may
|
||||
// no longer have. The version ID is server-derived from the agent's token.
|
||||
//nolint:gocritic // Agent needs system access to read its own template version's declared providers.
|
||||
sysCtx := dbauthz.AsSystemRestricted(ctx)
|
||||
templateVersion, err := api.Database.GetTemplateVersionByID(sysCtx, build.TemplateVersionID)
|
||||
if err != nil {
|
||||
return declaredExternalAuthCandidates{}, xerrors.Errorf("get template version: %w", err)
|
||||
}
|
||||
|
||||
var declared []database.ExternalAuthProvider
|
||||
if err := json.Unmarshal(templateVersion.ExternalAuthProviders, &declared); err != nil {
|
||||
return declaredExternalAuthCandidates{}, xerrors.Errorf("unmarshal template version external auth providers: %w", err)
|
||||
}
|
||||
|
||||
var (
|
||||
candidates []*externalauth.Config
|
||||
out declaredExternalAuthCandidates
|
||||
)
|
||||
for _, provider := range declared {
|
||||
idx := slices.IndexFunc(api.ExternalAuthConfigs, func(extAuth *externalauth.Config) bool {
|
||||
return extAuth.ID == provider.ID
|
||||
})
|
||||
if idx < 0 {
|
||||
out.missingIDs = append(out.missingIDs, provider.ID)
|
||||
continue
|
||||
}
|
||||
extAuth := api.ExternalAuthConfigs[idx]
|
||||
if extAuth.Regex == nil || !extAuth.Regex.MatchString(match) {
|
||||
continue
|
||||
}
|
||||
candidates = append(candidates, extAuth)
|
||||
}
|
||||
|
||||
for _, candidate := range candidates {
|
||||
out.matchedIDs = append(out.matchedIDs, candidate.ID)
|
||||
}
|
||||
// Only a single match is actionable. Leave config nil when several match so
|
||||
// the caller reports the collision instead of picking one arbitrarily.
|
||||
if len(candidates) == 1 {
|
||||
out.config = candidates[0]
|
||||
}
|
||||
return out, nil
|
||||
}
|
||||
|
||||
func (api *API) workspaceAgentsExternalAuthListen(ctx context.Context, rw http.ResponseWriter, previous *database.ExternalAuthLink, externalAuthConfig *externalauth.Config, workspace database.Workspace, gitRef chatGitRef) {
|
||||
// Since we're ticking frequently and this sign-in operation is rare,
|
||||
// we are OK with polling to avoid the complexity of pubsub.
|
||||
|
||||
@@ -3876,3 +3876,334 @@ func TestWorkspaceAgentsExternalAuthExpiresAt(t *testing.T) {
|
||||
"ExpiresAt should be zero when the token has no expiry")
|
||||
})
|
||||
}
|
||||
|
||||
// fakeExternalAuthConfig builds a minimal, network-free external auth
|
||||
// provider config: RefreshToken short-circuits because the seeded link's
|
||||
// AccessToken already matches what the fake OAuth2 config would return, and
|
||||
// ValidateURL is omitted so the token is always treated as valid.
|
||||
func fakeExternalAuthConfig(id, token string, regex *regexp.Regexp) *externalauth.Config {
|
||||
return &externalauth.Config{
|
||||
InstrumentedOAuth2Config: &testutil.OAuth2Config{
|
||||
Token: &oauth2.Token{
|
||||
AccessToken: token,
|
||||
RefreshToken: "refresh-" + id,
|
||||
Expiry: dbtime.Now().Add(24 * time.Hour),
|
||||
},
|
||||
},
|
||||
ID: id,
|
||||
Regex: regex,
|
||||
Type: codersdk.EnhancedExternalAuthProviderGitHub.String(),
|
||||
RefreshGroup: new(singleflight.Group),
|
||||
}
|
||||
}
|
||||
|
||||
// TestWorkspaceAgentsExternalAuthTemplateScoped covers PLAT-190: when a
|
||||
// GIT_ASKPASS-style request supplies only a hostname (never an ID), the
|
||||
// server must prefer the requesting workspace's own template-declared
|
||||
// providers over a blind, order-dependent scan of every deployment-configured
|
||||
// provider.
|
||||
func TestWorkspaceAgentsExternalAuthTemplateScoped(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
const (
|
||||
matchHost = "https://github.com"
|
||||
idBroad = "provider-broad"
|
||||
idDot = "provider-dotfiles"
|
||||
idOther = "provider-other"
|
||||
)
|
||||
githubRegex := regexp.MustCompile(`^(https?://)?github\.com(/.*)?$`)
|
||||
gitlabRegex := regexp.MustCompile(`^(https?://)?gitlab\.com(/.*)?$`)
|
||||
|
||||
// setup creates a deployment with the given providers (in the given
|
||||
// order), a workspace built from a template version declaring
|
||||
// declaredIDs, and seeds a valid ExternalAuthLink for every provider in
|
||||
// linkProviderIDs so any of them could be returned if selection picked
|
||||
// the wrong one. Providers omitted from linkProviderIDs are left
|
||||
// unauthenticated (no link), to exercise the authenticate-URL flow.
|
||||
setup := func(t *testing.T, providers []*externalauth.Config, declaredIDs, linkProviderIDs []string) (agentClient *agentsdk.Client) {
|
||||
t.Helper()
|
||||
|
||||
client, db := coderdtest.NewWithDatabase(t, &coderdtest.Options{
|
||||
ExternalAuthConfigs: providers,
|
||||
})
|
||||
first := coderdtest.CreateFirstUser(t, client)
|
||||
_, user := coderdtest.CreateAnotherUser(t, client, first.OrganizationID)
|
||||
|
||||
declared, err := json.Marshal(func() []database.ExternalAuthProvider {
|
||||
out := make([]database.ExternalAuthProvider, len(declaredIDs))
|
||||
for i, id := range declaredIDs {
|
||||
out[i] = database.ExternalAuthProvider{ID: id}
|
||||
}
|
||||
return out
|
||||
}())
|
||||
require.NoError(t, err)
|
||||
|
||||
tv := dbfake.TemplateVersion(t, db).Seed(database.TemplateVersion{
|
||||
OrganizationID: first.OrganizationID,
|
||||
CreatedBy: first.UserID,
|
||||
}).Do()
|
||||
err = db.UpdateTemplateVersionExternalAuthProvidersByJobID(dbauthz.AsProvisionerd(context.Background()), database.UpdateTemplateVersionExternalAuthProvidersByJobIDParams{
|
||||
JobID: tv.TemplateVersion.JobID,
|
||||
ExternalAuthProviders: declared,
|
||||
UpdatedAt: dbtime.Now(),
|
||||
})
|
||||
require.NoError(t, err)
|
||||
|
||||
for _, id := range linkProviderIDs {
|
||||
dbgen.ExternalAuthLink(t, db, database.ExternalAuthLink{
|
||||
ProviderID: id,
|
||||
UserID: user.ID,
|
||||
OAuthAccessToken: id + "-token",
|
||||
OAuthExpiry: dbtime.Now().Add(24 * time.Hour),
|
||||
})
|
||||
}
|
||||
|
||||
r := dbfake.WorkspaceBuild(t, db, database.WorkspaceTable{
|
||||
OrganizationID: first.OrganizationID,
|
||||
OwnerID: user.ID,
|
||||
TemplateID: tv.Template.ID,
|
||||
}).Seed(database.WorkspaceBuild{
|
||||
TemplateVersionID: tv.TemplateVersion.ID,
|
||||
}).WithAgent().Do()
|
||||
|
||||
return agentsdk.New(client.URL, agentsdk.WithFixedToken(r.AgentToken))
|
||||
}
|
||||
|
||||
// A template declaring only idDot must always resolve to
|
||||
// idDot's token for a host both providers match, regardless of which
|
||||
// order the two providers are configured in deployment-wide.
|
||||
for _, tc := range []struct {
|
||||
name string
|
||||
order []string
|
||||
}{
|
||||
{"DeclaredProviderLast", []string{idBroad, idDot}},
|
||||
{"DeclaredProviderFirst", []string{idDot, idBroad}},
|
||||
} {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
t.Parallel()
|
||||
ctx := testutil.Context(t, testutil.WaitShort)
|
||||
|
||||
providers := make([]*externalauth.Config, len(tc.order))
|
||||
for i, id := range tc.order {
|
||||
providers[i] = fakeExternalAuthConfig(id, id+"-token", githubRegex)
|
||||
}
|
||||
agentClient := setup(t, providers, []string{idDot}, []string{idBroad, idDot})
|
||||
|
||||
resp, err := agentClient.ExternalAuth(ctx, agentsdk.ExternalAuthRequest{Match: matchHost})
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, idDot+"-token", resp.AccessToken,
|
||||
"must resolve to the template-declared provider regardless of deployment config order")
|
||||
})
|
||||
}
|
||||
|
||||
// A template declaring no providers at all falls back to today's
|
||||
// existing full-deployment scan (unchanged, potentially ambiguous
|
||||
// behavior in that specific case remains explicitly out of scope).
|
||||
t.Run("NoDeclaredProvidersFallsBackToFullScan", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
ctx := testutil.Context(t, testutil.WaitShort)
|
||||
|
||||
providers := []*externalauth.Config{
|
||||
fakeExternalAuthConfig(idBroad, idBroad+"-token", githubRegex),
|
||||
fakeExternalAuthConfig(idDot, idDot+"-token", githubRegex),
|
||||
}
|
||||
agentClient := setup(t, providers, nil, []string{idBroad, idDot})
|
||||
|
||||
resp, err := agentClient.ExternalAuth(ctx, agentsdk.ExternalAuthRequest{Match: matchHost})
|
||||
require.NoError(t, err)
|
||||
// Legacy behavior: last matching entry in deployment config order wins.
|
||||
require.Equal(t, idDot+"-token", resp.AccessToken)
|
||||
})
|
||||
|
||||
// A template declaring only a provider for an unrelated host must
|
||||
// still resolve a genuinely different host via the fallback scan,
|
||||
// rather than losing access to hosts the template never mentioned.
|
||||
t.Run("UnrelatedHostStillResolvesViaFallback", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
ctx := testutil.Context(t, testutil.WaitShort)
|
||||
|
||||
providers := []*externalauth.Config{
|
||||
fakeExternalAuthConfig(idDot, idDot+"-token", githubRegex),
|
||||
fakeExternalAuthConfig(idOther, idOther+"-token", gitlabRegex),
|
||||
}
|
||||
// Template only declares idDot (for github.com); idOther (gitlab.com)
|
||||
// is never declared, but must still work for its own host.
|
||||
agentClient := setup(t, providers, []string{idDot}, []string{idDot, idOther})
|
||||
|
||||
resp, err := agentClient.ExternalAuth(ctx, agentsdk.ExternalAuthRequest{Match: "https://gitlab.com"})
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, idOther+"-token", resp.AccessToken)
|
||||
})
|
||||
|
||||
// When several of the template's own declared providers match a
|
||||
// hostname, the server must return a clear error rather than silently
|
||||
// pick one. 404 specifically, so `coder gitaskpass` warns and defers to
|
||||
// git's own credential behavior.
|
||||
t.Run("AmbiguousDeclaredSetReturnsError", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
ctx := testutil.Context(t, testutil.WaitShort)
|
||||
|
||||
providers := []*externalauth.Config{
|
||||
fakeExternalAuthConfig(idBroad, idBroad+"-token", githubRegex),
|
||||
fakeExternalAuthConfig(idDot, idDot+"-token", githubRegex),
|
||||
}
|
||||
agentClient := setup(t, providers, []string{idBroad, idDot}, []string{idBroad, idDot})
|
||||
|
||||
_, err := agentClient.ExternalAuth(ctx, agentsdk.ExternalAuthRequest{Match: matchHost})
|
||||
require.Error(t, err)
|
||||
var sdkErr *codersdk.Error
|
||||
require.ErrorAs(t, err, &sdkErr)
|
||||
require.Equal(t, http.StatusNotFound, sdkErr.StatusCode())
|
||||
require.Contains(t, sdkErr.Message, idBroad)
|
||||
require.Contains(t, sdkErr.Message, idDot)
|
||||
})
|
||||
|
||||
// A declared provider that the deployment no longer configures must not
|
||||
// let another provider for the same host stand in for it, even though
|
||||
// that provider would satisfy a deployment-wide scan.
|
||||
t.Run("MissingDeclaredProviderDoesNotFallBack", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
ctx := testutil.Context(t, testutil.WaitShort)
|
||||
|
||||
// Only idBroad is configured. The template declares idDot, which an
|
||||
// administrator has since removed from the deployment config.
|
||||
providers := []*externalauth.Config{
|
||||
fakeExternalAuthConfig(idBroad, idBroad+"-token", githubRegex),
|
||||
}
|
||||
agentClient := setup(t, providers, []string{idDot}, []string{idBroad})
|
||||
|
||||
_, err := agentClient.ExternalAuth(ctx, agentsdk.ExternalAuthRequest{Match: matchHost})
|
||||
require.Error(t, err)
|
||||
var sdkErr *codersdk.Error
|
||||
require.ErrorAs(t, err, &sdkErr)
|
||||
require.Equal(t, http.StatusNotFound, sdkErr.StatusCode())
|
||||
require.Contains(t, sdkErr.Message, idDot,
|
||||
"should name the declared provider the deployment no longer configures")
|
||||
require.NotContains(t, sdkErr.Message, idBroad,
|
||||
"must not offer an undeclared provider as a substitute")
|
||||
})
|
||||
|
||||
// A stale declaration for one host must not block an unambiguous
|
||||
// declared match for a different host. Only the fallback is withheld.
|
||||
t.Run("MissingDeclaredProviderDoesNotBlockOtherDeclaredMatch", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
ctx := testutil.Context(t, testutil.WaitShort)
|
||||
|
||||
providers := []*externalauth.Config{
|
||||
fakeExternalAuthConfig(idOther, idOther+"-token", gitlabRegex),
|
||||
}
|
||||
// idDot (github.com) is declared but no longer configured, while
|
||||
// idOther (gitlab.com) is both declared and configured.
|
||||
agentClient := setup(t, providers, []string{idDot, idOther}, []string{idOther})
|
||||
|
||||
resp, err := agentClient.ExternalAuth(ctx, agentsdk.ExternalAuthRequest{Match: "https://gitlab.com"})
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, idOther+"-token", resp.AccessToken)
|
||||
})
|
||||
|
||||
// Once narrowed to a single declared candidate, the existing
|
||||
// authenticate-URL flow must still work unchanged for a provider the
|
||||
// owner has not yet authenticated with.
|
||||
t.Run("OptionalUnauthenticatedDeclaredProviderReturnsAuthURL", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
ctx := testutil.Context(t, testutil.WaitShort)
|
||||
|
||||
providers := []*externalauth.Config{
|
||||
fakeExternalAuthConfig(idBroad, idBroad+"-token", githubRegex),
|
||||
fakeExternalAuthConfig(idDot, idDot+"-token", githubRegex),
|
||||
}
|
||||
// idDot is declared and matches the hostname, but has no seeded link.
|
||||
agentClient := setup(t, providers, []string{idDot}, []string{idBroad})
|
||||
|
||||
resp, err := agentClient.ExternalAuth(ctx, agentsdk.ExternalAuthRequest{Match: matchHost})
|
||||
require.NoError(t, err)
|
||||
require.Empty(t, resp.AccessToken)
|
||||
require.Contains(t, resp.URL, "/external-auth/"+idDot,
|
||||
"should prompt for the declared provider specifically, not idBroad")
|
||||
})
|
||||
}
|
||||
|
||||
// TestWorkspaceAgentsExternalAuthMultipleTemplates covers the headline
|
||||
// scenario from PLAT-190: two workspaces, built from two different
|
||||
// templates that each declare a different single provider, must each
|
||||
// independently resolve to their own template's provider - never each
|
||||
// other's - regardless of deployment config order or concurrent activity.
|
||||
func TestWorkspaceAgentsExternalAuthMultipleTemplates(t *testing.T) {
|
||||
t.Parallel()
|
||||
ctx := testutil.Context(t, testutil.WaitShort)
|
||||
|
||||
const (
|
||||
matchHost = "https://github.com"
|
||||
id1 = "provider-1"
|
||||
id2 = "provider-2"
|
||||
)
|
||||
githubRegex := regexp.MustCompile(`^(https?://)?github\.com(/.*)?$`)
|
||||
|
||||
client, db := coderdtest.NewWithDatabase(t, &coderdtest.Options{
|
||||
ExternalAuthConfigs: []*externalauth.Config{
|
||||
fakeExternalAuthConfig(id1, id1+"-token", githubRegex),
|
||||
fakeExternalAuthConfig(id2, id2+"-token", githubRegex),
|
||||
},
|
||||
})
|
||||
first := coderdtest.CreateFirstUser(t, client)
|
||||
_, user := coderdtest.CreateAnotherUser(t, client, first.OrganizationID)
|
||||
|
||||
declare := func(t *testing.T, id string) dbfake.TemplateVersionResponse {
|
||||
t.Helper()
|
||||
declared, err := json.Marshal([]database.ExternalAuthProvider{{ID: id}})
|
||||
require.NoError(t, err)
|
||||
tv := dbfake.TemplateVersion(t, db).Seed(database.TemplateVersion{
|
||||
OrganizationID: first.OrganizationID,
|
||||
CreatedBy: first.UserID,
|
||||
}).Do()
|
||||
err = db.UpdateTemplateVersionExternalAuthProvidersByJobID(dbauthz.AsProvisionerd(context.Background()), database.UpdateTemplateVersionExternalAuthProvidersByJobIDParams{
|
||||
JobID: tv.TemplateVersion.JobID,
|
||||
ExternalAuthProviders: declared,
|
||||
UpdatedAt: dbtime.Now(),
|
||||
})
|
||||
require.NoError(t, err)
|
||||
return tv
|
||||
}
|
||||
tv1 := declare(t, id1)
|
||||
tv2 := declare(t, id2)
|
||||
|
||||
dbgen.ExternalAuthLink(t, db, database.ExternalAuthLink{
|
||||
ProviderID: id1, UserID: user.ID, OAuthAccessToken: id1 + "-token", OAuthExpiry: dbtime.Now().Add(24 * time.Hour),
|
||||
})
|
||||
dbgen.ExternalAuthLink(t, db, database.ExternalAuthLink{
|
||||
ProviderID: id2, UserID: user.ID, OAuthAccessToken: id2 + "-token", OAuthExpiry: dbtime.Now().Add(24 * time.Hour),
|
||||
})
|
||||
|
||||
build := func(t *testing.T, tv dbfake.TemplateVersionResponse) *agentsdk.Client {
|
||||
t.Helper()
|
||||
r := dbfake.WorkspaceBuild(t, db, database.WorkspaceTable{
|
||||
OrganizationID: first.OrganizationID,
|
||||
OwnerID: user.ID,
|
||||
TemplateID: tv.Template.ID,
|
||||
}).Seed(database.WorkspaceBuild{
|
||||
TemplateVersionID: tv.TemplateVersion.ID,
|
||||
}).WithAgent().Do()
|
||||
return agentsdk.New(client.URL, agentsdk.WithFixedToken(r.AgentToken))
|
||||
}
|
||||
agent1 := build(t, tv1)
|
||||
agent2 := build(t, tv2)
|
||||
|
||||
var wg sync.WaitGroup
|
||||
var resp1, resp2 agentsdk.ExternalAuthResponse
|
||||
var err1, err2 error
|
||||
wg.Add(2)
|
||||
go func() {
|
||||
defer wg.Done()
|
||||
resp1, err1 = agent1.ExternalAuth(ctx, agentsdk.ExternalAuthRequest{Match: matchHost})
|
||||
}()
|
||||
go func() {
|
||||
defer wg.Done()
|
||||
resp2, err2 = agent2.ExternalAuth(ctx, agentsdk.ExternalAuthRequest{Match: matchHost})
|
||||
}()
|
||||
wg.Wait()
|
||||
|
||||
require.NoError(t, err1)
|
||||
require.NoError(t, err2)
|
||||
require.Equal(t, id1+"-token", resp1.AccessToken, "workspace built from template 1 must always get provider 1's token")
|
||||
require.Equal(t, id2+"-token", resp2.AccessToken, "workspace built from template 2 must always get provider 2's token")
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user