CBG-4775: Skip re-validating unchanged OIDC providers on database config updates.

[RIT3shSapata]

CBG-4775

Previously, any _config update revalidated every configured OIDC provider's discovery/issuer info, even providers that weren't being added or changed. This meant, e.g., removing one OIDC provider could fail with a validation error (or panic) because of an unrelated, already-accepted provider - even though that provider wasn't part of the update.

• Add OIDCProvider.ShouldValidate (auth/oidc.go) and DatabaseContext.OIDCValidationRequired (db/database.go), called from handlePutDbConfig (rest/admin_api.go) before config validation runs. Only providers not already known to the running database get flagged for validation; rest/config.go's discovery check is now gated on this flag.
• Add TestNoOIDCValidationOnRemoval (rest/oidc_api_test.go): table-driven coverage for removing a default/non-default provider while others remain, removing/adding providers alongside a permanently-invalid one, and promoting a different provider to default - verified by reusing pre/post-update JWTs to confirm removed providers stop authenticating and surviving/promoted ones keep working.

Pre-review checklist

• Logging sensitive data? Make sure it's tagged (e.g. base.UD(docID), base.MD(dbName))
• Updated relevant information in the API specifications (such as endpoint descriptions, schemas, ...) in docs/api

Dependencies (if applicable)

• Link upstream PRs
• Update Go module dependencies when merged

Integration Tests

• https://jenkins.sgwdev.com/job/SyncGatewayIntegration/1024/
• https://jenkins.sgwdev.com/job/SyncGatewayIntegration/1044/

[factory-droid]

Droid finished @RIT3shSapata's task —— View job

---

[Copilot]

Pull request overview

This PR aims to prevent _config updates from re-validating every configured OIDC provider (discovery/issuer), so that updates like removing a provider don’t fail due to unrelated, previously-accepted providers.

Changes:

• Introduces an internal per-provider flag (OIDCProvider.ShouldValidate) intended to gate discovery-time validation.
• Adds DatabaseContext.OIDCValidationRequired and calls it from handlePutDbConfig to selectively flag providers for validation on config updates.
• Adds a new table-driven test (TestNoOIDCValidationOnRemoval) covering provider removals/additions with an intentionally invalid provider present.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
rest/oidc_api_test.go	Adds table-driven test coverage for OIDC provider removal/update scenarios.
rest/config.go	Gates OIDC discovery validation behind a new ShouldValidate flag.
rest/admin_api.go	Marks providers for validation prior to validating _config updates.
db/database.go	Adds OIDCValidationRequired to decide which providers require validation.
auth/oidc.go	Adds ShouldValidate field to OIDCProvider.

[factory-droid]

The change achieves the goal of not revalidating unrelated OIDC providers on /_config updates, but the current ShouldValidate gating is implemented as a persisted per-provider flag and is not consistently set across validation entrypoints. That combination can disable OIDC discovery validation in some flows (notably database creation and edits to existing providers) and can lead to surprising behavior if the flag leaks into persisted config.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (4)

db/database.go:816

• oidcProviderDiscoveryConfigChanged compares InsecureSkipVerify, but that field is not part of the persisted/provider config (it’s set at runtime from db-level unsupported.oidc_tls_skip_verify in startOnlineProcesses). During _config validation the incoming providers will typically have InsecureSkipVerify=false, so if the db is running with oidc_tls_skip_verify=true this function will treat every provider as “changed” and force re-validation on every config update (defeating the purpose of this PR).

	return existing.Issuer != incoming.Issuer ||
		existing.DiscoveryURI != incoming.DiscoveryURI ||
		existing.InsecureSkipVerify != incoming.InsecureSkipVerify ||
		existing.DisableConfigValidation != incoming.DisableConfigValidation ||
		base.ValDefault(existing.ClientID, "") != base.ValDefault(incoming.ClientID, "")

rest/oidc_api_test.go:3065

• DefaultProvider: base.Ptr(defaultProvider) will set default_provider to an explicit empty string when no provider is marked default. That differs from omitting the field (nil) and can lead to confusing configs in these tests (and any future reuse of this helper).

			providers, defaultProvider := buildProviders(test.providers, servers)
			oidcOptions := auth.OIDCOptions{
				Providers:       providers,
				DefaultProvider: base.Ptr(defaultProvider),
			}

rest/oidc_api_test.go:3083

• Same issue on the updated config: when newDefaultProvider is empty, sending default_provider:"" is different from omitting it (nil) and can change behavior for multi-provider configs.

			updatedProviders, newDefaultProvider := buildProviders(test.updatedProviders, servers)
			oidcOptions = auth.OIDCOptions{
				Providers:       updatedProviders,
				DefaultProvider: base.Ptr(newDefaultProvider),
			}

rest/oidc_api_test.go:3136

• The doc comment and test name say validation is “skipped” for in-place provider changes, but the test asserts a 400 (i.e., validation is expected to run and reject the update). This looks like a stale comment/name after the OIDCValidationRequired change-detection was added.

// TestOIDCValidationSkippedOnInPlaceProviderChange demonstrates a gap in OIDCValidationRequired
// (db/database.go): it only checks whether a provider's NAME is new, never whether an existing,
// same-named provider's content (Issuer, DiscoveryURI, etc.) actually changed. So updating a
// provider's issuer in-place - to something that no longer matches its own discovery document -
// incorrectly skips re-validation, since the name alone is enough to mark it "already known".

[bbrks]

A few things I spotted

[bbrks]

If this is a db config upsert without updating existing OIDC providers, this function will essentially be requesting validation for all existing providers even if the config update was unrelated.

This I think is one of the downsides of marking a 'SkipValidation' opt-out instead of 'ForceRevalidation' opt-in ?

Should be easy enough to get test coverage of a db config POST that doesn't touch OIDC when there's an invalid provider set

[bbrks]

This seems like a dev-time repro test of a bug that now needs removing or at least the description/comments inverting to reflect the current behaviour?

[bbrks]

This function is named like it would return a bool - but really it's setting a validation flag on the providers. I would rename this to make it more obvious what it is doing. E.g. SetOIDCValidationFlag?

[bbrks]

Is it worth a bit of refactoring to make the function that sets the per-provider validation rules to accommodate the global validateOIDC bool? So that there's only one way to skip validation?

E.g. OIDCValidationRequired could take a bool which forces all providers to have the skipped flag set? And then we could drop the special validateOIDC handling from dbConfig.validate