morningman commented on PR #62714: URL: https://github.com/apache/doris/pull/62714#issuecomment-4392646040
# Improvement Suggestions for PR #62714 **PR**: [[feat](fe) Validate authentication integration properties on DDL](https://github.com/apache/doris/pull/62714) **Review Date**: 2026-05-06 --- Suggestions are listed in rough order of importance. ## 1. The PR title and description severely understate what this change does ★★★ The PR title is `[feat](fe) Validate authentication integration properties on DDL`, and the body only describes DDL-time validation plus the plugin-less `check_validation` escape hatch. But the actual commit message — `[fix](fe) Improve OIDC authentication diagnostics` — names the other half of the change: early rejection of OIDC ID tokens at the runtime layer, a client-visible failure message for the OIDC + JIT-disabled case, and two new public error message constants on `AuthenticationIntegrationRuntime`. If a reviewer reads only the PR title and description, the OIDC behavior changes — including a user-facing error string contract — will slip past unnoticed. From a release-note standpoint these are user-visible diagnostic improvements and deserve to be called out. **Recommendations:** - Either broaden the PR title to cover both topics (e.g. `[feat](fe) Validate auth integration on DDL and improve OIDC diagnostics`), or split this into two PRs. - Add an "OIDC diagnostic improvements" section to the PR description that lists the new error strings (`OIDC_ID_TOKEN_REJECTED_MESSAGE`, `OIDC_JIT_DISABLED_MESSAGE`). These are user-facing string contracts and should be public. - The release note section currently says "None"; add at least one line about the OIDC diagnostic improvements. ## 2. The semantics of `UNSET PROPERTIES('check_validation')` are hard to discover and easy to misuse ★★★ ```java private UnsetValidationConfig extractUnsetValidationConfig(Set<String> propertiesToUnset) { Set<String> sanitizedPropertiesToUnset = new java.util.LinkedHashSet<>(); boolean checkValidation = true; for (String key : propertiesToUnset) { if (CHECK_VALIDATION_PROPERTY.equalsIgnoreCase(key)) { checkValidation = false; // listing it in the unset set is enough to skip validation continue; } sanitizedPropertiesToUnset.add(key); } return new UnsetValidationConfig(sanitizedPropertiesToUnset, checkValidation); } ``` Concerns: 1. `check_validation` is never persisted — the SET path strips it before storage — so unsetting it in a user's mental model is "removing a property that doesn't exist." Nobody would guess that this is also the switch that disables validation for this particular ALTER. 2. The semantics are asymmetric with the SET path: `SET PROPERTIES('check_validation' = 'false')` requires an explicit `'true'` / `'false'` value (anything else throws `DdlException`), but the UNSET path has no value at all — merely listing the key flips the flag. 3. There is no dedicated test for this UNSET behavior (see §4 of the full review). The SET path has `testSkipValidationPropertyIsTransient`; the UNSET path has none. 4. The only documentation lives in the commit message. **Recommendations:** - Preferred: have the UNSET path reject `check_validation` with a `DdlException` that points users to `SET PROPERTIES(..., 'check_validation' = 'false')`. Since `check_validation` is not actually a stored property, unsetting it has no real meaning. - Alternative: keep the current behavior, but document it explicitly and add tests like `testSkipValidationOnUnset` and `testCheckValidationKeyIsCaseInsensitiveOnUnset`. ## 3. The `"oidc"` literal is duplicated across files ★★ `AuthenticationIntegrationRuntime` introduces the constant: ```java private static final String OIDC_INTEGRATION_TYPE = "oidc"; ``` But `AuthenticationPluginAuthenticator.isRejectedOidcIdTokenRequest` (and the analogous check in `AuthenticationIntegrationAuthenticator`) still hard-codes `"oidc".equalsIgnoreCase(integration.getType())`. If the type string is ever renamed or normalized (e.g. to `"openid_connect"`), all three sites have to be kept in sync — which is exactly the kind of thing that gets missed. **Recommendation:** promote `OIDC_INTEGRATION_TYPE` to `public static final` (or move it into a shared constants class such as `CredentialType`) and reference it from all three locations. ## 4. `ValidationConfig` and `UnsetValidationConfig` are isomorphic and worth collapsing ★ The two nested static classes are nearly identical — one wraps a `Map`, the other a `Set`, and both carry the same `boolean checkValidation`. Options: - Use a record-style holder, or a `Pair<T, Boolean>` helper where `T` is `Map` or `Set`. - Or skip the holder entirely and return the sanitized collection while writing the boolean to a local variable in the caller. This is not blocking — purely a readability cleanup. ## 5. The contract of `AuthenticationPlugin.validate()` needs to be tightened ★★ The SPI's javadoc on `validate()` is just: *"Validate configuration (called when Integration is created/updated)."* This PR places `validate()` on the synchronous DDL path, which means: - The master FE runs `plugin.validate()` while holding the `AuthenticationIntegrationMgr` write lock. - A slow or blocking validate hurts ALTER latency and stretches lock-hold time, affecting other DDLs. The bundled LDAP and Password plugins do property-only checks, so this is fine today. But the SPI does not forbid a third-party plugin from probing the LDAP server or OAuth issuer inside `validate()`. **Recommendations:** 1. Tighten the javadoc on `AuthenticationPlugin.validate()`: explicitly state that it MUST NOT perform blocking network I/O and may be invoked synchronously on the DDL critical section, where slow validation degrades other DDLs. 2. Or, move the validate call outside the write lock — invoke `validateAuthenticationIntegration` after `extractValidationConfig` but before `writeLock()`. The cost is losing mutual exclusion with concurrent edits, which is not really required here since validation does not depend on shared mutable state. 3. If network-style validation is ever needed, expose a timeout or async knob. ## 6. `AuthenticationPluginAuthenticator#authenticate` calls `isRejectedOidcIdTokenRequest` twice ★ ```java public AuthenticateResponse authenticate(AuthenticateRequest request) throws IOException { AuthenticationRequest pluginRequest = toPluginRequest(request); if (isRejectedOidcIdTokenRequest(pluginRequest)) { return unsupportedCredentialResponse(pluginRequest); // first check } if (!plugin.supports(pluginRequest)) { return unsupportedCredentialResponse(pluginRequest); } ... } private AuthenticateResponse unsupportedCredentialResponse(AuthenticationRequest pluginRequest) { if (isRejectedOidcIdTokenRequest(pluginRequest)) { // second check return AuthenticateResponse.failed(...OIDC_ID_TOKEN_REJECTED_MESSAGE...); } return AuthenticateResponse.failed(...ACCESS_DENIED...); } ``` The first `isRejectedOidcIdTokenRequest` check is redundant — removing it does not change behavior, since `unsupportedCredentialResponse` performs the same check internally and routes the response correctly. **Suggested simplification:** ```java public AuthenticateResponse authenticate(AuthenticateRequest request) throws IOException { AuthenticationRequest pluginRequest = toPluginRequest(request); if (isRejectedOidcIdTokenRequest(pluginRequest) || !plugin.supports(pluginRequest)) { return unsupportedCredentialResponse(pluginRequest); } ... } ``` One fewer call to the same function, and the control flow is tighter. ## 7. `clientVisibleMessage` and `detailMessage` are passed the same constant ★ ```java AuthenticationFailureSummary.forClientVisibleFailure( AuthenticationFailureType.BAD_CREDENTIAL, OIDC_ID_TOKEN_REJECTED_MESSAGE, // detail OIDC_ID_TOKEN_REJECTED_MESSAGE); // clientVisible ``` `AuthenticationFailureSummary` keeps these as separate fields so the server-side log can carry richer context while the client sees only what is necessary. Passing the same constant for both is fine for this particular message (it carries no sensitive context), but the moment someone wants to enrich the server log with e.g. `integrationName` or a request id, every call site has to change. **Recommendation:** add a convenience overload `forClientVisibleFailure(type, sameMessage)` so passing one string is the natural form. Alternatively, embed server-only context (such as `integration.getName()`) into the detail message at this site, so the client still sees `OIDC_ID_TOKEN_REJECTED_MESSAGE` but the server log gains useful detail. ## 8. `extractUnsetValidationConfig` uses a fully qualified `java.util.LinkedHashSet` ★ ```java Set<String> sanitizedPropertiesToUnset = new java.util.LinkedHashSet<>(); ``` The file already imports `java.util.LinkedHashMap` and `java.util.Set` but not `java.util.LinkedHashSet`, so this single line had to use the fully qualified name. Pure style. **Recommendation:** add an `import java.util.LinkedHashSet;` and use the unqualified name, matching the surrounding code. ## 9. Add a class-level note in `AuthenticationIntegrationMgr` about the transient-property pattern ★ The class javadoc on `AuthenticationIntegrationMgr` is just `"Manager for AUTHENTICATION INTEGRATION metadata."`. There are several non-obvious invariants that future contributors should know about: - `check_validation` is a transient flag and is intentionally never persisted. - Sanitizing happens before the write lock; validation happens inside it. - `alterAuthenticationIntegrationComment` does not validate, by design (comment changes cannot invalidate plugin config). A short javadoc summarizing these conventions — and stating that any future transient property must follow the same sanitize-before-persist pattern — would prevent surprises when this is extended. --- ## Missing test coverage to consider adding In addition to the suggestions above, the following gaps in test coverage are worth filling: 1. **No dedicated test for `alterAuthenticationIntegrationUnsetProperties` with `check_validation`.** The SET path has `testSkipValidationPropertyIsTransient`, but listing `check_validation` in the unset set has no assertion at all. Given how counter-intuitive that path is (see §2 above), this is a clear gap. 2. **No test for case-insensitive matching end-to-end.** The construction logic uses `equalsIgnoreCase`, but every test passes the lowercase `"check_validation"`. A single assertion using `CHECK_VALIDATION` or `Check_Validation` would lock down the contract. 3. **No regression test for the strict default.** The regression suite only adds the transient flag. Adding a `CREATE AUTHENTICATION INTEGRATION ... PROPERTIES('type'='ldap')` (without `check_validation`) that fails because the LDAP plugin factory is not loaded would prove that strict validation is the default in plugin-less environments. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
