fix(security): prevent SSO configuration changes from locking users out#30499
fix(security): prevent SSO configuration changes from locking users out#30499mohityadav766 wants to merge 4 commits into
Conversation
Misconfiguring SSO (notably an empty/`none` OIDC prompt, or a malformed principal domain with enforcement on) could lock every user out of a deployment. Harden the security-config path end to end so a change can be verified before it goes live and recovered afterward. Validation: - Add OidcPromptPolicy as the single provider-aware source of truth for the OIDC `prompt`: rejects provider-unsupported values (e.g. `login` on Google) and the silent-sign-in `none` with a message pointing to the safe value. Now covers public clients too; replaces the scattered/inconsistent checks in GoogleAuthValidator and OidcDiscoveryValidator. - Validate `principalDomain` format when Enforce Principal Domain is on. Apply path: - PUT /system/security/config now validates before persisting (previously only PATCH did), sharing one persist-and-reload helper. - Fix a latent "Unknown provider" error for BASIC/OPENMETADATA in the validation switch, which the PUT change would otherwise have surfaced. Recovery: - Add POST /system/security/config/revert to restore the configuration that was live before the most recent change and reload auth. Not gated on validation, so it recovers even when the new identity provider is unreachable; the admin session survives a config change so it stays reachable. UI + docs: - "Restore Previous Configuration" button + confirmation in the SSO form. - Fix misleading principalDomain/prompt placeholders and add field help explaining the lockout risk; strengthen the new-config save warning with Test Login and CLI recovery guidance; add an SSO troubleshooting doc. Tests: OidcPromptPolicyTest, SystemRepositoryPrincipalDomainTest, and two SystemResourceIT cases (validate rejects prompt=none; revert restores the prior config). Ref: open-metadata/openmetadata-collate#5235 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
github-actions
Bot
added
backend
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| public Response revertSecurityConfig(@Context SecurityContext securityContext) { | ||
| authorizer.authorizeAdmin(securityContext); | ||
| SecurityConfiguration previous = | ||
| SecurityConfigurationManager.getInstance().getPreviousSecurityConfig(); | ||
| Response response; | ||
| if (previous == null) { | ||
| response = | ||
| Response.status(Response.Status.NOT_FOUND) | ||
| .entity("No previous security configuration is available to revert to") | ||
| .build(); | ||
| } else { | ||
| try { | ||
| persistAndReloadSecurityConfig(previous); | ||
| response = Response.ok(previous).build(); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
⚠️ Edge Case: Repeated revert re-applies the broken configuration
revertSecurityConfig persists previous via persistAndReloadSecurityConfig, and reloadSecuritySystem() sets previousSecurityConfig = getCurrentSecurityConfig() (the now-current, i.e. the just-reverted-away broken config) before loading the restored one. So after one revert, previousSecurityConfig points at the broken config, and a second revert (or an accidental double-click / repeated recovery attempt) re-applies exactly the config the admin was trying to escape — the opposite of recovery. Consider snapshotting the last known-good config separately from the generic reload bookkeeping, or clearing/not overwriting previousSecurityConfig during a revert so revert is idempotent rather than a toggle.
Make revert idempotent instead of toggling between good/broken configs.:
// In revert, capture the good config, then prevent reload from making the
// broken config the new 'previous'. e.g. after persistAndReloadSecurityConfig(previous):
// manager.setPreviousSecurityConfig(null);
// so a second revert returns 404 instead of re-applying the broken config.
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| String currentUsername = SecurityUtil.getUserName(securityContext); | ||
| SecurityValidationResponse validationResponse = | ||
| systemRepository.validateSecurityConfiguration( | ||
| securityConfig, applicationConfig, currentUsername); | ||
| Response response; | ||
| if (validationResponse.getStatus() != SecurityValidationResponse.Status.SUCCESS) { | ||
| logValidationErrors(validationResponse); | ||
| response = Response.status(Response.Status.BAD_REQUEST).entity(validationResponse).build(); | ||
| } else { | ||
| persistAndReloadSecurityConfig(securityConfig); | ||
| response = Response.ok(securityConfig).build(); | ||
| } | ||
| return response; |
There was a problem hiding this comment.
⚠️ Bug: PUT security config now hard-fails on live IdP reachability
updateSecurityConfig (PUT) now rejects the apply whenever validateSecurityConfiguration returns non-SUCCESS. That validation path invokes live provider checks (e.g. GoogleAuthValidator.validateGoogleCredentials contacts Google's token/authorization endpoints), so a transient IdP or network outage — or any validator false positive — will block an admin from applying an otherwise-correct configuration, which previously always persisted. Confirm this is intended and, if so, ensure connectivity/transient failures degrade to a warning rather than a hard BAD_REQUEST, or document that operators must use the CLI recovery path when validation is unavailable.
Avoid blocking valid config applies during IdP/network outages.:
// Distinguish 'config invalid' from 'validator could not reach IdP'.
// For transient/connectivity failures, log a warning and allow persist
// (or return a distinct 503) instead of a blanket 400 that blocks valid configs.
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| public static FieldError validate(AuthProvider provider, String prompt) { | ||
| FieldError result = null; | ||
| if (!nullOrEmpty(prompt) && SUPPORTED.containsKey(provider)) { | ||
| String[] tokens = prompt.trim().toLowerCase(Locale.ROOT).split("\\s+"); |
There was a problem hiding this comment.
When an administrator enters a differently-cased prompt such as Select_Account, validation lowercases it and accepts it, but the login flow sends the original value to the identity provider, causing case-sensitive providers to reject a configuration that passed validation and potentially locking users out.
Knowledge Base Used: Auth and Security: Authentication, Authorization, Secrets, and SCIM
| "restore": "استعادة", | ||
| "restore-default-plural": "استعادة الافتراضيات", | ||
| "restore-entity": "استعادة {{entity}}", | ||
| "restore-previous-configuration": "Restore Previous Configuration", |
There was a problem hiding this comment.
The new restore label, confirmation, and success message remain English placeholders in every changed non-English locale, so users encounter a partially untranslated security-recovery workflow. Replace the propagated placeholders with translations for each locale.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| systemRepository.createOrUpdate( | ||
| new Settings() | ||
| .withConfigType(AUTHORIZER_CONFIGURATION) | ||
| .withConfigValue(securityConfig.getAuthorizerConfiguration())); |
There was a problem hiding this comment.
When the second settings write fails during the new revert flow, the authentication settings have already committed in a separate transaction while the authorizer settings remain unchanged, causing the database to hold a mixed security configuration that is loaded on a later restart or reload. Persist both settings in one encompassing transaction before reloading.
Knowledge Base Used: Auth and Security: Authentication, Authorization, Secrets, and SCIM
❌ UI Checkstyle Failed❌ ESLint + Prettier + Organise Imports (src)One or more source files have linting or formatting issues. ❌ Licence HeaderOne or more files are missing or have an outdated Apache 2.0 licence header. Affected files
Fix locally (fast - only checks files changed in this branch): make ui-checkstyle-changed |
There was a problem hiding this comment.
Pull request overview
Hardens SSO/security configuration changes to reduce the risk of locking deployments out by adding provider-aware validation (OIDC prompt, principalDomain) and introducing a revert safety net (API + UI) to restore the last-known-good configuration.
Changes:
- Added centralized OIDC
promptvalidation (OidcPromptPolicy) and principal domain format validation (when enforcement is enabled). - Updated
PUT /system/security/configto validate before persisting and addedPOST /system/security/config/revertto restore the previous configuration. - Added UI “Restore Previous Configuration” flow, updated SSO form helpers/warnings, and added an SSO troubleshooting doc.
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| openmetadata-ui/src/main/resources/ui/src/rest/securityConfigAPI.ts | Adds UI REST client for /system/security/config/revert. |
| openmetadata-ui/src/main/resources/ui/src/constants/SSO.constant.ts | Updates SSO form placeholders/help text for prompt and principalDomain. |
| openmetadata-ui/src/main/resources/ui/src/components/SettingsSso/SSOConfigurationForm/SSOConfigurationForm.tsx | Adds “Restore Previous Configuration” button + confirmation modal and calls revert API. |
| openmetadata-ui/src/main/resources/ui/public/locales/en-US/SSO/ssoTroubleshooting.md | Adds troubleshooting/recovery documentation for common lockout scenarios. |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/en-us.json | Adds new i18n keys for revert flow + strengthens save warning text. |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/ar-sa.json | Adds new i18n keys for revert flow (currently English placeholders). |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/de-de.json | Adds new i18n keys for revert flow (currently English placeholders). |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/es-es.json | Adds new i18n keys for revert flow (currently English placeholders). |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/fr-fr.json | Adds new i18n keys for revert flow (currently English placeholders). |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/gl-es.json | Adds new i18n keys for revert flow (currently English placeholders). |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/he-he.json | Adds new i18n keys for revert flow (currently English placeholders). |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/ja-jp.json | Adds new i18n keys for revert flow (currently English placeholders). |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/ko-kr.json | Adds new i18n keys for revert flow (currently English placeholders). |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/mr-in.json | Adds new i18n keys for revert flow (currently English placeholders). |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/nl-nl.json | Adds new i18n keys for revert flow (currently English placeholders). |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/pr-pr.json | Adds new i18n keys for revert flow (currently English placeholders). |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/pt-br.json | Adds new i18n keys for revert flow (currently English placeholders). |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/pt-pt.json | Adds new i18n keys for revert flow (currently English placeholders). |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/ru-ru.json | Adds new i18n keys for revert flow (currently English placeholders). |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/sv-se.json | Adds new i18n keys for revert flow (currently English placeholders). |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/th-th.json | Adds new i18n keys for revert flow (currently English placeholders). |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/tr-tr.json | Adds new i18n keys for revert flow (currently English placeholders). |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/zh-cn.json | Adds new i18n keys for revert flow (currently English placeholders). |
| openmetadata-ui/src/main/resources/ui/src/locale/languages/zh-tw.json | Adds new i18n keys for revert flow (currently English placeholders). |
| openmetadata-service/src/main/java/org/openmetadata/service/security/auth/validator/OidcPromptPolicy.java | Introduces provider-aware OIDC prompt validation and blocks none as a lockout risk. |
| openmetadata-service/src/test/java/org/openmetadata/service/security/auth/validator/OidcPromptPolicyTest.java | Adds unit coverage for prompt policy behavior across providers. |
| openmetadata-service/src/main/java/org/openmetadata/service/security/auth/validator/OidcDiscoveryValidator.java | Removes duplicated/conflicting prompt validation in discovery validator. |
| openmetadata-service/src/test/java/org/openmetadata/service/security/auth/validator/OidcDiscoveryValidatorTest.java | Removes tests that covered the removed prompt validation path. |
| openmetadata-service/src/main/java/org/openmetadata/service/security/auth/validator/GoogleAuthValidator.java | Removes scattered prompt validation now covered centrally. |
| openmetadata-service/src/main/java/org/openmetadata/service/security/auth/SecurityConfigurationManager.java | Exposes previous security config for revert endpoint. |
| openmetadata-service/src/main/java/org/openmetadata/service/resources/system/SystemResource.java | Adds validate-before-persist on PUT, shared persist+reload helper, and revert endpoint. |
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/SystemRepository.java | Adds OidcPromptPolicy + principalDomain format validation; fixes BASIC/OPENMETADATA provider validation switch. |
| openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/SystemRepositoryPrincipalDomainTest.java | Adds unit tests for principalDomain validation helper. |
| openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/SystemResourceIT.java | Adds IT coverage for prompt rejection and revert behavior. |
| private void persistAndReloadSecurityConfig(SecurityConfiguration securityConfig) { | ||
| AuthenticationConfiguration authConfig = securityConfig.getAuthenticationConfiguration(); | ||
| systemRepository.autoPopulatePublicKeyUrlsIfNeeded(authConfig); | ||
| systemRepository.createOrUpdate( | ||
| new Settings().withConfigType(AUTHENTICATION_CONFIGURATION).withConfigValue(authConfig)); | ||
| systemRepository.createOrUpdate( | ||
| new Settings() | ||
| .withConfigType(AUTHORIZER_CONFIGURATION) | ||
| .withConfigValue(securityConfig.getAuthorizerConfiguration())); | ||
| SettingsCache.invalidateSettings(AUTHENTICATION_CONFIGURATION.toString()); | ||
| SettingsCache.invalidateSettings(AUTHORIZER_CONFIGURATION.toString()); | ||
| SecurityConfigurationManager.getInstance().reloadSecuritySystem(); | ||
| } |
| FieldError error = OidcPromptPolicy.validate(provider, "none"); | ||
| assertNotNull(error, provider + " should reject prompt=none"); | ||
| assertEquals(ValidationErrorBuilder.FieldPaths.OIDC_PROMPT, error.getField()); | ||
| assertTrue(error.getError().toLowerCase().contains("silent")); |
| "restore": "Wiederherstellen", | ||
| "restore-default-plural": "Standardeinstellungen wiederherstellen", | ||
| "restore-entity": "{{entity}} wiederherstellen", | ||
| "restore-previous-configuration": "Restore Previous Configuration", |
🔴 Playwright Results — workflow failedValidated commit ✅ 0 passed · ❌ 0 failed · 🟡 0 flaky · ⏭️ 0 skipped · 🧰 0 lifecycle flaky Pipeline and setup failures (5)
Performance⚪ Performance metrics unavailable; see the CI and reporting failures above.
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
Jest test CoverageUI tests summary
|
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 34 changed files in this pull request and generated 21 comments.
Comments suppressed due to low confidence (5)
openmetadata-service/src/main/java/org/openmetadata/service/resources/system/SystemResource.java:936
- persistAndReloadSecurityConfig() ignores the Response returned by SystemRepository.createOrUpdate(), so a failed settings write (500 Response) can still proceed to cache invalidation + reload, potentially leaving a partially applied / inconsistent security state. Also, prompt validation is case/whitespace-insensitive but the login handler forwards the stored prompt value verbatim; normalizing before persisting avoids runtime failures from values like " Select_Account ".
private void persistAndReloadSecurityConfig(SecurityConfiguration securityConfig) {
AuthenticationConfiguration authConfig = securityConfig.getAuthenticationConfiguration();
systemRepository.autoPopulatePublicKeyUrlsIfNeeded(authConfig);
systemRepository.createOrUpdate(
new Settings().withConfigType(AUTHENTICATION_CONFIGURATION).withConfigValue(authConfig));
systemRepository.createOrUpdate(
new Settings()
.withConfigType(AUTHORIZER_CONFIGURATION)
.withConfigValue(securityConfig.getAuthorizerConfiguration()));
openmetadata-service/src/main/java/org/openmetadata/service/resources/system/SystemResource.java:1066
- patchSecurityConfig() duplicates persistence + cache invalidation + reload logic that now exists in persistAndReloadSecurityConfig(), and also misses the auto-populate step for publicKeyUrls. Calling the helper here keeps PUT/PATCH/revert consistent and avoids drift in future changes.
Settings authSettings =
new Settings()
.withConfigType(AUTHENTICATION_CONFIGURATION)
.withConfigValue(updatedConfig.getAuthenticationConfiguration());
Settings authzSettings =
new Settings()
.withConfigType(AUTHORIZER_CONFIGURATION)
.withConfigValue(updatedConfig.getAuthorizerConfiguration());
systemRepository.createOrUpdate(authSettings);
systemRepository.createOrUpdate(authzSettings);
SettingsCache.invalidateSettings(AUTHENTICATION_CONFIGURATION.toString());
SettingsCache.invalidateSettings(AUTHORIZER_CONFIGURATION.toString());
SecurityConfigurationManager.getInstance().reloadSecuritySystem();
return Response.noContent().build();
openmetadata-service/src/main/java/org/openmetadata/service/resources/system/SystemResource.java:992
- This wraps the original exception but drops the cause, which makes debugging revert failures harder. Prefer including the caught exception as the RuntimeException cause.
} catch (Exception e) {
LOG.error("Failed to revert security configuration", e);
throw new RuntimeException("Failed to revert security configuration: " + e.getMessage());
}
openmetadata-ui/src/main/resources/ui/src/constants/SSO.constant.ts:641
- These new Principal Domain placeholder/help strings are user-facing but hardcoded, so they won't be localized. Prefer moving them to i18n keys so non-English deployments get translated guidance (especially important for lockout warnings).
'ui:placeholder': 'e.g. yourcompany.com',
'ui:help':
"Your organization's email domain (e.g. 'yourcompany.com'), without a scheme or '@'. When 'Enforce Principal Domain' is enabled, users whose email domain does not match are denied access, so an incorrect value locks everyone out.",
openmetadata-ui/src/main/resources/ui/src/locale/languages/de-de.json:2149
- This locale entry is still an English placeholder. Please provide German translations for the newly added SSO recovery strings (e.g.,
restore-previous-configuration,sso-config-revert-success,sso-revert-confirmation) instead of shipping English text underde-de.
"restore-previous-configuration": "Restore Previous Configuration",
| } catch (Exception e) { | ||
| LOG.error("Failed to update security configuration", e); | ||
| throw new RuntimeException("Failed to update security configuration: " + e.getMessage()); | ||
| } |
| export const revertSecurityConfiguration = async (): Promise< | ||
| AxiosResponse<SecurityConfiguration> | ||
| > => { | ||
| return APIClient.post<undefined, AxiosResponse<SecurityConfiguration>>( | ||
| '/system/security/config/revert' | ||
| ); | ||
| }; |
| oidcPrompt: { | ||
| 'ui:title': 'OIDC Prompt', | ||
| 'ui:placeholder': 'e.g. select_account', | ||
| 'ui:help': | ||
| "Controls the identity provider's login screen. Recommended: 'select_account', which always shows the account picker. Avoid 'none' or leaving this empty — 'none' requests a silent sign-in that locks out users without an active provider session. Supported values vary by provider (e.g. Google does not accept 'login').", | ||
| }, |
| "restore": "استعادة", | ||
| "restore-default-plural": "استعادة الافتراضيات", | ||
| "restore-entity": "استعادة {{entity}}", | ||
| "restore-previous-configuration": "Restore Previous Configuration", |
| "restore": "Restaurar", | ||
| "restore-default-plural": "Restaurar valores predeterminados", | ||
| "restore-entity": "Restaurar {{entity}}", | ||
| "restore-previous-configuration": "Restore Previous Configuration", |
| "restore": "Återställ", | ||
| "restore-default-plural": "Återställ standardvärden", | ||
| "restore-entity": "Återställ {{entity}}", | ||
| "restore-previous-configuration": "Restore Previous Configuration", |
| "restore": "กู้คืน", | ||
| "restore-default-plural": "คืนค่าเริ่มต้น", | ||
| "restore-entity": "กู้คืน {{entity}}", | ||
| "restore-previous-configuration": "Restore Previous Configuration", |
| "restore": "Geri Yükle", | ||
| "restore-default-plural": "Varsayılanları Geri Yükle", | ||
| "restore-entity": "{{entity}} Geri Yükle", | ||
| "restore-previous-configuration": "Restore Previous Configuration", |
| "restore": "恢复", | ||
| "restore-default-plural": "恢复默认值", | ||
| "restore-entity": "恢复{{entity}}", | ||
| "restore-previous-configuration": "Restore Previous Configuration", |
| "restore": "還原", | ||
| "restore-default-plural": "還原預設值", | ||
| "restore-entity": "還原 {{entity}}", | ||
| "restore-previous-configuration": "Restore Previous Configuration", |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 34 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
openmetadata-service/src/main/java/org/openmetadata/service/resources/system/SystemResource.java:932
persistAndReloadSecurityConfigignores theResponsereturned bysystemRepository.createOrUpdate(...). SinceSystemRepository#createOrUpdatecatches exceptions and returns a 500Responseinstead of throwing, this code will still invalidate caches and reload security even when a settings write failed, leaving the deployment in an unexpected state.
Handle the returned responses and fail fast before cache invalidation/reload when either update is not successful.
private void persistAndReloadSecurityConfig(SecurityConfiguration securityConfig) {
AuthenticationConfiguration authConfig = securityConfig.getAuthenticationConfiguration();
systemRepository.autoPopulatePublicKeyUrlsIfNeeded(authConfig);
systemRepository.createOrUpdate(
new Settings().withConfigType(AUTHENTICATION_CONFIGURATION).withConfigValue(authConfig));
openmetadata-ui/src/main/resources/ui/src/locale/languages/de-de.json:2150
- These newly-added i18n keys are still English in a non-English locale file (e.g.
"Restore Previous Configuration", revert confirmation/success text). PerAGENTS.md:117, placeholders inserted byyarn i18nshould be replaced with real translations before merge.
Please provide translations for these new keys across all non-en-us locale files updated in this PR.
"restore-entity": "{{entity}} wiederherstellen",
"restore-previous-configuration": "Restore Previous Configuration",
"restore-version": "v{{version}} wiederherstellen",
| private void persistAndReloadSecurityConfig(SecurityConfiguration securityConfig) { | ||
| AuthenticationConfiguration authConfig = securityConfig.getAuthenticationConfiguration(); | ||
| systemRepository.autoPopulatePublicKeyUrlsIfNeeded(authConfig); | ||
| systemRepository.createOrUpdate( | ||
| new Settings().withConfigType(AUTHENTICATION_CONFIGURATION).withConfigValue(authConfig)); | ||
| systemRepository.createOrUpdate( | ||
| new Settings() | ||
| .withConfigType(AUTHORIZER_CONFIGURATION) | ||
| .withConfigValue(securityConfig.getAuthorizerConfiguration())); |
| message = | ||
| "Prompt 'none' requests a silent sign-in that only succeeds when the user already has a " | ||
| + "live session with the identity provider; users with an expired or missing session " | ||
| + "are locked out with no login screen. Use '" | ||
| + RECOMMENDED | ||
| + "' or 'login' instead."; | ||
| } |
| if (!nullOrEmpty(prompt) && SUPPORTED.containsKey(provider)) { | ||
| String[] tokens = prompt.trim().toLowerCase(Locale.ROOT).split("\\s+"); | ||
| result = validateTokens(provider, tokens); | ||
| } | ||
| return result; |
| "restore": "استعادة", | ||
| "restore-default-plural": "استعادة الافتراضيات", | ||
| "restore-entity": "استعادة {{entity}}", | ||
| "restore-previous-configuration": "Restore Previous Configuration", |
There was a problem hiding this comment.
please add translations
There was a problem hiding this comment.
Do we need to expose it to UI or just a admin thing for now?
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Gitar
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 34 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (22)
openmetadata-service/src/main/java/org/openmetadata/service/resources/system/SystemResource.java:935
- persistAndReloadSecurityConfig ignores failures from SystemRepository.createOrUpdate(...) because that method returns a Response (including 500 on error) rather than throwing. As a result, a DB write failure can be silently ignored and the endpoint can still proceed to reload/return 200, leaving the deployment in an inconsistent state. Prefer calling systemRepository.updateSetting(...) (which throws on failure) or checking the returned Response status and aborting on non-2xx.
systemRepository.createOrUpdate(
new Settings().withConfigType(AUTHENTICATION_CONFIGURATION).withConfigValue(authConfig));
systemRepository.createOrUpdate(
new Settings()
.withConfigType(AUTHORIZER_CONFIGURATION)
openmetadata-service/src/main/java/org/openmetadata/service/resources/system/SystemResource.java:932
- persistAndReloadSecurityConfig writes AUTHENTICATION_CONFIGURATION and AUTHORIZER_CONFIGURATION in two separate repository calls/transactions. If the second write fails (or the process crashes between writes), the DB can end up with a mixed security configuration that neither fully represents the old state nor the new state, which is particularly risky for auth changes. Consider adding a single
@Transactionrepository method that persists both settings atomically (and only then invalidates caches + reloads).
private void persistAndReloadSecurityConfig(SecurityConfiguration securityConfig) {
AuthenticationConfiguration authConfig = securityConfig.getAuthenticationConfiguration();
systemRepository.autoPopulatePublicKeyUrlsIfNeeded(authConfig);
systemRepository.createOrUpdate(
new Settings().withConfigType(AUTHENTICATION_CONFIGURATION).withConfigValue(authConfig));
openmetadata-service/src/main/java/org/openmetadata/service/security/auth/validator/OidcPromptPolicy.java:71
- OidcPromptPolicy validation normalizes case/whitespace for comparison, but the saved prompt value is later forwarded as-is at runtime (AuthenticationCodeFlowHandler puts oidcConfiguration.prompt directly into the OIDC request params). This means values like " Select_Account " can pass validation but still be sent with padding/mixed case, which some IdPs reject. Consider normalizing the prompt before persisting (trim + collapse whitespace + lower-case with Locale.ROOT) or normalizing right before adding it to the auth request parameters, so runtime behavior matches validation expectations.
public static FieldError validate(AuthProvider provider, String prompt) {
FieldError result = null;
if (!nullOrEmpty(prompt) && SUPPORTED.containsKey(provider)) {
String[] tokens = prompt.trim().toLowerCase(Locale.ROOT).split("\\s+");
result = validateTokens(provider, tokens);
}
openmetadata-ui/src/main/resources/ui/src/locale/languages/zh-tw.json:4139
- These newly added locale entries are still English placeholders (e.g.,
label.restore-previous-configuration,message.sso-config-revert-success,message.sso-revert-confirmation). Per repo i18n guidelines, non-en locales should not ship English placeholders; please provide proper zh-TW translations (or intentionally document any terms left in English).
openmetadata-ui/src/main/resources/ui/src/locale/languages/zh-cn.json:4139 - These newly added locale entries are still English placeholders (e.g.,
label.restore-previous-configuration,message.sso-config-revert-success,message.sso-revert-confirmation). Per repo i18n guidelines, non-en locales should not ship English placeholders; please provide proper zh-CN translations (or intentionally document any terms left in English).
openmetadata-ui/src/main/resources/ui/src/locale/languages/tr-tr.json:4139 - These newly added locale entries are still English placeholders (e.g.,
label.restore-previous-configuration,message.sso-config-revert-success,message.sso-revert-confirmation). Per repo i18n guidelines, non-en locales should not ship English placeholders; please provide proper tr-TR translations (or intentionally document any terms left in English).
openmetadata-ui/src/main/resources/ui/src/locale/languages/th-th.json:4139 - These newly added locale entries are still English placeholders (e.g.,
label.restore-previous-configuration,message.sso-config-revert-success,message.sso-revert-confirmation). Per repo i18n guidelines, non-en locales should not ship English placeholders; please provide proper th-TH translations (or intentionally document any terms left in English).
"sso-config-revert-success": "Restored the previous security configuration. Reloading…",
openmetadata-ui/src/main/resources/ui/src/locale/languages/sv-se.json:4139
- These newly added locale entries are still English placeholders (e.g.,
label.restore-previous-configuration,message.sso-config-revert-success,message.sso-revert-confirmation). Per repo i18n guidelines, non-en locales should not ship English placeholders; please provide proper sv-SE translations (or intentionally document any terms left in English).
"sso-config-revert-success": "Restored the previous security configuration. Reloading…",
openmetadata-ui/src/main/resources/ui/src/locale/languages/ru-ru.json:4139
- These newly added locale entries are still English placeholders (e.g.,
label.restore-previous-configuration,message.sso-config-revert-success,message.sso-revert-confirmation). Per repo i18n guidelines, non-en locales should not ship English placeholders; please provide proper ru-RU translations (or intentionally document any terms left in English).
"sso-config-revert-success": "Restored the previous security configuration. Reloading…",
openmetadata-ui/src/main/resources/ui/src/locale/languages/pt-pt.json:4139
- These newly added locale entries are still English placeholders (e.g.,
label.restore-previous-configuration,message.sso-config-revert-success,message.sso-revert-confirmation). Per repo i18n guidelines, non-en locales should not ship English placeholders; please provide proper pt-PT translations (or intentionally document any terms left in English).
"sso-config-revert-success": "Restored the previous security configuration. Reloading…",
openmetadata-ui/src/main/resources/ui/src/locale/languages/pt-br.json:4139
- These newly added locale entries are still English placeholders (e.g.,
label.restore-previous-configuration,message.sso-config-revert-success,message.sso-revert-confirmation). Per repo i18n guidelines, non-en locales should not ship English placeholders; please provide proper pt-BR translations (or intentionally document any terms left in English).
"sso-config-revert-success": "Restored the previous security configuration. Reloading…",
openmetadata-ui/src/main/resources/ui/src/locale/languages/pr-pr.json:4139
- These newly added locale entries are still English placeholders (e.g.,
label.restore-previous-configuration,message.sso-config-revert-success,message.sso-revert-confirmation). Per repo i18n guidelines, non-en locales should not ship English placeholders; please provide proper pr-PR translations (or intentionally document any terms left in English).
"sso-config-revert-success": "Restored the previous security configuration. Reloading…",
openmetadata-ui/src/main/resources/ui/src/locale/languages/nl-nl.json:4139
- These newly added locale entries are still English placeholders (e.g.,
label.restore-previous-configuration,message.sso-config-revert-success,message.sso-revert-confirmation). Per repo i18n guidelines, non-en locales should not ship English placeholders; please provide proper nl-NL translations (or intentionally document any terms left in English).
"sso-config-revert-success": "Restored the previous security configuration. Reloading…",
openmetadata-ui/src/main/resources/ui/src/locale/languages/mr-in.json:4139
- These newly added locale entries are still English placeholders (e.g.,
label.restore-previous-configuration,message.sso-config-revert-success,message.sso-revert-confirmation). Per repo i18n guidelines, non-en locales should not ship English placeholders; please provide proper mr-IN translations (or intentionally document any terms left in English).
"sso-config-revert-success": "Restored the previous security configuration. Reloading…",
openmetadata-ui/src/main/resources/ui/src/locale/languages/ko-kr.json:4139
- These newly added locale entries are still English placeholders (e.g.,
label.restore-previous-configuration,message.sso-config-revert-success,message.sso-revert-confirmation). Per repo i18n guidelines, non-en locales should not ship English placeholders; please provide proper ko-KR translations (or intentionally document any terms left in English).
"sso-config-revert-success": "Restored the previous security configuration. Reloading…",
openmetadata-ui/src/main/resources/ui/src/locale/languages/ja-jp.json:4139
- These newly added locale entries are still English placeholders (e.g.,
label.restore-previous-configuration,message.sso-config-revert-success,message.sso-revert-confirmation). Per repo i18n guidelines, non-en locales should not ship English placeholders; please provide proper ja-JP translations (or intentionally document any terms left in English).
"sso-config-revert-success": "Restored the previous security configuration. Reloading…",
openmetadata-ui/src/main/resources/ui/src/locale/languages/he-he.json:4139
- These newly added locale entries are still English placeholders (e.g.,
label.restore-previous-configuration,message.sso-config-revert-success,message.sso-revert-confirmation). Per repo i18n guidelines, non-en locales should not ship English placeholders; please provide proper he-HE translations (or intentionally document any terms left in English).
"sso-config-revert-success": "Restored the previous security configuration. Reloading…",
openmetadata-ui/src/main/resources/ui/src/locale/languages/gl-es.json:4139
- These newly added locale entries are still English placeholders (e.g.,
label.restore-previous-configuration,message.sso-config-revert-success,message.sso-revert-confirmation). Per repo i18n guidelines, non-en locales should not ship English placeholders; please provide proper gl-ES translations (or intentionally document any terms left in English).
"sso-config-revert-success": "Restored the previous security configuration. Reloading…",
openmetadata-ui/src/main/resources/ui/src/locale/languages/fr-fr.json:4139
- These newly added locale entries are still English placeholders (e.g.,
label.restore-previous-configuration,message.sso-config-revert-success,message.sso-revert-confirmation). Per repo i18n guidelines, non-en locales should not ship English placeholders; please provide proper fr-FR translations (or intentionally document any terms left in English).
"sso-config-revert-success": "Restored the previous security configuration. Reloading…",
openmetadata-ui/src/main/resources/ui/src/locale/languages/es-es.json:4139
- These newly added locale entries are still English placeholders (e.g.,
label.restore-previous-configuration,message.sso-config-revert-success,message.sso-revert-confirmation). Per repo i18n guidelines, non-en locales should not ship English placeholders; please provide proper es-ES translations (or intentionally document any terms left in English).
"sso-config-revert-success": "Restored the previous security configuration. Reloading…",
openmetadata-ui/src/main/resources/ui/src/locale/languages/de-de.json:4139
- These newly added locale entries are still English placeholders (e.g.,
label.restore-previous-configuration,message.sso-config-revert-success,message.sso-revert-confirmation). Per repo i18n guidelines, non-en locales should not ship English placeholders; please provide proper de-DE translations (or intentionally document any terms left in English).
"sso-config-revert-success": "Restored the previous security configuration. Reloading…",
openmetadata-ui/src/main/resources/ui/src/locale/languages/ar-sa.json:4139
- These newly added locale entries are still English placeholders (e.g.,
label.restore-previous-configuration,message.sso-config-revert-success,message.sso-revert-confirmation). Per repo i18n guidelines, non-en locales should not ship English placeholders; please provide proper ar-SA translations (or intentionally document any terms left in English).
"sso-config-revert-success": "Restored the previous security configuration. Reloading…",
|
Problem
Misconfiguring SSO can lock every user (including admins) out of a deployment. A customer hit this by leaving the OIDC
promptempty (Azure then attempted a silent sign-in against a stale session and hard-failed withAADSTS50058— no login screen), and separately by setting aprincipalDomainvalue that "shouldn't have been allowed." Validation will always miss something, so this hardens the whole config path: verify before it goes live, and recover cleanly afterward.Origin: Slack thread · open-metadata/openmetadata-collate#5235
What changed
Validation
OidcPromptPolicy— one provider-aware source of truth for the OIDCprompt. Rejects provider-unsupported values (e.g.loginon Google,consenton Cognito) and the silent-sign-innonewith a message naming the safe value (select_account). Now runs for public clients too, and replaces the scattered/inconsistent checks inGoogleAuthValidatorandOidcDiscoveryValidator.principalDomainformat validation when Enforce Principal Domain is on (an invalid value denies every non-matching user).Apply path
PUT /system/security/confignow validates before persisting (previously onlyPATCHdid), via a shared persist-and-reload helper."Unknown provider"error forBASIC/OPENMETADATAin the validation switch that the PUT change would otherwise have surfaced.Recovery — the safety net
POST /system/security/config/revertrestores the configuration that was live before the most recent change and reloads auth. Not gated on validation, so it recovers even when the just-applied IdP is unreachable. The admin's session survives a config change (OM-issued token +updateConfiguration, not a session wipe), so this stays reachable.UI + docs
https://accounts.google.complaceholders onprincipalDomain(→yourcompany.com) andprompt(→select_account) and addedui:helpexplaining the lockout risk.remove-security-configCLI recovery.ssoTroubleshooting.md(per-provider prompt matrix, principalDomain, recovery runbook).Testing
OidcPromptPolicyTest(9),SystemRepositoryPrincipalDomainTest(2),OidcDiscoveryValidatorTest(25) — pass.SystemResourceIT:test_validateSecurityConfig_rejectsSilentPromptNone,test_revertSecurityConfig_restoresPreviousConfiguration(need the containerized IT stack to run).Notes for reviewers
yarn i18nstate) — the 16 non-en locales need a translator pass./signin, where the CLI recovery (surfaced in the warning + doc) is the path.🤖 Generated with Claude Code
Greptile Summary
This PR hardens SSO configuration and recovery.
Confidence Score: 2/5
The PR does not appear safe to merge while accepted mixed-case prompts can still break login and revert persistence can leave authentication and authorization settings inconsistent.
Prompt validation canonicalizes only its comparison value while the login flow sends the original prompt, and the revert helper still commits its two security-setting writes independently; the untranslated recovery strings also remain outstanding.
Files Needing Attention: openmetadata-service/src/main/java/org/openmetadata/service/security/auth/validator/OidcPromptPolicy.java; openmetadata-service/src/main/java/org/openmetadata/service/security/AuthenticationCodeFlowHandler.java; openmetadata-service/src/main/java/org/openmetadata/service/resources/system/SystemResource.java; openmetadata-ui/src/main/resources/ui/src/locale/languages/*.json
Important Files Changed
Sequence Diagram
sequenceDiagram participant Admin participant UI participant SystemResource participant Validator participant SettingsDB participant SecurityManager Admin->>UI: Save SSO configuration UI->>SystemResource: PUT /system/security/config SystemResource->>Validator: Validate authentication and authorizer settings alt Validation succeeds SystemResource->>SettingsDB: Persist authentication and authorizer settings SystemResource->>SecurityManager: Reload security system SystemResource-->>UI: Updated configuration else Validation fails SystemResource-->>UI: 400 validation response end Admin->>UI: Restore previous configuration UI->>SystemResource: POST /system/security/config/revert SystemResource->>SecurityManager: Read previous configuration SystemResource->>SettingsDB: Persist previous settings SystemResource->>SecurityManager: Reload security system SystemResource-->>UI: Restored configurationReviews (4): Last reviewed commit: "Merge branch 'main' into oidc-prompt-pro..." | Re-trigger Greptile
Context used (3)