Add support for dynamic Dashboards URL via cluster settings by praveenvenkat06 · Pull Request #6275 · opensearch-project/security

praveenvenkat06 · 2026-07-01T10:52:12Z

Description

Enhancement — Introduces a new dynamic cluster setting plugins.security.dashboards.dashboards_url that allows operators to update the OpenSearch Dashboards URL used in the SAML authentication flow at runtime, without modifying the security index or reloading configuration.

Old behavior: The Dashboards URL (kibana_url) was read once from the SAML authenticator's security-index configuration and required a config reload to change.

New behavior: The URL is resolved dynamically at every call site using the following precedence:

Dynamic cluster setting (plugins.security.dashboards.dashboards_url) — if set and non-empty
Fallback to kibana_url in the security configuration

The SAML settings cache (Saml2SettingsProvider) is automatically invalidated when the setting value changes, so updates take effect immediately with no restart required.

Issues Resolved

[6015]

Testing

Manual testing is done and dynamic Dashboards URL changes are reflected at runtime. Unit tests have been added for the implementation.

Check List

New functionality includes testing
New functionality has been documented - Will be done once the PR gets approved
New Roles/Permissions have a corresponding security dashboards plugin PR
API changes companion pull request created - Will be done once the PR gets approved
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Praveen Raj V <praveenvenkat06@gmail.com>

github-actions · 2026-07-01T10:53:02Z

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit af99002.

⛔ Hard block: Issues at High severity or above will block this PR from merging.

Path	Line	Severity	Description
src/main/java/org/opensearch/security/setting/DashboardsUrlSetting.java	42	medium	The cluster change message explicitly logs the full URL value at what is likely an INFO-level log, yet the setting is declared with Setting.Property.Sensitive (which suppresses it from the cluster-settings API). If the URL contains embedded credentials (basic-auth form: http://user:pass@host), they will appear in server logs in plaintext, creating a credential-exposure inconsistency with the Sensitive annotation.
src/main/java/org/opensearch/security/auth/http/saml/Saml2SettingsProvider.java	162	low	isDashboardsUrlChanged() logs both the previous and current dashboards URL at DEBUG level. If the URL contains embedded credentials, each URL rotation would write old and new credentials to the debug log. Low risk in practice (DEBUG is usually off in production), but inconsistent with the Sensitive property intent.
src/main/java/org/opensearch/security/setting/DashboardsUrlSetting.java	28	low	The dynamic cluster setting accepts any arbitrary string with no URL format validation. A cluster-admin could set a non-HTTP scheme (e.g., file://, javascript:) that would be silently accepted and later used in URI resolution inside getAbsoluteAcsEndpoint, potentially causing unexpected SAML ACS endpoint behavior. No malicious intent observed; this is a defensive-validation gap.

The table above displays the top 10 most important findings.

Total: 3 | Critical: 0 | High: 0 | Medium: 1 | Low: 2

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

github-actions · 2026-07-01T10:53:47Z

PR Reviewer Guide 🔍

(Review updated until commit `af99002`)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review Thread Safety `getCached()` reads and writes `cachedSaml2Settings`, `metadataUpdateTime`, and `lastUsedDashboardsUrl` without synchronization. Under concurrent SAML authentication requests, one thread could observe a partially updated state (e.g., `cachedSaml2Settings` populated but `lastUsedDashboardsUrl` still stale), causing subsequent `isDashboardsUrlChanged()` checks to incorrectly invalidate or not invalidate the cache. Additionally, `dashboardsUrlSupplier.get()` is called twice in `getCached()` (once in `isDashboardsUrlChanged()` and once when setting `lastUsedDashboardsUrl`), so a change between those two calls could leave the cache tied to a URL value different from what was actually used to build settings. Saml2Settings getCached() throws SamlConfigException { Instant tempLastUpdate = null; if (this.metadataResolver instanceof RefreshableMetadataResolver && this.isUpdateRequired()) { this.cachedSaml2Settings = null; tempLastUpdate = ((RefreshableMetadataResolver) this.metadataResolver).getLastUpdate(); } if (this.isDashboardsUrlChanged()) { this.cachedSaml2Settings = null; } if (this.cachedSaml2Settings == null) { String currentDashboardsUrl = this.dashboardsUrlSupplier.get(); this.cachedSaml2Settings = this.get(); this.metadataUpdateTime = tempLastUpdate; this.lastUsedDashboardsUrl = currentDashboardsUrl; } return this.cachedSaml2Settings; } Static Mutable State `GuiceHolder.dashboardsUrlSetting` is a static field set via a public setter, which is fragile: tests set it to `null` in `finally` blocks, but if multiple `HTTPSamlAuthenticator` instances exist across nodes/tests concurrently, they all share this single static reference. This also breaks the pattern of proper dependency injection used elsewhere and can leak state between tests when a test forgets to reset it. private static volatile OpensearchDynamicSetting<String> dashboardsUrlSetting; private static ExtensionsManager extensionsManager; @Inject public GuiceHolder( final RepositoriesService repositoriesService, final TransportService remoteClusterService, IndicesService indicesService, PitService pitService, ExtensionsManager extensionsManager, BackendRegistry backendRegistry, AuditLogImpl auditLog ) { GuiceHolder.repositoriesService = repositoriesService; GuiceHolder.remoteClusterService = remoteClusterService.getRemoteClusterService(); GuiceHolder.indicesService = indicesService; GuiceHolder.pitService = pitService; GuiceHolder.extensionsManager = extensionsManager; GuiceHolder.backendRegistry = backendRegistry; GuiceHolder.auditLog = auditLog; } public static RepositoriesService getRepositoriesService() { return repositoriesService; } public static RemoteClusterService getRemoteClusterService() { return remoteClusterService; } public static IndicesService getIndicesService() { return indicesService; } public static PitService getPitService() { return pitService; } public static ExtensionsManager getExtensionsManager() { return extensionsManager; } public static BackendRegistry getBackendRegistry() { return backendRegistry; } public static AuditLog getAuditLog() { return auditLog; } public static OpensearchDynamicSetting<String> getDashboardsUrlSetting() { return dashboardsUrlSetting; } public static void setDashboardsUrlSetting(OpensearchDynamicSetting<String> setting) { dashboardsUrlSetting = setting; } Behavior Change on Missing URL When `dashboardsUrl` is null/empty for a relative acsEndpoint, the code now returns the relative `acsEndpoint` string instead of failing. Previously, `new URI(null).resolve(...)` would throw NPE, aborting the flow. Silently returning a relative URL may cause downstream SAML validation to accept a malformed ACS URL or produce confusing errors. Consider throwing an explicit exception instead. private String getAbsoluteAcsEndpoint(String acsEndpoint) { try { URI acsEndpointUri = new URI(acsEndpoint); if (acsEndpointUri.isAbsolute()) { return acsEndpoint; } else { String dashboardsUrl = dashboardsUrlSupplier.get(); if (dashboardsUrl == null \|\| dashboardsUrl.isEmpty()) { log.error("Dashboards URL is not configured; cannot resolve relative acsEndpoint: {}", acsEndpoint); return acsEndpoint; } return new URI(dashboardsUrl).resolve(acsEndpointUri).toString(); } } catch (URISyntaxException e) { log.error("Could not parse URI for acsEndpoint: {}", acsEndpoint); return acsEndpoint;

github-actions · 2026-07-01T10:54:17Z

PR Code Suggestions ✨

Latest suggestions up to af99002

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Avoid clobbering metadata update timestamp When only the Dashboards URL changes (metadata not stale), `tempLastUpdate` is null, so `metadataUpdateTime` will be overwritten with null. This will cause the next `isUpdateRequired()` call to throw an NPE on `metadataUpdateTime.isAfter(...)` if the metadata resolver is refreshable. Only update `metadataUpdateTime` when `tempLastUpdate` is non-null. src/main/java/org/opensearch/security/auth/http/saml/Saml2SettingsProvider.java [129-138] if (this.isDashboardsUrlChanged()) { this.cachedSaml2Settings = null; } if (this.cachedSaml2Settings == null) { String currentDashboardsUrl = this.dashboardsUrlSupplier.get(); this.cachedSaml2Settings = this.get(); - this.metadataUpdateTime = tempLastUpdate; + if (tempLastUpdate != null) { + this.metadataUpdateTime = tempLastUpdate; + } this.lastUsedDashboardsUrl = currentDashboardsUrl; } Suggestion importance[1-10]: 8 __ Why: Valid concern: when only the Dashboards URL changes, `tempLastUpdate` remains null and would overwrite `metadataUpdateTime`, potentially causing an NPE in `isUpdateRequired()`. This is a real bug fix.	Medium
General	Fail fast when Dashboards URL missing Returning the original relative `acsEndpoint` when the Dashboards URL is unavailable silently yields an invalid absolute URL downstream, which can produce misleading errors or an incorrect JWT audience/redirect. It would be safer to throw a clear exception here so the failure mode is explicit. src/main/java/org/opensearch/security/auth/http/saml/AuthTokenProcessorHandler.java [407-412] String dashboardsUrl = dashboardsUrlSupplier.get(); if (dashboardsUrl == null \|\| dashboardsUrl.isEmpty()) { - log.error("Dashboards URL is not configured; cannot resolve relative acsEndpoint: {}", acsEndpoint); - return acsEndpoint; + throw new RuntimeException( + "Dashboards URL is not configured; cannot resolve relative acsEndpoint: " + acsEndpoint + ); } return new URI(dashboardsUrl).resolve(acsEndpointUri).toString(); Suggestion importance[1-10]: 4 __ Why: The suggestion has merit as returning a relative URL silently could cause downstream confusion, but the current implementation already logs an error, and throwing a RuntimeException may be too aggressive without broader context.	Low
General	Restrict mutable global setter visibility Exposing a mutable static setter via `GuiceHolder` creates a global mutable singleton that can be modified from anywhere (including tests), leading to leakage across test runs and hard-to-debug state issues. Consider passing the `dashboardsUrlSetting` reference into the authenticator/provider directly during construction (e.g., via the existing DI/configuration path), or at least restrict the setter to package/test visibility. src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java [3030-3036] public static OpensearchDynamicSetting<String> getDashboardsUrlSetting() { return dashboardsUrlSetting; } -public static void setDashboardsUrlSetting(OpensearchDynamicSetting<String> setting) { +static void setDashboardsUrlSetting(OpensearchDynamicSetting<String> setting) { dashboardsUrlSetting = setting; } Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid design concern about mutable global state, but restricting to package-private may break existing usage patterns and is only a minor stylistic improvement.	Low

Previous suggestions

Suggestions up to commit 33b24e7

Category	Suggestion	Impact
Possible issue	Avoid race by reading supplier once `dashboardsUrlSupplier.get()` is invoked twice (once in `isDashboardsUrlChanged()` and again here) and its value may change between the calls, causing a race where `lastUsedDashboardsUrl` is set to a value different from the one actually used by `this.get()`. Capture the value once and pass it consistently, or read it before `get()` and reuse. src/main/java/org/opensearch/security/auth/http/saml/Saml2SettingsProvider.java [129-138] -if (this.isDashboardsUrlChanged()) { +String currentDashboardsUrl = this.dashboardsUrlSupplier.get(); +if (!Objects.equals(this.lastUsedDashboardsUrl, currentDashboardsUrl)) { this.cachedSaml2Settings = null; } if (this.cachedSaml2Settings == null) { - String currentDashboardsUrl = this.dashboardsUrlSupplier.get(); this.cachedSaml2Settings = this.get(); this.metadataUpdateTime = tempLastUpdate; this.lastUsedDashboardsUrl = currentDashboardsUrl; } Suggestion importance[1-10]: 7 __ Why: Valid concern: calling the supplier twice could lead to inconsistent state between `lastUsedDashboardsUrl` and the actual URL used in `get()`. Capturing the value once is a meaningful correctness improvement.	Medium
General	Remove Sensitive property from URL setting Marking the setting as `Sensitive` will cause it to be filtered from settings APIs and logs, but a Dashboards URL is not sensitive information and users may reasonably expect to view/verify it via `GET _cluster/settings`. Consider removing `Property.Sensitive` unless intentional. src/main/java/org/opensearch/security/setting/DashboardsUrlSetting.java [31-33] private static Setting<String> getSetting() { - return Setting.simpleString(SETTING, Setting.Property.NodeScope, Setting.Property.Dynamic, Setting.Property.Sensitive); + return Setting.simpleString(SETTING, Setting.Property.NodeScope, Setting.Property.Dynamic); } Suggestion importance[1-10]: 5 __ Why: A Dashboards URL is generally not sensitive; marking it as Sensitive would filter it from APIs and hinder operator visibility. This is a reasonable design consideration though possibly intentional.	Low
	Fail fast on missing Dashboards URL Returning a relative `acsEndpoint` as an absolute endpoint when the Dashboards URL is missing will produce an invalid SAML AssertionConsumerService URL and can cause downstream authentication failures that are hard to diagnose. Prefer throwing an explicit exception so the misconfiguration surfaces clearly rather than silently returning an invalid value. src/main/java/org/opensearch/security/auth/http/saml/AuthTokenProcessorHandler.java [407-412] String dashboardsUrl = dashboardsUrlSupplier.get(); if (dashboardsUrl == null \|\| dashboardsUrl.isEmpty()) { - log.error("Dashboards URL is not configured; cannot resolve relative acsEndpoint: {}", acsEndpoint); - return acsEndpoint; + throw new RuntimeException( + "Dashboards URL is not configured; cannot resolve relative acsEndpoint: " + acsEndpoint + ); } return new URI(dashboardsUrl).resolve(acsEndpointUri).toString(); Suggestion importance[1-10]: 5 __ Why: Failing fast on misconfiguration provides better diagnostic feedback than silently returning an invalid endpoint, but this is a debatable design choice and the current behavior logs an error.	Low
	Clear static setting reference on close The static `dashboardsUrlSetting` field in `GuiceHolder` is mutable global state that will leak across tests and, in production, could retain stale references if the plugin is re-initialized. Consider clearing it on `close()` to avoid stale state and ensure test isolation is not relied upon by resetting to null in the setter's teardown paths. src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java [2599] private static volatile OpensearchDynamicSetting<String> dashboardsUrlSetting; +@Override +public void close() { + dashboardsUrlSetting = null; +} + Suggestion importance[1-10]: 4 __ Why: Clearing static state on close is a reasonable hygiene improvement for test isolation and to avoid stale references, but its impact is limited since the plugin lifecycle typically doesn't reinitialize in production.	Low

Suggestions up to commit afbf218

Category	Suggestion	Impact
Possible issue	Capture URL once to avoid race The `dashboardsUrlSupplier.get()` is invoked twice (once in `isDashboardsUrlChanged()` and again here) and the value may change between calls, causing `lastUsedDashboardsUrl` to be stored with a value different from the one actually used to build the settings. Capture the URL once before the change check and reuse it to keep the cached settings and `lastUsedDashboardsUrl` consistent. src/main/java/org/opensearch/security/auth/http/saml/Saml2SettingsProvider.java [129-138] -if (this.isDashboardsUrlChanged()) { +String currentDashboardsUrl = this.dashboardsUrlSupplier.get(); +if (!Objects.equals(this.lastUsedDashboardsUrl, currentDashboardsUrl)) { + log.debug( + "Dashboards URL has changed from '{}' to '{}'; SAML settings cache will be refreshed", + this.lastUsedDashboardsUrl, + currentDashboardsUrl + ); this.cachedSaml2Settings = null; } if (this.cachedSaml2Settings == null) { - String currentDashboardsUrl = this.dashboardsUrlSupplier.get(); this.cachedSaml2Settings = this.get(); this.metadataUpdateTime = tempLastUpdate; this.lastUsedDashboardsUrl = currentDashboardsUrl; } Suggestion importance[1-10]: 7 __ Why: Valid concern: `dashboardsUrlSupplier.get()` is called twice and could return different values, leading to inconsistent state between the cached settings and `lastUsedDashboardsUrl`. The refactor improves correctness.	Medium
General	Fail fast on missing Dashboards URL Returning the raw relative `acsEndpoint` when the Dashboards URL is missing will silently produce an invalid absolute ACS URL used downstream in SAML request construction, likely leading to broken redirects rather than a clear failure. Prefer throwing a clear configuration exception so the misconfiguration surfaces immediately. src/main/java/org/opensearch/security/auth/http/saml/AuthTokenProcessorHandler.java [407-412] String dashboardsUrl = dashboardsUrlSupplier.get(); if (dashboardsUrl == null \|\| dashboardsUrl.isEmpty()) { log.error("Dashboards URL is not configured; cannot resolve relative acsEndpoint: {}", acsEndpoint); - return acsEndpoint; + throw new RuntimeException("Dashboards URL is not configured; cannot resolve relative acsEndpoint: " + acsEndpoint); } return new URI(dashboardsUrl).resolve(acsEndpointUri).toString(); Suggestion importance[1-10]: 5 __ Why: Valid point that silently returning the relative endpoint may cause obscure downstream failures, though the behavior change could also break flows that previously tolerated missing URLs. Moderate impact.	Low
General	Restrict visibility of static mutator Exposing a public static setter for `dashboardsUrlSetting` allows arbitrary callers (or leaked test state) to overwrite plugin-wide state. Consider restricting the setter to package-private or `@VisibleForTesting`, so production code paths cannot mutate the singleton after initialization. src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java [2650-2656] public static OpensearchDynamicSetting<String> getDashboardsUrlSetting() { return dashboardsUrlSetting; } -public static void setDashboardsUrlSetting(OpensearchDynamicSetting<String> setting) { +@VisibleForTesting +static void setDashboardsUrlSetting(OpensearchDynamicSetting<String> setting) { dashboardsUrlSetting = setting; } Suggestion importance[1-10]: 4 __ Why: Reasonable encapsulation improvement, but the setter is called from `createComponents` in the same class, so restricting it to package-private is feasible but has minor impact.	Low

Suggestions up to commit 008a6b7

Category	Suggestion	Impact
Possible issue	Avoid race in URL change detection There is a race condition between reading `currentDashboardsUrl` for comparison in `isDashboardsUrlChanged()` and re-reading it again after `this.get()` executes. If the supplier value changes between these calls, `lastUsedDashboardsUrl` may be updated to a value that does not match what was actually used to build `cachedSaml2Settings`, leading to stale settings not being detected on the next call. Capture the value once and reuse it. src/main/java/org/opensearch/security/auth/http/saml/Saml2SettingsProvider.java [129-138] - if (this.isDashboardsUrlChanged()) { + String currentDashboardsUrl = this.dashboardsUrlSupplier.get(); + if (!Objects.equals(this.lastUsedDashboardsUrl, currentDashboardsUrl)) { + log.debug( + "Dashboards URL has changed from '{}' to '{}'; SAML settings cache will be refreshed", + this.lastUsedDashboardsUrl, + currentDashboardsUrl + ); this.cachedSaml2Settings = null; } if (this.cachedSaml2Settings == null) { - String currentDashboardsUrl = this.dashboardsUrlSupplier.get(); this.cachedSaml2Settings = this.get(); this.metadataUpdateTime = tempLastUpdate; this.lastUsedDashboardsUrl = currentDashboardsUrl; } Suggestion importance[1-10]: 6 __ Why: Valid concern about a potential race condition where the supplier value could change between the `isDashboardsUrlChanged()` check and the re-read after `this.get()`, potentially causing stale settings to be undetected. Impact is minor since dashboards URL changes are rare.	Low
Possible issue	Guard against null Dashboards URL If `dashboardsUrlSupplier.get()` returns null (e.g., cluster setting cleared and legacy config also missing), `new URI(null)` will throw `NullPointerException` rather than the handled `URISyntaxException`, breaking the fallback flow. Add an explicit null/empty check and log or fall back gracefully. src/main/java/org/opensearch/security/auth/http/saml/AuthTokenProcessorHandler.java [407-408] - return new URI(this.kibanaRootUrl).resolve(acsEndpointUri).toString(); - } else { String dashboardsUrl = dashboardsUrlSupplier.get(); + if (dashboardsUrl == null \|\| dashboardsUrl.isEmpty()) { + log.error("Dashboards URL is not configured; cannot resolve relative ACS endpoint: {}", acsEndpoint); + return acsEndpoint; + } return new URI(dashboardsUrl).resolve(acsEndpointUri).toString(); - } Suggestion importance[1-10]: 6 __ Why: Correctly identifies that `new URI(null)` throws NPE rather than the caught URISyntaxException, which could break error handling. Adding a null check improves robustness of the fallback flow.	Low
General	Avoid leaking static mutable global state Using a static mutable holder to bridge plugin state into authenticators creates hidden global state that leaks across test cases and node restarts. If a plugin instance is re-created (e.g., in tests or reloads), the previous instance's setting reference remains, potentially serving stale values. Consider passing the setting/supplier through authenticator construction, or at least clearing it during plugin shutdown/close. src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java [2650-2656] public static OpensearchDynamicSetting<String> getDashboardsUrlSetting() { return dashboardsUrlSetting; } public static void setDashboardsUrlSetting(OpensearchDynamicSetting<String> setting) { dashboardsUrlSetting = setting; } + public static void clearDashboardsUrlSetting() { + dashboardsUrlSetting = null; + } + Suggestion importance[1-10]: 3 __ Why: Valid design observation about static mutable state, but the `improved_code` merely adds a `clearDashboardsUrlSetting()` method without changing the fundamental design; it's a marginal improvement.	Low

Suggestions up to commit 410b3dd

Category	Suggestion	Impact
Possible issue	Guard against null Dashboards URL If `dashboardsUrlSupplier.get()` returns null (e.g., neither the dynamic setting nor `kibana_url` is configured), `new URI(null)` will throw a `NullPointerException` which is not caught by the `URISyntaxException` handler below. Validate the supplier result and log/return a meaningful error to avoid unexpected NPE at runtime. src/main/java/org/opensearch/security/auth/http/saml/AuthTokenProcessorHandler.java [407-408] String dashboardsUrl = dashboardsUrlSupplier.get(); +if (dashboardsUrl == null \|\| dashboardsUrl.isEmpty()) { + log.error("Dashboards URL is not configured; cannot resolve relative acsEndpoint: {}", acsEndpoint); + return acsEndpoint; +} return new URI(dashboardsUrl).resolve(acsEndpointUri).toString(); Suggestion importance[1-10]: 6 __ Why: Valid concern: if both dynamic setting and `kibana_url` are unset, `new URI(null)` would throw an NPE not caught by `URISyntaxException`. The suggestion adds a reasonable null/empty guard.	Low
General	Fix concurrency in cache invalidation `getCached()` is not synchronized, but `cachedSaml2Settings`, `metadataUpdateTime`, and `lastUsedDashboardsUrl` can be accessed concurrently by multiple request threads. Concurrent invalidation and re-population may cause inconsistent state (e.g., stale `lastUsedDashboardsUrl` paired with new settings). Synchronize the method or make the fields volatile with proper atomic update semantics. src/main/java/org/opensearch/security/auth/http/saml/Saml2SettingsProvider.java [129-137] -if (this.isDashboardsUrlChanged()) { - this.cachedSaml2Settings = null; +synchronized (this) { + if (this.isDashboardsUrlChanged()) { + this.cachedSaml2Settings = null; + } + + if (this.cachedSaml2Settings == null) { + String currentDashboardsUrl = this.dashboardsUrlSupplier.get(); + this.cachedSaml2Settings = this.get(); + this.metadataUpdateTime = tempLastUpdate; + this.lastUsedDashboardsUrl = currentDashboardsUrl; + } } -if (this.cachedSaml2Settings == null) { - String currentDashboardsUrl = this.dashboardsUrlSupplier.get(); - this.cachedSaml2Settings = this.get(); - this.metadataUpdateTime = tempLastUpdate; - this.lastUsedDashboardsUrl = currentDashboardsUrl; -} - Suggestion importance[1-10]: 5 __ Why: Concurrency concern is valid since `getCached()` can be invoked concurrently and there's a potential for inconsistent state between `cachedSaml2Settings` and `lastUsedDashboardsUrl`. However, similar concurrency issues existed prior for `metadataUpdateTime`, so impact is moderate.	Low
General	Avoid static holder for dynamic setting Storing the dynamic setting in a static holder introduces cross-test/state leakage (tests must manually reset it in `finally` blocks) and prevents proper isolation when multiple plugin instances exist. Consider injecting the setting into `HTTPSamlAuthenticator` via a proper mechanism (e.g., constructor or a dedicated service) rather than a static Guice holder. src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java [2599] +private static volatile OpensearchDynamicSetting<String> dashboardsUrlSetting; - Suggestion importance[1-10]: 3 __ Why: The design concern about static state is legitimate but the suggestion doesn't provide a concrete improved code change (existing and improved code are identical), making it more of a design comment than an actionable suggestion.	Low

Suggestions up to commit fa756db

Category	Suggestion	Impact
Possible issue	Detect null-to-value URL change correctly The Dashboards URL change detection skips the case where `lastUsedDashboardsUrl` is null but `currentDashboardsUrl` is non-null (i.e., URL was set after cache was populated with null). Use `java.util.Objects.equals` to correctly detect all changes including null↔non-null transitions. src/main/java/org/opensearch/security/auth/http/saml/Saml2SettingsProvider.java [152-158] if (refreshableMetadataResolver.getLastUpdate().isAfter(this.metadataUpdateTime)) { log.debug("IdP metadata has been updated; SAML settings cache will be refreshed"); return true; } // Check if Dashboards URL has changed String currentDashboardsUrl = this.dashboardsUrlSupplier.get(); -if (this.lastUsedDashboardsUrl != null && !this.lastUsedDashboardsUrl.equals(currentDashboardsUrl)) { +if (!java.util.Objects.equals(this.lastUsedDashboardsUrl, currentDashboardsUrl)) { Suggestion importance[1-10]: 7 __ Why: Valid observation: the current check `this.lastUsedDashboardsUrl != null && !this.lastUsedDashboardsUrl.equals(currentDashboardsUrl)` misses the case where `lastUsedDashboardsUrl` is null but `currentDashboardsUrl` becomes non-null, so a runtime update from unset→set would not invalidate the cache. Using `Objects.equals` correctly handles all transitions.	Medium
Possible issue	Invalidate cache on URL change for non-refreshable resolvers When `metadataResolver` is not a `RefreshableMetadataResolver`, `isUpdateRequired` is never called, so cache is never invalidated when the Dashboards URL changes. Ensure the URL-change check runs regardless of resolver type so dynamic URL updates take effect. src/main/java/org/opensearch/security/auth/http/saml/Saml2SettingsProvider.java [120-135] Saml2Settings getCached() throws SamlConfigException { Instant tempLastUpdate = null; if (this.metadataResolver instanceof RefreshableMetadataResolver && this.isUpdateRequired()) { this.cachedSaml2Settings = null; tempLastUpdate = ((RefreshableMetadataResolver) this.metadataResolver).getLastUpdate(); + } else if (this.cachedSaml2Settings != null + && !java.util.Objects.equals(this.lastUsedDashboardsUrl, this.dashboardsUrlSupplier.get())) { + this.cachedSaml2Settings = null; } if (this.cachedSaml2Settings == null) { String currentDashboardsUrl = this.dashboardsUrlSupplier.get(); this.cachedSaml2Settings = this.get(); this.metadataUpdateTime = tempLastUpdate; this.lastUsedDashboardsUrl = currentDashboardsUrl; } return this.cachedSaml2Settings; } Suggestion importance[1-10]: 7 __ Why: Correct edge case: when the resolver is not a `RefreshableMetadataResolver`, `isUpdateRequired` is never invoked and the URL-change detection is bypassed, so dynamic URL updates would not take effect. The suggestion appropriately extends invalidation to that path.	Medium
General	Make cross-thread static field volatile The `dashboardsUrlSetting` static field is read from authenticator threads while being set from `createComponents`, which introduces a data race. Mark the field `volatile` to ensure visibility across threads (matching common practice for these Guice-held statics). src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java [2599] private static PrivilegesConfiguration privilegesConfiguration; -private static OpensearchDynamicSetting<String> dashboardsUrlSetting; +private static volatile OpensearchDynamicSetting<String> dashboardsUrlSetting; Suggestion importance[1-10]: 5 __ Why: Reasonable concern about visibility of the static field across threads; adding `volatile` is a low-risk improvement, though the practical impact is limited since setting occurs once during startup.	Low

Signed-off-by: Praveen Raj V <praveenvenkat06@gmail.com>

github-actions · 2026-07-01T15:24:22Z

Persistent review updated to latest commit fa756db

Signed-off-by: Praveen Raj V <praveenvenkat06@gmail.com>

github-actions · 2026-07-01T16:49:34Z

Persistent review updated to latest commit 410b3dd

…ster-setting-support-for-dashboards_url

github-actions · 2026-07-01T16:57:23Z

Persistent review updated to latest commit 008a6b7

codecov · 2026-07-01T18:54:28Z

Codecov Report

❌ Patch coverage is 81.81818% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.01%. Comparing base (96b5429) to head (008a6b7).

Files with missing lines	Patch %	Lines
...nsearch/security/setting/DashboardsUrlSetting.java	57.14%	3 Missing ⚠️
...rity/auth/http/saml/AuthTokenProcessorHandler.java	66.66%	2 Missing ⚠️
...security/auth/http/saml/Saml2SettingsProvider.java	85.71%	2 Missing ⚠️
...security/auth/http/saml/HTTPSamlAuthenticator.java	90.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6275      +/-   ##
==========================================
- Coverage   75.02%   75.01%   -0.01%     
==========================================
  Files         451      452       +1     
  Lines       29248    29285      +37     
  Branches     4407     4411       +4     
==========================================
+ Hits        21942    21968      +26     
- Misses       5267     5279      +12     
+ Partials     2039     2038       -1

Files with missing lines	Coverage Δ
.../opensearch/security/OpenSearchSecurityPlugin.java	`85.23% <100.00%> (+0.12%)`	⬆️
...g/opensearch/security/support/ConfigConstants.java	`95.65% <ø> (ø)`
...security/auth/http/saml/HTTPSamlAuthenticator.java	`69.19% <90.00%> (+0.77%)`	⬆️
...rity/auth/http/saml/AuthTokenProcessorHandler.java	`51.44% <66.66%> (-0.32%)`	⬇️
...security/auth/http/saml/Saml2SettingsProvider.java	`62.50% <85.71%> (+2.31%)`	⬆️
...nsearch/security/setting/DashboardsUrlSetting.java	`57.14% <57.14%> (ø)`

... and 7 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Signed-off-by: Praveen Raj V <praveenvenkat06@gmail.com>

github-actions · 2026-07-05T17:25:24Z

Persistent review updated to latest commit afbf218

…dashboards_url

github-actions · 2026-07-08T05:12:01Z

Persistent review updated to latest commit 33b24e7

…dashboards_url

github-actions · 2026-08-26T19:19:48Z

Persistent review updated to latest commit af99002

praveenvenkat06 · 2026-09-03T06:29:25Z

Hi @cwperks, can this be merged?

Add support for dynamic Dashboards URL via cluster settings

f19e2d8

Signed-off-by: Praveen Raj V <praveenvenkat06@gmail.com>

praveenvenkat06 requested review from DarshitChanpura, Rishav9852Kumar, RyanL1997, cwperks, derek-ho, finnegancarroll, nibix, reta, shikharj05 and willyborankin as code owners July 1, 2026 10:52

praveenvenkat06 marked this pull request as draft July 1, 2026 10:53

JUnits for dynamic Dashboards URL implementation

fa756db

Signed-off-by: Praveen Raj V <praveenvenkat06@gmail.com>

Implement PR code suggestions

410b3dd

Signed-off-by: Praveen Raj V <praveenvenkat06@gmail.com>

Merge remote-tracking branch 'upstream/main' into feature/dynamic-clu…

008a6b7

…ster-setting-support-for-dashboards_url

praveenvenkat06 marked this pull request as ready for review July 1, 2026 16:55

Rishav9852Kumar reviewed Jul 2, 2026

View reviewed changes

Comment thread src/main/java/org/opensearch/security/auth/http/saml/AuthTokenProcessorHandler.java

Update AuthTokenProcessorHandler.java

afbf218

Signed-off-by: Praveen Raj V <praveenvenkat06@gmail.com>

praveenvenkat06 requested a review from Rishav9852Kumar July 6, 2026 04:46

Merge branch 'main' into feature/dynamic-cluster-setting-support-for-…

33b24e7

…dashboards_url

Merge branch 'main' into feature/dynamic-cluster-setting-support-for-…

af99002

…dashboards_url

Navigation Menu

Uh oh!

Add support for dynamic Dashboards URL via cluster settings - #6275

Add support for dynamic Dashboards URL via cluster settings#6275
praveenvenkat06 wants to merge 7 commits into
opensearch-project:mainfrom
praveenvenkat06:feature/dynamic-cluster-setting-support-for-dashboards_url

praveenvenkat06 commented Jul 1, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 1, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 1, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 1, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 1, 2026

Uh oh!

github-actions Bot commented Jul 1, 2026

Uh oh!

github-actions Bot commented Jul 1, 2026

Uh oh!

codecov Bot commented Jul 1, 2026

Uh oh!

Uh oh!

github-actions Bot commented Jul 5, 2026

Uh oh!

github-actions Bot commented Jul 8, 2026

Uh oh!

github-actions Bot commented Aug 26, 2026

Uh oh!

praveenvenkat06 commented Sep 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

praveenvenkat06 commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Testing

Check List

Uh oh!

github-actions Bot commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Analyzer ❗

Uh oh!

github-actions Bot commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit af99002)

Uh oh!

github-actions Bot commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

github-actions Bot commented Jul 1, 2026

Uh oh!

github-actions Bot commented Jul 1, 2026

Uh oh!

github-actions Bot commented Jul 1, 2026

Uh oh!

codecov Bot commented Jul 1, 2026

Codecov Report

Uh oh!

Uh oh!

github-actions Bot commented Jul 5, 2026

Uh oh!

github-actions Bot commented Jul 8, 2026

Uh oh!

github-actions Bot commented Aug 26, 2026

Uh oh!

praveenvenkat06 commented Sep 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

praveenvenkat06 commented Jul 1, 2026 •

edited

Loading

github-actions Bot commented Jul 1, 2026 •

edited

Loading

github-actions Bot commented Jul 1, 2026 •

edited

Loading

(Review updated until commit `af99002`)

github-actions Bot commented Jul 1, 2026 •

edited

Loading