feat(coderd_oauth2_provider_settings): manage the OAuth2 DCR toggle - #399
feat(coderd_oauth2_provider_settings): manage the OAuth2 DCR toggle#399BobbyHo wants to merge 5 commits into
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:
|
| # | Test | Result |
|---|---|---|
| TC1, TC20, TC22 | Not declaring the resource touches nothing | ✅ Pass |
| TC2 | First create against a never-configured deployment | ✅ Pass |
| 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 |
| TC16, TC19 | Data source reads without owning | ✅ Pass |
| TC13, TC17 | Member token refused on both paths | ✅ Pass |
| TC23, TC24 | Auditor: read works, write refused | ✅ Pass |
| TC9 | Failed destroy stays retryable | ✅ Pass |
| TC10 | Provider version predates the feature | ✅ Pass (manual-only) |
| TC11, TC18 | New provider against a pre-#27316 coderd | ✅ Pass |
| TC12 | Coder upgraded in the same apply | N/A — collapses into TC11, verified there |
| TC14 | Concurrent mutation mid-apply | ✅ Pass (assertion strengthened — see below) |
| TC15 | Two blocks targeting the same singleton | ✅ Pass |
| — | Plan-time warning (beyond the proposal) | ✅ Pass |
| — | oauth2 experiment disabled (beyond the proposal) |
Five defects in my own verification plan also surfaced, two of which were assertions that could not fail. Documented at the end, since they affect how the results should be read.
0. Prerequisites
Setup commands and output
# Terminal 1: dev coderd on the #27316 branch
cd ~/work/coder-oauth-public-client-support
git rev-parse --abbrev-ref HEAD # coder-eng-3056-dcr-flag
./scripts/develop.sh --db-resetversion: v2.35.3-devel+569a0eb341
buildinfo: 200
username: admin roles: owner
GET /api/v2/oauth2-provider/settings: 200
{"dynamic_client_registration_enabled": false}
The 200 is the load-bearing assertion — a 404 would mean the server isn't on #27316 and nothing below would be meaningful.
# Build the provider and point Terraform at it
go build -o "$(go env GOPATH)/bin/terraform-provider-coderd" .
cat > ~/.terraformrc <<EOF
provider_installation {
dev_overrides { "coder/coderd" = "$(go env GOPATH)/bin" }
direct {}
}
EOFprovider branch: coder-eng-3083-oauth-dcr-flag @ e7f2ca0
provider.go:239 NewOAuth2ProviderSettingsResource,
provider.go:249 NewOAuth2ProviderSettingsDataSource,
terraform v1.15.7 on darwin_arm64
Test users for the authorization cases, created over the HTTP API so the owner's CLI session isn't clobbered:
curl -X POST "$BASE_URL/api/v2/users" ... # tf-member
curl -X POST "$BASE_URL/api/v2/users" ... # tf-auditor
curl -X PUT "$BASE_URL/api/v2/users/$AUDITOR_ID/roles" -d '{"roles": ["auditor"]}'tf-member roles: (none)
tf-auditor roles: auditor
auditor GET 200
auditor PUT 403
member GET 403
member PUT 403
This last block was the riskiest assumption in the plan — if the auditor's GET had returned 403, TC23/TC24 would have been resting on a false premise. It holds, so the auditor is a genuine read-yes/write-no principal.
✅ Pass — endpoint present on a freshly reset DB, provider built and overridden, and coderd's RBAC behaves as TC23/TC24 assume.
1. Not declaring the resource touches nothing (TC1, TC20, TC22)
Verifies the backward-compatibility claim: shipping a new resource type doesn't retroactively affect existing configs. Starts from a live value of true so an accidental write would be visible — starting from the false default would hide it.
sequenceDiagram
participant U as Owner
participant TF as terraform
participant S as coderd
U->>S: PUT settings {enabled: true} (out of band)
S-->>U: 200 OK
Note over TF: config declares only terraform_data,<br/>no coderd_oauth2_provider_settings block
U->>TF: terraform apply
TF-->>U: 1 added
Note over TF,S: zero requests to coderd of any kind
U->>S: GET settings
S-->>U: 200 OK, still true
Commands and output
curl -X PUT "$BASE_URL/api/v2/oauth2-provider/settings" -d '{"dynamic_client_registration_enabled": true}'
cat > main.tf <<'EOF'
resource "terraform_data" "unrelated" { input = "before" }
EOF
TF_LOG=DEBUG terraform apply -auto-approvePlan: 1 to add, 0 to change, 0 to destroy.
Apply complete! Resources: 1 added, 0 changed, 0 destroyed.
settings requests: 0
Counting every coderd request, not just settings ones:
mentions of the coderd binary: 6 (plugin discovery / override resolution)
ConfigureProvider events: 0
requests to /api/v2: 0 <- not even users/me or entitlements
TC20 — mutate the unrelated resource ("before" → "after"):
Plan: 0 to add, 1 to change, 0 to destroy.
Apply complete! Resources: 0 added, 1 changed, 0 destroyed.
settings requests: 0
mentions of the resource type: 0
requests to coderd at all (api/v2): 0
TC22 — the live value:
{"dynamic_client_registration_enabled": true}✅ Pass, and the guarantee is stronger than "no settings requests". Configure() calls client.User(ctx, Me) and client.Entitlements(ctx), so those two requests are the fingerprint of the provider being configured. Zero of them means Terraform pruned the coderd provider node entirely — despite provider.tf declaring a full config block with a valid token — because no resource in the graph belonged to it. The blast radius isn't limited by careful coding in Create/Read; it's limited by Terraform's graph.
2. First create against a never-configured deployment (TC2)
Verifies Create() and the shared patch() helper.
Commands and output
curl -X PUT ... -d '{"dynamic_client_registration_enabled": false}'
cat > main.tf <<'EOF'
resource "coderd_oauth2_provider_settings" "dcr" {
dynamic_client_registration_enabled = true
}
EOF
terraform apply -auto-approve # coderd_oauth2_provider_settings.dcr will be created
+ resource "coderd_oauth2_provider_settings" "dcr" {
+ dynamic_client_registration_enabled = true
}
Plan: 1 to add, 0 to change, 1 to destroy.
Apply complete! Resources: 1 added, 0 changed, 1 destroyed.
"Overwriting an out-of-band value" occurrences: 0
method=PUT tf_rpc=ApplyResourceChange url=.../api/v2/oauth2-provider/settings
live: {"dynamic_client_registration_enabled":true}
state: {"dynamic_client_registration_enabled":true}
(1 to destroy is §1's leftover terraform_data, not a defect.)
✅ Pass — and the request pattern was predicted from the source before running. ModifyPlan bails at if data.DynamicClientRegistrationEnabled.ValueBool() { return } before calling OAuth2ProviderSettings, so a planned true should produce no plan-time request. tf_rpc=ApplyResourceChange on the only request confirms zero requests during PlanResourceChange. A GET there would mean the early-return ladder is ordered wrong even though the visible output looked identical.
Also worth recording: the provider links codersdk v2.34.0-rc.0.0.20260727150003-569a0eb34112 — a pseudo-version pinned to the same commit the dev server runs. Provider and server are in lockstep, so no fake-vs-real skew from SDK drift is possible.
3. Declared but the required attribute omitted (TC4)
Commands and output
cat > main.tf <<'EOF'
resource "coderd_oauth2_provider_settings" "dcr" {
}
EOF
TF_LOG=DEBUG terraform planError: Missing required argument
on main.tf line 1, in resource "coderd_oauth2_provider_settings" "dcr":
1: resource "coderd_oauth2_provider_settings" "dcr" {
The argument "dynamic_client_registration_enabled" is required, but no
definition was found.
settings requests: 0
any api/v2 request: 0
[DEBUG] provider: using plugin: version=6 <- plugin WAS launched
[ERROR] vertex "coderd_oauth2_provider_settings.dcr" error: Missing required argument
✅ Pass — Terraform launched the plugin (it needs the schema), but the rejection is a graph-vertex evaluation error, and 0 requests to /api/v2 proves Configure() never ran. This is what makes Required the right choice for a singleton whose only attribute is the whole payload: an empty block is structurally inexpressible, the error names file and line, and it costs zero network calls. With Optional, resource "..." "dcr" {} would be valid config that means nothing, needing a runtime check to reject at apply time what core rejects here for free.
4. Drift detection (TC5)
Commands and output
coder oauth2-provider dcr disable # out of band, as an admin would
terraform planDynamic client registration is now disabled.
live: true -> false
coderd_oauth2_provider_settings.dcr: Refreshing state...
~ dynamic_client_registration_enabled = false -> true
Plan: 0 to add, 1 to change, 0 to destroy.
live value after the plan: false (plan made no write)
"Overwriting an out-of-band value": 0
✅ Pass. Refreshing state... confirms Read() ran rather than the diff coming from stale state, and the live value is untouched afterward — a refresh that "fixed" drift by writing would make TC5 vacuous. No warning, correctly: prior state is non-null, so ModifyPlan returns at the !req.State.Raw.IsNull() guard.
Two environment notes for anyone reproducing this:
./scripts/coder-dev.shdoes not run on stock macOS (coder's scripts need bash ≥ 4.0; macOS ships 3.2). Either prepend brew's bash, or callbuild/coder_darwin_arm64directly — it links to the binarydevelop.shjust built, so it's the same CLI at the same commit as the server.terraform showafter a bareplanreports the stored value, not the refreshed one.planrefreshes in memory only; onlyapplypersists. Asserting onshowhere would make correct drift detection look broken.
5. No diff when the config already matches (TC6)
The negative control for §4 — if a matching config still showed a diff, drift detection would be noise rather than signal.
Commands and output
terraform apply -auto-approve # reconverge
terraform plan -detailed-exitcode ~ dynamic_client_registration_enabled = false -> true
Apply complete! Resources: 0 added, 1 changed, 0 destroyed.
live: true state: true
No changes. Your infrastructure matches the configuration.
exit code: 0
Requests attributed by endpoint:
during the apply: GET tf_rpc=ReadResource (refresh)
PUT tf_rpc=ApplyResourceChange (Update -> patch)
during the plan: GET tf_rpc=ReadResource (1 request, status=200, no PUT)
✅ Pass — -detailed-exitcode returned 0; 2 would have meant a lingering diff. Note Update() doesn't re-GET after writing (it sets state from the plan), which is why the apply shows one PUT rather than a PUT plus a confirming read.
Incidental finding: Configure() ran four times during one apply (users/me ×4, entitlements ×4). Terraform configures a provider once per graph walk, and the entitlement cache lives on the CoderdProviderData instance — so "entitlements are fetched once" is true per walk, not per apply.
6. Update is in-place, not a replacement (TC7)
Commands and output
~ update in-place
# coderd_oauth2_provider_settings.dcr will be updated in-place
~ dynamic_client_registration_enabled = true -> false
Plan: 0 to add, 1 to change, 0 to destroy.
must be replaced: 0
forces replacement: 0
-/+: 0
destroy and then create: 0
coderd_oauth2_provider_settings.dcr: Modifying...
coderd_oauth2_provider_settings.dcr: Modifications complete after 0s
Apply complete! Resources: 0 added, 1 changed, 0 destroyed.
live: false state: false
✅ Pass — all four replacement indicators checked explicitly rather than inferred from the presence of "in-place", and the apply confirms with Modifying…, no Destroying…/Creating….
Subtlety worth stating: because Delete() resets to the false default, a true → false replace would produce the same final value as an in-place update — invisible in the end state. The danger is the other direction: a false → true replace would destroy (resetting to false) then create, leaving DCR disabled in between, matching neither the old nor the new config. So the assertion has to be on the plan's verb, not the resulting value.
Second asymmetry confirmation, and this one has teeth: the planned value here is false — the direction that triggers §8's warning — yet no warning fired, because this is an update and an update renders the change as a visible diff.
7. Destroy resets to the documented default (TC8)
Commands and output
live before destroy: {"dynamic_client_registration_enabled":true}
# coderd_oauth2_provider_settings.dcr will be destroyed
- dynamic_client_registration_enabled = true -> null
Destroy complete! Resources: 1 destroyed.
method=GET tf_rpc=ReadResource <- pre-destroy refresh
method=PUT tf_rpc=ApplyResourceChange <- Delete() resetting to the default
DELETE requests to the endpoint: 0
live after destroy: {"dynamic_client_registration_enabled":false}
state list: []
✅ Pass — Delete() issued a PUT, never a DELETE. Not a shortcut: the API exposes no delete verb, so "unmanage it" can only be expressed as "write the documented default". The consequence is worth being explicit about — a destroy does not return the deployment to a never-configured state; it leaves a site_configs row holding false. Indistinguishable through the API (the server coalesces both), but not the same thing, which is why TC2's true sql.ErrNoRows branch can only be exercised once per --db-reset.
This also exercises the third ModifyPlan guard. All three now observed holding:
| guard | exercised by | outcome |
|---|---|---|
req.Plan.Raw.IsNull() (destroy) |
§7 | silent |
!req.State.Raw.IsNull() (update) |
§4 enable, §6 disable | silent |
planned value is true (enabling) |
§2 | silent, and no GET |
Which leaves exactly one path that should warn — a create that disables — as §8's subject.
8. The adoption footgun, and the plan-time warning (TC3)
Verifies the one case where this resource can surprise you, plus the ModifyPlan advisory added in this PR (not in the original proposal).
sequenceDiagram
participant U as Owner
participant TF as terraform
participant P as provider
participant S as coderd
Note over S: live = true, set out of band
Note over TF: no state entry at all
U->>TF: terraform plan (config says false)
TF->>P: ModifyPlan
Note over P: req.State.Raw.IsNull() -> a create,<br/>and planned value is false -> check the live value
P->>S: GET settings
S-->>P: true
P-->>TF: Warning: Overwriting an out-of-band value
TF-->>U: + 1 to add (no diff shown for the value)
Commands and output
rm -f terraform.tfstate terraform.tfstate.backup
coder oauth2-provider dcr enable
# main.tf says false
terraform plan # coderd_oauth2_provider_settings.dcr will be created
+ dynamic_client_registration_enabled = false
Plan: 1 to add, 0 to change, 0 to destroy.
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.
Applying anyway:
Creation complete after 0s
Warning: Overwriting an out-of-band value
Apply complete! Resources: 1 added, 0 changed, 0 destroyed. exit: 0
live after apply: false state: false
✅ Pass on both halves — the footgun is real (a create with 1 to add and nothing hinting the live value was true), and the warning fires without blocking.
The advisory's cost is now measured, not assumed:
§2 (planned true): method=PUT tf_rpc=ApplyResourceChange <- no plan-time request
§8 (planned false): method=GET tf_rpc=PlanResourceChange <- the advisory's read
ReadResource calls in this run: 0
ReadResource: 0 is what makes it airtight — there's no state, so nothing to refresh, so the GET cannot be attributed to Read(). Zero extra requests when enabling, one GET when disabling on a create. A naive "always GET and compare" would add a plan-time round trip to every plan of every deployment to produce a warning useful in one direction.
Two details:
- The warning anchors to
main.tf line 2— the attribute, not the block, fromAddAttributeWarningagainstpathDynamicClientRegistrationEnabled. - It appears 3 times in the apply log (apply re-plans internally, so
ModifyPlanruns on multiple walks). Terraform de-duplicates for display, so any "warned exactly once" assertion would fail against the raw log. That it survives into apply output is what makes it a real safety net: the dangerous workflow isapply -auto-approvein CI, where nobody reads a plan.
9. Import adopts the live value safely (TC21)
The direct answer to §8.
Commands and output
rm -f terraform.tfstate terraform.tfstate.backup
coder oauth2-provider dcr enable
terraform import coderd_oauth2_provider_settings.dcr oauth2_provider_settingsImport prepared! / Import successful!
PUT requests to the settings endpoint: 0
all requests: method=GET tf_rpc=ReadResource
state: true live: true config: = false
~ 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
✅ Pass — one request, a GET, zero PUTs. The GET is tagged ReadResource, not ImportResourceState: ImportState() issues no request at all, it only declares the resource identity, and Terraform then calls Read(). That's exactly why this singleton needs no parsable import ID — there's nothing to extract from req.ID, so oauth2_provider_settings is a pure placeholder.
Set against §8 from identical starting conditions (live true, config false, no state):
| §8: apply directly | §9: 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 |
0 requests at PlanResourceChange is the sharp confirmation that ModifyPlan exited at the state guard — it didn't merely decline to warn, it never reached the GET. Detecting a create with req.State.Raw.IsNull() rather than a heuristic like "does state hold a value?" is what makes that true; the alternative would warn on every post-import plan, training admins to ignore the one warning that matters.
10. The data source reads without owning (TC16, TC19)
Commands and output
cat > main.tf <<'EOF'
data "coderd_oauth2_provider_settings" "current" {}
output "dcr_enabled" {
value = data.coderd_oauth2_provider_settings.current.dynamic_client_registration_enabled
}
EOF
terraform apply -auto-approve # twice
coder oauth2-provider dcr disable # TC19
terraform apply -auto-approve# first apply
Changes to Outputs: + dcr_enabled = true
Apply complete! Resources: 0 added, 0 changed, 0 destroyed.
PUT requests: 0 all settings requests: method=GET tf_rpc=ReadDataSource
# repeat apply
Apply complete! Resources: 0 added, 0 changed, 0 destroyed.
PUT requests: 0 live still true
# TC19, after an out-of-band disable
terraform output: false PUT requests: 0
{"mode":"data","type":"coderd_oauth2_provider_settings",
"values":{"dynamic_client_registration_enabled":false}}
✅ Pass — zero PUTs on both applies, and 0 added, 0 changed, 0 destroyed while still producing an output. The repeat apply carries the weight: a data source that wrote even once would break the coexistence guarantee, and the plausible failure mode isn't the first read but a later apply "reconciling" its recorded value.
Two structural details: the request is tagged ReadDataSource, distinct from the resource's ReadResource even though both call the same Client.OAuth2ProviderSettings; and "mode":"data" in state is what makes TC16's claim structural rather than behavioural — no lifecycle, no destroy, nothing for a second configuration to contend over. There is no code path from a data source to a PUT.
11. Authorization (TC13, TC17, TC23, TC24) and a retryable failed destroy (TC9)
sequenceDiagram
participant M as Member token
participant A as Auditor token
participant S as coderd
M->>S: GET/PUT settings
S-->>M: 403 both ways (TC13, TC17)
A->>S: GET settings
S-->>A: 200 OK (TC24: the data source works)
A->>S: PUT settings
S-->>A: 403 (TC23: the resource cannot be owned)
Commands and output
TC17 — data source, member token:
Error: Client Error
with data.coderd_oauth2_provider_settings.current,
unable to read OAuth2 provider settings, got error: GET
http://localhost:3000/api/v2/oauth2-provider/settings: unexpected status code
403: Forbidden.
[ERROR] vertex "data.coderd_oauth2_provider_settings.current" error: Client Error
Planning encountered errors, so plan is not applyable
TC13 — resource, member token:
unable to update OAuth2 provider settings, got error: PUT ... 403: Forbidden.
requests: method=PUT tf_rpc=ApplyResourceChange
state entries: []
TC24 — data source, auditor token:
Apply complete! Resources: 0 added, 0 changed, 0 destroyed.
terraform output: false PUT requests: 0
TC23 — same auditor token, both ways:
Import successful! PUT requests during import: 0
state value: {"dynamic_client_registration_enabled":false}
# then apply
unable to update OAuth2 provider settings, got error: PUT ... 403: Forbidden.
state survived the failed apply: [coderd_oauth2_provider_settings.dcr]
TC9 (§11a) — destroy with the auditor token, then retry as owner:
Error: unable to update OAuth2 provider settings ... 403: Forbidden.
state list: [coderd_oauth2_provider_settings.dcr]
live value (untouched): false
# live set to true first so the reset is observable, then owner token restored
retry exit: 0
Destroy complete! Resources: 1 destroyed.
state list: []
live value: {"dynamic_client_registration_enabled":false}
✅ Pass on all five.
- Checked before trusting TC17: a plain member can read
/api/v2/entitlements(200) and/users/me(200), soConfigure()succeeds and the 403 is isolated to the settings endpoint. Had entitlements been forbidden, this step would have failed during provider configuration and tested nothing. - TC13's empty state is the important half — a failed create left no entry, so there's no phantom resource for a later plan to update or destroy.
- TC23 earns its place. Under the member token every operation fails identically, so nothing is learned about where the boundary sits — the provider could be refusing client-side and the output would look the same. With the auditor, one token in one working directory yields
Import successful!and a hard403on apply. That asymmetry can only come from the server. - TC9's state retention is the subtle safety property. Had the provider removed the state entry despite the failed reset, the admin would believe DCR was reset while the deployment kept the last-applied value — and no future
destroywould ever retry, because Terraform would no longer know the resource exists.
One deviation, deliberate: TC9's final assertion is weak as written, because after the auditor's failed destroy the live value is already false, so a successful retry writes false over false and passes even if Delete() did nothing. I set live to true first to make the reset observable.
Nit found: the diagnostic says "unable to update" for create, update, and destroy failures, since patch() is shared and passes a fixed action string. Defensible (the wire operation really is a PUT), but it doesn't tell an admin which Terraform operation failed — the resource address and PUT verb are what disambiguate.
12a. Provider version predates the feature (TC10) — the manual-only case
Commands and output
mv ~/.terraformrc ~/.terraformrc.disabled # disable the dev override
# provider.tf pins version = "0.0.22"
rm -rf .terraform .terraform.lock.hcl
terraform init && terraform plan# .terraform.lock.hcl — proof of which binary ran
provider "registry.terraform.io/coder/coderd" {
version = "0.0.22"
constraints = "0.0.22"Error: Invalid resource type
on main.tf line 1, in resource "coderd_oauth2_provider_settings" "dcr":
1: resource "coderd_oauth2_provider_settings" "dcr" {
The provider coder/coderd does not support resource type
"coderd_oauth2_provider_settings".
settings requests: 0
api/v2 requests: 0
dev-override warnings: 0
Restoring the override, with the identical main.tf:
exit: 0
# coderd_oauth2_provider_settings.dcr will be created
Plan: 1 to add, 0 to change, 0 to destroy.
✅ Pass. Three independent confirmations this is core rejecting the config against the published schema: dev-override warnings: 0 (so it really was 0.0.22, not our local build), api/v2 requests: 0 (so Configure() never ran and no deployment was needed), and the error names the resource type rather than an attribute or API response.
The restore step doubles as a clean A/B control: same main.tf, byte-for-byte, went from Invalid resource type to Plan: 1 to add with only the provider binary swapped. It also clarifies the failure's shape — fixed by bumping required_providers, and not fixable by upgrading Coder, the exact opposite of TC11.
12b. New provider, old coderd (TC11, TC18)
Verifies oauth2ProviderSettingsDiag: a 404 becomes an actionable "Unsupported Coder Version" naming the minimum release, not a decode failure and not a phantom "resource deleted" diff.
Commands and output
# Terminal 1
git checkout origin/main --detach # HEAD is now at daf655dff8
./scripts/develop.sh --db-resetHEAD: daf655dff8 (DETACHED) == origin/main
version: v2.35.3-devel+daf655dff8
GET settings: 404 PUT settings: 404
GET body: {"message":"Route not found."}
whoami: admin roles: owner <- token valid, so the 404 is not an auth artifact
TC11, create path (no prior state → Create() → PUT):
Error: Unsupported Coder Version
with coderd_oauth2_provider_settings.dcr,
on main.tf line 1, in resource "coderd_oauth2_provider_settings" "dcr":
Unable to update OAuth2 provider settings: the deployment returned 404 for
/api/v2/oauth2-provider/settings. This endpoint requires Coder version 2.35.0
or later; upgrade the deployment, or remove `coderd_oauth2_provider_settings`
from your configuration. Original error: PUT ...
unexpected end of JSON input: 0
cannot unmarshal: 0
invalid character: 0
state after failed apply: []
TC11, refresh path (prior state present → Read() → GET). The state file was produced by the provider itself against the endpoint-capable server and replayed here — it carries no server identity, only {"dynamic_client_registration_enabled": true}, so it's byte-for-byte the state an admin holds after pointing an existing config at an older deployment:
state before: {"dynamic_client_registration_enabled":true}
Error: Unsupported Coder Version
Unable to read OAuth2 provider settings: the deployment returned 404 ...
Original error: GET ...
raised at: tf_rpc=ReadResource <- the refresh path, not apply
will be created: 0
must be replaced: 0
has been deleted: 0
Objects have changed outside: 0
will be destroyed: 0
1 to add: 0
state after: [coderd_oauth2_provider_settings.dcr] = true <- survived intact
TC18, data source:
Error: Unsupported Coder Version
with data.coderd_oauth2_provider_settings.current,
Unable to read OAuth2 provider settings: the deployment returned 404 ...
raised at: tf_rpc=ReadDataSource
OAuth2 Experiment Not Enabled: 0
CODER_EXPERIMENTS mentions: 0
✅ Pass on all three, with zero decode failures anywhere and 2.35.0 named in every message.
The six zeros on the refresh path are the assertion that earns Read()'s comment. A 404 on refresh is the canonical signal for "the remote object is gone", and the reflexive implementation is resp.State.RemoveResource(ctx) — correct for most resources. Had Read() followed the reflex, this plan would have shown 1 to add: silently proposing to recreate the setting on a deployment that cannot host it, while discarding the state entry recording the real configuration. The create path alone can never verify this, since with no state there's nothing to phantom-delete.
// Deliberately not treated as "resource deleted": this setting is a
// deployment singleton that always exists on a supported deployment,
// so a 404 means the endpoint is missing, not the resource.Note the action word tracks the operation (Unable to **update** on create, Unable to **read** on refresh), so the shared helper is parameterized rather than hardcoding one verb. And Route not found. is chi's unmatched-route response — a different failure from the experiment gate's 403 on a current deployment. Both mean "this deployment can't do it", with opposite remedies, so keeping them distinguishable matters (asserted explicitly on the last two counters).
TC12 is not separately executable (needs a real cluster and the helm provider in the graph). Its documented outcome is that an unordered apply collapses into TC11, verified above. Mitigation is documented in examples/resources/coderd_oauth2_provider_settings/resource.tf as a tiered set: depends_on at minimum, a readiness-gate terraform_data polling /api/v2/buildinfo as the stronger pattern, two separate applies as most robust.
13. Concurrency and singleton semantics (TC14, TC15)
Commands and output
TC14 — saved plan file, external write in between:
converge: live = state = false
Plan: 0 to add, 1 to change, 0 to destroy. Saved the plan to: tfplan
other actor: PUT {"dynamic_client_registration_enabled":false}
apply tfplan → Apply complete! 0 added, 1 changed, 0 destroyed. exit 0, no Error, no Warning
live after: true
The measurement that actually demonstrates the claim:
apply tfplan → GETs: 0 PUTs: 1 <- blind, no re-read
apply (no saved plan) → GETs: 1 PUTs: 1 <- refreshes first
And with the plan's target already matching live at apply time:
apply tfplan → Apply complete! 0 added, 1 changed, 0 destroyed.
GETs: 0 PUTs: 1
TC15 — two blocks, same singleton:
live before: true
Apply complete! Resources: 2 added, 0 changed, 0 destroyed.
a: Creating... b: Creating... <- interleaved: applied in parallel
live after: false <- b won the race
state: a → true b → false
exit code: 2
# coderd_oauth2_provider_settings.a will be updated in-place
~ dynamic_client_registration_enabled = false -> true
Warning: Overwriting an out-of-band value
with coderd_oauth2_provider_settings.b,
on main.tf line 6, ...
✅ Pass on both, with TC14's assertion strengthened.
TC14 as written is vacuous, and this is worth flagging for anyone reusing the plan. The attribute is a boolean, and a diff exists only because live ≠ config — so the "other actor" can only write the value live already holds (a no-op, which is what the doc's -d '{"…": false}' does) or the value Terraform is about to write (agreement). Either way the end state is true, so live = true cannot distinguish "Terraform clobbered a concurrent change" from "nothing happened". A lost update is not constructible on a two-valued attribute in one plan/apply cycle.
What is verifiable: applying a saved plan issues zero GETs, so it cannot detect a concurrent change — no read to compare against, no If-Match, no version field. The refresh in the contrast line is exactly the step a saved plan skips. And the write is unconditional: Terraform still PUTs and still reports 1 changed against a deployment already holding the target. That reframes TC14 usefully — "last write wins" isn't a provider property at all but a property of the saved-plan workflow plus an API with no concurrency primitives, and the provider couldn't fix it if it wanted to.
TC15's durable damage is in state, not on the server. Both entries recorded their own value, so state claims DCR is simultaneously true (per a) and false (per b). Reality can only match one, so every future plan shows the loser needing an update — and applying it makes the other block the loser. The two flap forever, one diff per apply, with no error ever raised. Terraform also applied them in parallel, so the winner is a genuine race, not a function of declaration order; b won this run and that isn't guaranteed to reproduce.
Unplanned bonus: the plan-time warning fires here, and only for b (which plans false while live is true). a plans true and correctly stays silent. Not designed for this case, but it means the only plan-time signal that a double declaration is broken comes from the warning this PR adds — Terraform itself reports a perfectly ordinary 2 to add.
14. Cleanup
Commands and output
/tmp/tf-dcr-manual removed
~/.terraformrc removed
~/.terraformrc.disabled none (§12a's restore was clean)
$ terraform providers # normal Terraform work unaffected
└── provider[registry.terraform.io/hashicorp/random]
old member token → 401 # dead, DB wiped by §12b
✅ Pass, local steps only — the server-side steps were skipped deliberately, not forgotten. By the time cleanup ran, the deployment was origin/main with a reset DB, so the reset PUT would 404 and tf-member/tf-auditor no longer existed (verified: their tokens return 401). ~/.terraformrc is the one that mattered — it's global, so leaving it would silently redirect coder/coderd to a locally built binary in every unrelated Terraform project on the machine.
Finding: the oauth2 experiment gate — fixed in abcb900
Nothing above tested whether the DCR flag can be changed when the OAuth2 experiment is off, and it turns out that path was unreachable from scripts/develop.sh entirely.
coderd gates the whole route (coderd/coderd.go):
r.Route("/oauth2-provider", func(r chi.Router) {
r.Use(
apiKeyMiddleware,
httpmw.RequireExperimentWithDevBypass(api.Experiments, codersdk.ExperimentOAuth2),
)With the experiment off, RequireExperiment answers 403 before any handler runs, so the setting can be neither read nor changed. That's the correct behaviour and it's enforced server-side. Why every step above sailed past it — two reasons stacked:
CODER_EXPERIMENTS=oauth2 # in the dev server's environment
/api/v2/experiments → ["oauth2"]
version: v2.35.3-devel+569a0eb341 # -devel ⇒ buildinfo.IsDev() is true
The experiment was enabled and the build is -devel, so RequireExperimentWithDevBypass returns at buildinfo.IsDev() without consulting the experiment list. Even with CODER_EXPERIMENTS= empty, a -devel build keeps answering 200 — no configuration of the dev server can produce the experiment-off 403. Only a fake or a real release binary can, which is why the new coverage is unit tests rather than another manual section: a manual step here would be a step that cannot fail.
Severity — not a correctness bug, but a real UX gap:
- The apply fails, so no bad state is written.
- Not a misdiagnosis: the gate is middleware on a registered route, so it's
403, never404. An admin on a current Coder with the experiment off is not wrongly told to upgrade Coder. - But it was reported as a bare
Client Error, indistinguishable at a glance from the RBAC denials in §11, and the docs never mentioned the requirement.
And it isn't an edge case: the experiment is off by default, and ExperimentsSafe lists only ExperimentMinimumImplicitMember, so --experiments='*' does not enable it. A stock release deployment returns 403 — likely the most common first encounter with this resource.
Fix (abcb900): 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. Mirrors isWorkspaceSharingExperimentOff in organization_resource.go, which solves the same problem for the workspace-sharing experiment. The requirement is now on both schemas and in both examples.
Discriminating on message text is unavoidable (coderd reuses 403 for both) but degrades safely: a reworded upstream message falls back to the generic Client Error this branch replaced. A 404 still takes the version branch — asserted in TC18 above.
Corrections to the verification plan
Five defects in my own plan surfaced while running it. Two produced assertions that could not fail, which is the failure mode worth naming: output that looks like a pass.
| # | Defect | Consequence if unnoticed |
|---|---|---|
| 1 | The scratch provider.tf omitted required_providers |
Terraform resolved hashicorp/coderd, not coder/coderd, so the dev override never matched. §1 printed settings requests: 0 while nothing had run at all. |
| 2 | grep -c 'PUT /api/v2/…' never matches this log format |
The SDK logs method=PUT … url=http://…; verb and path are never adjacent. §9's and §10's central "never writes" claim would print 0 even after a hundred PUTs. |
| 3 | TC14's assertion is vacuous on a boolean | See §13 — "last write wins" was unprovable as written. |
| 4 | ./scripts/coder-dev.sh needs bash ≥ 4.0 (macOS ships 3.2) |
§4, §8, §9, §10 all abort before doing anything. |
| 5 | Cleanup runs after §12b has destroyed its own targets | Server-side cleanup silently no-ops. |
Defect 2 was caught only by running the doc's pattern against a log known to contain a PUT:
doc pattern 'PUT /api/v2/oauth2-provider/settings' : 0 <- false negative
correct method=PUT + settings url : 1 <- the real PUT
The through-line: every one of these produced output that looked correct. settings requests: 0, PUT requests: 0, live value = true — all technically true, none meaning what they appeared to mean. What caught them was checking the mechanism rather than the outcome: reading tf_rpc= tags to see which RPC made each request, confirming ReadResource: 0 so a GET couldn't be misattributed, and testing every negative assertion against a positive control. For a plan whose assertions are mostly negative — "no request", "no warning", "no write" — a broken test and a real pass are indistinguishable from the output alone.
Also worth recording, from §12b: verify the deployment by HTTP status, not by branch name. The first restart landed on coder-eng-3062-dcr-flag-ui, a plausible-sounding DCR branch that contains the endpoint (30 commits diverged from origin/main, GET → 200). TC11 against it would have succeeded, producing a baffling "failure" for a test whose premise was simply false. The tells were the 200 and HEAD sitting on a named branch rather than detached.
Automated equivalents
Every case above also has an automated test that's passing, except TC10 and TC12 as noted.
| Manual step | Automated test |
|---|---|
| TC1, TC20, TC22 | …NotDeclared/TC1_TC20_NotDeclared, …NotDeclared/TC22_NotDeclaredLiveTrue |
| TC2 | …Resource/TC2_CreateAgainstNeverConfigured |
| TC3 | …Resource/TC3_CreateOverwritesOutOfBandValue |
| TC4 | …Resource/TC4_RequiredAttributeOmitted |
| TC5, TC6 | …Resource/TC5_DriftDetection, …Resource/TC6_NoDiffWhenDefaultMatches |
| TC7, TC8 | …Resource/TC7_UpdateInPlace, …Resource/TC8_DestroyResetsToDefault |
| TC9 | …Resource/TC9_DestroyFailsCleanly |
| TC10 | none — not testable in-process |
| TC11, TC18 | …Resource/TC11_OldCoderd, …DataSource/TC18_OldCoderd |
| TC12 | none — collapses into TC11 |
| TC13, TC17 | …Resource/TC13_Forbidden, …DataSource/TC17_Forbidden |
| TC14, TC15 | …Resource/TC14_ConcurrentExternalMutationLastWriteWins, …Resource/TC15_TwoBlocksSameSingleton |
| TC16, TC19 | …DataSource/TC16_ReadOnlyNeverPuts, …DataSource/TC19_FreshDeploymentDefaultsFalse |
| TC21 | …Resource/TC21_ImportAdoptsLiveValue |
| TC23, TC24 | …Resource/TC23_ReadOkWriteForbidden, …DataSource/TC24_ReadableByTokenThatCannotWrite |
| Plan-time warning | TestOAuth2ProviderSettingsModifyPlan (8 cases) |
oauth2 experiment |
TestIsOAuth2ExperimentOff (7 cases), TestOAuth2ProviderSettingsExperimentDiag, …Resource/ExperimentDisabled, …DataSource/ExperimentDisabled |
TF_ACC=1 go test ./internal/provider \
-run '^Test(Acc)?(OAuth2ProviderSettings|IsOAuth2ExperimentOff|DCREnabledOrDefault)' -count=1ok github.com/coder/terraform-provider-coderd/internal/provider 9.2s
41 subtests passed, 0 failures
| // It does still require the deployment's `oauth2` experiment to be enabled: the | ||
| // experiment gates the entire /api/v2/oauth2-provider route, so a read-only | ||
| // token does not get you around it. | ||
| data "coderd_oauth2_provider_settings" "current" {} |
There was a problem hiding this comment.
Is there a race condition here?
If resource "coderd_oauth2_provider_settings" "dcr" is also set to change these values?
There was a problem hiding this comment.
Confirmed: without depends_on, the data source reads at plan time, so this count uses the stale pre-apply value and lags one apply behind the resource. depends_on fixes ordering but makes dependents "(known after apply)".
Suggest removing the data source from this PR entirely — the standalone use case is thin, and it can return later with a hard "never declare alongside the resource" rule.
Coder Agents on behalf of @Emyrk.
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.
| // reuses 403 for both — but it degrades safely: a reworded message falls through | ||
| // to the generic "Client Error" that this branch replaced, which still surfaces | ||
| // coderd's own text. | ||
| func isOAuth2ExperimentOff(err error) bool { |
There was a problem hiding this comment.
Suggest dropping this classifier (and the fake's verbatim message copy): coderd's own 403 message already names the experiment, so passing it through is just as actionable with no text-matching to rot. If the richer remedy text matters, file a coder/coder issue for a machine-readable error code instead.
Coder Agents on behalf of @Emyrk.
| // update but wrong for this resource: it owns the value outright, so | ||
| // omitting it would make Create/Update no-ops and, worse, turn Delete's | ||
| // reset-to-default into a silent no-op. | ||
| _, err := r.Client.PutOAuth2ProviderSettings(ctx, codersdk.OAuth2ProviderSettings{ |
There was a problem hiding this comment.
Nit: patch() discards the PUT response; set state from the returned settings instead of the plan value. Equivalent today, stays honest if the API ever normalizes the value.
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.
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, plus a read-only data source of the same name. Both are 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. The data source exists because this setting is a deployment-wide singleton — only one configuration can own it — and because it only issues aGET, it works with tokens that can read but not write the deployment config.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 (9) — 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 used by both the resource and the data source.internal/provider/provider.go3. The data source
internal/provider/oauth2_provider_settings_data_source.goComputedbool,GETonly, neverPUT— the invariant that makes it safe to declare alongside a resource owned by a different configuration.4. 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(14 subtests) andTestAccOAuth2ProviderSettingsNotDeclared(2 subtests).internal/provider/oauth2_provider_settings_data_source_test.goTestAccOAuth2ProviderSettingsDataSource, 5 subtests.5. Examples (
docs/is generated from these)examples/resources/coderd_oauth2_provider_settings/resource.tfexamples/resources/coderd_oauth2_provider_settings/import.shimport {}block forms.examples/data-sources/coderd_oauth2_provider_settings/data-source.tfcountgated on the value — rather than a bare attribute reference.Generated files (2) — from
make gen, no need to review directlydocs/resources/oauth2_provider_settings.md,docs/data-sources/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. The data source
Largely a mirror of the resource's
Read. The thing to check is the absence of any write path.4. 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, the data source 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.5. 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.