Fix #232: Trusted-device bridge protocol changed, breaking 2FA#233
Draft
ebarrere wants to merge 1 commit intotimlaing:mainfrom
Draft
Fix #232: Trusted-device bridge protocol changed, breaking 2FA#233ebarrere wants to merge 1 commit intotimlaing:mainfrom
ebarrere wants to merge 1 commit intotimlaing:mainfrom
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe hsa2_bridge module now accepts Apple's newer trusted-device bridge push protocol variant that includes ChangesBridge flowid Protocol Support
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Pylint (4.0.5)pyicloud/hsa2_bridge.pyThanks 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
🧹 Nitpick comments (1)
pyicloud/hsa2_bridge.py (1)
220-229: ⚡ Quick winAdd regression tests for both identifier variants
Please add unit tests for: (1) payload with
sessionUUID, (2) payload with onlyflowid, and (3) payload with neither. This path is protocol-sensitive and currently untested in this PR.🤖 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 `@pyicloud/hsa2_bridge.py` around lines 220 - 229, Add unit tests covering the session identifier resolution in the HSA2 bridge: (1) a payload containing sessionUUID should result in cls being constructed with session_uuid equal to validated.session_uuid, (2) a payload containing only flowid should result in session_uuid equal to validated.flow_id, and (3) a payload missing both should raise PyiCloudTrustedDevicePromptException. Target the code path that sets resolved_session_uuid from validated.session_uuid or validated.flow_id and the constructor call cls(payload=..., session_uuid=...), asserting the expected session_uuid or exception for each case.
🤖 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 `@pyicloud/hsa2_bridge.py`:
- Around line 159-165: The validator _validate_session_uuid currently raises
ValueError for blank session identifiers which prevents flow_id fallback; change
its behavior to treat blank strings as None instead of raising: in the
`@field_validator` for "session_uuid" and "flow_id" (function
_validate_session_uuid) detect if value is not None and value.strip() is empty,
and return None rather than raising an error, keeping the return type
Optional[str] so a present-but-blank session_uuid will be treated as missing and
allow flow_id fallback to proceed.
---
Nitpick comments:
In `@pyicloud/hsa2_bridge.py`:
- Around line 220-229: Add unit tests covering the session identifier resolution
in the HSA2 bridge: (1) a payload containing sessionUUID should result in cls
being constructed with session_uuid equal to validated.session_uuid, (2) a
payload containing only flowid should result in session_uuid equal to
validated.flow_id, and (3) a payload missing both should raise
PyiCloudTrustedDevicePromptException. Target the code path that sets
resolved_session_uuid from validated.session_uuid or validated.flow_id and the
constructor call cls(payload=..., session_uuid=...), asserting the expected
session_uuid or exception for each case.
🪄 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
Run ID: 71295618-c989-442c-9904-47fa496e02f5
📒 Files selected for processing (1)
pyicloud/hsa2_bridge.py
Comment on lines
+159
to
165
| @field_validator("session_uuid", "flow_id") | ||
| @classmethod | ||
| def _validate_session_uuid(cls, value: str) -> str: | ||
| def _validate_session_uuid(cls, value: Optional[str]) -> Optional[str]: | ||
| """Reject blank bridge session identifiers.""" | ||
| if not value.strip(): | ||
| raise ValueError("sessionUUID must not be blank") | ||
| if value is not None and not value.strip(): | ||
| raise ValueError("sessionUUID/flowid must not be blank") | ||
| return value |
There was a problem hiding this comment.
Allow fallback when sessionUUID is present-but-blank
Right now, a blank sessionUUID raises validation even if flowid is valid. Treat blank identifiers as None so flowid fallback can still work.
Suggested patch
`@field_validator`("session_uuid", "flow_id")
`@classmethod`
def _validate_session_uuid(cls, value: Optional[str]) -> Optional[str]:
"""Reject blank bridge session identifiers."""
- if value is not None and not value.strip():
- raise ValueError("sessionUUID/flowid must not be blank")
- return value
+ if value is None:
+ return None
+ normalized = value.strip()
+ return normalized or None🤖 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 `@pyicloud/hsa2_bridge.py` around lines 159 - 165, The validator
_validate_session_uuid currently raises ValueError for blank session identifiers
which prevents flow_id fallback; change its behavior to treat blank strings as
None instead of raising: in the `@field_validator` for "session_uuid" and
"flow_id" (function _validate_session_uuid) detect if value is not None and
value.strip() is empty, and return None rather than raising an error, keeping
the return type Optional[str] so a present-but-blank session_uuid will be
treated as missing and allow flow_id fallback to proceed.
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.
Breaking change
NA, as far as I know.
Proposed change
Fix #232 Trusted-device bridge protocol changed, breaking 2FA. Apple appears to have changed the protocol, so we must adapt or be left behind.
Type of change
Example of code:
Clear cache, attempt login
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed: