feat(coderd_oauth2_provider_settings): manage the OAuth2 DCR toggle - #399
Conversation
`codersdk.OAuth2ProviderSettings` and the `OAuth2ProviderSettings` / `PutOAuth2ProviderSettings` client methods are introduced by coder/coder#27316, which is required by the `coderd_oauth2_provider_settings` resource. TEMPORARY PIN: #27316 has not merged, so no released coder/coder version exposes these symbols. This pins the PR's head commit (569a0eb3411212ff080b836f514d1dfe4386ccc7, branch coder-eng-3056-dcr-flag) so the resource compiles and its tests run. Re-pin to a released version before merging. Transitively bumps the `go` directive to 1.26.5 and ~12 indirect dependencies. Refs #395
coder/coder#27316 adds a deployment-level toggle for OAuth2 Dynamic Client Registration (RFC 7591), reachable via `coder oauth2-provider dcr enable|disable` or a raw `PUT /api/v2/oauth2-provider/settings`. Neither is declarative, so a deployment managed by this provider had no way to express "DCR is enabled here" as Terraform state, short of a `local-exec` provisioner that participates in neither plan/diff, drift detection, nor destroy. Add a `coderd_oauth2_provider_settings` resource and a data source of the same name, modelled on `coderd_organization_sync_settings`: a singleton wrapping a GET-for-read / PUT-for-write API with no delete endpoint. Resource: - `dynamic_client_registration_enabled` is Required. Opting out means not declaring the resource; a plain Optional attribute would refresh to a concrete bool against a null plan and produce a perpetual diff. - Create and Update share one idempotent PUT; the API has no separate create. - Delete resets to the documented default (false), since no DELETE verb exists. A failed reset keeps the resource in state so destroy can retry. - ImportState allows adopting an already-configured deployment without overwriting it. The import ID is an unused placeholder: the resource is a deployment-wide singleton and Read() takes no parameters. - ModifyPlan warns at plan time when a first apply would disable DCR on a deployment where it is currently enabled. Deliberately asymmetric: a live `false` is indistinguishable from never-configured, so warning in that direction would fire on every greenfield apply. - A 404 is reported as an unsupported Coder version rather than treated as a deleted resource. This singleton always exists on a supported deployment, so 404 can only mean the endpoint is missing; removing state would hide a version problem behind a phantom diff. Data source: read-only, GET only, never PUT. The setting is a deployment singleton, so only one configuration can own the resource; any other that needs the value reads it here. It also works with tokens holding read but not write access on the deployment config, a broader set of roles than the resource requires. `DynamicClientRegistrationEnabled` is a `*bool`, so that a PUT can omit it to leave the value unchanged. Both write paths therefore send an explicit non-nil pointer: this resource owns the value outright, and omitting it would make Create/Update no-ops and silently turn Delete's reset-to-default into a no-op. Reads go through `dcrEnabledOrDefault`, which falls back to the deployment default if the field is ever nil -- a GET is documented to always return non-nil, so that branch is defensive against a contract violation. Tests run against a recording httptest fake rather than `integration.StartCoder`, which pulls a published coder image that does not yet contain #27316. The fake also allows asserting requests that must *not* happen -- import never PUTs, the data source never PUTs, an undeclared resource issues no calls at all -- and injecting 403/404/5xx responses that a real deployment will not produce on demand. It records whether the DCR field was present on each PUT, not just its value, so a regression that stopped sending it would fail rather than pass silently. Closes #395
d106a78 to
e7f2ca0
Compare
… 403 coderd gates the whole `/api/v2/oauth2-provider` route behind the `oauth2` experiment via `httpmw.RequireExperimentWithDevBypass`. With the experiment off, route middleware answers 403 before any handler runs, so the DCR setting can be neither read nor changed. That is the correct behaviour, and it is enforced server-side; the problem was how the provider reported it. A bare "Client Error" is indistinguishable at a glance from the RBAC denial a Member or Auditor token produces, and neither the schema docs nor the examples mentioned the requirement. The experiment is off by default and is not covered by `--experiments='*'` (`ExperimentsSafe` lists only `ExperimentMinimumImplicitMember`), so a stock release deployment hits this on a first apply. `oauth2ProviderSettingsDiag` now separates the experiment gate from an RBAC denial and raises "OAuth2 Experiment Not Enabled", naming `CODER_EXPERIMENTS=oauth2` / `--experiments=oauth2` while preserving coderd's own message. This mirrors `isWorkspaceSharingExperimentOff` in `organization_resource.go`, which solves the same problem for the workspace-sharing experiment. Discriminating on message text is unavoidable because coderd reuses 403 for both refusals, but it degrades safely: a reworded upstream message falls back to the generic "Client Error" this branch replaced. A 404 still takes the "Unsupported Coder Version" branch, so the two remedies -- upgrade Coder versus change its configuration -- stay distinguishable. The path cannot be reached through `scripts/develop.sh`, because development builds bypass the experiment check outright, so coverage is a fake serving the real 403 body verbatim plus a table test for the discriminator that concentrates on the inputs which must *not* match: a false positive would relabel every genuine permission error as "enable the experiment".
Manual verification:
|
| Change | Effect on this verification |
|---|---|
Data source removed (1b55aac) |
TC16–TC19 and TC24 withdrawn → 24 cases became 19 |
Diagnostic verbs (a2b0ee6, M1) |
TC13 now expects unable to **create** and TC9 unable to **reset**, where both previously said update. The old run's captured output asserted the wrong text |
State from the PUT response (a2b0ee6, R1) |
Deliberately not manually observable — see Why R1 has no manual test |
This still matters more than usual for this resource: every automated test runs against an httptest fake, because integration.StartCoder pulls a published image and none contains coder/coder#27316 yet (it has merged, but is not in a release). So this is the only thing exercising the provider against a real coderd.
Summary
| # | Test | Result |
|---|---|---|
| TC1, TC20, TC22 | Not declaring the resource touches nothing | ✅ Pass |
| TC2 | First create against a never-configured deployment | ✅ Pass — now genuinely hits sql.ErrNoRows |
| TC4 | Declared but the required attribute omitted | ✅ Pass |
| TC5 | Drift detection after an out-of-band CLI change | ✅ Pass |
| TC6 | No diff when config already matches | ✅ Pass |
| TC7 | Update is in-place, not a replacement | ✅ Pass |
| TC8 | Destroy resets to the documented default via PUT |
✅ Pass |
| TC3 | Adoption footgun + the plan-time warning added in this PR | ✅ Pass |
| TC21 | Import adopts the live value with a GET only |
✅ Pass |
| TC13 | Member token refused | ✅ Pass — confirms M1: create |
| TC23 | Auditor: read works, write refused | ✅ Pass — confirms M1: update |
| TC9 | Failed destroy stays retryable | ✅ Pass — confirms M1: reset |
| TC14 | Concurrent mutation mid-apply | ✅ Pass |
| TC15 | Two blocks targeting the same singleton | ✅ Pass |
| TC10 | Provider version predates the feature | |
| TC11 | New provider against a pre-#27316 coderd | |
| TC12 | Coder upgraded in the same apply | N/A — collapses into TC11 |
| — | Plan-time warning (beyond the proposal) | ✅ Pass |
| — | oauth2 experiment disabled (beyond the proposal) |
Not manually reachable — dev builds bypass the gate |
16 of 19 cases re-run and passing. TC10 and TC11 were not re-run; TC12 is n/a — see Gaps for what that costs and why.
Environment: ./scripts/develop.sh does not work on this machine (4 gaps)
Worth recording because the previous run used develop.sh and this one could not. Three of the four gaps stem from one cause: git checkout resets file mtimes, so make wants to re-run the entire codegen pipeline.
| # | Failure | Cause | Fix |
|---|---|---|---|
| 1 | You need at least bash 4.0 |
macOS ships bash 3.2 | PATH="$(brew --prefix bash)/bin:$PATH" |
| 2 | env: node: No such file or directory |
checkout made site/**/*.tsx newer than the built output |
touch site/out/index.html |
| 3 | sqlc: command not found |
same mtime problem on the DB codegen | touch the four committed generated files |
| 4 | server did not become ready in 1m0s |
develop.sh unconditionally starts pnpm --dir ./site dev, which needs node; when it dies the health wait tears the API down |
none |
node, sqlc, protoc and protoc-gen-go are all missing, and there is no Brewfile or install-tools target, so the gap is open-ended. Gap 4 has no workaround — the site dev server is not optional in develop.sh.
What was run instead — coderd directly, no installs:
# The binary develop.sh builds before dying is fine
./build/coder_darwin_arm64 server \
--http-address 0.0.0.0:3000 --access-url http://127.0.0.1:3000 \
--experiments oauth2 --dangerous-allow-cors-requests=true
# with CODER_PG_CONNECTION_URL pointing at a dedicated postgres:17-alpine
# container on 5442, so the postgres already on 5432 is untouchedAdmin/member/auditor users created over the HTTP API, since coder login cannot run unattended.
Deviations that change what "under test" means:
- Server built from
coder-eng-3062-dcr-flag-ui@aa69a4ca03, notfbac6024. Verified equivalent rather than assumed: that branch isfbac6024plus three commits, andgit diff fbac6024 <branch> -- . ':!site'is empty. The settings API, RBAC and database layer are byte-identical to the pinned commit. - No web UI (no node). This plan makes no UI assertions.
--experiments oauth2passed explicitly. A-develbuild bypasses the check anyway, but this removes ambiguity.
The M1 fix, confirmed on a real server
patch() passed a hard-coded "update" to the diagnostic helper, so a failed create and a failed destroy both reported "unable to update". All three verbs are now verified end to end:
| Operation | Verb | Case | Evidence |
|---|---|---|---|
| First apply, no prior state | create |
TC13 | 'unable to create': 2, 'unable to update': 0 |
| Apply over existing state | update |
TC23 | 'unable to update': 2, 'unable to create': 0 |
| Destroy | reset |
TC9 | 'unable to reset': 2, 'unable to update': 0 |
Output
TC13 — member token, first apply:
APPLY EXIT CODE: 1
│ unable to create OAuth2 provider settings, got error: PUT
│ http://localhost:3000/api/v2/oauth2-provider/settings: unexpected status
│ code 403: Forbidden.
'unable to create': 2 ← was 'update' before a2b0ee6
'unable to update': 0
requests: method=PUT tf_rpc=ApplyResourceChange
state entries: []
TC9 — failed destroy under the auditor token:
DESTROY EXIT CODE: 1
│ unable to reset OAuth2 provider settings, got error: PUT
│ …unexpected status code 403: Forbidden.
'unable to reset': 2 ← was 'update' before a2b0ee6
'unable to update': 0
state list: [coderd_oauth2_provider_settings.dcr] ← survived
live value (untouched): true
Empty state after TC13's failed create means no phantom entry for a later plan to update or destroy. State surviving TC9's failed destroy is the opposite requirement, and equally important: had the entry been dropped, the admin would believe DCR was reset while the deployment kept the last-applied value, and no later destroy would retry.
Why R1 has no manual test
R1 changed Create/Update to build state from the PUT response body rather than the plan value. This is deliberately not manually testable: coderd echoes exactly what it stored, so plan-sourced and response-sourced state are indistinguishable against a real server. Forcing a divergence needs a fake, which is what StateComesFromThePutResponse and AbsentPutResponseFieldFallsBackToThePlanValue do.
What the real server did confirm is the cost: TC6's reconverging apply issued exactly two requests, GET + PUT, not three. Because the endpoint returns the stored value in its own response, no follow-up GET is needed — response-sourced state costs zero extra round trips.
Improvements over the previous run
TC2 now genuinely exercises the server's sql.ErrNoRows branch
The previous run carried an explicit caveat that this branch was never reached: §1's out-of-band PUT leaves a site_configs row, so the server only ever read "row exists, holds false". Deleting the row directly is equivalent to --db-reset and far cheaper:
DELETE FROM site_configs WHERE key = 'oauth2_dcr_enabled'; -- DELETE 1
rows named oauth2_dcr_enabled now: 0
GET status: 200 body: {"dynamic_client_registration_enabled":false}
The GET returning 200 with false rather than erroring is the verification of the server's coalesce behaviour, and it is the precondition making oauth2ProviderSettingsDefaultDCR = false correct in the provider.
The apply then reads clean, without the stray 1 to destroy earlier runs showed (that was this plan's own bookkeeping — a terraform_data left over from TC1):
Apply complete! Resources: 1 added, 0 changed, 0 destroyed.
method=PUT tf_rpc=ApplyResourceChange ← GET: 0, so no plan-time request
live: true state: true db row: oauth2_dcr_enabled=true
Three-way agreement — live API, Terraform state, and the database row. State alone would prove only that the provider believes it wrote; the live read alone would not prove Terraform recorded what it wrote; the row proves the write reached storage.
Audit-log evidence, which closes three assertions the debug log cannot
coderd audits every settings PUT (#27316), and the debug log does not contain request bodies. Three places where this mattered:
TC22 — a value check cannot detect a write that sets the same value, nor a write-then-write-back. The audit log can:
oauth2_provider_settings writes recorded: 1
time: 2026-07-29T17:07:54.587589Z user_agent: curl/8.7.1 ← NOT Terraform
diff: {"dynamic_client_registration_enabled":{"old":false,"new":true}}
One write, attributable to the setup curl by user-agent and by timestamp (one second before the first apply). §1's claim now rests on three independent signals rather than one: zero requests in the debug log, an unmoved live value, and no Terraform-authored audit entry.
TC8 — the previous run noted the destroy's PUT body was unobservable, leaving the resulting false as the only evidence. Now direct:
ua: Go-http-client/1.1 diff: {"…":{"old":true,"new":false}}
This verifies a specific claim in patch()'s comment: the field was sent explicitly as false, not omitted. Had it been omitted, the server treats nil as "leave the current value alone", so the audit would show no transition and the value would have stayed true. Positive proof the reset is a real write rather than the silent no-op omission would cause.
TC14 — the previous run recorded that "last write wins" is not constructible on a boolean attribute, since the other actor can only write the value live already holds or the value Terraform is about to write. The audit log breaks the tie:
ua: curl/8.7.1 diff: {"…":{"old":true,"new":false}}
ua: Go-http-client/1.1 diff: {} ← Terraform's PUT
An empty diff on an audited write is the signature being looked for: the request genuinely happened, and changed nothing because the value was already correct. That separates "no request made" from "request made, no effect" — direct proof the provider writes unconditionally rather than comparing first.
A pre-condition gap found in TC23
TC23's apply initially exited 0 and refused nothing. Not a provider bug — the config matched the imported value, so Terraform planned no change, issued no PUT, and never got a 403:
No changes. Your infrastructure matches the configuration.
Apply complete! Resources: 0 added, 0 changed, 0 destroyed.
The step only works if the config differs from the live value, so a real write is attempted. This is the vacuous-pass trap in a new costume — here surfacing as an apparent failure to refuse rather than a false pass. Now recorded as a load-bearing pre-condition.
Selected results
TC1 / TC20 / TC22 — not declaring the resource touches nothing
APPLY EXIT CODE: 0
Plan: 1 to add, 0 to change, 0 to destroy.
Apply complete! Resources: 1 added, 0 changed, 0 destroyed.
settings requests: 0
mentions of the resource type: 0
requests to coderd at all (api/v2): 0
ConfigureProvider events: 0
live value after apply: {"dynamic_client_registration_enabled":true}
The guarantee is stronger than "no settings requests". Configure() calls users/me and entitlements unconditionally, so zero of those means Terraform pruned the coderd provider node entirely — despite provider.tf declaring a valid config with a working token — because no resource in the graph belonged to it.
TC20 repeats it for a modification (1 changed, verified landed in state) rather than a create. The non-empty plan is the point: an empty plan would make the zeros vacuous.
Required was never evaluated, because Required is a property of a declared block, not of the provider. Adding this resource does not retroactively affect existing configurations.
TC3 — the adoption footgun, and the warning this PR adds
Live true, config false, no state:
# coderd_oauth2_provider_settings.dcr will be created
+ dynamic_client_registration_enabled = false
Plan: 1 to add, 0 to change, 0 to destroy.
Nothing anywhere in that diff says live is currently true. And the mitigation:
│ Warning: Overwriting an out-of-band value
│
│ with coderd_oauth2_provider_settings.dcr,
│ on main.tf line 2, in resource "coderd_oauth2_provider_settings" "dcr":
│ 2: dynamic_client_registration_enabled = false
│
│ `dynamic_client_registration_enabled` is currently `true` on this
│ deployment, and applying will set it to `false`. Terraform has no prior
│ state for this resource, so this change is not shown as a diff.
│
│ If you meant to adopt the deployment's existing value rather than overwrite
│ it, run `terraform import coderd_oauth2_provider_settings.<name>
│ oauth2_provider_settings` first.
Anchored to the attribute, proven from the log rather than inferred from the line number:
diagnostic_attribute="AttributeName(\"dynamic_client_registration_enabled\")"
diagnostic_severity=WARNING tf_rpc=PlanResourceChange
The advisory's cost, measured. ReadResource was called zero times — there is no state, so Terraform had nothing to refresh, meaning the plan-time GET cannot be attributed to Read(). It came from ModifyPlan alone:
§2 (planned true): method=PUT tf_rpc=ApplyResourceChange ← no plan-time request
§8 (planned false): method=GET tf_rpc=PlanResourceChange ← the advisory's read
So the feature costs zero extra requests when enabling and one GET when disabling on a create. A naive "always GET and compare" would have added a round trip to every plan of every deployment.
It warns, it does not block — exit 0, zero Error: blocks, and the audit trail records a real true → false transition at a moment when the plan showed no diff. That asymmetry is the footgun.
TC21 — import is the safe alternative, from identical starting conditions
Import successful!
PUT requests to the settings endpoint: 0
all requests: tf_rpc=ReadResource method=GET
state: true ← took the LIVE value
live: true ← undisturbed
config: false ← still disagrees
~ dynamic_client_registration_enabled = true -> false
Plan: 0 to add, 1 to change, 0 to destroy.
warning occurrences: 0
requests at tf_rpc=PlanResourceChange: 0
| TC3: apply directly | TC21: import first | |
|---|---|---|
| plan shows | 1 to add, no value diff |
~ true -> false |
| warning | raised | none needed |
| plan-time GET | 1 (the advisory) | 0 |
| live value after | silently false |
still true, pending review |
The 0 requests at PlanResourceChange confirms ModifyPlan exited at the !req.State.Raw.IsNull() guard — it did not merely decline to warn, it never reached the GET. Detecting a create with req.State.Raw.IsNull() rather than a heuristic is what makes that correct; a check like "does state hold a value?" would have warned on every post-import plan, training admins to ignore the one warning that matters.
The single request is tagged ReadResource, not ImportResourceState — ImportState() issues no request itself. It only declares identity, and Terraform then calls Read(). That is why the singleton needs no parsable import ID.
TC15 — two blocks, and the state damage that follows
Apply complete! Resources: 2 added, 0 changed, 0 destroyed. ← no "already exists"
live after: false ← b won
state: a -> {"dynamic_client_registration_enabled":true}
b -> {"dynamic_client_registration_enabled":false}
PLAN EXIT CODE: 2
# coderd_oauth2_provider_settings.a will be updated in-place
~ dynamic_client_registration_enabled = false -> true
Both entries recorded their own configured value, so state claims DCR is simultaneously true and false. Every future plan shows the loser needing an update, and applying it makes the other block the loser — they flap forever, one diff per apply, with no error at any point.
The warning fires for b only (planning false against a live true) and correctly stays silent for a. Not designed for this case, but it means the only plan-time signal that a double declaration is wrong comes from the warning this PR adds — Terraform itself reports an ordinary 2 to add.
Both blocks applied in parallel, so which wins is a genuine race rather than declaration order. b won on both runs; nothing orders them.
Gaps
TC10 and TC11 were not re-run, and this is the one real hole in this verification.
- TC10 (provider version predates the feature) needs
dev_overridesdisabled and a published provider pulled from the registry. - TC11 (new provider, pre-#27316 coderd) needs a second coderd built at an older commit. Given the four toolchain gaps above, a second build at a different commit is a substantial detour.
What it costs. These two are the only cases with no automated equivalent. TC10 cannot be reached in-process at all, and TC11's refresh path — a 404 arriving when prior state exists — is the one thing verifying Read()'s refusal to treat 404 as "resource deleted". Both passed on 2026-07-27 against e7f2ca0, and nothing in 1b55aac or a2b0ee6 touched Read() or oauth2ProviderSettingsDiag's 404 branch — but that is an argument from code inspection, not from this run.
Also unchanged from the previous run: the oauth2 experiment-disabled path is not manually reachable, because -devel builds bypass the gate via RequireExperimentWithDevBypass. It is covered by TestIsOAuth2ExperimentOff (7 cases), TestOAuth2ProviderSettingsExperimentDiag, and …Resource/ExperimentDisabled.
One finding still open, now with fresh evidence. The server reports v2.35.3-devel+aa69a4ca03, so the nearest tag is v2.35.3 — which predates #27316 (merged 2026-07-28). The first release containing the endpoint must therefore be later than v2.35.3, and oauth2ProviderSettingsMinVersion = "2.35.0" is provably too low. The correct value is not yet knowable, since no release contains the endpoint at all.
There was a problem hiding this comment.
Main ask (on the data-source example thread): remove the data source from this PR. Remaining inline comments are non-blocking. Procedural: keep as draft until coder/coder#27316 merges, then swap the pin, go mod tidy, and re-verify 2.35.0.
Coder Agents on behalf of @Emyrk.
There was a problem hiding this comment.
Blocking:
(1) remove the data source (see the example thread);
(2) replace the go.mod pin once coder/coder#27316 ships (re-verify 2.35.0, go mod tidy). Inline comments are non-blocking.
Coder Agents on behalf of @Emyrk.
…dcr-flag # Conflicts: # go.mod # go.sum
Replace the temporary pin on coder/coder#27316's branch head (569a0eb3, `coder-eng-3056-dcr-flag`) with the squash commit that landed it on main, fbac6024. `codersdk.OAuth2ProviderSettings` now comes from main rather than from an unmerged PR, so this branch no longer depends on unreviewed code. The SDK contract is unchanged from the pinned branch head: `DynamicClientRegistrationEnabled *bool` with a `dynamic_client_registration_enabled,omitempty` tag, and `GET`/`PUT` on `/api/v2/oauth2-provider/settings`. The `feat!` marker on the upstream commit denotes a server-side behavior change -- DCR now defaults to disabled -- which matches the default this resource already resets to on destroy. Transitively bumps aws-sdk-go-v2, terraform-provider-coder, klauspost/compress, and valyala/fasthttp.
Remove the read-only `coderd_oauth2_provider_settings` data source, per review. Declaring it alongside the resource in one configuration produces a deterministic stale read: a data source with a fully-known config and no `depends_on` resolves during the plan phase, while the resource's PUT happens during apply, so any plan-time consumer of the data source sees the pre-apply value and lags one apply behind. The reviewed example fed that value to a `count`, which is the worst case. `count` is structural rather than a value, so it must be settled at plan time and is never re-evaluated during apply. Without `depends_on` the count is known-but-stale and the run silently produces a contradictory end state; with `depends_on` it becomes unknown and the configuration fails to plan at all. There is no correct spelling of that example. The data source's stated safety invariant -- that it only ever issues a GET -- rules out write/write conflicts but says nothing about read/write ordering, which is why the existing tests all passed. Every data source test exercised it in isolation. Removing rather than documenting around it: the read-without-owning use case is real but thin, and it can return later behind an enforced "never declare alongside the resource" rule rather than advisory prose. Removes the data source, its tests, its example, and its generated doc, and drops the resource schema's pointer to it. The fake test server and `oauth2ProviderSettingsDiag` stay: both are still used by the resource.
…state from the PUT response Two review nits on `patch()`, taken together because both change its signature. Name the operation that failed. `patch()` passed a hard-coded "update" to `oauth2ProviderSettingsDiag`, so a failed create reported "unable to update OAuth2 provider settings" and a failed `terraform destroy` reported it too. Every write path shares one idempotent PUT, but the diagnostic should name what the user asked for: callers now pass "create", "update", and "reset". Two `ExpectError` regexes cemented the wrong wording and are updated with it. Build state from the PUT response rather than the plan. `patch()` discarded the settings the endpoint echoes back and `Create`/`Update` wrote the plan value into state. The two agree today -- the handler answers with `ptr.Ref(resolvedEnabled)` and stores exactly what it is sent -- so this is not a bug fix. It is so that if the API ever starts normalizing the value, the divergence fails loudly instead of leaving a falsehood in state to surface as drift on a later plan. Since the attribute is Required, Terraform requires final state to equal the planned value, so what this buys is detection, not tolerance. The nil branch deliberately does not use `dcrEnabledOrDefault`: that coerces an absent field to false, which here would turn a successful `true` apply into a spurious "Provider produced inconsistent result after apply". Falling back to the value just sent is the only handling that preserves the contract. Both behaviours are pinned by new tests, each verified to fail against the previous implementation -- the `dcrEnabledOrDefault` variant reproduces that exact error. The fake grows two hooks to make the response distinguishable from the request, which a real deployment never is.
…lassifier Remove `isOAuth2ExperimentOff` and the bespoke "OAuth2 Experiment Not Enabled" diagnostic, per review. coderd answers 403 for two unrelated reasons -- an RBAC denial and the `oauth2` experiment gate -- and its own message already distinguishes them: the gate names the experiment, an RBAC denial is the generic "Forbidden." The generic branch renders the `codersdk.Error`, which includes that message, so passing it through is just as actionable. What the classifier cost was a text match on wording copied from coderd's `httpmw.RequireExperiment`. That can silently degrade to a no-op when the wording changes upstream, and the test guarding it could not catch that, because the fake supplied the same copied string the matcher looked for -- the fake's own comment conceded as much. Nothing had ever observed the match against a real deployment either: the gate is unreachable on a `-devel` build, which bypasses it entirely, so every run of that path was the fake agreeing with itself. If a richer remedy is wanted, it needs a machine-readable error code from coderd rather than string matching here. Tests keep the property that matters and drop the one that could rot. `TestIsOAuth2ExperimentOff` (7 cases pinning the match) is gone. `TestOAuth2ProviderSettingsExperimentDiag` becomes `TestOAuth2ProviderSettingsDiag`, asserting that a 403 of either cause produces one generic diagnostic containing the server's message, plus the surviving 404 version hint and a non-SDK error case. The fake's verbatim message constant is replaced by `SetForbiddenWithMessage`, and both it and the acceptance subtest now use an obviously-synthetic string, so no test depends on coderd's exact wording. `docs/` is unchanged: the schema's experiment warning describes the deployment requirement, not the diagnostic, so it stays accurate.
…on to 2.37.0 `oauth2ProviderSettingsMinVersion` claimed 2.35.0, which was wrong, and the constant is named in the 404 diagnostic — so an admin on a 2.35.x release was being told to upgrade to a version that would not have fixed their 404. The endpoint arrived with coder/coder#27316, merged 2026-07-28 as `fbac6024`. By then the 2.34, 2.35 and 2.36 release branches were all already cut, and none contains the commit: git merge-base --is-ancestor fbac6024 origin/release/2.34 -> no git merge-base --is-ancestor fbac6024 origin/release/2.35 -> no git merge-base --is-ancestor fbac6024 origin/release/2.36 -> no git tag --contains fbac6024 -> (empty) release/2.36 carries commits dated after the merge, so it was branched beforehand and has taken cherry-picks since — but not this one, and a `feat!` breaking change would not be backported into a cut release branch. That makes 2.37 the first line that can carry the endpoint. Still unreleased: v2.35.3 (2026-07-27) is the newest 2.35.x, and the commit is in no tag. The comment records that this should be re-checked once 2.37.0 ships, in case the release lands under a different number. Addresses the "re-verify 2.35.0" half of the blocking review. The unrelated v2.35.0 references in ai_provider_resource.go are for Bedrock STS assume-role support and are untouched.
…ding Trims duplicated warnings in the schema description and example.tf/import.sh to terser wording per review feedback on #399.
2 participants
Note
Unblocked — coder/coder#27316 has merged as
fbac6024.go.modnow pins that squash commit onmaininstead of the PR's branch head (569a0eb3, branchcoder-eng-3056-dcr-flag), so nothing here depends on unreviewed code any more. No releasedcoder/codercontainscodersdk.OAuth2ProviderSettingsyet, so this stays amainpseudo-version pin — the same approach #388 took. Ready for review.Upstream landed as
feat!, but the break is server-side behavior (DCR now defaults to disabled); the SDK contract this PR consumes is unchanged from the previously pinned branch head —DynamicClientRegistrationEnabled *bool, andGET/PUTon/api/v2/oauth2-provider/settings.coder/coder#27316adds a deployment-level toggle for OAuth2 Dynamic Client Registration (RFC 7591), giving admins two ways to flip it: thecoder oauth2-provider dcr enable|disableCLI command, or a rawPUT /api/v2/oauth2-provider/settings. Neither is declarative. A team already managing their deployment through this provider — alongsidecoderd_organization,coderd_license,coderd_organization_sync_settings— has no way to express "DCR is enabled on this deployment" as Terraform-managed state. The only Terraform-shaped option is alocal-execprovisioner shelling out to the CLI, which participates in neither plan/diff, drift detection, norterraform destroy.Add a
coderd_oauth2_provider_settingsresource wrapping the samecodersdkcontract. It is modelled oncoderd_organization_sync_settings, the closest existing precedent: a singleton wrapping a GET-for-read / PUT-for-write API with no delete endpoint. The resource has oneRequiredboolean;CreateandUpdateshare a single idempotentPUT;Deleteresets to the documented default (false) because noDELETEverb exists;ImportStatelets an admin adopt an already-configured deployment without overwriting it; andModifyPlanraises a plan-time warning when a first apply would disable DCR on a deployment where it is currently enabled.Note
An earlier revision of this PR also added a read-only data source of the same name. It has been removed (review), so the net diff no longer contains it.
Declaring it alongside the resource produced a deterministic stale read: a data source with a fully-known config and no
depends_onresolves in the plan phase, while the resource'sPUThappens during apply, so any plan-time consumer saw the pre-apply value and lagged one apply behind. The example fed that value to acount, the worst case —countis structural, so it must be settled at plan time and is never re-evaluated during apply. Withoutdepends_onit is known-but-stale and the run silently ends in a self-contradictory state; withdepends_onit becomes unknown and the configuration will not plan at all (Invalid count argument). There is no correct spelling of that example.The data source's stated safety invariant — that it only ever issues a
GET— forecloses write/write conflicts but is silent on read/write ordering, which is why its tests all passed: every one exercised it in isolation. It can return later behind an enforced "never declare alongside the resource" rule rather than advisory prose.Closes #395. Addresses ENG-3083. Requires coder/coder#27316, which has merged.
Where this sits in the request path
sequenceDiagram autonumber participant U as Admin (Terraform user) participant TF as terraform participant P as terraform-provider-coderd<br/>(NEW) participant SDK as codersdk.Client participant API as coderd participant DB as site_configs<br/>(oauth2_dcr_enabled) Note over U,DB: Adoption path — import first, never overwrites U->>TF: terraform import ...dcr oauth2_provider_settings TF->>P: ImportState(req) Note over P: NEW: placeholder state. req.ID carries<br/>no meaning for a singleton TF->>P: Read(state) (core issues this next) P->>SDK: OAuth2ProviderSettings(ctx) SDK->>API: GET /api/v2/oauth2-provider/settings API->>DB: SELECT oauth2_dcr_enabled DB-->>API: true (live value, never overwritten) API-->>P: 200 OK P->>TF: state.Set — matches reality, no PUT issued Note over U,DB: Apply U->>TF: dynamic_client_registration_enabled = true TF->>P: ModifyPlan(plan) Note over P: NEW: on a create that would disable a<br/>live-enabled deployment, warn at plan time TF->>P: Create/Update(plan) P->>SDK: PutOAuth2ProviderSettings(ctx, {true}) SDK->>API: PUT /api/v2/oauth2-provider/settings API->>API: authorize(ActionUpdate, ResourceDeploymentConfig) API->>DB: UPSERT oauth2_dcr_enabled = true API-->>P: 200 OK (audited) P->>TF: state.Set(data) Note over U,DB: Destroy — no DELETE endpoint exists TF->>P: Delete(state) Note over P: NEW: reset to the default via the same PUT P->>SDK: PutOAuth2ProviderSettings(ctx, {false}) SDK->>API: PUT /api/v2/oauth2-provider/settings API->>DB: UPSERT oauth2_dcr_enabled = false P->>TF: state removedFiles changed: manual vs. generated
Reviewers should focus on the manual files. The generated ones are
make genoutput that follows mechanically from the schema and examples, and don't need direct review.Manual files (8) — click to expand, grouped the same way as "Suggested review order" below
1. Dependency (isolated in commits
e6283baand07735ed)go.mod,go.sumgithub.com/coder/coder/v2to exposecodersdk.OAuth2ProviderSettings, pinned to the merged squash commitfbac6024onmain— see the note at the top. Transitively bumps thegodirective to 1.26.5 and 19 indirect dependencies.2. The resource
internal/provider/oauth2_provider_settings_resource.goRequiredbool;Read;Create/Updatevia a sharedpatch();Deleteresets tofalse;ImportState;ModifyPlan;dcrEnabledOrDefault, the nil-safe read of the SDK's*bool; andoauth2ProviderSettingsDiag, the shared 404 → minimum-version error every CRUD path routes through.internal/provider/provider.go3. Tests
internal/provider/oauth2_provider_settings_fake_test.gofakeCoderd: anhttpteststand-in serving the twoConfigure()endpoints plusGET/PUTon the settings endpoint. Records every request in order (method, path, PUT body) and can inject403/404/5xxper verb, with pre-GET/pre-PUThooks for simulating mid-apply mutation.internal/provider/oauth2_provider_settings_resource_test.goTestOAuth2ProviderSettingsModifyPlan(8 table cases, plain unit test, no Terraform binary) plusTestAccOAuth2ProviderSettingsResource(15 subtests) andTestAccOAuth2ProviderSettingsNotDeclared(2 subtests).4. Examples (
docs/is generated from these)examples/resources/coderd_oauth2_provider_settings/resource.tfexamples/resources/coderd_oauth2_provider_settings/import.shimport {}block forms.Generated files (1) — from
make gen, no need to review directlydocs/resources/oauth2_provider_settings.md.Suggested review order
1. The SDK contract (in coder/coder#27316, not this PR)
OAuth2ProviderSettingsis a single boolean, and both client methods are a bareGET/PUTwith no identifying parameter. Everything below is shaped by how small that contract is — in particular, a parameterlessRead()is what makesImportStatecheap to support correctly.2. The resource
Read in this order;
oauth2ProviderSettingsDiagis a leaf the others call, so it comes last.Schema— whydynamic_client_registration_enabledisRequired. Opting out of managing DCR means not declaring the resource, not declaring it with the field blank. A plainOptionalattribute is not viable here:Read()writes a concrete bool while an omitted config plans as null, producing a permanentfalse -> nulldiff on every plan.Delete— the one semantic decision that isn't "copy the existing pattern". The API has noDELETEverb, so this resets to the documented default via the samePUT, mirroringOrganizationSyncSettingsResource.Delete. A failed reset leaves the resource in state soterraform destroycan retry.ImportState— the import ID is an unused placeholder, since this is a deployment-wide singleton andRead()takes no parameters. The framework requires the handler to write something, so it writes a placeholder that the framework's follow-upRead()immediately overwrites with the live value.patch()anddcrEnabledOrDefault— the SDK field is a*boolso that a PUT can omit it to mean "leave this alone". Both write paths deliberately send an explicit non-nil pointer: this resource owns the value outright, and omitting it would makeCreate/Updateno-ops and silently turnDelete's reset-to-default into a no-op. The fake records whether the field was present on each PUT, not just its value, so that regression fails a test rather than passing quietly. On the read side a GET is documented to always return non-nil, sodcrEnabledOrDefault's nil branch is defensive against a contract violation, not an expected response.ModifyPlan— warns when a first apply would disable DCR on a deployment where it is currently enabled. Deliberately asymmetric: a livefalseis indistinguishable from never-configured, because the server coalesces both, so warning in that direction would fire on every greenfield apply. A failed lookup here is intentionally silent rather than a plan error —Create()makes the identical call moments later and reports it with wording matching the operation that actually failed.oauth2ProviderSettingsDiag— maps a 404 to an actionable minimum-version error. Deliberately notisNotFound(): that helper also maps a 400"must be an existing uuid or username"to not-found, which is meaningless for a parameterless endpoint. Note this resource must not treat 404 as "resource deleted" the way most resources here do — the singleton always exists on a supported deployment, so 404 can only mean the endpoint is missing, and removing state would hide a version problem behind a phantom diff.3. Tests
oauth2_provider_settings_fake_test.gofirst — the assertions below only make sense once you know the fake records every request.TestOAuth2ProviderSettingsModifyPlan— fastest, and the clearest statement of the plan-time hook's contract.On why a fake rather than
integration.StartCoder: that helper pulls a publishedghcr.io/coder/coderimage, and although #27316 has merged, no published image contains it yet, so a real-server test cannot exist until it ships in a release. Independently, a fake is the only way to assert the requests that must not happen — import never issues aPUT, an undeclared resource issues no settings calls at all — and to produce responses a real deployment won't give on demand (404 from an old version, 403 from a role that would otherwise need provisioning, a transient 500). Adding oneintegration.StartCoderhappy-path test to catch fake-vs-real divergence is the obvious follow-up once #27316 ships in a release.4. Examples
Read last;
docs/follows mechanically.Explicitly out of scope
dynamic_client_registration_enabledonly.oauth2experiment. That is a coderd server startup flag (--experiments/CODER_EXPERIMENTS) with no runtime API, so it belongs to whichever provider manages the deployment's infrastructure — ahelm_release, adocker_container, cloud-init — not to this one.