Rcv prompt updates v3 - #403
Conversation
oracle-contributor-agreement
Bot
added
the
OCA Verified
dustin-sale
left a comment
dustin-sale
left a comment
There was a problem hiding this comment.
Review by @dustin-sale via Codex.
Requested changes
server.py:4340— [P1] Do not expose profile credentials through unauthenticated HTTP.server.py:802— [P1] Preserve the Recovery invocation authorization scope.server.py:799— [P1] Require a valid public OAuth base URL.server.py:527— [P1] Use the shared caller-scoped authentication path.
Additional review notes
test_legacy_recovery_tools.py:357— [P2] Assert the exact user agent for every auth path.CHANGELOG.md:3— [P2] Document the 3.0.0 breaking changes.
Validation
make lint, scoped tests, lock validation, and package build passed. Tests reported 132 passed, 9 xfailed, and 90.01% coverage; git diff --check found four whitespace errors.
See the inline comments in this review for evidence, impact, and suggested remediation.
54843e9 to
5f3b79c
Compare
|
Addressed all the review comments. Thank you for reviewing this. |
dustin-sale
left a comment
dustin-sale
left a comment
There was a problem hiding this comment.
Review by @dustin-sale via Codex.
Requested changes
server.py:876— [P1] Route hosted HTTP authentication throughoracle-mcp-common.
Additional review notes
README.md:73— [P2] Remove unsupported profile-backed HTTP instructions.onboard_database_to_recovery_service.txt:186— [P3] Clear the remaining whitespace failures.
Validation
make lint, scoped tests (132 passed, 90.42% coverage), lock validation, package build, and GitHub CI passed. git diff --check found three whitespace errors.
See the inline comments in this review for evidence, impact, and suggested remediation.
|
Incorporated review comments. Please review n approve. |
dustin-sale
left a comment
dustin-sale
left a comment
There was a problem hiding this comment.
Review by @dustin-sale via Codex.
Requested changes
server.py:1192— [P1] Preserve caller isolation when tokens omitsub.
Additional review notes
None.
Validation
Lint, 163 scoped tests with 90.69% coverage, lock validation, package build, whitespace checks, and GitHub CI passed. A targeted probe reproduced a caller-cache collision for distinct tokens sharing one OAuth client ID.
See the inline comments in this review for evidence, impact, and suggested remediation.
|
|
||
| access = get_access_token() | ||
| claims = (getattr(access, "claims", None) or {}) if access is not None else {} | ||
| subject = claims.get("sub") or getattr(access, "client_id", None) |
There was a problem hiding this comment.
Comment from @dustin-sale via Codex.
[P1] Preserve caller isolation when tokens omit sub
Evidence: _caller_cache_key() falls back from the token's sub claim to access.client_id before considering the raw token. An OAuth client ID identifies the registered application, not the authenticated human, so multiple users can share it. A probe with two distinct tokens, empty claims, and the same client ID returned the same cache key. The existing _mcp_actor_id() also notes that tokens can omit sub and uses jti instead. Current tests cover different sub values and a missing access token, but not this condition.
Impact: the compartment listing uses access_level="ACCESSIBLE" and is cached under this key. A restricted user sharing the registered client with a more privileged user can therefore receive the first user's cached compartment metadata and drive child-compartment expansion with it.
Requested change: use a caller-specific claim such as sub; when it is absent, hash jti or the raw token before considering client_id, or skip caching. Add a regression test proving distinct tokens with the same client ID never share a compartment-cache entry.
[fix]: pr-comment-P1.3 - fix fallback to client_id
|
Review comments merged kindly review. |
dustin-sale
left a comment
dustin-sale
left a comment
There was a problem hiding this comment.
Review by @dustin-sale via Codex.
Requested changes
No blocking changes requested.
Additional review notes
None.
Validation
make lint, make test project=oci-recovery-mcp-server (166 passed, 90.69% coverage), uv lock --check, uv build, and git diff --check passed. GitHub CI is green.
See the inline comments in this review for evidence, impact, and suggested remediation.
3 participants
Description
This change delivers Recovery MCP Server v3.0.0, expanded Recovery Service and Database Service read coverage, and guided operational workflows for Cloud Protect onboarding.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Added targeted unit tests covering:
Guidance-tool availability and input validation.
Recovery and Recovery Database tool behavior, including legacy coverage retained during test-suite restructuring.Multi-Tenant OAuth authentication and tenancy header routing.
Onboard testing :
Prompt
codex> "Using recovery service mcp server onboard database DB0729 with ip 100.102.44.215 to recovery service."
Final Result:
DB0729 is onboarded to Recovery Service.
The initial health is WARNING — Waiting for archive logs, which is expected immediately after onboarding; the first scheduled task is due at 13:20 UTC. Real-time redo remains disabled. I did not enable it because that is a separate configuration change and requires a
Checklist: