Repository navigation
Commit fcb40ad
fix(service-settings): Google login saves ON when its credentials come from the server (#24874)
Fixes #24844
Clause-②: no
## What changes
`auth.google_client_id` and `auth.google_client_secret` in
`packages/services/service-settings/src/manifests/auth.manifest.ts` are
now `required: false` (they were `required: true`). Nothing else moves:
no default, `visible` rule, option, label, help text or translation
bundle. The `google_enabled` help already states the conjunction (Google
is offered once a client ID and a client secret both come through, saved
below or set as `GOOGLE_CLIENT_ID` / `GOOGLE_CLIENT_SECRET` on the
server, which win over saved values). It still reads true, so it is
unchanged. A code comment above the toggle now says why the two keys are
not `required`.
One changeset: `@objectstack/service-settings` patch.
## Why
- `SettingsService.setMany` judges `required` against the STORED value.
It reads only `OS_*` env names (`envKeyOf`), so the `GOOGLE_*` pair is
invisible to it.
- plugin-auth offers Google when the toggle is on and both credentials
are available, saved or from the env pair, with the env pair winning
(`applyEnvSocialProviderFallbacks` at boot, the Google block of
`bindAuthSettings`). It needs the pair to be available. It never needs
it stored.
- So on an env-credential deployment, the declaration refused a save
that the runtime would honour. `setMany('auth', { google_enabled: true
})` answered `SETTINGS_VALIDATION` with `google_client_id/required` and
`google_client_secret/required`. The TOUCH gate judges both, because
their `visible` reads `google_enabled`. An admin who switched Google off
on the page could not switch it back on there.
Route C (the resolver reports the env pair as the source) needs
`settings-service.ts` or a spec-level env-name declaration, and both are
outside this card. See the decision record below.
## The offer matrix, before and after
This was measured with a one-off probe (not a repo file). It uses a real
`SettingsService`, the real `auth` manifest from
`builtinSettingsManifests`, a real `AuthPlugin` booted with that service
as `settings`, and `AuthManager.getPublicConfig().socialProviders`. The
credential values are fake. **Before** is the base build at
`df8bb40a03`. **After** is this branch's build at `6c1589e41c`.
"Offered" is the providers the login page lists; the source in brackets
is whose client ID the registered provider carries.
| deployment | admin action | before: save | before: offered | after:
save | after: offered |
|---|---|---|---|---|---|
| env pair | none (toggle unset) | n/a | google (env) | n/a | google
(env) |
| env pair | save ON | **REFUSED** (`required` x2) | google (env),
toggle stays unset | saved | google (env) |
| env pair | save OFF | saved | none | saved | none |
| env pair | save OFF, then ON | ON **REFUSED** | **none (stuck off)** |
saved | google (env) |
| stored pair | none | n/a | google (stored) | n/a | google (stored) |
| stored pair | save ON | saved | google (stored) | saved | google
(stored) |
| stored pair | save OFF | saved | none | saved | none |
| stored pair | save OFF, then ON | saved | google (stored) | saved |
google (stored) |
| env + stored | none | n/a | google (env) | n/a | google (env) |
| env + stored | save ON | saved | google (env) | saved | google (env) |
| env + stored | save OFF | saved | none | saved | none |
| env + stored | save OFF, then ON | saved | google (env) | saved |
google (env) |
| no credentials | none | n/a | none | n/a | none |
| no credentials | save ON | **REFUSED** (`required` x2) | none | saved
| none |
| no credentials | save OFF | saved | none | saved | none |
| no credentials | save OFF, then ON | ON **REFUSED** | none | saved |
none |
Four more rows check what a cleared credential does. Their "before"
column comes from an emulated base: this branch's build with `required:
true` put back on the two keys in memory, which is the only behavioural
difference in `service-settings`. The emulation's 16 shared rows were
checked against the real base build's reading above, and they match
exactly.
| deployment | admin action | before: save | before: offered | after:
save | after: offered |
|---|---|---|---|---|---|
| stored pair | client ID saved as an empty string | **REFUSED**
(`google_client_id/required`) | google (stored) | saved | none |
| stored pair | client ID reset (`null`) | saved | none | saved | none |
| env + stored | client ID saved as an empty string | **REFUSED** |
google (env) | saved | google (env) |
| env + stored | client ID reset (`null`) | saved | google (env) | saved
| google (env) |
**Reading.** For every stored state, the providers offered are the same
before and after. Only the saves that the base refused change. Three
states can now be saved:
- the toggle ON on an env deployment, which offers Google, the same as
the default;
- the toggle ON with nothing available, which offers no Google, the same
as the default;
- a credential stored as an empty string. plugin-auth reads that as
unset (`asTrimmedString`). The base already reached the same offered
state with a `null` reset, as the rows above show.
Who may sign in, and which providers the login page offers, do not
change for any deployment shape.
## Pins
`packages/services/service-settings/src/manifests/auth.manifest.test.ts`,
on a real `SettingsService` with the real `auth` manifest:
- **declares neither credential key required.**
- **saves Google ON with both credential fields empty, from unset and
after an OFF.** The service's env map carries fake `GOOGLE_CLIENT_ID` /
`GOOGLE_CLIENT_SECRET` values. `getNamespace` hands plugin-auth
`google_enabled: true` from the `global` source and both credentials
from `default`, so plugin-auth falls back to the env pair.
- **control: a stored credential pair still saves and reads back.**
The plugin-auth half, which maps stored or env values to offered
providers, is already pinned in
`packages/plugins/plugin-auth/src/auth-plugin.test.ts`:
- "enables Google from env credentials when google_enabled is explicit
true";
- "enables Google from settings-managed credentials";
- "keeps deployment Google credentials ahead of settings-managed
credentials";
- "disables env-backed Google when google_enabled is explicit false".
This PR does not touch plugin-auth.
## Ablation (one-off, no permanent file)
`required: true` was put back on both keys through
`scripts/ablation-replace.mjs`: the anchor hit once, and the blob
changed `146f1574445f` to `04ddcdc314d2`. Then `pnpm --filter
@objectstack/service-settings exec vitest run --maxWorkers=2
src/manifests/auth.manifest.test.ts` ran:
- FAIL "declares neither credential key required": `google_client_id:
expected true to be false`.
- FAIL "saves Google ON with both credential fields empty, from unset
and after an OFF": the promise rejected with `code:
'SETTINGS_VALIDATION'` and fields `google_client_id/required` and
`google_client_secret/required`.
- `Tests 2 failed | 16 passed (18)`. The stored-credential control is
among the passes.
The restore ran `git checkout HEAD -- PATH` on the absolute path.
Afterwards the blob was `146f1574445f`, equal to the HEAD blob; `git
diff HEAD` was 0 bytes; `git status --porcelain` showed 0 lines. The
file then ran 18 of 18 green. The test imports the manifest from source,
so no rebuild was needed for this ablation.
## Verification (head `6c1589e41c`)
- **Closure:** `pnpm --workspace-concurrency=2 --filter
'@objectstack/service-settings^...' --filter
'@objectstack/plugin-auth...' build`, exit 0. This adds plugin-auth and
service-settings themselves to the dispatch's closure, for the probe.
Both were rebuilt at the final head.
- **Tests:** `pnpm --filter @objectstack/service-settings test` gave
`Test Files 54 passed (54)` and `Tests 906 passed (906)`.
- **Typecheck:** `pnpm --filter @objectstack/service-settings
typecheck`, exit 0. `tsc --listFiles` includes
`src/manifests/auth.manifest.test.ts`.
- **Gates:** `node scripts/pm/dispatch-gates.mjs --commands --repo
objectstack-ai/objectstack` derived 66 commands. 65 exited 0. `pnpm
check:dual-build-cjs-loads` exited 3 (PREREQUISITE NOT MET): it reads
every package's `dist/` and needs a whole-tree build, so it was NOT
MEASURED locally; CI's `Lint & Repo Gates` runs it.
- **Reconciliation:** `--ran` printed `✓ dispatch-gates --ran: 66
derived famil(ies) accounted for — 65 run, 1 NOT-MEASURED (1 DERIVED
from a recorded exit 3)`.
## Decision record (options A / B / C from the card), on the four axes
- **Real business need.**
- The writer is the Setup → Authentication form (`PUT
/api/settings/auth`, which calls `setMany`). The reader is plugin-auth's
`bindAuthSettings`.
- `content/docs/deployment/environment-variables.mdx` documents the env
pair as a supported deployment shape.
- The refusal is measured above, including the stuck-off row.
- **B** and **C** both serve this need. **A** leaves the stuck-off
state. C's extra, showing "set via env" on the page, has no measured
pull.
- **Long-term soundness.**
- **B** makes the declaration say what the runtime needs: a credential
that is available, not one that is stored.
- **C** is also truthful. It teaches the resolver a non-`OS_` env name,
which is new resolver surface in the 18.x C8 territory.
- **A** keeps a declaration that is wrong on a documented deployment
shape.
- **Making AI mistakes harder.**
- **B** removes a constraint that is broader than the runtime's need,
and adds no consumer-side tolerance. The manifest stays strict, and the
help carries the conjunction.
- Under **A**, an author who reads `required: true` would conclude that
the credentials must be stored. That invites bootstrap code that copies
the env secrets into settings.
- **C** adds a mapping declaration that is one more thing to get wrong.
- **Startup focus.**
- **B** adds no surface and removes one declared constraint.
- **C** is a capability expansion with no named pull.
- **A** costs nothing and fixes nothing.
**Recommendation and route taken: B.**
One forward note, a reading and not measured here:
objectstack-ai/objectui#13239 makes the console refuse Save while a
visible `required` field is empty. With `required: true` kept, that
check would block every Save on the Authentication page of an
env-credential deployment while Google is on, which is its default. With
`required: false`, the console and the server agree.
## Related
- #24776 is the parent card. Its PR #24842 changed only the help text.
- objectstack-ai/objectui#13239 is the console's required check, which
reads the same `required` flag.
## Acceptance notes
- An empty-string credential is reachable through the API only.
objectstack-ai/objectui#13145 settled that the console clears a field
with `null`, and `null` already saved before this change.
- Saving the toggle ON while nothing is available is accepted. The page
cannot show that the server's pair is in force (that would be route C);
the `google_enabled` help says it instead.
- The probe's real-base reading used builds at `df8bb40a03`. The
final-head reading used builds at `6c1589e41c`, after merging
`origin/main` (which brought #24853's OAuth-registration status change
in plugin-auth, unrelated to the Google binding).
---
_Generated by [Claude
Code](https://claude.ai/code/session_011TdZ8GjixNZFLz1xJD9zzK)_
Co-authored-by: Claude <noreply@anthropic.com>1 parent 7035682 commit fcb40ad
3 files changed
Lines changed: 104 additions & 2 deletions
File tree
- .changeset
- packages/services/service-settings/src/manifests
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
2 | 2 | | |
3 | 3 | | |
4 | 4 | | |
| 5 | + | |
5 | 6 | | |
6 | 7 | | |
7 | 8 | | |
| |||
281 | 282 | | |
282 | 283 | | |
283 | 284 | | |
| 285 | + | |
| 286 | + | |
| 287 | + | |
| 288 | + | |
| 289 | + | |
| 290 | + | |
| 291 | + | |
| 292 | + | |
| 293 | + | |
| 294 | + | |
| 295 | + | |
| 296 | + | |
| 297 | + | |
| 298 | + | |
| 299 | + | |
| 300 | + | |
| 301 | + | |
| 302 | + | |
| 303 | + | |
| 304 | + | |
| 305 | + | |
| 306 | + | |
| 307 | + | |
| 308 | + | |
| 309 | + | |
| 310 | + | |
| 311 | + | |
| 312 | + | |
| 313 | + | |
| 314 | + | |
| 315 | + | |
| 316 | + | |
| 317 | + | |
| 318 | + | |
| 319 | + | |
| 320 | + | |
| 321 | + | |
| 322 | + | |
| 323 | + | |
| 324 | + | |
| 325 | + | |
| 326 | + | |
| 327 | + | |
| 328 | + | |
| 329 | + | |
| 330 | + | |
| 331 | + | |
| 332 | + | |
| 333 | + | |
| 334 | + | |
| 335 | + | |
| 336 | + | |
| 337 | + | |
| 338 | + | |
| 339 | + | |
| 340 | + | |
| 341 | + | |
| 342 | + | |
| 343 | + | |
| 344 | + | |
| 345 | + | |
| 346 | + | |
| 347 | + | |
| 348 | + | |
| 349 | + | |
| 350 | + | |
| 351 | + | |
| 352 | + | |
| 353 | + | |
| 354 | + | |
| 355 | + | |
| 356 | + | |
| 357 | + | |
| 358 | + | |
| 359 | + | |
| 360 | + | |
| 361 | + | |
| 362 | + | |
| 363 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
442 | 442 | | |
443 | 443 | | |
444 | 444 | | |
| 445 | + | |
| 446 | + | |
| 447 | + | |
| 448 | + | |
| 449 | + | |
| 450 | + | |
| 451 | + | |
| 452 | + | |
| 453 | + | |
445 | 454 | | |
446 | 455 | | |
447 | 456 | | |
| |||
455 | 464 | | |
456 | 465 | | |
457 | 466 | | |
458 | | - | |
| 467 | + | |
459 | 468 | | |
460 | 469 | | |
461 | 470 | | |
| |||
464 | 473 | | |
465 | 474 | | |
466 | 475 | | |
467 | | - | |
| 476 | + | |
468 | 477 | | |
469 | 478 | | |
470 | 479 | | |
| |||
0 commit comments