fix: show wizard and insightful error when SSO is unreachable (crw-11958)#342
fix: show wizard and insightful error when SSO is unreachable (crw-11958)#342adietish wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughOIDC metadata resolution now detects unreachable providers, converts those failures into localized SSO errors with Token authentication guidance, and preserves causes. Browser login reuses a resolver instance, with tests covering host extraction, exception classification, and failure mapping. ChangesOIDC unreachable-host handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RedHatAuthSessionManager
participant OidcProviderMetadataResolver
participant OIDCDiscovery
participant SsoLoginExceptionFailed
RedHatAuthSessionManager->>OidcProviderMetadataResolver: resolve()
OidcProviderMetadataResolver->>OIDCDiscovery: request provider metadata
OIDCDiscovery-->>OidcProviderMetadataResolver: unreachable failure
OidcProviderMetadataResolver->>SsoLoginExceptionFailed: create localized failure with cause
SsoLoginExceptionFailed-->>RedHatAuthSessionManager: throw login error
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #342 +/- ##
==========================================
+ Coverage 0.00% 32.80% +32.80%
==========================================
Files 4 111 +107
Lines 26 4588 +4562
Branches 0 872 +872
==========================================
+ Hits 0 1505 +1505
- Misses 26 2876 +2850
- Partials 0 207 +207 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/main/kotlin/com/redhat/devtools/gateway/auth/oidc/OidcProviderMetadataResolver.kt (1)
41-43: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd finite timeouts to the OIDC discovery request.
OIDCProviderConfigurationRequest.toHTTPRequest().send()does not set connect/read timeouts by default, so a non-responding provider can suspend login indefinitely. Set finiteconnectTimeoutandreadTimeouton the returnedHTTPRequest, for example 10s connect and a higher read timeout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/com/redhat/devtools/gateway/auth/oidc/OidcProviderMetadataResolver.kt` around lines 41 - 43, Update the OIDC discovery request in OidcProviderMetadataResolver by configuring finite connectTimeout and readTimeout values on the HTTPRequest returned by OIDCProviderConfigurationRequest.toHTTPRequest() before calling send(), using approximately 10 seconds for connection establishment and a higher finite duration for reading the response.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/kotlin/com/redhat/devtools/gateway/auth/oidc/OidcProviderMetadataResolver.kt`:
- Around line 67-74: Update isSsoUnreachable to classify NoRouteToHostException
as an unreachable SSO cause alongside the existing network exceptions, and add a
cause-chain test covering this exception so route failures receive the
Token-authentication SSO guidance.
In `@src/main/resources/messages/DevSpacesBundle.properties`:
- Line 28: Update the
connector.wizard_step.openshift_connection.error.sso_unreachable message to
escape the apostrophe in “you're” for MessageFormat compatibility, using the
existing wording and placeholder unchanged.
In
`@src/test/kotlin/com/redhat/devtools/gateway/auth/oidc/OidcProviderMetadataResolverTest.kt`:
- Around line 62-76: Make the `resolve maps unreachable host to
SsoLoginException with Token hint` test deterministic by replacing the external
`.invalid` host resolution with an injected request/transport failure or
controlled local endpoint configured with bounded timing. Preserve the existing
`SsoLoginException.Failed` assertions, including the message hints and
unreachable cause validation, without relying on real DNS or unbounded network
access.
---
Nitpick comments:
In
`@src/main/kotlin/com/redhat/devtools/gateway/auth/oidc/OidcProviderMetadataResolver.kt`:
- Around line 41-43: Update the OIDC discovery request in
OidcProviderMetadataResolver by configuring finite connectTimeout and
readTimeout values on the HTTPRequest returned by
OIDCProviderConfigurationRequest.toHTTPRequest() before calling send(), using
approximately 10 seconds for connection establishment and a higher finite
duration for reading the response.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a9d571e8-a78e-49d5-97c0-09d7782805c8
📒 Files selected for processing (4)
src/main/kotlin/com/redhat/devtools/gateway/auth/oidc/OidcProviderMetadataResolver.ktsrc/main/kotlin/com/redhat/devtools/gateway/auth/session/RedHatAuthSessionManager.ktsrc/main/resources/messages/DevSpacesBundle.propertiessrc/test/kotlin/com/redhat/devtools/gateway/auth/oidc/OidcProviderMetadataResolverTest.kt
| connector.wizard_step.openshift_connection.text.redhat_sso_info=Authenticate using Red Hat SSO (Sandbox only) | ||
| connector.wizard_step.openshift_connection.text.redhat_sso_token_note=Token will not be saved to kubeconfig | ||
| connector.wizard_step.openshift_connection.text.pipeline_token_comment=Pipeline tokens require special handling | ||
| connector.wizard_step.openshift_connection.error.sso_unreachable=Cannot reach SSO provider ({0}). If you're behind a corporate proxy or firewall, try using Token authentication instead. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 'MessageFormat|BundleBase|DevSpacesBundle' src/mainRepository: redhat-developer/devspaces-gateway-plugin
Length of output: 39850
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== DevSpacesBundle.kt =="
cat -n src/main/kotlin/com/redhat/devtools/gateway/DevSpacesBundle.kt
echo
echo "== target bundle line =="
cat -n src/main/resources/messages/DevSpacesBundle.properties | sed -n '20,32p'
echo
echo "== call sites involving sso_unreachable/message formatter deps =="
cat -n src/main/kotlin/com/redhat/devtools/gateway/auth/oidc/OidcProviderMetadataResolver.kt | sed -n '75,90p'
rg -n "class DynamicBundle|DynamicBundle|MessageFormat|ResourceBundle|message\\(" . --glob '*.{kt,java,groovy}'Repository: redhat-developer/devspaces-gateway-plugin
Length of output: 10602
🌐 Web query:
IntelliJ platform DynamicBundle getMessage String format MessageFormat apostrophe quote semantics
💡 Result:
In the IntelliJ Platform, DynamicBundle uses Java's MessageFormat for string substitution [1][2]. Because MessageFormat interprets certain characters as special syntax, you must adhere to specific escaping rules for apostrophes and single quotes in your resource bundle property files [1][3]. The fundamental rules are: 1. Escape Single Quotes: If your string contains a single quote (apostrophe) and is processed by MessageFormat (i.e., it contains placeholders like {0}), you must double the single quote to render it correctly [1][3]. For example, "doesn't" becomes "doesn''t" [1][3]. 2. When to Escape: Escaping is only required if the string is treated as a MessageFormat pattern [3]. IntelliJ's property file inspection tool typically identifies strings that contain placeholders (e.g., {0}) as MessageFormat patterns and will flag unescaped single quotes as potential errors [3]. Strings without placeholders do not require this escaping [1]. 3. Nested Patterns (ChoiceFormat): If your message includes a nested pattern, such as a ChoiceFormat, the escaping requirements increase [1][3]. Because the nested content is evaluated as a separate format string, single quotes may need to be doubled twice. For example, a single quote needed in the result of a choice variant would require four single quotes ('''') [1]. 4. Best Practice: A widely recommended practice to avoid these complexities in human-readable text is to use the typographic right single quotation mark (’) (U+2019) for actual apostrophes, reserving the standard ASCII apostrophe/single quote (') (U+0027) exclusively for MessageFormat syntax escaping [4]. IntelliJ provides IDE support, such as inspections in property files, to help identify and correct these MessageFormat pattern issues [1][3]. If you are working in Java/Kotlin code, you can rely on DynamicBundle's methods, which handle the underlying ResourceBundle and MessageFormat delegation automatically [1][5][6].
Citations:
- 1: https://plugins.jetbrains.com/docs/intellij/internationalization.html
- 2: https://www.plugin-dev.com/intellij/general/plugin-localization/
- 3: https://www.jetbrains.com/help/inspectopedia/DevKitPropertiesMessageValidation.html
- 4: https://unicode-org.github.io/icu/userguide/format_parse/messages/
- 5: https://github.com/JetBrains/intellij-community/blob/a103d3ac/platform/util/src/com/intellij/AbstractBundle.kt
- 6: https://github.com/JetBrains/intellij-community/blob/master/platform/core-api/src/com/intellij/DynamicBundle.java
🌐 Web query:
IntelliJ AbstractBundle DynamicBundle format pattern quote escaping apostrophe before placeholder MessageFormat
💡 Result:
When using DynamicBundle in the IntelliJ Platform, message formatting relies on standard java.text.MessageFormat, which uses single quotes (') as escape characters for both placeholders and literal text [1][2]. To correctly escape apostrophes or display single quotes in your message patterns, follow these rules: 1. Basic Escaping: To include a single literal apostrophe (') in a message that contains a placeholder, you must use two consecutive single quotes ('') [1][2]. For example, the string "File {0} doesn''t exist" will render as "File doesn't exist" [1]. 2. Plain Text: If a message does not contain any placeholders (no curly braces like {0}), MessageFormat is not invoked, and single quotes do not need to be escaped [1]. 3. Choice Formats (Nested Patterns): When using a choice format, the sub-message is treated as a new MessageFormat pattern. This introduces an additional layer of nesting, requiring you to double the existing escape sequences [1][2]. To display a single apostrophe inside a choice format, you must use four consecutive single quotes ('''') [1][2]. Example for choice format: {0,choice,1#One person doesn''t|2#{0} people don''''t} like MessageFormat [1] In this example: - The first variant uses '' to render a single apostrophe [1]. - The second variant uses '''' to render a single apostrophe because it is nested within the choice pattern [1][2]. When implementing, ensure you are using DynamicBundle as recommended (delegating to an instance rather than extending the class) to handle localization and resource resolution correctly [1][3][4]. The IntelliJ platform automatically validates these patterns and will report errors in properties files if quotes are unpaired or incorrectly escaped [2].
Citations:
- 1: https://plugins.jetbrains.com/docs/intellij/internationalization.html
- 2: https://www.jetbrains.com/help/inspectopedia/DevKitPropertiesMessageValidation.html
- 3: https://github.com/JetBrains/intellij-community/blob/master/platform/core-api/src/com/intellij/DynamicBundle.java
- 4: https://www.plugin-dev.com/intellij/general/plugin-localization/
🌐 Web query:
Java MessageFormat single quote apostrophe before placeholder escaping single quote literal
💡 Result:
In Java's MessageFormat, a single quote (apostrophe) must be represented by doubling it (i.e., using two single quotes: '') throughout the pattern string [1][2][3]. When you need to include a literal single quote, simply replace every instance of ' with ''. For example, the word "doesn't" should be written as "doesn''t" within a MessageFormat pattern [4]. It is important to note that a pair of single quotes (e.g., '{0}') is used to quote arbitrary characters (treating them as literals rather than format elements) [1][3]. Because of this, if you want to output a literal single quote, you must double it (''), even if it appears before a placeholder [1][2]. If you are working with languages that frequently use apostrophes (such as French), the doubling requirement can become burdensome [5]. Common approaches to handle this include: 1. Doubling quotes programmatically: Use string replacement (e.g., str.replace("'", "''")) before passing the string to MessageFormat [5][4]. 2. Using typographic apostrophes: Some developers use the right single quotation mark character (’) instead of the standard ASCII apostrophe (') [5]. Since ’ is not a special metacharacter in MessageFormat, it does not require escaping [5]. 3. Alternative libraries: For more robust internationalization support, some projects migrate to ICU4J (International Components for Unicode), which offers more flexible handling of message patterns [5].
Citations:
- 1: https://docs.oracle.com/en/java/javase/23/docs/api/java.base/java/text/MessageFormat.html
- 2: https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/text/MessageFormat.html
- 3: https://developer.android.com/reference/java/text/MessageFormat
- 4: https://stackoverflow.com/questions/17544794/escaping-single-quotes-for-java-messageformat
- 5: https://stackoverflow.com/questions/37920973/how-to-use-single-quotes-with-messageformat
🌐 Web query:
site:github.com/RedHat-DevSpacesGateway-plugin OidcProviderMetadataResolver ssoUnreachableMessage connector.wizard_step.openshift_connection.error.sso_unreachable
💡 Result:
The terms OidcProviderMetadataResolver, ssoUnreachableMessage, and connector.wizard_step.openshift_connection.error.sso_unreachable appear to be specific application-level configuration keys, error codes, or localization strings, likely belonging to a private or proprietary software project. While OIDC provider metadata resolution is a standard concept used in libraries like node-oidc-provider [1][2] and Apache Pulsar's OIDC authentication module [3], the specific identifiers provided do not correspond to public APIs or documented error codes in widely available open-source frameworks. The structure connector.wizard_step.openshift_connection.error.sso_unreachable suggests a UI-driven wizard for configuring OpenShift connections where an SSO service is expected but cannot be reached [4][5]. If you are encountering this error, it indicates that the application's configuration wizard is failing to validate or connect to the configured OpenID Connect (SSO) endpoint. Troubleshooting steps should include: 1. Verifying that the Issuer URL or Discovery endpoint provided in the configuration is reachable from the network where the application is running [1][3]. 2. Checking for proxy or firewall settings that might be blocking the connection between the application and the OIDC provider. 3. Reviewing the application logs for a more detailed error message accompanying the sso_unreachable code, as this often contains the underlying HTTP error or timeout exception [3][6].
Citations:
- 1: panva/node-oidc-provider@c5bd90f
- 2: https://github.com/panva/node-oidc-provider/blob/HEAD/docs/README.md
- 3: https://github.com/apache/pulsar/blob/1617bb22/pulsar-broker-auth-oidc/src/main/java/org/apache/pulsar/broker/authentication/oidc/OpenIDProviderMetadataCache.java
- 4: https://github.com/debezium/debezium-ui/blob/eece9492/src/app/pages/createConnector/CreateConnectorWizard.tsx
- 5: NIFI-15816 - Connector wizard apache/nifi#11126
- 6: megalus/django-google-sso@e53accb
🌐 Web query:
site:github.com/intellij-community AbstractBundle String.format getMessage quote escaping MessageFormat
💡 Result:
AbstractBundle-style messages in IntelliJ follow java.text.MessageFormat, not String.format, so use {0}, {1}, etc. for arguments. In MessageFormat, a single quote starts/ends quoted literal text, so to render an apostrophe you must double it: ''. (docs.oracle.com)
So, for example:
It's {0}→ needs escaping asIt''s {0}Hello, {0}→ fine as-is
If you want, I can also point you to the exact IntelliJ source file/class that does the formatting.
Citations:
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
from argparse import ArgumentParser
class MessageFormatFormatError(Exception):
pass
class MessageFormat:
def __init__(self, pattern):
self.pattern = pattern
def format(self, arguments):
result = []
pos = 0
arg_index = 0
num = len(arguments)
pattern_length = len(self.pattern)
while pos < pattern_length:
ch = self.pattern[pos]
if ch == "'":
pos += 1
if pos < pattern_length:
if self.pattern[pos] == "'":
result.append("'")
pos += 1
continue
temp = pos
while pos < pattern_length and self.pattern[pos] != "'":
pos += 1
quoted = self.pattern[temp:pos]
if quoted.startswith("{") and quoted.endswith("}"):
raise MessageFormatFormatError(f"can't format quoted placeholder: {self.pattern}")
result.append(quoted)
pos += 1
elif ch == "{":
temp = pos
pos += 1
while pos < pattern_length and self.pattern[pos] != "}":
pos += 1
form = self.pattern[temp + 1:pos]
pos += 1
if arg_index >= num:
raise MessageFormatFormatError(f"can't format value for argument {arg_index} because there are only {num} arguments")
result.append(arguments[arg_index])
arg_index += 1
else:
result.append(ch)
pos += 1
return "".join(result)
def js_format(strings):
s = strings[0]
args = strings[1:]
def replacer(i):
for j, pos in enumerate(i.args):
if pos < 0 or pos >= len(i.args) or (pos == len(i.args) - 1 and pos != j):
raise Exception("invalid argument index")
return i.values[pos]
# naive JS-style simulation based on the source pattern for this message shape
i = None
print("js_format not fully implemented")
if __name__ == "__main__":
pattern = "Cannot reach SSO provider ({0}). If you're behind a corporate proxy or firewall, try using Token authentication instead."
args = ["example"]
mf = MessageFormat(pattern)
try:
print(mf.format(args))
except Exception as e:
print("MessageFormat exception:", repr(e))
# Show where first apostrophe ends quoted block in MessageFormat, if present
p = pattern
apost = p.index("'")
block = p[apost+1:]
print("block after first apostrophe:", block[:50])
PYRepository: redhat-developer/devspaces-gateway-plugin
Length of output: 384
Escape the apostrophe before the placeholder text.
DevSpacesBundle.message formats with MessageFormat; because this value contains {0}, If you're... drops the apostrophe and any text following it until the next apostrophe. Use If you''re, or rewrite as If you are behind....
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/resources/messages/DevSpacesBundle.properties` at line 28, Update
the connector.wizard_step.openshift_connection.error.sso_unreachable message to
escape the apostrophe in “you're” for MessageFormat compatibility, using the
existing wording and placeholder unchanged.
Source: Path instructions
| @Test | ||
| fun `resolve maps unreachable host to SsoLoginException with Token hint`() { | ||
| val resolver = OidcProviderMetadataResolver( | ||
| authUrl = "https://oidc-unreachable-host.invalid/auth/realms/test/" | ||
| ) | ||
|
|
||
| val error = assertThrows<SsoLoginException.Failed> { | ||
| runBlocking { resolver.resolve() } | ||
| } | ||
|
|
||
| assertThat(error.message).contains("Cannot reach SSO provider (oidc-unreachable-host.invalid)") | ||
| assertThat(error.message).contains("Token authentication") | ||
| assertThat(error.cause).isNotNull | ||
| assertThat(isSsoUnreachable(error.cause!!)).isTrue() | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Make the failure-mapping test deterministic.
This test performs real DNS/HTTP resolution inside runBlocking with no test-level timeout. CI can hang or behave differently under DNS interception, even though the test only needs to verify exception mapping. Inject a request/transport or use a controlled local endpoint with bounded timing.
As per path instructions: “Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.” This is a test-suite availability issue, not a style preference.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/test/kotlin/com/redhat/devtools/gateway/auth/oidc/OidcProviderMetadataResolverTest.kt`
around lines 62 - 76, Make the `resolve maps unreachable host to
SsoLoginException with Token hint` test deterministic by replacing the external
`.invalid` host resolution with an injected request/transport failure or
controlled local endpoint configured with bounded timing. Preserve the existing
`SsoLoginException.Failed` assertions, including the message hints and
unreachable cause validation, without relying on real DNS or unbounded network
access.
Source: Path instructions
…958) Map DNS/connect/timeout failures when resolving OIDC metadata to a user-facing hint to try Token auth; resolve metadata lazily on login. Signed-off-by: Andre Dietisheim <adietish@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
…tadata() Signed-off-by: Andre Dietisheim <adietish@redhat.com>
2 participants
fixes https://redhat.atlassian.net/browse/CRW-11958
with this fix one gets to the Devspaces connection wizard. An error is then displayed once the user tries to connect:
