Add support for dynamic Dashboards URL via cluster settings

[praveenvenkat06]

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:

1. Dynamic cluster setting (plugins.security.dashboards.dashboards_url) — if set and non-empty
2. 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.

[github-actions]

PR Code Analyzer ❗

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

Path	Line	Severity	Description
src/main/java/org/opensearch/security/auth/http/saml/Saml2SettingsProvider.java	183	medium	The SAML SP Assertion Consumer Service URL is constructed from a dynamically controllable cluster setting (SECURITY_DASHBOARDS_URL). A malicious cluster admin can redirect SAML assertions to an attacker-controlled endpoint by updating this setting at runtime, enabling SAML token hijacking without touching the security index.
src/main/java/org/opensearch/security/auth/http/saml/AuthTokenProcessorHandler.java	410	medium	The ACS endpoint resolution in getAbsoluteAcsEndpoint() uses the dynamically-supplied dashboards URL as a base URI. If the cluster setting is updated to an attacker-controlled host, relative ACS endpoints will be resolved against it, redirecting SAML responses to that host — a form of SSRF/open-redirect in the SAML flow.
src/main/java/org/opensearch/security/auth/http/saml/HTTPSamlAuthenticator.java	181	low	getDashboardsUrl() logs the full URL (including any embedded credentials in the form http://user:pass@host) at debug level. If the Dashboards URL ever contains credentials, they would be written to logs. The setting is marked Sensitive in DashboardsUrlSetting but that protection does not extend to these debug log statements.

The table above displays the top 10 most important findings.

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

---

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]

PR Reviewer Guide 🔍

(Review updated until commit 33b24e7)

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 Possible Race Condition In getCached(), dashboardsUrlSupplier.get() is invoked twice: once inside isDashboardsUrlChanged() and again when caching the new value in lastUsedDashboardsUrl. If the dynamic setting changes between these two calls, the cache will store a URL that differs from the one actually used to build cachedSaml2Settings, causing subsequent change detection to miss the update or trigger an unnecessary rebuild. Capture the value once and reuse it for both the comparison and the cache update. 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; } Thread Safety getCached() mutates cachedSaml2Settings, metadataUpdateTime, and now lastUsedDashboardsUrl without synchronization. Previously only metadata refresh could invalidate the cache; now every request may invalidate it based on an external dynamic setting. Concurrent SAML authentication requests could observe partially-updated state (e.g., cache cleared but not repopulated, or lastUsedDashboardsUrl updated before cachedSaml2Settings), producing settings inconsistent with the current dashboards URL. 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; } Global Mutable State GuiceHolder.setDashboardsUrlSetting exposes a mutable static field intended for cross-component wiring, but it is publicly settable (as demonstrated by tests calling it with null). Any caller may overwrite or clear the reference at runtime, breaking SAML authentication silently since getDashboardsUrl() will fall back to kibanaRootUrl without error. Consider restricting visibility or making the setter one-shot/package-private. public static OpensearchDynamicSetting<String> getDashboardsUrlSetting() { return dashboardsUrlSetting; } public static void setDashboardsUrlSetting(OpensearchDynamicSetting<String> setting) { dashboardsUrlSetting = setting; }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 33b24e7

Explore these optional code suggestions:

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

---

Previous suggestions

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
	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
	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
	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
	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

Suggestions up to commit f19e2d8

Category	Suggestion	Impact
Possible issue	Detect all URL change transitions The change-detection condition skips the case where lastUsedDashboardsUrl is null but currentDashboardsUrl is now non-null (e.g., the dynamic setting was set after initial cache with no kibana_url). Use Objects.equals to correctly detect any transition, including null-to-value. src/main/java/org/opensearch/security/auth/http/saml/Saml2SettingsProvider.java [150-158] // 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)) { log.debug( "Dashboards URL has changed from '{}' to '{}'; SAML settings cache will be refreshed", this.lastUsedDashboardsUrl, currentDashboardsUrl ); return true; } Suggestion importance[1-10]: 7 __ Why: Correct observation: the current condition misses null-to-non-null transitions, which is a realistic scenario when the dynamic setting is applied after startup. Using Objects.equals fixes this properly.	Medium
	Guard against null Dashboards URL If the dynamic setting is cleared and kibana_url was never set, dashboardsUrlSupplier.get() may return null, causing a NullPointerException in new URI(null) rather than a clear configuration error. Validate the value and throw an informative exception (or return acsEndpoint unchanged) when it is null/empty. src/main/java/org/opensearch/security/auth/http/saml/AuthTokenProcessorHandler.java [404-409] if (acsEndpointUri.isAbsolute()) { return acsEndpoint; } else { String dashboardsUrl = dashboardsUrlSupplier.get(); + if (dashboardsUrl == null || dashboardsUrl.trim().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 defensive check; if both dynamic setting and kibana_url are unset, new URI(null) would throw NPE. The fix improves robustness and error clarity, though it's an edge case.	Low
General	Record URL after cache is built Capturing currentDashboardsUrl before calling this.get() risks recording a stale value if the setting changes during the SAML settings build, causing the cache to appear valid on the next check when it isn't. Capture the URL after the successful build, or capture it once and pass the same value into get() to ensure consistency. src/main/java/org/opensearch/security/auth/http/saml/Saml2SettingsProvider.java [128-133] if (this.cachedSaml2Settings == null) { - String currentDashboardsUrl = this.dashboardsUrlSupplier.get(); this.cachedSaml2Settings = this.get(); this.metadataUpdateTime = tempLastUpdate; - this.lastUsedDashboardsUrl = currentDashboardsUrl; + this.lastUsedDashboardsUrl = this.dashboardsUrlSupplier.get(); } Suggestion importance[1-10]: 5 __ Why: Valid race-condition concern about capturing the URL before building settings; the reordering ensures consistency between cached settings and recorded URL, though impact is minor in practice.	Low
	Ensure thread-safe static setting access GuiceHolder fields are static, so setDashboardsUrlSetting mutates shared state without synchronization while getDashboardsUrlSetting may be called concurrently from authenticator threads. Make the field volatile (or use an AtomicReference) to ensure visibility across threads. src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java [1214-1217] +dashboardsUrlSetting.registerClusterSettingsChangeListener(clusterService.getClusterSettings()); +// Make dashboardsUrlSetting available through GuiceHolder for SAML authenticators +GuiceHolder.setDashboardsUrlSetting(dashboardsUrlSetting); Suggestion importance[1-10]: 4 __ Why: Reasonable concern about visibility of static mutable state across threads, but the improved_code is identical to existing_code, so the suggestion doesn't actually apply the recommended fix.	Low

[github-actions]

Persistent review updated to latest commit fa756db

[github-actions]

Persistent review updated to latest commit 410b3dd

[github-actions]

Persistent review updated to latest commit 008a6b7

[codecov]

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.

[github-actions]

Persistent review updated to latest commit afbf218

[github-actions]

Persistent review updated to latest commit 33b24e7