Always include link in response if SMTP not configured#561
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughProvider/service docstrings added; email provider now raises ServerCommunicationError on non-200, network, or timeout errors; callers (registration, password reset, resend, SMS eligibility) handle or propagate these exceptions and use a tri-state email-service result to decide link disclosure and email-related responses. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant CommService as CommService (is_channel_enabled)
participant EmailProvider as ASAB_Iris
Client->>Handler: request (register / reset / resend / sms eligibility)
Handler->>CommService: is_channel_enabled(email/sms)
CommService->>EmailProvider: HTTP request to Iris
alt 200 OK
EmailProvider-->>CommService: 200 OK (enabled)
CommService-->>Handler: True
Handler->>Client: proceed (include link or send email / eligible)
else non-200 / network / timeout
EmailProvider-->>CommService: non-200 or error
CommService--xHandler: raises ServerCommunicationError
Handler-->>Client: conservative response (no link, `email_sent` with tech_err, or not eligible)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
seacatauth/credentials/change_password/handler.py (1)
348-353:⚠️ Potential issue | 🟠 MajorUnhandled
ServerCommunicationErrorcan cause information disclosure.The method's docstring (lines 318-319) states it "must always return a positive response as a measure to avoid sensitive information disclosure." However,
can_send_to_targetat line 349 can now raiseServerCommunicationError(since it internally callsis_enabled()which raises on external service failures).An unhandled exception here would result in an HTTP 500 error, potentially disclosing that the credentials exist.
🐛 Proposed fix: wrap in try/except
# Check if password reset link can be sent - if not await self.ChangePasswordService.CommunicationService.can_send_to_target(credentials, "email"): + try: + can_send = await self.ChangePasswordService.CommunicationService.can_send_to_target(credentials, "email") + except exceptions.ServerCommunicationError: + L.log(asab.LOG_NOTICE, "Lost password reset failed: Cannot check email service availability.", struct_data={ + "cid": credentials_id, "from": access_ips}) + # Avoid information disclosure + return asab.web.rest.json_response(request, {"result": "OK"}) + if not can_send: L.log(asab.LOG_NOTICE, "Lost password reset failed: No way to communicate password reset link.", struct_data={ "cid": credentials_id, "from": access_ips}) # Avoid information disclosure return asab.web.rest.json_response(request, {"result": "OK"})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@seacatauth/credentials/change_password/handler.py` around lines 348 - 353, Wrap the call to self.ChangePasswordService.CommunicationService.can_send_to_target in a try/except that catches ServerCommunicationError (and optionally broader communication exceptions), log the failure with context using L.log (same level/struct_data as the existing branch), and return the same positive JSON response via asab.web.rest.json_response(request, {"result": "OK"}) to avoid information disclosure; update the code around the can_send_to_target call so both the False result and any ServerCommunicationError follow the identical logging and return path.
🧹 Nitpick comments (1)
seacatauth/communication/providers/email_iris.py (1)
38-45:can_send_to_targetcan propagateServerCommunicationErrorfromis_enabled().This method calls
self.is_enabled()at line 39, which can now raiseServerCommunicationError. The exception will propagate through this method to callers.This is actually beneficial for the handler's try/except block (which catches this exception), but the
CommunicationService.can_send_to_targetdocstring inservice.pyshould be updated to document this potential exception, similar tois_channel_enabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@seacatauth/communication/providers/email_iris.py` around lines 38 - 45, The docstring for CommunicationService.can_send_to_target must be updated to document that it can propagate ServerCommunicationError from underlying calls (via self.is_enabled()), similar to the existing is_channel_enabled docstring; locate the CommunicationService.can_send_to_target method and add wording that callers should expect and may need to handle ServerCommunicationError (in addition to returns/ KeyError behavior) and reference that this mirrors is_channel_enabled's documented behavior for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@seacatauth/communication/service.py`:
- Around line 76-88: Update the two unprotected call sites to handle
exceptions.ServerCommunicationError when calling Service.is_channel_enabled:
wrap the invocation of is_channel_enabled in a try/except that catches
exceptions.ServerCommunicationError and treats failures as False (log or ignore
as appropriate). Specifically, modify can_send_email in
seacatauth/credentials/registration/handler.py and is_eligible in
seacatauth/authn/login_factors/smscode.py so they call await
<service>.is_channel_enabled(channel) inside a try block and return False if
exceptions.ServerCommunicationError is raised; keep the existing behavior for
other exceptions unchanged.
---
Outside diff comments:
In `@seacatauth/credentials/change_password/handler.py`:
- Around line 348-353: Wrap the call to
self.ChangePasswordService.CommunicationService.can_send_to_target in a
try/except that catches ServerCommunicationError (and optionally broader
communication exceptions), log the failure with context using L.log (same
level/struct_data as the existing branch), and return the same positive JSON
response via asab.web.rest.json_response(request, {"result": "OK"}) to avoid
information disclosure; update the code around the can_send_to_target call so
both the False result and any ServerCommunicationError follow the identical
logging and return path.
---
Nitpick comments:
In `@seacatauth/communication/providers/email_iris.py`:
- Around line 38-45: The docstring for CommunicationService.can_send_to_target
must be updated to document that it can propagate ServerCommunicationError from
underlying calls (via self.is_enabled()), similar to the existing
is_channel_enabled docstring; locate the CommunicationService.can_send_to_target
method and add wording that callers should expect and may need to handle
ServerCommunicationError (in addition to returns/ KeyError behavior) and
reference that this mirrors is_channel_enabled's documented behavior for
consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef38064d-de26-49e9-8cf7-818c866b54ea
📒 Files selected for processing (4)
seacatauth/communication/providers/abc.pyseacatauth/communication/providers/email_iris.pyseacatauth/communication/service.pyseacatauth/credentials/change_password/handler.py
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
seacatauth/credentials/registration/handler.py (1)
298-311:⚠️ Potential issue | 🟠 MajorHandle
ServerCommunicationErrorinresend_invitationto avoid 500s.At Line 298-311, only
MessageDeliveryErroris handled. Communication failures from the provider can still raiseServerCommunicationError, which currently escapes and breaks the response contract.Suggested fix
try: await self.RegistrationService.CommunicationService.invitation( credentials=credentials, registration_uri=registration_url, tenants=tenants, expires_at=expiration, ) + except exceptions.ServerCommunicationError: + response_data["email_sent"] = { + "result": "ERROR", + "tech_err": "Cannot connect to email service.", + "error": "SeaCatAuthError|Cannot connect to email service", + } + return asab.web.rest.json_response(request, response_data) except exceptions.MessageDeliveryError: response_data["email_sent"] = { "result": "ERROR", "tech_err": "Failed to send invitation link.", "error": "SeaCatAuthError|Failed to send invitation link", } return asab.web.rest.json_response(request, response_data)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@seacatauth/credentials/registration/handler.py` around lines 298 - 311, The invitation send block in resend_invitation only catches exceptions.MessageDeliveryError so provider communication failures (exceptions.ServerCommunicationError) bubble up and cause 500s; update the try/except around RegistrationService.CommunicationService.invitation in resend_invitation to also catch exceptions.ServerCommunicationError (or a tuple of both exception types) and handle it the same way as MessageDeliveryError by setting response_data["email_sent"] with an ERROR result and appropriate tech_err/error fields and returning asab.web.rest.json_response(request, response_data).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@seacatauth/authn/login_factors/smscode.py`:
- Around line 15-21: The code in smscode.py currently catches
exceptions.ServerCommunicationError in the MFA eligibility check and returns
False, which treats "provider unreachable" as "provider disabled" (fail-open);
instead do not suppress the communication error — either re-raise the
ServerCommunicationError so the caller can handle it or explicitly treat an
unknown state as eligible (fail-closed) depending on policy. Locate the check
using AuthenticationService.CommunicationService.is_channel_enabled("sms") in
the SMS factor class/method and remove the except that returns False; replace it
with re-raising the caught ServerCommunicationError (or returning True if your
policy is to fail-closed), so outages don't silently disable SMS MFA.
In `@seacatauth/credentials/change_password/handler.py`:
- Around line 236-246: The condition that builds can_get_link_in_response is too
strict: change the conjunction to a disjunction so links are allowed when SMTP
is disabled or when we cannot email the target; specifically update the
expression that uses email_service_enabled and can_email_to_target in the
ChangePassword handler (look for ChangePasswordService.CommunicationService
usage and the can_get_link_in_response assignment) from "email_service_enabled
is False and can_email_to_target is False" to "email_service_enabled is False or
can_email_to_target is False", preserving the authz.has_superuser_access()
check.
---
Outside diff comments:
In `@seacatauth/credentials/registration/handler.py`:
- Around line 298-311: The invitation send block in resend_invitation only
catches exceptions.MessageDeliveryError so provider communication failures
(exceptions.ServerCommunicationError) bubble up and cause 500s; update the
try/except around RegistrationService.CommunicationService.invitation in
resend_invitation to also catch exceptions.ServerCommunicationError (or a tuple
of both exception types) and handle it the same way as MessageDeliveryError by
setting response_data["email_sent"] with an ERROR result and appropriate
tech_err/error fields and returning asab.web.rest.json_response(request,
response_data).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 645daab5-0221-48aa-884d-d4c1dbdfd08e
📒 Files selected for processing (3)
seacatauth/authn/login_factors/smscode.pyseacatauth/credentials/change_password/handler.pyseacatauth/credentials/registration/handler.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
seacatauth/credentials/change_password/handler.py (1)
245-249:⚠️ Potential issue | 🟠 MajorHandle reset-link creation failures explicitly.
init_password_reset(...)is called without exception handling in both paths. If link creation fails, this can bubble up as 500, which conflicts with the method contract that failures should return a structured error response.Proposed fix
# Superusers receive the password reset link in response if can_get_link_in_response: - password_reset_url = await self.ChangePasswordService.init_password_reset( - credentials, - expiration=json_data.get("expiration"), - ) + try: + password_reset_url = await self.ChangePasswordService.init_password_reset( + credentials, + expiration=json_data.get("expiration"), + ) + except Exception: + L.exception("Password reset failed: Failed to create reset link.", struct_data={"cid": credentials_id}) + return asab.web.rest.json_response(request, status=400, data={ + "result": "ERROR", + "tech_err": "Failed to create password reset link.", + "error": "SeaCatAuthError|Failed to create password reset link", + }) response_data["password_reset_url"] = password_reset_url url_disclosed = True @@ else: if not password_reset_url: - password_reset_url = await self.ChangePasswordService.init_password_reset( - credentials, - expiration=json_data.get("expiration"), - ) + try: + password_reset_url = await self.ChangePasswordService.init_password_reset( + credentials, + expiration=json_data.get("expiration"), + ) + except Exception: + L.exception("Password reset failed: Failed to create reset link.", struct_data={"cid": credentials_id}) + email_delivery_result = { + "result": "ERROR", + "tech_err": "Failed to create password reset link.", + "error": "SeaCatAuthError|Failed to create password reset link", + } + response_data["email_sent"] = email_delivery_result + response_data.update(email_delivery_result) + return asab.web.rest.json_response(request, response_data, status=400)Also applies to: 271-275
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@seacatauth/credentials/change_password/handler.py` around lines 245 - 249, The call to ChangePasswordService.init_password_reset (used when can_get_link_in_response is True and likewise in the other branch) is unprotected and can raise, causing a 500 instead of the method's structured error response; wrap both calls to init_password_reset in try/except, catch expected exceptions (or Exception as fallback), log the error, and return the appropriate structured error payload (same shape the handler uses for other failures) instead of allowing the exception to propagate—ensure you reference the local variable password_reset_url only after a successful call and set an error response path when the call fails.
🧹 Nitpick comments (1)
seacatauth/credentials/change_password/handler.py (1)
234-235: Keep tri-state email availability explicit in response mapping.
email_service_enabledis documented as tri-state (True/False/None), butif not email_service_enabledmergesFalseandNone. Consider handlingNoneseparately so transient communication failures are not reported as “service not available.”Also applies to: 254-261
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@seacatauth/credentials/change_password/handler.py` around lines 234 - 235, The response mapping currently treats email_service_enabled as truthy/falsey (using "if not email_service_enabled"), which collapses the tri-state (True/False/None); update the logic in the change-password handler to explicitly branch on email_service_enabled is True, is False, and is None when computing response fields (use explicit comparisons instead of truthiness checks) and ensure credentials_have_email (variable credentials_have_email) is used together with these explicit checks to set the three-state outcome; apply the same explicit tri-state handling to the other block referenced (lines around the second occurrence, previously 254-261) so transient errors (None) are reported distinctly from a definite False service availability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@seacatauth/authn/login_factors/smscode.py`:
- Around line 20-22: Update the misleading comment and the exception re-raise in
the SMS code factor error path: change the comment that says "Unable to
determine if SMS provider is enabled, treat as not eligible" to reflect that the
error is being propagated, and replace "raise e" with a bare "raise" so the
original traceback is preserved; locate the logging call using L.error(...) in
smscode.py and apply these two edits around that block.
---
Outside diff comments:
In `@seacatauth/credentials/change_password/handler.py`:
- Around line 245-249: The call to ChangePasswordService.init_password_reset
(used when can_get_link_in_response is True and likewise in the other branch) is
unprotected and can raise, causing a 500 instead of the method's structured
error response; wrap both calls to init_password_reset in try/except, catch
expected exceptions (or Exception as fallback), log the error, and return the
appropriate structured error payload (same shape the handler uses for other
failures) instead of allowing the exception to propagate—ensure you reference
the local variable password_reset_url only after a successful call and set an
error response path when the call fails.
---
Nitpick comments:
In `@seacatauth/credentials/change_password/handler.py`:
- Around line 234-235: The response mapping currently treats
email_service_enabled as truthy/falsey (using "if not email_service_enabled"),
which collapses the tri-state (True/False/None); update the logic in the
change-password handler to explicitly branch on email_service_enabled is True,
is False, and is None when computing response fields (use explicit comparisons
instead of truthiness checks) and ensure credentials_have_email (variable
credentials_have_email) is used together with these explicit checks to set the
three-state outcome; apply the same explicit tri-state handling to the other
block referenced (lines around the second occurrence, previously 254-261) so
transient errors (None) are reported distinctly from a definite False service
availability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2a16e487-b685-4bb5-bfda-1b6e68f9f302
📒 Files selected for processing (2)
seacatauth/authn/login_factors/smscode.pyseacatauth/credentials/change_password/handler.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
seacatauth/credentials/registration/handler.py (1)
298-311:⚠️ Potential issue | 🟡 MinorMissing
ServerCommunicationErrorhandler inresend_invitation.The
_prepare_invitationmethod handles bothServerCommunicationError(lines 222-228) andMessageDeliveryError(lines 229-235) from the invitation call. However,resend_invitationonly catchesMessageDeliveryError. IfServerCommunicationErroris raised during the invitation call, it will propagate uncaught.Proposed fix to add missing exception handler
try: await self.RegistrationService.CommunicationService.invitation( credentials=credentials, registration_uri=registration_url, tenants=tenants, expires_at=expiration, ) + except exceptions.ServerCommunicationError: + response_data["email_sent"] = { + "result": "ERROR", + "tech_err": "Cannot connect to the email service.", + "error": "SeaCatAuthError|Cannot connect to the email service", + } + return asab.web.rest.json_response(request, response_data) except exceptions.MessageDeliveryError: response_data["email_sent"] = { "result": "ERROR", "tech_err": "Failed to send invitation link.", "error": "SeaCatAuthError|Failed to send invitation link", } return asab.web.rest.json_response(request, response_data)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@seacatauth/credentials/registration/handler.py` around lines 298 - 311, The resend_invitation handler currently only catches exceptions.MessageDeliveryError from RegistrationService.CommunicationService.invitation; add a matching except block for exceptions.ServerCommunicationError (mirroring the handling in _prepare_invitation) so ServerCommunicationError is caught and response_data["email_sent"] is set to the same ERROR payload (or appropriate tech_err/message) and returned via asab.web.rest.json_response(request, response_data); locate this in the resend_invitation function and follow the same response structure used for MessageDeliveryError to ensure consistent error handling.
🧹 Nitpick comments (1)
seacatauth/credentials/change_password/handler.py (1)
234-261: Inconsistent handling of communication error compared to registration handler.When
email_service_enabledisNone(communication error) and the user is not a superuser, this code falls through to line 254 wherenot Noneevaluates toTrue, returning "Email service not available." In contrast,registration/handler.pyhas an explicit early check at lines 159-164 that returns "Email service is temporarily unavailable" for this case.Consider adding explicit handling to distinguish between "service disabled" and "communication error":
Proposed fix for consistency
can_get_link_in_response: bool = authz.has_superuser_access() or email_service_enabled is False + + if email_service_enabled is None and not can_get_link_in_response: + return asab.web.rest.json_response(request, status=400, data={ + "result": "ERROR", + "tech_err": "Email service is temporarily unavailable.", + }) # Superusers receive the password reset link in response🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@seacatauth/credentials/change_password/handler.py` around lines 234 - 261, The code treats email_service_enabled = None (communication error) the same as False (service disabled) which causes an incorrect "Email service not available" error for non-superusers; update the logic around ChangePasswordService.CommunicationService.is_channel_enabled, email_service_enabled, and can_get_link_in_response so that if email_service_enabled is None and the caller is not a superuser you log a communication error and return/prepare an email_delivery_result indicating the service is temporarily unavailable (match the wording used in registration/handler), rather than falling through to the "not available" branch; implement this by adding an explicit check for email_service_enabled is None before the existing if not email_service_enabled block, logging the communication error and setting the appropriate "result", "tech_err" and "error" fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@seacatauth/credentials/registration/handler.py`:
- Around line 298-311: The resend_invitation handler currently only catches
exceptions.MessageDeliveryError from
RegistrationService.CommunicationService.invitation; add a matching except block
for exceptions.ServerCommunicationError (mirroring the handling in
_prepare_invitation) so ServerCommunicationError is caught and
response_data["email_sent"] is set to the same ERROR payload (or appropriate
tech_err/message) and returned via asab.web.rest.json_response(request,
response_data); locate this in the resend_invitation function and follow the
same response structure used for MessageDeliveryError to ensure consistent error
handling.
---
Nitpick comments:
In `@seacatauth/credentials/change_password/handler.py`:
- Around line 234-261: The code treats email_service_enabled = None
(communication error) the same as False (service disabled) which causes an
incorrect "Email service not available" error for non-superusers; update the
logic around ChangePasswordService.CommunicationService.is_channel_enabled,
email_service_enabled, and can_get_link_in_response so that if
email_service_enabled is None and the caller is not a superuser you log a
communication error and return/prepare an email_delivery_result indicating the
service is temporarily unavailable (match the wording used in
registration/handler), rather than falling through to the "not available"
branch; implement this by adding an explicit check for email_service_enabled is
None before the existing if not email_service_enabled block, logging the
communication error and setting the appropriate "result", "tech_err" and "error"
fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 32fbb19e-6fa9-4639-a125-8e92294003b8
📒 Files selected for processing (3)
seacatauth/authn/login_factors/smscode.pyseacatauth/credentials/change_password/handler.pyseacatauth/credentials/registration/handler.py
🚧 Files skipped from review as they are similar to previous changes (1)
- seacatauth/authn/login_factors/smscode.py
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
seacatauth/credentials/registration/handler.py (1)
298-311:⚠️ Potential issue | 🟠 MajorMissing
ServerCommunicationErrorhandler, inconsistent with_prepare_invitation.
_prepare_invitation(lines 222-228) catches bothServerCommunicationErrorandMessageDeliveryErrorfrominvitation(), but this method only catchesMessageDeliveryError. If the exception can occur, this will result in an unhandled 500 error.Proposed fix to add missing exception handler
try: await self.RegistrationService.CommunicationService.invitation( credentials=credentials, registration_uri=registration_url, tenants=tenants, expires_at=expiration, ) + except exceptions.ServerCommunicationError: + response_data["email_sent"] = { + "result": "ERROR", + "tech_err": "Cannot connect to the email service.", + "error": "SeaCatAuthError|Cannot connect to the email service", + } + return asab.web.rest.json_response(request, response_data) except exceptions.MessageDeliveryError: response_data["email_sent"] = { "result": "ERROR", "tech_err": "Failed to send invitation link.", "error": "SeaCatAuthError|Failed to send invitation link", } return asab.web.rest.json_response(request, response_data)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@seacatauth/credentials/registration/handler.py` around lines 298 - 311, The current try/except around self.RegistrationService.CommunationService.invitation only catches exceptions.MessageDeliveryError but not ServerCommunicationError (unlike _prepare_invitation), which can produce an unhandled 500; add an except for exceptions.ServerCommunicationError that mirrors the MessageDeliveryError handling (set response_data["email_sent"] = {"result": "ERROR", "tech_err": "Failed to send invitation link.", "error": "SeaCatAuthError|Failed to send invitation link"} and return asab.web.rest.json_response(request, response_data)). Ensure the new except is placed alongside the existing except for MessageDeliveryError in the same method that calls RegistrationService.CommunicationService.invitation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@seacatauth/credentials/registration/handler.py`:
- Around line 289-295: The current check conflates email_service_enabled False
and None; update the handling around the email_service_enabled variable so False
returns an "Email service disabled" error and None returns an "Email service
health check failed" error (matching the fix in _prepare_invitation); set
response_data["email_sent"]["result"/"tech_err"/"error"] accordingly and return
via asab.web.rest.json_response(request, response_data) so callers can
distinguish disabled vs check-failed states.
- Around line 207-213: The current check treats email_service_enabled == False
and == None the same; update the handler's response block that builds
response_data["email_sent"] (in registration/handler.py) to branch on the actual
value of email_service_enabled: if it is False set "tech_err" to "Email service
not available." and "error" to "SeaCatAuthError|Email service not available",
but if it is None set "tech_err" to "Email service temporarily unavailable." and
"error" to "SeaCatAuthError|Email service temporarily unavailable" (matching the
wording used earlier at line 163); keep the return as
asab.web.rest.json_response(request, response_data).
- Around line 222-228: Remove the unreachable except block handling
exceptions.ServerCommunicationError in the invitation() handler: delete the
except exceptions.ServerCommunicationError: ... return ... block (the block at
lines shown) because build_and_send_message() wraps provider errors into
MessageDeliveryError; ensure the existing except handlers still include the
except MessageDeliveryError branch so only MessageDeliveryError (and other
actually thrown exceptions) are handled.
---
Outside diff comments:
In `@seacatauth/credentials/registration/handler.py`:
- Around line 298-311: The current try/except around
self.RegistrationService.CommunationService.invitation only catches
exceptions.MessageDeliveryError but not ServerCommunicationError (unlike
_prepare_invitation), which can produce an unhandled 500; add an except for
exceptions.ServerCommunicationError that mirrors the MessageDeliveryError
handling (set response_data["email_sent"] = {"result": "ERROR", "tech_err":
"Failed to send invitation link.", "error": "SeaCatAuthError|Failed to send
invitation link"} and return asab.web.rest.json_response(request,
response_data)). Ensure the new except is placed alongside the existing except
for MessageDeliveryError in the same method that calls
RegistrationService.CommunicationService.invitation.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e2a31476-8113-418b-9039-b092d626e6f7
📒 Files selected for processing (1)
seacatauth/credentials/registration/handler.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Documentation