Repository navigation
Fixes #34464: Accept upper/mixed-case Azure AD tenant GUIDs in SAML validation - #34522
detail-app[bot] wants to merge 1 commit into
Conversation
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
✅ Playwright Results — workflow succeededValidated commit ✅ 4685 passed · ❌ 0 failed · 🟡 1 flaky · ⏭️ 1 skipped · 🧰 0 lifecycle flaky PerformanceBlocking targets: ✅ met · Optimization targets: 🟡 in progress Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting. 🕒 Full workflow signal wall (to summary) 1h 55m 59s ⏱️ Max setup 4m 40s · max shard execution 19m 22s · max shard-job elapsed before upload 25m 4s · reporting 19s 🌐 222.53 requests/attempt · 2.20 app boots/UI scenario · 40.53% common-shard skew Optimization targets still in progress:
🟡 1 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
|
✅ SSO Login Nightly — PASSED View workflow run · Commit
Totals: 84 passed · 0 failed · 0 flaky · 0 skipped · duration 18m 24s |
|
Superseded by #35537, which combines this change with other queued fixes. |
🚦 Removed from the merge queue —
|
…, SAML tenant GUIDs, dead-code removals (open-metadata#35537) * Fixes open-metadata#35106: gate certification tag strip on supportsCertification in BundleWarmupBatcher (open-metadata#35167) * chore(searchIndex): remove unused ReindexingMetrics methods (open-metadata#35392) * chore: remove unused TeamsMessage Fact/Action model classes (open-metadata#35386) * chore: remove unused awaitVectorCompletionWithDetails and VectorCompletionResult (open-metadata#35389) * Fixes open-metadata#35078: authorize VIEW_BASIC on lineage edge-details read endpoints (open-metadata#35140) * Fixes open-metadata#34464: Accept upper/mixed-case Azure AD tenant GUIDs in SAML validation (open-metadata#34522) --------- Co-authored-by: mohitdeuex <mohit.y@deuexsolutions.com>
Detail bug report: View on Detail
Describe your changes:
Fixes #34464
I fixed a case-sensitivity defect in
SamlValidator.validateAzureAdTenantthat rejected valid Azure AD tenant GUIDs entered in upper- or mixed-case, because:[a-f0-9\\-]{36}admitted only lowercase hex digits, so a GUID likeA1B2C3D4-E5F6-7890-1234-567890ABCDEFfailed with "Invalid Azure AD tenant ID format" before any network check ran; andissuer.contains(tenantId)comparison was case-sensitive, and Azure's OpenID-config endpoint returns a lowercase issuer even for an uppercase path-segment GUID — so fixing the regex alone would convert the error into "Tenant ID mismatch".These are one logical defect (case-sensitive tenant-ID handling end-to-end). The config simply could not be saved for an uppercase/mixed-case tenant GUID; no security or data-integrity impact.
Type of change:
High-level design:
N/A — small change.
The fix normalizes the extracted tenant ID to lowercase once (
tenantId.toLowerCase(Locale.ROOT)) right after extraction, then uses the normalized value for both the GUID format check and the OpenID-config URL construction, and for theissuer.contains(...)comparison. The admin's original (pre-normalization) input is preserved in all error messages, so the admin still sees what they typed.Tests:
Use cases covered
https://sts.windows.net/A1B2C3D4-.../) → config saves successfully.Unit tests
openmetadata-service/src/test/java/org/openmetadata/service/security/auth/validator/SamlValidatorTest.java— addedvalidateSamlConfigurationAcceptsUppercaseAzureTenantGuid, which drives the publicvalidateSamlConfigurationentry point (the same pathSystemRepository.validateSecurityConfigurationcalls in production) with an uppercase entity-ID GUID, a mocked lowercase issuer, and a reachable in-process SSO server. It assertsnull(success). This test fails on the unfixed code withInvalid Azure AD tenant ID format: A1B2C3D4-...and would also fail if only the regex were loosened (the lowercase issuer would notcontainsthe uppercase tenant, producing "Tenant ID mismatch"), so it guards both halves of the defect.Backend integration tests
Ingestion integration tests
Playwright (UI) tests
Manual testing performed
curl https://login.microsoftonline.com/72F988BF-86F1-41AF-91AB-2D7CD011DB47/v2.0/.well-known/openid-configurationreturns HTTP 200 with anissuerwhose path GUID is the lower-cased form — identical to the lowercase-GUID response.SamlValidator.validateSamlConfiguration(no mocks) against the real public Microsoft Azure AD tenant72f988bf-86f1-41af-91ab-2d7cd011db47, once with the entity ID GUID in upper case and once in lower case. Both returnednull(success) with a real HTTPS roundtrip tologin.microsoftonline.com.docker/development/docker-compose.ymlfailed because Docker Hub returned429 Too Many Requestspullingmysql:8.0, and noopenmetadata/serverimage is cached locally; the Elasticsearch service (cached elastic image) started fine. The substitute live probe above exercises the exact production code path the server uses at config-save time, so the behavioral guarantee is covered.UI screen recording / screenshots:
Not applicable.
Checklist:
Fixes <issue-number>: <short explanation>Fixes #<issue-number>above.Automatic Fixes PRs can be configured here.