fix: fallback for self-signed unraid certs#184
fix: fallback for self-signed unraid certs#184ruaan-deysel merged 4 commits intoruaan-deysel:mainfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughImplement SSL certificate error handling with automatic fallback to unverified SSL for Unraid connections. Introduces new Changes
Sequence DiagramsequenceDiagram
participant User
participant ConfigFlow
participant UnraidClient
participant SSLHandler
User->>ConfigFlow: Test connection (HTTPS with self-signed cert)
ConfigFlow->>UnraidClient: Create client (verify_ssl=True)
UnraidClient->>SSLHandler: Attempt connection
SSLHandler-->>UnraidClient: SSLCertVerificationError
UnraidClient-->>ConfigFlow: SSLCertificateError raised
ConfigFlow->>ConfigFlow: Close current client
ConfigFlow->>UnraidClient: Create new client (verify_ssl=False)
ConfigFlow->>SSLHandler: Retry connection
SSLHandler-->>UnraidClient: Connection successful
UnraidClient-->>ConfigFlow: Connection confirmed
ConfigFlow->>ConfigFlow: Set _use_ssl=False
ConfigFlow->>ConfigFlow: Log successful retry
ConfigFlow-->>User: Connection successful (SSL disabled)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerLet us write the prompt for your AI agent so you can ship faster (with fewer bugs). View plan for ticket: ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
This PR fixes a regression (issue #170) where the Unraid integration fails when Unraid's "Use SSL/TLS" setting is enabled with a self-signed certificate. The fix introduces a dedicated SSLCertificateError exception class and refactors the connection retry logic to attempt verify_ssl=False only when a certificate verification failure is specifically detected (rather than any connection error containing "ssl" in the string). It also ensures CONF_SSL is correctly persisted to config entries during reauth and reconfigure flows.
Changes:
- Introduces
SSLCertificateErroras a subclass ofCannotConnectErrorto distinguish SSL cert failures from generic connection errors, and uses it in_validate_connectionexception mapping and_handle_generic_error - Refactors
_test_connectionto only retry withverify_ssl=FalseonSSLCertificateError(not anyCannotConnectError), with a renamedfallback_clientfor clarity and explicitapi_client.close()paths - Updates reauth and reconfigure flows to write
CONF_SSLfromself._use_sslinto the config entry data; adds corresponding tests
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
custom_components/unraid/config_flow.py |
New SSLCertificateError exception; refined SSL retry logic; CONF_SSL persisted in reauth/reconfigure flows |
tests/test_config_flow.py |
New tests for SSL fallback in user/reauth flows, CONF_SSL persistence, and non-SSL no-retry behavior; asserts client closure |
uv.lock |
Adds unraid-api 1.6.0 as a tracked dependency; bumps ruff from 0.14.14 to 0.15.0; removes three greenlet s390x wheel entries |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@custom_components/unraid/config_flow.py`:
- Around line 313-315: The current branch that raises SSLCertificateError based
on generic "ssl" or "certificate" substrings is too broad; restrict it to
certificate-verification failures only by checking for concrete verification
signals: if the exception is an instance of ssl.SSLCertVerificationError (or the
library-specific cert verification exception) or if error_str contains
verification-specific phrases such as "certificate verify failed",
"CERTIFICATE_VERIFY_FAILED", "self signed certificate", or "certificate has
expired" then raise SSLCertificateError(msg) from err; otherwise do not convert
the error to SSLCertificateError. Keep references to the existing variables and
symbol names (error_str, err, SSLCertificateError) so you update the same branch
in config_flow.py.
- Around line 420-423: The reconfigure path replaces the whole entry data with
only user_input plus CONF_SSL (merged_data) which can drop previously saved keys
like CONF_PORT; instead, build merged_data by starting from
reconfigure_entry.data (or a shallow copy) and update it with user_input and
CONF_SSL so existing persisted fields are preserved, then pass that merged_data
into self.hass.config_entries.async_update_entry (referencing reconfigure_entry,
merged_data, CONF_SSL, user_input).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ceb18c5f-8068-4b15-8f99-d36f0f1d865c
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
custom_components/unraid/config_flow.pytests/test_config_flow.py
Co-Authored-By: Copilot <175728472+Copilot@users.noreply.github.com>
| assert entry.data[CONF_API_KEY] == "new-key" | ||
| assert entry.data[CONF_SSL] is False | ||
|
|
||
|
|
There was a problem hiding this comment.
There is a missing blank line between test_reconfigure_flow_updates_ssl_flag_when_cert_changes (ending at line 1258) and test_reconfigure_flow_connection_error (starting at line 1259). Python style (PEP 8) and the project's ruff configuration require two blank lines between top-level function definitions. This will likely cause a lint failure.
|
@Snuffy2 Thanks for the help and contribution. Appreciate it! |
Description
This pull request refactors SSL certificate error handling in the Unraid integration's config flow, introducing a dedicated exception for SSL certificate verification failures and improving the retry logic for self-signed certificates. It also ensures that the
CONF_SSLflag is correctly set and updated in configuration entries during reauth and reconfigure flows. Comprehensive tests have been added and updated to verify the new behavior.Related Issue
Fixes #170
Changes Made
Improved SSL error handling and retry logic
SSLCertificateErrorexception to distinguish SSL certificate verification failures from generic connection errors, and updated error handling throughout the config flow to use this exception. [1] [2] [3]verify_ssl=Falseonly when an SSL certificate error is detected, ensuring that self-signed certificates are handled gracefully and avoiding unnecessary retries for other connection errors. [1] [2]Configuration entry updates
CONF_SSLflag in config entries based on the outcome of the connection test, ensuring that the SSL verification status is accurately reflected in the entry data. [1] [2]Test enhancements
CONF_SSLflag during reauth and reconfigure flows. Tests now distinguish between SSL certificate errors and other connection errors. [1] [2] [3] [4]SSLCertificateErrorclass. [1] [2]Testing
./scripts/lint)pytest)./scripts/develop)Checklist
Screenshots (if applicable)
Additional Context
📌 Reminder: Please keep pull requests small and focused on a single issue or feature. This makes review and testing much easier! If you have multiple changes, please submit separate PRs. See CONTRIBUTING.md for more details.
Summary by CodeRabbit
Release Notes
Bug Fixes
New Features
Tests